PATCH/PUT Append to create a new resource #56

bourgeoa · 2023-11-30T15:31:08Z

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

See solid/web-access-control-spec#122

michielbdejong · 2023-12-01T08:16:48Z

test/surface/create.test.ts

@@ -350,7 +350,7 @@ describe('Create', () => {
        "<#patch> a solid:InsertDeletePatch;\n" +
        "  solid:inserts { <#hello> <#linked> <#world> .}.\n",
      });
-      expect(result.status).toEqual(403);
+      expect(result.status).toEqual(200);


Other OK responses should be allowed too.

michielbdejong · 2023-12-01T08:17:02Z

test/surface/create.test.ts

@@ -589,10 +589,10 @@ describe('Create', () => {
        "<#patch> a solid:InsertDeletePatch;\n" +
        "  solid:inserts { <#hello> <#linked> <#world> .}.\n",
      });
-      expect(result.status).toEqual(403);
+      expect(result.status).toEqual(200);


Not sure if I agree. This test is about required permissions on c/, right?

michielbdejong · 2023-12-01T08:18:22Z

test/surface/create.test.ts

@@ -313,7 +313,7 @@ describe('Create', () => {
    testAllowed('Write', 'Write');
    testAllowed('Append', 'Write');

-    it(`is disallowed without Write on c/r`, async () => {
+    it(`is allowed with Append on c/r`, async () => {


The name to the test artifact below should be changed to match the test purpose to avoid future confusion:

const testing = `disallowed-default`;

OK I commit my changes to testing

michielbdejong · 2023-12-01T08:19:32Z

test/surface/create.test.ts

@@ -627,7 +627,7 @@ describe('Create', () => {
        "<#patch> a solid:InsertDeletePatch;\n" +
        "  solid:inserts { <#hello> <#linked> <#world> .}.\n",
      });
-      expect(result.status).toEqual(403);
+      expect(result.status).toEqual(200);


Not sure if I agree. This test is about required permissions on c/, right?

@michielbdejong I should have added this link as a clarification on WAC Append solid/web-access-control-spec#122

csarven · 2023-12-04T11:44:52Z

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 · 2023-12-04T12:51:40Z

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 :

Append for inserts only
Read/write for ?
Write for delete

csarven · 2023-12-04T13:02:20Z

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:

Creating a new resource with PATCH (inserts)
Adding information to a resource with PATCH (inserts)
Deleting information from a resource with PATCH (deletes)

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 · 2023-12-06T11:47:14Z

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"?

This updates the text to match what we decided in solid#105 (comment). At the time we decided not to update the spec text, but now that the spec text is more detailed, the current statement is not correctly conveying that access to both the containing folder and the non-existing resource URL is required. See the confusion that was created by this in solid-contrib/web-access-control-tests#56 which was an (I think incorrect) reaction to solid#122.

michielbdejong · 2023-12-06T12:05:42Z

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: solid/web-access-control-spec#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 · 2023-12-06T13:25:25Z

[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 · 2023-12-11T16:24:18Z

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 · 2023-12-11T16:43:46Z

@michielbdejong

shall I replace in the 3 tests concerned the it by itIs("APPEND-CREATE") or any other word you should prefer ?
and edit the env.js
What ranch number do you want to use ?

csarven · 2023-12-11T16:47:20Z

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.

) * Further clarify requirements for PUT-to-create and PATCH-to-create This updates the text to match what we decided in #105 (comment). At the time we decided not to update the spec text, but now that the spec text is more detailed, the current statement is not correctly conveying that access to both the containing folder and the non-existing resource URL is required. See the confusion that was created by this in solid-contrib/web-access-control-tests#56 which was an (I think incorrect) reaction to #122. * Apply suggestions from code review Co-authored-by: Ted Thibodeau Jr <[email protected]> --------- Co-authored-by: Sarven Capadisli <[email protected]> Co-authored-by: Ted Thibodeau Jr <[email protected]>

edwardsph

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

edwardsph · 2023-12-19T15:46:08Z

test/surface/create.test.ts

@@ -350,7 +350,7 @@ describe('Create', () => {
        "<#patch> a solid:InsertDeletePatch;\n" +
        "  solid:inserts { <#hello> <#linked> <#world> .}.\n",
      });
-      expect(result.status).toEqual(403);
+      expect(result.status).toEqual(200);


Other OK responses should be allowed too.

edwardsph · 2023-12-19T15:46:37Z

test/surface/create.test.ts

@@ -313,7 +313,7 @@ describe('Create', () => {
    testAllowed('Write', 'Write');
    testAllowed('Append', 'Write');

-    it(`is disallowed without Write on c/r`, async () => {
+    it(`is allowed with Append on c/r`, async () => {


The name to the test artifact below should be changed to match the test purpose to avoid future confusion:

const testing = `disallowed-default`;

bourgeoa · 2023-12-19T16:42:00Z

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 · 2023-12-19T16:44:36Z

@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 [email protected] with Append PATCH which was a blocker for chat with Append only and message signature on [email protected]

Could you review Append PUT nodeSolidServer/node-solid-server#1746 ?

CxRes · 2024-06-12T18:16:53Z

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

When a new resource is created as a result of PATCH, allow 201 as a valid status code.

Allow server to send 201 on PATCH create

PATCH Append on new file

a567231

bourgeoa requested a review from michielbdejong November 30, 2023 15:31

bourgeoa linked an issue Nov 30, 2023 that may be closed by this pull request

PATCH/PUT Append for new file #55

Open

michielbdejong requested changes Dec 1, 2023

View reviewed changes

michielbdejong mentioned this pull request Dec 6, 2023

Further clarify requirements for PUT-to-create and PATCH-to-create solid/web-access-control-spec#128

Merged

bourgeoa changed the title ~~PATCH Append on new file~~ PATCH Append to create a new file Dec 11, 2023

bourgeoa requested a review from michielbdejong December 11, 2023 16:47

update tests for PUT with Append

ca44c67

edwardsph reviewed Dec 19, 2023

View reviewed changes

update testing

8dcf691

bourgeoa changed the title ~~PATCH Append to create a new file~~ PATCHPUT Append to create a new resource Dec 19, 2023

bourgeoa changed the title ~~PATCHPUT Append to create a new resource~~ PATCH/PUT Append to create a new resource Dec 19, 2023

bourgeoa added 3 commits December 31, 2023 09:23

wrong test

59278f4

precondition if-none-match 412

e63fc5c

fails on dot file .dummy

312494c

Allow server to send 201 on PATCH create

9d3723a

When a new resource is created as a result of PATCH, allow 201 as a valid status code.

Merge pull request #57 from CxRes/CxRes-fix-patch-code

a18d54f

Allow server to send 201 on PATCH create

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PATCH/PUT Append to create a new resource #56

PATCH/PUT Append to create a new resource #56

bourgeoa commented Nov 30, 2023 •

edited

Loading

michielbdejong Dec 1, 2023

edwardsph Dec 19, 2023

michielbdejong Dec 1, 2023

michielbdejong Dec 1, 2023

edwardsph Dec 19, 2023 •

edited

Loading

bourgeoa Dec 19, 2023

michielbdejong Dec 1, 2023

bourgeoa Dec 1, 2023 •

edited

Loading

csarven commented Dec 4, 2023

bourgeoa commented Dec 4, 2023

csarven commented Dec 4, 2023 •

edited by bourgeoa

Loading

michielbdejong commented Dec 6, 2023

michielbdejong commented Dec 6, 2023

bourgeoa commented Dec 6, 2023

bourgeoa commented Dec 11, 2023

bourgeoa commented Dec 11, 2023

csarven commented Dec 11, 2023

edwardsph left a comment

edwardsph Dec 19, 2023

edwardsph Dec 19, 2023 •

edited

Loading

bourgeoa commented Dec 19, 2023

bourgeoa commented Dec 19, 2023 •

edited

Loading

CxRes commented Jun 12, 2024

PATCH/PUT Append to create a new resource #56

Are you sure you want to change the base?

PATCH/PUT Append to create a new resource #56

Conversation

bourgeoa commented Nov 30, 2023 • edited Loading

michielbdejong Dec 1, 2023

Choose a reason for hiding this comment

edwardsph Dec 19, 2023

Choose a reason for hiding this comment

michielbdejong Dec 1, 2023

Choose a reason for hiding this comment

michielbdejong Dec 1, 2023

Choose a reason for hiding this comment

edwardsph Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

bourgeoa Dec 19, 2023

Choose a reason for hiding this comment

michielbdejong Dec 1, 2023

Choose a reason for hiding this comment

bourgeoa Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

csarven commented Dec 4, 2023

bourgeoa commented Dec 4, 2023

csarven commented Dec 4, 2023 • edited by bourgeoa Loading

michielbdejong commented Dec 6, 2023

michielbdejong commented Dec 6, 2023

bourgeoa commented Dec 6, 2023

bourgeoa commented Dec 11, 2023

bourgeoa commented Dec 11, 2023

csarven commented Dec 11, 2023

edwardsph left a comment

Choose a reason for hiding this comment

edwardsph Dec 19, 2023

Choose a reason for hiding this comment

edwardsph Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

bourgeoa commented Dec 19, 2023

bourgeoa commented Dec 19, 2023 • edited Loading

CxRes commented Jun 12, 2024

bourgeoa commented Nov 30, 2023 •

edited

Loading

edwardsph Dec 19, 2023 •

edited

Loading

bourgeoa Dec 1, 2023 •

edited

Loading

csarven commented Dec 4, 2023 •

edited by bourgeoa

Loading

edwardsph Dec 19, 2023 •

edited

Loading

bourgeoa commented Dec 19, 2023 •

edited

Loading