search

solid-contrib / web-access-control-tests

Tests if a Solid server implements web access control correctly
MIT License
2 stars 5 forks source link

PATCH/PUT Append to create a new resource #56

Open bourgeoa opened 9 months ago

bourgeoa commented 9 months ago

NSS has moved to approve that PATCH Append is enough on non existing files see #55 and https://github.com/nodeSolidServer/node-solid-server/pull/1745

See https://github.com/solid/web-access-control-spec/issues/122

csarven commented 9 months ago

What does "NSS has moved to approve" mean?

"PATCH Append is enough on non existing files" -- That alone is not correct. What's intended by the spec is:

Create requires Append (or Write) on C/ and Write on C/R.

Is the PR here based on NSS's opinion/preference of how a behaviour should be and therefore the tests? Why should NSS's approach/change change the tests here? Are these tests only intended for NSS? How are other implementations behaving?

What is the current interpretation of the spec by NSS and tests, and what is the proposed change here?

bourgeoa commented 9 months ago

Is the PR here based on NSS's opinion/preference of how a behaviour should be and therefore the tests? Why should NSS's approach/change change the tests here? Are these tests only intended for NSS? How are other implementations behaving?

It is the reverse. NSS follows the tests. Tests now should change. They were wrong. NSS in his CI uses the test-suite.

Create requires Append (or Write) on C/ and Write on C/R

Write on c/r. Well it depends on PATCH action :

csarven commented 9 months ago

Title of the issue is "PATCH Append on new file". Should that be changed?

I just want to be clear on what's intended to change in this PR exactly. Can indicate one or more of the following that's being done in this PR:

And for each selected, can you please indicate what is exactly being corrected/changed in this PR? That would help me understand 1) what's interpreted of the spec is correct, and 2) verify/review the tests.

michielbdejong commented 9 months ago

Ah yes, I agree with @csarven the phrase "Append on new file" is confusing - maybe you mean "PATCH to create" or "Append to an empty file"?

michielbdejong commented 9 months ago

Agree with @csarven that "create requires Append (or Write) on C/ and Write on C/R" (as long as we're talking about PUT and PATCH; POST is of course a separate story).

I created a PR to attempt to further clarify the spec text: https://github.com/solid/web-access-control-spec/pull/128

AFAIK, what I wrote in that PR is a) what we want and b) what the current tests test for, but happy to be corrected on either or both.

bourgeoa commented 9 months ago

[x] Creating a new resource with PATCH (inserts)

The purpose of this PR is only updating tests related to creating a new resource with PATCH with inserts only.

bourgeoa commented 9 months ago

Ah yes, I agree with @csarven the phrase "Append on new file" is confusing - maybe you mean "PATCH to create" or "Append to an empty file"?

PR title changed

bourgeoa commented 9 months ago

@michielbdejong

csarven commented 9 months ago

I'm not too familiar reading the syntax that's in this PR or these .ts files, so I'll use plain language with an example re:

The purpose of this PR is only updating tests related to creating a new resource with PATCH with inserts only.

Given:

PATCH C/R:

If Agent has Write or Append on C/ and Write on C/R, then 2xx (typically 201, and 200 is common too). Otherwise, 403.

And on a related Note ( https://solid.github.io/web-access-control-spec/#container-permissions ):

When a server supports the creation of intermediate containers in the process of creating a resource, the server applies the same requirements for the create operation for each container.

bourgeoa commented 9 months ago

I haven't got my head around what the final state of the discussion is for these tests and clarifications in the spec. If the spec hasn't changed, then is the implication that these tests were incorrect, yet passing on both NSS and CSS? That confuses me as the spec-tests showed CSS passing but NSS having some failures in this area

I agree that NSS was failing in this area and CSS is correct. I tested this manually on CSS test-servers.

bourgeoa commented 9 months ago

@edwardsph @michielbdejong

Other OK responses should be allowed too.

Shall I change all of them to 2xx ?

I have modified the NSS CI to be able to use a test branch, and also added some tests in NSS This allowed to test these changes on NSS. All tests are passing and CI are OK I have published a NSS@5.7.8 with Append PATCH which was a blocker for chat with Append only and message signature on mashlib@1.8.9

Could you review Append PUT https://github.com/nodeSolidServer/node-solid-server/pull/1746 ?

CxRes commented 3 months ago

If new resources are being created and no response body is being sent, the status code needs to be 201. See nodeSolidServer/node-solid-server#1786