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

Clarify which resource requires permissions for creation #95

Closed RubenVerborgh closed 3 years ago

RubenVerborgh commented 3 years ago

Following https://github.com/solid/web-access-control-tests/issues/41#issuecomment-898295543, I'm hereby making a suggestion to clarify the text.

When editing, however, I also stumbled upon another question:

the server MUST match an Authorization allowing the acl:Append or acl:Write access privilege on the container

Should we, in addition to—or instead of—mentioning those two terms, add a link to which one is appropriate? Or is are literally any of those appropriate?

I think that we mean to say:

Replaces https://github.com/solid/specification/pull/297

michielbdejong commented 3 years ago

There are two cases when creating c/r:

Whether those permissions are granted directly through accessTo or inherited from a parent's default modes, doesn't matter.

A common pitfall among implementers is to only check permission when creating the new resource itself, but not to check any permission when creating the containment triple(s).

This also applies to containment triples created further towards the root, in case of mkdir -p behaviour.

RubenVerborgh commented 3 years ago

Ugh, things are much more complex, as https://github.com/solid/web-access-control-tests/issues/41#issuecomment-898959493 explains.

@csarven I'm afraid we will need an overhaul of that entire section. The text is not correct all.

Take the current version:

When an operation requests to create a resource as a member of a container resource, the server MUST match an Authorization allowing the acl:Append or acl:Write access privilege on the container identified via acl:default.

That is not true. What this statement says is:

but that is not what happens.

What we need is:

When an operation requests to create a resource as a member of a container resource, the server MUST deny access unless it matches…

And the remainder of that paragraph needs to be completed with whatever text from https://github.com/solid/web-access-control-tests/issues/41#issuecomment-898959493 is correct.

csarven commented 3 years ago

As mentioned in that test issue comment, the data (AFAIK)/spec is correct.

What we need is:

Matching is explained in several places in the spec. The requirement is strictly about what's needed to allow access.


The current spec:

When an operation requests to create a resource as a member of a container resource, the server MUST match an Authorization allowing the acl:Append or acl:Write access privilege on the container for new members.

Whether default or accessTo (or both) are required on container's ACL resource (which would have the container URI as value) depends on the HTTP request.

HTTP request method indicates the purpose of the operation against the request target. PUT's request target is the resource to be created in the container, e.g., PUT /foo. Whereas for POST, it'd be POST /.

As the HTTP PUT method requests to create or replace the resource state, the acl:Write access mode would be required.

That requires:

Append (or Write) on C/ and Write on C/R.


I suggest to close this PR.

RubenVerborgh commented 3 years ago

Matching is explained in several places in the spec.

That's fine; what I'm saying is that this text is incorrect:

When an operation requests to create a resource as a member of a container resource, the server MUST match an Authorization allowing the acl:Append or acl:Write access privilege on the container identified via acl:default.

As written, it means that a server is not spec-compliant if it cannot find a match. The text literally says that the server MUST match; hence, if it does not, it is not compliant.

I know what the text is supposed to mean, it's just not what is written. Or am I mistaken?

Plus, the way the text is written, cumulative effects are unclear. Is this a necessary or sufficient condition? One cannot tell from the way it is written. In my phrasing ("the server MUST deny"), it is clear that it is a necessary condition, but not sufficient.


I suggest to close this PR.

The language suggestion stands regardless of the above discussion; "for new members" is unclear to me.

csarven commented 3 years ago

That's fine; what I'm saying is that this text is incorrect:

Right, but that was the proposal you made. It is not in the spec.

"for new members" is unclear to me.

Ack. The following from the current PR is about the same to me - a bit redundant:

(as needed by the operation) on the resource to be created.

But we can go with that. Thanks!

RubenVerborgh commented 3 years ago

Thanks.

It is not in the spec.

It is… will create new issues/PRs to follow up.

csarven commented 3 years ago

It is… will create new issues/PRs to follow up.

That's a contradictory statement.

The initial commit of the PR included the text:

identified via acl:default

is obviously not in the spec. You stated that what you wrote is incorrect.

RubenVerborgh commented 3 years ago

@csarven There were multiple incorrect parts beyond the one I proposed; my comment was explicitly about the other parts. Opened #96 to address that.

csarven commented 3 years ago

All I'm saying is that you quoted your own proposal as incorrect, not the one that's actually in the spec. Conformance was already explained.