Correctly handle authorization edge cases based on resource existence

[joachimvh]

📁 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:

• ResourceSet interface with hasResource function, which is extended by ResourceStore.
• CachedResourceSet that uses a WeakMap
• We now extract the correct access modes from a request (create/delete/etc. only when relevant)
• 404 is returned when an agent has a) no access b) the resource does not exist, and c) the operation would not create the resource.
• The WebACL reader now correctly checks the parent container existence for create/delete operations.
• There is a cool table in the integrations tests to automate many of the tests on all of this.

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

[RubenVerborgh]

Beautiful 🥲🥲🥲

[RubenVerborgh]

Any spec ref here would be helpful.

[RubenVerborgh]

Idem

[RubenVerborgh]

Suggested change

	// [ '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

[RubenVerborgh]

Requires a spec issue?

[RubenVerborgh]

Disclosing whether a resource exists requires Read access on its container.

[joachimvh]

Solution will be to also require Read permissions for DELETE requests that target a container.

[joachimvh]

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.

[RubenVerborgh]

So this is a case where @csarven's table is stricter than we are; looks like this will need fixing.

[joachimvh]

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.

[joachimvh]

Support for OPTIONS request has been added so they now also work in the table.

[RubenVerborgh]

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?

[RubenVerborgh]

always read the parent permissions for every request

…if the resource does not exist?

Proposal:

Suggested change

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

[RubenVerborgh]

Suggested change

	// [ '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

[RubenVerborgh]

So this is a case where @csarven's table is stricter than we are; looks like this will need fixing.

[joachimvh]

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?

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 modesExtractor should output a key/value object with the keys being identifiers and the values being the modes required on those resources, as originally discussed in the related issue.

[RubenVerborgh]

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.

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?

It depends a bit on if this is a WebACL thing, or a general Solid permissions thing.

Exactly; what I wrote above would also work for another permissions thing that differently arranges "need to be allowed to know that X exists".

If only WebACL requires read permissons on a parent container to influence the response code, we should add an existence check in that reader.

Yes; so that's why we'd send the Existence bit over in modes as a required check in some cases. And then the reader would now: ah, for ACL, existence check means looking at the parent in these cases.

If this is a general Solid thing, the modesExtractor should output a key/value object with the keys being identifiers and the values being the modes required on those resources, as originally discussed in the related issue.

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.

[joachimvh]

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 ldp:contains triples of a parent container exposes information about its child resources and vice versa. A DELETE on a non-existing resource needs to return 401 if you don't have read permissions on the parent container, because otherwise this would expose that the container does not have a corresponding ldp:contains triple. Specifically mentioned in the comment above as "Disclosing whether a resource exists requires Read access on its container.". So that follows from the Solid definitions, not the WebACL ones.

Returning an Existence mode is essentially the same as encoding the information "Does the agent have Read permissions on the parent container", independent of the authorization scheme being used.