search

nodeSolidServer / node-solid-server

Solid server on top of the file-system in NodeJS
https://solidproject.org/for-developers/pod-server
Other
1.77k stars 297 forks source link

fix: Return 201 when PATCH creates a new resource #1786

Open CxRes opened 1 month ago

CxRes commented 1 month ago

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 commented 1 month ago

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

bourgeoa commented 1 month ago

@CxRes I think we need some confirmation on specification see this discussion https://github.com/solid/specification/issues/14#issuecomment-683480525

CxRes commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

@bourgeoa Excuse my ignorance, but what is WHERE?

csarven commented 3 weeks ago

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 commented 2 weeks ago

@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 commented 6 days ago

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.