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 https://github.com/solid/specification/issues/14#issuecomment-683480525

[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]

200 in the case of PATCH was intentional, not a typo at the time of writing https://github.com/solid/specification/issues/14#issuecomment-683480525 . 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.

https://github.com/solid/specification/issues/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 https://github.com/solid/specification/issues/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 https://github.com/solid/specification/issues/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 https://github.com/nodeSolidServer/node-solid-server/blob/843c7718cd366599272905c8e3a57f7af2ef0479/lib/handlers/patch.js#L153-L157

[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 https://github.com/solid/specification/issues/14#issuecomment-1272166350

[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.

[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 https://github.com/solid-contrib/web-access-control-tests/pull/57 on the specific branch. Please review and merge!

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