search

solid / web-access-control-spec

Web Access Control (WAC)
https://solid.github.io/web-access-control-spec/
MIT License
121 stars 25 forks source link

Describe current ACL-doc-search behaviour of NSS #70

Closed michielbdejong closed 3 years ago

michielbdejong commented 5 years ago

This PR aims to update the spec to clarify the current behaviour of node-solid-server and inrupt pod-server.

Fixes part of #63.

There is a follow-up issue at https://github.com/solid/specification/issues/55 which discusses whether we want to change this behaviour in the 1.0 spec.

michielbdejong commented 5 years ago

cc @acoburn

michielbdejong commented 5 years ago

See https://github.com/solid/node-solid-server/blob/master/lib/acl-checker.js#L111 for the line where effectively NSS decides to use a given ACL or move on to the next one. The only condition it seems to be checking is whether the ACL doc exists at all.

michielbdejong commented 5 years ago

There are multiple WAC implementations (besides NSS) which make use of this specification

Can you list them? AFAIK none of them are used to run Solid pod servers in production, right?

acoburn commented 5 years ago

Trellis Fedora Potentially others

The other thing is the acl-check library does check for acl:default: https://github.com/solid/acl-check/blob/master/src/acl-check.js#L114-L123

Are you absolutely certain that NSS does not apply these rules?

Otto-AA commented 5 years ago

Fixes #63.

It is not fixing this issue, only the part brought up by @acoburn . The original question there remains.

megoth commented 5 years ago

Honestly, I find the WAC-spec quite confusing at times (I worked on both server-side via acl-check and client-side via sharingPane).

The current NSS implementation looks for ACL-files and stops at the first ACL-resource it finds (the section that @michielbdejong refers to).

Lately I do think this implementation is wrong though: It should look for ACL-resources until it finds permissions to inherit (per the section on ACL Inheritance Algorithm). So if the first ACL-resource does not contain any triples with acl:default, I interpret that it should continue looking.

Example: Given the following structure:

|- /
 |- /.acl (contains triples with `acl:default`)
 |- /container/
  |- /container/.acl (contains no triples with `acl:default`)
  |- /container/resource

To find the corresponding acl:default triple, the algorithm say that you should traverse the parents until you get /.acl, and not stop at /container/.acl. When passing the store to the acl-check methods (that @acoburn referred to) I interpret it further that it contains all triples in /container/.acl plus the triples related to acl:default from /.acl (excluding the rest).

Again, this is how I understand it today, and with that I realize that Kjetil and I probably implemented this wrong in NSS earlier.

RubenVerborgh commented 5 years ago

Lately I do think this implementation is wrong though: It should look for ACL-resources until it finds permissions to inherit (per the section on ACL Inheritance Algorithm).

While I agree that this is indeed what that written version says, we do not know at the moment whether this version corresponds to the intended behavior. That is, it might be an inaccurate description of the process we had, or the process we had might not be technically the best solution.

The reason we are speccing this at much more detail, is exactly to have these kinds of discussions.

So in my opinion, the question is not what implementations say or what documents do, but rather what the wisest choice regarding behavior is.

Concretely, I find the "until it finds permissions to inherit" behavior dangerous. Not only is it underspecified as currently written down, even when it is, it would mean that a simple typo or mis-serialization (i.e., butchered URL) could have disastrous results. The behavior I endorse is that, when during traversal an ACL file is encountered, it MUST be respected. If that ACL file contains no applicable rule, or happens to be syntactically invalid, then no one has permissions until the ACL file is fixed.

kjetilk commented 5 years ago

Yeah, not sure if we did it correctly, but we did have some discussion with @timbl on this, didn't we @megoth? So, I tend to think that we did capture the intention here...

RubenVerborgh commented 5 years ago

Even then, we should critically examine the consequences of either choice.

dmitrizagidulin commented 5 years ago

The current NSS implementation looks for ACL-files and stops at the first ACL-resource it finds

Fwiw, I can confirm that this was the original intention of the WAC spec (and NSS implementation). Specifically not continue looking until it finds an inherit directive.

But I agree with @RubenVerborgh that:

the question is not what implementations say or what documents do, but rather what the wisest choice regarding behavior is.

And in that regard, I second the unease with the proposed "until it finds permissions to inherit" change.