Test for non-existing resources

[surilindur]

Hi,

This pull request is not finished, but I decided to open it in hopes of getting some feedback on whether the changes seem to make sense or not, and whether the structure of them is reasonable. There were some changes to the Community Solid Server permission tests in pull request #1185, adding the permission test table, and I was assigned the task to implement similar ones for the specification test suite here.

The major difference between the test suite and the CSS tests appears to be with how non-existing resources are handled, for example to avoid disclosing unnecessary information about resources that exist but are not accessible to whoever is making the request, or vice versa. Within the CSS, this appears to have been done with acl:default, but I am unsure how to implement it here (maybe by writing it to the container's permissions manually?). Actually, I need to check what inheritable permissions actually do, whether they change the container as well or only its children.

But in any case, here are:

• Initial drafts of checks for non-existing resources, added to web-access-control/protected-operation files (with added inheritable permissions on containers because I did not find another way to get permissions for resources that do not exist).
• Added web-access-control/protected-operation/access-permissions-* tests that try to be similar to the CSS tests, as an idea for how to do container permissions in addition to resource permissions.

Apologies in advance if the changes or new tests look weird, I will fix them based on feedback and I will also try to improve them myself later.

[edwardsph]

It's great to see a community contribution to tests and this is really good work. You picked one of the most difficult areas of testing but it is really important that we pick up this aspect of accidental information disclosure so thank you!

I can see why you had to add inherited access to the container for fictive resources. I just need to think through whether that could distort the results of the other tests if permissions were not managed properly by a server. I haven't had a chance to look at the new tests yet but they look like an interesting approach - are you suggesting we could replace the read tests using your approach? I'll have time to look in more detail next week.

[surilindur]

Thank you for the comment! My plan was not to replace the read tests, I think, but rather to try something different with the way they are structured. In the current existing tests, the resources are created at the start and then used in the examples table, but I wanted to try creating each resource (and its parent container) specifically for each line in the examples, because of how the container and resource permissions and their inheritance were handled in the CSS tests.

But I am not trying to replace the existing tests, sorry, so I will try to merge the changes to resource creation and the new tests with the existing ones, instead. I will add a new comment when I have done that and the changes are more legible. My apologies. 😓

[edwardsph]

No need to apologise! I am really interested in the direction you are taking.

[kjetilk]

Very interesting contribution, thank you, @surilindur !

I have looked at it, and I just wanted to make sure I understand the effect here. So, is the following an accurate description:

A fictive resource is reserved but not created, so that the harness has given it a URI, but not actually created it. Subsequent requests will therefore be done towards a non-existent resource.

Is that a correct understanding of the mechanics here?

[surilindur]

Yes, that is the basic idea. The term 'fictive' is probably not technical, it is just a random word that was short enough but somehow hopefully describes what the resource is like. If there is a better term or a more technical one, I would be happy to change to that one instead. 🙂

I originally tried to create the resource and then delete it, but I assume the reserve function (as documented in the readme) serves the same purpose for getting a URL for a file that does not actually exist on the server (and should therefore work for that purpose). Changing the resources to be created separately for each line in the examples tables should also allow for having inherited permissions for non-existing files and not inherited permissions for existing files in the same table without breaking each other by accident, I hope.

I will try to update the PR tomorrow by moving all the read-related test changes to the existing read feature files, and the write-related tests to corresponding write feature files.

[csarven]

Only following the discussion - I did not read the code changes:

It sounds like a scenario where a URI is issued (or reserved) but not have a representation? If so, typical response would be a 404 if the requesting agent has access to know about the URI's existence, otherwise, a 403. The part that's unclear to me sounds like it relates to Slug cases... but as I've mentioned elsewhere, server should not indirectly reveal the existence of a URI (when they don't have access to know) by generating/issuing a new URI. Besides that, updating the issued/reserved URI should be successful.

[edwardsph]

You are all correct about reserving a resource - the URL is generated but nothing is created.
I hope to check this PR out later today.

[edwardsph]

I've started looking through this in detail. I think there is a significant difference with your approach which we need to consider. Taking the Bob tests as an example, the Background runs for every row in the examples tables, however, in the original code, it only creates 6 resources to be used by all the subsequent tests in each scenario due to the callonce mechanism.

    # Create 3 test resources with read access for Bob
    * def testsR = callonce createResources ['read']
    # Create 3 test resources with append, write, control access for Bob
    * def testsAWC = callonce createResources ['append', 'write', 'control']

Each test then pulls the relevant resource out of the maps above. If I combine this with the inherited tests, that would be a total of 12 resource created for 96 tests.

In your code, you create just the one resource needed for each test. However, the following line is not called once per Scenario Outline but once for each Scenario that is generated when the examples are applied to the outline.

* def testResource = createResource(container, resource, type)

By my calculation, that is 140 resources for the 140 tests. We have to be very careful when re-using resources like I did because we need the tests to be isolated so that they can run in parallel. The callonce mechanism ensures that all threads pick up the same resources. If we were modifying the resources, this would be a very bad idea but since we are just reading, I was going for efficiency.

I still need to get my head around the actual tests - perhaps we could still use your approach to creating resources into an initial setup under callonce and then use a function to pick out the correct resource in place of your createResource call.

What do people think? Is minimising resources worthwhile given that when we do similar tests for create, delete and update we probably can't re-use resources. If one test fails, it will leave things in an indeterminate state so we would need tests to be better isolated.

[kjetilk]

What do people think? Is minimising resources worthwhile given that when we do similar tests for create, delete and update we probably can't re-use resources. If one test fails, it will leave things in an indeterminate state so we would need tests to be better isolated.

I don't know, I would generally trust your knowledge on that @edwardsph . But I suppose many of these tests would result in a 404 (and that's still worthwhile), but those that are create operations would result in a 20x would have an altered state and would therefore not be useable for subsequent tests unless it is intended. But then, I might not have entirely understood the idea here.

So, it is probably good practice to try to minimise resources, but we need not be terribly concerned about it.

[edwardsph]

It is really hard to get your head around all the situations we are testing for and how to do that thoroughly. I can see that your method of setting up the test resource has merits, especially as we look to write tests. It also helps with the fictive resource tests which I had not included originally. I'm still concerned that we might be missing some potential issues in implementations and I've tried to point to some of those concerns below for us to discuss.

As a general comment on the Javascript, I would prefer to use === and !== for comparisons. Is that OK with you?

[edwardsph]

Suggested change

	| Bob | can | GET | fictive | R | inherited | 404 |
	| Bob | cannot | GET | fictive | R | inherited | 404 |

There are other examples of this but I think the scenario description needs to end up describing what happened. If we expect a 404 then in this scenario Bob cannot access the resource

[surilindur]

Ohhhh yes, that is true. Thank you. I fixed some of those, but I forgot to check all of them it seems. I will go through them again.

[edwardsph]

You don't appear to every set all - is that left over from something else? It would be good to simplify this if it is not needed.

[surilindur]

It was a leftover from the original tests, that had all permissions applied on the container when the focus was on testing resource permissions. But at least with the CSS, it seems fine to not assign all permissions on container in those cases, so I decided not to use all as a result.

[edwardsph]

How do we ensure we are testing exactly what we expect? The original test deliberately granted Bob full access to the container (with nothing inheritable), then gave a specific mode to the child resource and tested whether access control did what we expected on that resource. So Bob cannot read even if given all other access modes, and Bob can read when given read but nothing else. I'm confirming that no accidental access is granted and also confirming that the tests are valid. I saw some previous tests where all read tests passed because the Alice user (owner) had been used instead of Bob so the tests were all invalid. Your test works a little differently. You grant no access to the container and read on the resource then check Bob can read. I don't see the equivalent of my other test. Did I miss it? What are your thoughts on that?

I also think there is a problem here when the container permissions are "no". The value of testContainerPermissions ends up undefined but you still use that to call setAgentAccess (similar with setInheritableAgentAccess). It results in an incomplete ACR. Perhaps I should change the test harness to allow undefined to be passed to the method and then don't create any policy for that agent rather than create a policy with no access modes. However, whether or not that is needed may be impacted by what we agree for the first part of this comment.

[surilindur]

I think this is related to the comment above, with the leftover all permissions, that I ended up not using. There are indeed no tests with all permissions on container and limited permissions on resource, as there were originally. Maybe that is problematic, I just did not view it that way, being relatively new to these things still. My apologies.

In the original tests, when checking if Bob can read a resource (with permissions on the resource), all permissions were granted on the container (with no inherited ones) and only read permissions were granted on the resource. In the new ones, no permissions are granted on the container and only the permission being tested is granted on the resource.

The AWC but no R tests I moved to the write-access-{agent,bob,public}.feature because those were concerned with write permissions. But the similar changes to container vs resource permissions were done there, as well.

Would it make sense to revert the tests back to assigning all permissions on container (not inherited) when testing on resources alone?

That last part about undefined being used seems like a bug indeed, in my resource setup script, and I will fix it. When no permissions are assigned, the function should not be called. I forgot to add the checks. Thank you!

[surilindur]

It seems I cannot reply to the topmost message, so I will write it here instead. For JavaScript, I will indeed need to change the comparison operators, I keep forgetting that. Thank you.

The tests are maybe laid out on a confusing way, but I have tried to add tests for:

• R in read-access-{agent,bob,public}.feature
• WAC in write-access-{agent,bob,public}.feature, sometimes splitting W, A and C, as they were always handled separately in the CSS tests (to check that they are not linked, I assume, by implementation?)
• Directly on resource, with none on container, so that an implementation does not (mistakenly) have resources inherit container permissions when not supposed to (probably really unlikely, but can never know)
• Inherited from container, with none on resource, and always like this for fictive ones since they cannot have permissions on the resource

I can try to explain the reasoning better or in more detail when I know what to explain! 🙂

[edwardsph]

I've been working through your read tests and I have a better understanding of what you have done now, and I think it is great. Your logic on setting the resource or inherited container permissions makes sense to me. I have extracted the javascript functions at the start of each read tests into a common feature which is shared between the other features. I also reworked the access dataset setup to avoid creating invalid policies when the permissions are undefined. This is all looking great so tomorrow I'll try and do the same to the write tests and then see if I can push those changes to this PR.

[edwardsph]

@surilindur I've been very frustrated with how long it has taken me to give your PR the time it deserved. I have finally been through this and have taken your ideas a bit further. I think it is ready to review but I had to create a new PR #95 to add my changes. That PR will replace this one.

[surilindur]

No worries, I have also been really busy with other things. 🙂 Great to hear some of the ideas were of use, at least! I will check that other PR, then. Thank you.