Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify discovery, reading, and writing auxiliary resources #252

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

csarven
Copy link
Member

@csarven csarven commented Mar 16, 2021

This PR introduces minimal changes necessary to clarify discovery, reading and writing of ACL and Description resources (specifically under Auxiliary Resources) - to be consistent with the rest of the Protocol. Further re-organisation of content is left for future PRs.

The PR also address the key concerns raised in PRs #248 and #250 . If accepted and merged, then those two PRs should be closed immediately.


Access controls that are required to read-write ACL resources are specified by the WAC specification. The outer Protocol - in this case the Auxiliary Resources section - must take care to not introduce potential confusion or conflicts.

As it stands, access controls required to read-write Resource Descriptions are specified in the Auxiliary Resource section.


On the removal of line:

                 <p>To discover, read, create, or modify an ACL auxiliary resource, an <code>acl:agent</code> MUST have <code>acl:Control</code> privileges per the <a href="https://github.com/solid/web-access-control-spec#acl-inheritance-algorithm">ACL inheritance algorithm</a> on the resource directly associated with it.</p>

As per ACL Ontology: having Control access on a subject resource allows an agent to Read and Write the ACL resource that's associated with it. The text "read, create, or modify" is redundant and should be avoided.


WAC currently does not specify access controls needed for the discovery of an ACL resource via the subject resource it is associated with (ie. Link rel=acl). However, the discovery of a ACL resource associated with the subject resource is already described in the Auxiliary Resources section, and currently relies on Read rights:

Clients can discover auxiliary resources associated with a subject resource by making an HTTP HEAD or GET request on the target URL, and checking the HTTP Link header with the rel parameter [RFC8288].

It alludes to server including the Link header to allow a client to discover the ACL resource when agent has Read access on the subject resource.

Aside: It is still possible to specify the inclusion of the Link header also part of 4xx responses, but this is left for a future PR.


On the removal of line:

                 <p>A Solid server SHOULD sanity check ACL auxiliary resources upon creation or update to restrict invalid changes, such as by performing shape validation against authorization statements therein.</p>

The WAC spec will cover this in detail.


On modifying the lines for creating, modifying, discovering, reading description resources:

                 <p>To create or modify a description resource, an <code>acl:agent</code> MUST have <code>acl:Write</code> privileges per the <a href="https://github.com/solid/web-access-control-spec#acl-inheritance-algorithm">ACL inheritance algorithm</a> on the resource directly associated with it.</p>
                 <p>To discover or read a description resource, an <code>acl:agent</code> MUST have <code>acl:Read</code> privileges per the <a href="https://github.com/solid/web-access-control-spec#acl-inheritance-algorithm">ACL inheritance algorithm</a> on the resource directly associated with it.</p>

The change simply states that access privileges on description resources come from subject resource's.

For discovery, same reason as earlier when a client can observe the link relation targeting an auxiliary resource. At this time, the commonality is that Read is required on the subject resource for the purpose of discovery.

Copy link
Member

@acoburn acoburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. By removing unnecessary text here, this makes the protocol document more concise and consistent.

Copy link
Member

@bblfish bblfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it omits the problematic text of #250 that is ok for me. I don't have a good enough overview of the whole document to comment on the value of such a large change over and above the smaller change proposed in 250.

protocol.html Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Copy link
Member

@matthieubosquet matthieubosquet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -572,7 +572,7 @@ <h4 property="schema:name">Description Resource</h4>

<p>Servers MUST NOT directly associate more than one description resource to a subject resource.</p>

<p>When a HTTP request targets a description resource, the server MUST apply the authorization policy that is used for the subject resource that the description resource is associated with.</p>
<p>When an HTTP request targets a description resource, the server MUST apply the authorization policy that is used for the subject resource with which the description resource is associated.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we simplify further?
"A subject resource's authorization MUST apply to its description resource."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies that the resource's authorization is the thing taking action.

The imperative is on the server, to apply the same authorization as is on the resource to the description resource(s), and I think we've got just about the simplest, clearest possible wording.

Copy link
Member

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
this is a good clarification.

Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvement, avoids repetition.

Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for looking into this so thoroughly Sarven!

I closed #248 in favour of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants