Correctly handle authorization edge cases based on resource existence#1185
Conversation
| // In case an operation would return a 404, | ||
| // that is what should be returned even if the agent does not have the correct acces, | ||
| // as long as they have read access. | ||
| // 404 is returned if a resource does not exist and no attempt is being made to create it. |
There was a problem hiding this comment.
Any spec ref here would be helpful.
| [ 'PATCH', 'C/R', [ AM.append ], [ AM.write ], INSERT, N3, 205, 201 ], | ||
| [ 'PATCH', 'C/R', [ AM.append ], [ AM.write ], DELETE, N3, 401, 401 ], | ||
| // We don't return 404 yet in case a PATCH has no inserts and C/R does not exist | ||
| // [ 'PATCH', 'C/R', [], [ AM.read, AM.write ], DELETE, N3, 205, 404 ], |
There was a problem hiding this comment.
| // [ 'PATCH', 'C/R', [], [ AM.read, AM.write ], DELETE, N3, 205, 404 ], | |
| // Agreed upon deviation from the original table | |
| [ 'PATCH', 'C/R', [], [ AM.read, AM.write ], DELETE, N3, 205, 409 /* instead of 404 */ ], |
again with @csarven permitting
| [ 'DELETE', 'C/R', [ AM.append ], undefined, '', '', 401, 401 ], | ||
| [ 'DELETE', 'C/R', [ AM.append ], [ AM.read ], '', '', 401, 404 ], | ||
| // Not sure why 401 is suggested here instead of 404? | ||
| // [ 'DELETE', 'C/R', [ AM.write ], undefined, '', '', 205, 401 ], |
There was a problem hiding this comment.
Disclosing whether a resource exists requires Read access on its container.
There was a problem hiding this comment.
Solution will be to also require Read permissions for DELETE requests that target a container.
There was a problem hiding this comment.
Actually, that is the answer to the comment below. This one has the same issue as the GET above that we require the parent permissions in a situation where we don't easily have access to them. So this one is probably going to stay 401 for a bit then. I'll look if there is a possible change that also exposes parent permissions to the authorizer to handle this situation.
There was a problem hiding this comment.
So this is a case where @csarven's table is stricter than we are; looks like this will need fixing.
|
All the comments about requiring a spec issue of the 404 tests depend on what the spec says in general about that case, since that is what I couldn't find. |
This allows PermissionReaders to potentially only check the necessary access modes for potential performance optimization.
The function was also moved to the smaller interface ResourceSet.
Uses a WeakMap on the identifier to cache resource existence.
690cdf4 to
4226410
Compare
|
Support for OPTIONS request has been added so they now also work in the table. |
4226410 to
3ada1a3
Compare
There was a problem hiding this comment.
Excellent, I think we are good for all table rows except for one.
I have tagged @csarven in case he wants to double-check, but I am fine with procdeding.
That said, I think I might have a solution for the three remaining different rows (two of which are stricter and hence already acceptable): can we—in a new PR—introduce an Existence mode that the modesExtractor returns, and if not covered by the target itself, will require the WebACL permissionReader to check with the parent?
| [ 'GET', 'C/R', [], [ AM.read ], '', '', 200, 404 ], | ||
| [ 'GET', 'C/R', [ AM.read ], undefined, '', '', 200, 404 ], | ||
| // For the 404, we only check if C/R has read permissions, not C/ | ||
| // [ 'GET', 'C/R', [ AM.read ], [ AM.write ], '', '', 401, 404 ], |
There was a problem hiding this comment.
always read the parent permissions for every request
…if the resource does not exist?
Proposal:
| // [ 'GET', 'C/R', [ AM.read ], [ AM.write ], '', '', 401, 404 ], | |
| // Agreed upon deviation from the original table; more conservative interpretation allowed | |
| [ 'GET', 'C/R', [ AM.read ], [ AM.write ], '', '', 401, 401 /* instead of 404 */ ], |
with @csarven's blessing.
| [ 'PATCH', 'C/R', [ AM.append ], [ AM.write ], INSERT, N3, 205, 201 ], | ||
| [ 'PATCH', 'C/R', [ AM.append ], [ AM.write ], DELETE, N3, 401, 401 ], | ||
| // We don't return 404 yet in case a PATCH has no inserts and C/R does not exist | ||
| // [ 'PATCH', 'C/R', [], [ AM.read, AM.write ], DELETE, N3, 205, 404 ], |
There was a problem hiding this comment.
| // [ 'PATCH', 'C/R', [], [ AM.read, AM.write ], DELETE, N3, 205, 404 ], | |
| // Agreed upon deviation from the original table | |
| [ 'PATCH', 'C/R', [], [ AM.read, AM.write ], DELETE, N3, 205, 409 /* instead of 404 */ ], |
again with @csarven permitting
| [ 'DELETE', 'C/R', [ AM.append ], undefined, '', '', 401, 401 ], | ||
| [ 'DELETE', 'C/R', [ AM.append ], [ AM.read ], '', '', 401, 404 ], | ||
| // Not sure why 401 is suggested here instead of 404? | ||
| // [ 'DELETE', 'C/R', [ AM.write ], undefined, '', '', 205, 401 ], |
There was a problem hiding this comment.
So this is a case where @csarven's table is stricter than we are; looks like this will need fixing.
I feel like this goes against how the modes work/are interpreted. All other modes are binary permissions that you either have or don't, depending on what the reader returns. An Existence mode that has to be interpreted differently goes against that. It depends a bit on if this is a WebACL thing, or a general Solid permissions thing. If only WebACL requires read permissons on a parent container to influence the response code, we should add an existence check in that reader. If this is a general Solid thing, the |
3ada1a3 to
61b9ad4
Compare
I don't think so. The modes extractor will say "agents need to be able to Read and Write to the resource to perform the operation". So my proposal is to make it say "agents need to be able to ExistenceCheck and Read and Write to perform the operation". That is binary too?
Exactly; what I wrote above would also work for another permissions thing that differently arranges "need to be allowed to know that X exists".
Yes; so that's why we'd send the
I don't think so; I only want to encode "agent needs to be allowed to find out if the target resource exists" as a bit. The fact that in WebACL, this is checked on the parent, should only be in the WebACL-specific reader. |
We are not checking the parent container because of WebACL reasons though. We are checking the parent container because the Returning an |
📁 Related issues
Closes #1161 (sort of? Depends on the 404 interpretation)
✍️ Description
Targeting the 4.0.0 branch as several class signatures change.
List of changes in this PR:
ResourceSetinterface withhasResourcefunction, which is extended byResourceStore.CachedResourceSetthat uses a WeakMapRegarding the 404, this behaviour is based on the results from the tables in the linked issues. But I could not actually find references in the spec that this should happen which makes me wonder if this is actually still required? If yes I could use a spec reference because now I'm not sure how correct the implementation even is. Related to that: the tables also say to only return a Location header for POST requests if you have read permissions on the resource, which is also something we don't currently do.
The integration test table is based on the tables from the linked issues. It contains a few lines commented out where we have a mismatch with what was suggested. The question is if we are wrong there or if the table was just outdated.