Test for non-existing resources

[edwardsph]

This PR is based on work by @surilindur in PR #84 but created as a new PR to incorporate changes. PR #84 will be closed.

The original work was to add tests for operations against non-existing (fictive) resources. It also made improvements to the way test resources are created and their access controls are set up. This PR contains the original work plus an additional commit which extracts common code into a shared utils feature. It also improves performance by re-using resources that are only used in read tests. There are a couple of changes to the expected response codes that need to be highlighted too.

The tables suggest 404 in cases where the agent can know that the resource doesn't exist however it was also argued that 401/403 are valid options since the agent does not have the correct access and security issues should be processed ahead of CRUD operations.

• POST to a fictive resource with inherited read access - both 403 and 404 are valid.
• DELETE a fictive resource with inherited read access - both 403 and 404 are valid.

When a DELETE succeeds, CSS returns a 205 but the HTTP spec also allows 200, 202 and 204.

Lastly there are 3 cases where CSS returns a 404 which I think is incorrect and it should be 401/403. This is when the agent has W access (inheritable) to a container and attempts to delete a fictive resource. The tables make it clear that this should not be a 404 as the agent does not have R access to the container. This appears to be related to a line commented out in CSS's tables: https://github.com/CommunitySolidServer/CommunitySolidServer/blob/main/test/integration/PermissionTable.test.ts#L108

I think the next steps are to split out the write tests from here and link them to the relevant requirements in the spec, but I suggest we do that as follow-on work.

Please can you review this and then I can merge and get these tests into the test suite.

[michielbdejong]

Great work! Have you also ran these tests against the other servers, or only against CSS?

[edwardsph]

I've run them against ESS too and there are a couple of issues I've raised with the team, plus the fact that it doesn't yet implement n3 patch, of course. I haven't tried it on NSS.

…

On Mon, 10 Oct 2022, 18:53 Michiel de Jong, ***@***.***> wrote: Great work! Have you also ran these tests against the other servers, or only against CSS? — Reply to this email directly, view it on GitHub <#95 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACICDC3VFV2D5DVK3CESEQLWCRJZLANCNFSM6AAAAAAQ7YEYJE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- This e-mail, and any attachments thereto, is intended only for use by the addressee(s) named herein and may contain legally privileged, confidential and/or proprietary information. If you are not the intended recipient of this e-mail (or the person responsible for delivering this document to the intended recipient), please do not disseminate, distribute, print or copy this e-mail, or any attachment thereto. If you have received this e-mail in error, please respond to the individual sending the message, and permanently delete the email.

[kjetilk]

The return codes look good to me, but I think we should run this on NSS too before merging.

Also, I can't claim to understand the setup code now, but I'm confident in your abilities around that.

[edwardsph]

There are a number of errors with NSS, some already seen but a number of new ones related to these codes, which do appear to be errors. One thing we'll have to think about is whether the successful PATCH requests can also return 200 where the test allowed 201, 204, 205.

…

On Tue, 11 Oct 2022, 21:46 Kjetil Kjernsmo, ***@***.***> wrote: ***@***.**** approved this pull request. The return codes look good to me, but I think we should run this on NSS too before merging. Also, I can't claim to understand the setup code now, but I'm confident in your abilities around that. — Reply to this email directly, view it on GitHub <#95 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACICDC7NUIAAQTQPFIN7OXLWCXG27ANCNFSM6AAAAAAQ7YEYJE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- This e-mail, and any attachments thereto, is intended only for use by the addressee(s) named herein and may contain legally privileged, confidential and/or proprietary information. If you are not the intended recipient of this e-mail (or the person responsible for delivering this document to the intended recipient), please do not disseminate, distribute, print or copy this e-mail, or any attachment thereto. If you have received this e-mail in error, please respond to the individual sending the message, and permanently delete the email.

[kjetilk]

There are a number of errors with NSS, some already seen but a number of new ones related to these codes, which do appear to be errors.

Alright, sounds good!

One thing we'll have to think about is whether the successful PATCH requests can also return 200 where the test allowed 201, 204, 205.

OK! It seems 200 would be OK if the server returns a representation of the resource.

[edwardsph]

I've sorted the PATCH tests to allow 200. The PUT and POST tests should also allow 200, except where creating a new resource via PUT in which case only 201 is allowed. This resolves a number of the issues with NSS and the remaining new errors are related to NSS allowing deletion of resources despite having insufficient access.

I also removed a test which was duplicated by these tests.

If there are no objections, I will merge these tests based on the existing approval by the end of the week and release a new version.

[kjetilk]

👍

[csarven]

I haven't reviewed the contents of this PR introduces but aware of PR 84 and related work on specs.

No objection based on the summary provided in the original comment.

[damooo]

Hello all, Request to clarify on notion of the fictive resources.

As per solid-protocol, uri slash semantics are necessary but not sufficient for a containment statement to be valid. Implies,

• if a/b is known to be existing contained resource, then we can derive that a/ must be it's parent container, as the condition is necessary.
• But we cannot say a/b must be a contained child resource of a/ if we only know existance of a/ container, just because of slash delimited uri path. For, uri convention satisfaction alone is not sufficient to deduce such triple. Thus a/b._acl may be aux of a/b, instead being child of a/. Or it can be totally a barred name in resource naming policy. Or may not be an ldp::Resource at all.

Thus we can say a/b is child of a/, only after minting the child node, and <a/> ldp:contains <a/b> triple in the container rep. Till then such relation doesn't hold by mere satisfaction of one of the uri conventions.

Thus if a/b doesn't exist as child of a/ yet, then any acl:default authorizations in acl of a/ doesn't apply or inherit to a/b. As, acl:default applies only to resources that are ensured-sans-heuristics as children of a/ through ldp:contains.

And until, a request target c/r is ensured to be child of c/, we should not see c/ as any relavent resource for operations targeting c/r.

Am i missing something in this?

@edwardsph , @kjetilk , @csarven .

[kjetilk]

@damooo Many thanks for your attention to detail here! I believe that you're right here, and that we should give this proper treatment, especially in light of aux resources and how we distinguish those from normal resources.

Can I ask you to open a separate issue on this on the spec repo?

[csarven]

I agree that the specification should further clarify membership/containment. It was brought up before [citation needed].

I would add that we may need to allow auxiliary resources to have their own ACLs.