fix: Return 201 when PATCH creates a new resource

[CxRes]

A PATCH request may end up creating a new resource, in which case a 201 status code should be returned.

This is an equivalent change as that for PUT in #1785.

This does not update or add tests, please fix that!

[bourgeoa]

Thanks for raising this issue.
There are some issue to resolve in /test-suite/solid-crud-tests on pub/sub tests

[bourgeoa]

@CxRes
I think we need some confirmation on specification see this discussion solid/specification#14 (comment)

[CxRes]

I read the cited issue/comment. I think there is typo in the PATCH table, it should be 200 and 201 for exists and not exists respectively, just like the PUT table. @csarven?

[csarven]

tl;dr: it depends.

Generally speaking 201 better characterises the creation of a new resource but 200 is not wrong either. It does fundamentally depend on what the server prefers to respond to a particular request in the context of the resource semantics.

There are other related considerations: cases in which no Read access permission is given to the target resource, 201 would reveal that the resource did not exist before whereas 200 would not reveal it. Related: solid/specification#311

If these distinctions are not taken into account or there is no preference/intention to be very "strict" about the status code in terms of revealing the existence of a resource, then 201 will suffice, otherwise stick to 200 (if anything but for the time being.)

[csarven]

200 in the case of PATCH was intentional, not a typo at the time of writing solid/specification#14 (comment) . Note that the row with C/ Append; C/R Write does not have Read. 201 will absolutely reveal the prior non-existence of the resource when it is created but 200 would not. This is indeed a strict view of things. In contrast, I left 201 for PUT alone because that was straight off RFC 7231 and I didn't wish to introduce a wilful violation. The response codes for PATCH are not as explicitly defined elsewhere and so I found 200 to be appropriate (when there is no Read on C/R). Had C/R had Read, PATCH creating the resource would generally be 201.

solid/specification#311

[CxRes]

Thanks for the clarification @csarven! I stand corrected then...

So my trouble is this: when I am watching a container C/ for notifications, I am at present using status codes to determine what notification to send to the container when a resource in it changes. It would seem odd to send as:Update instead of as:Add when PATCH creates C/R. (I could use another mechanism, but that is another property to keep track off, say, on the Express response object).

Note that the row with C/ Append; C/R Write does not have Read.

I don't want to rehash solid/specification#311 and friends, but I am unclear why revealing the prior non-existence of the resource to a user-agent that already has write access is a problem? Was there some specific use case, or was this a first principles issue: that resources can be configured to have write access without read access, which seems strange specifically for PATCHing?

Also, there is the issue of consistency, if we are following RFC9110 for PUT, why would we treat PATCH differently?

Not looking for a response here, saying we need to revisit this in a STM!

[CxRes]

Maybe one more clarification for this, if the resource is configured such that the user-agent had read access, would at least in those cases 201 be the appropriate code?

[bourgeoa]

@CxRes

I don't want to rehash solid/specification#311 and friends, but I am unclear why revealing the prior non-existence of the resource to a user-agent that already has write access is a problem?

There has been lengthy discussions and it has been decided the following for PATCH

node-solid-server/lib/handlers/patch.js

Lines 153 to 157 in 843c771

	// Read access is required for DELETE and WHERE.
	// If we would allows users without read access,
	// they could use DELETE or WHERE to trigger 200 or 409,
	// and thereby guess the existence of certain triples.
	// DELETE additionally requires write access.

[CxRes]

@bourgeoa Excuse my ignorance, but what is WHERE?

[csarven]

I think Add would be the right activity when the container expands.

Yes, re comment in 311:

Noting that the use cases to write-only are rare, so in practice both read and write would be given to allow a write operation. However, this is orthogonal to how authorization rules could be set.

We can of course choose 201 for PATCH as with PUT (and yes, it would be appropriate too with the Read case on C/ or C/R), and that would generally be better, but 200 is not ruled out. 202 or 204 would also be okay. I was working with 1) "200 responses to state-changing methods" which is applicable to PATCH 2) PATCH itself didn't make the 200/201 distinction as PUT has 3) giving a bit more attention to authorization / security considerations. Some things didn't add up to me - which is why some see the ramblings as "philosophy".. and some others may look at it in terms of security - we can't pretend that the protocol is all "secure" if we also don't pay attention to minute differences between 200 and 201. See also last paragraph in solid/specification#14 (comment)

[CxRes]

@bourgeoa Given that all resources that are being PATCHed on NSS will necessarily have read access on them and @csarven agrees that 201 is an acceptable response in those case, I think we can safely accept and close this PR.

It might be worth considering if NSS might want to allow one to write without read access on certain resources, in which case we will want to revisit and split the response code between 200 (or 204) and 201 as appropriate.

[csarven]

I haven't tested the contents of this PR, so I can't vouch for whether it works as intended or breaks anything. The review is based on what's intended and the discussion in this PR (and elsewhere).

Approving with the understanding that 201 is a valid, but not the only, status code when a resource is created. If the server is also taking other things into account, such as whether the client has the necessary permissions to know the existence of the target resource, as well as the permission to create the resource, then 201 is indeed suitable. Again, the use of 200 is not wrong, and its use could perhaps even be better with Content-Location and payload including a representation, but perhaps that's a separate discussion.

[jeff-zucker]

I also don't see a problem with sending 201. The case of something bad happening when a person with write privs but not read privs finding out that the thing didn't exist before they created it seems unlikely and far-fetched.

[melvincarvalho]

We had a call on SolidOS yesterday, and this issue came up. Is there anything still blocking the merge?

[michielbdejong]

@CxRes NSS runs the web-access-control-tests from a branch called patchAppendNewDocument:

https://github.com/solid-contrib/web-access-control-tests/blob/patchAppendNewDocument/test/surface/create.test.ts#L389

If you make a change there in that branch and then retrigger the tests of this PR, I think it should work

[timbl]

As discussed on call today. Overriding as the test are the wrong version of the tests.. except I don't have priv to do so.

[CxRes]

@michielbdejong opened PR solid-contrib/web-access-control-tests#57 on the specific branch. Please review and merge!

I am not sure if I can trigger a test without the PR!