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

Write agent-specific ACL rules to a given ACL LitDataset #191

Merged
merged 5 commits into from Jun 29, 2020

Conversation

Vinnl
Copy link
Contributor

@Vinnl Vinnl commented Jun 23, 2020

New feature description

Given an ACL LitDataset, this PR adds setAgentResourceAccessModes() and setAgentDefaultAccessModes() that take an object with access modes (Read, Append, Write and/or Control), and make sure that the given modes are granted for the agent as resource or default access, respectively, and that any existing agent-specific modes that are not given are revoked.

Note that the approach is rather convoluted, though I'm not sure if there's a better way. Specifically, what these functions do:

  1. For every existing Access Rule that applies to the given Agent and the given Resource, remove the Agent from that Rule, and create a new Rule that duplicates all existing ACL-related Quads, except for the ones that make the Rule apply to the given Resource. In the common case, this should result in a new Rule that does not have any effect - see step 3.
  2. Add a new Access Rule that sets the passed Access Modes for the given Agent and the given Resource. (For either Resource access or default access, depending on the function.)
  3. Remove all Things that contain only ACL-related Quads, but that do not have any effect, e.g. because no Resource to which it applies is specified, or because it doesn't specify who is granted access.

It's somewhat convoluted but, assuming that the clean-up function is accurate, the only way that feels safe to me. Most of that work should also be re-usable when adding support for setting e.g. public or group access, in the future, which will run into the same concerns.

Checklist

  • All acceptance criteria are met.
  • Relevant documentation, if any, has been written/updated. -> Tutorial will be added when you can also save the updated ACL back to the Pod.
  • The changelog has been updated, if applicable.
  • New functions/types have been exported in index.ts, if applicable.
  • Commits in this PR are minimal and have descriptive commit messages.

@Vinnl Vinnl requested review from pmcb55 and NSeydoux June 23, 2020 13:49
@Vinnl Vinnl self-assigned this Jun 23, 2020
Copy link
Contributor

@NSeydoux NSeydoux left a comment

Choose a reason for hiding this comment

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

This implements all the features in the acceptance criteria, there are just a couple of open questions that e might want to consider, as they will come back when we extend these features to groups for instance.

src/acl.test.ts Outdated Show resolved Hide resolved
src/acl.ts Show resolved Hide resolved
src/acl.ts Show resolved Hide resolved
src/acl/agent.ts Show resolved Hide resolved
src/acl/agent.test.ts Show resolved Hide resolved
src/acl/agent.test.ts Show resolved Hide resolved
src/acl/agent.test.ts Show resolved Hide resolved
src/acl/agent.ts Show resolved Hide resolved
@NSeydoux
Copy link
Contributor

This approach expects the developer to specify the ACL document in which the access right should be added. How complex would it be to only require the developer to specify the target resource, webid and desired access modes, to discover the appropriate ACL document automatically, and to modify the access rights without introducing side-effects ? The present functions could still be exposed for optimizations, when one already has the link to the target ACL document, but a function of a higher abstraction that does the ACL discovery under the cover could be exposed for convenience.

@Vinnl
Copy link
Contributor Author

Vinnl commented Jun 24, 2020

@NSeydoux It wouldn't be too complex, except that it involves choices that we can't make for the developer. Specifically, what do we do if a Resource does not have its own ACL? Do we create it for them, do we modify the Container's Resource, do we throw an error? (And if the latter, then the developer will still be forced to write code to discover and modify the right ACL...)

@NSeydoux
Copy link
Contributor

NSeydoux commented Jun 24, 2020

Is it the developer's choice to make in the first place though ? As long as the obtained result is spec-compliant and doesn't have side-effects, should clients care where the ACLs are stored ?

As you said in an earlier comment, it is likely that most resources have their own ACL document, so it would be technically in-spec, when setting resource-scpecific access for an agent, to

  • If the resource has its own ACL document, update this document with the new modes for the specified agent
  • If the resource does not have its own ACL document, create it, populate it with the current ACL rules that apply to the resource by using the inheritance algorithm, and then update it with the new modes for the specified agent OR find the ACL document that applies through the inheritance mechanism, and update this one with the new resource-scoped access (provided that resource-scoped ACL rules can be specified in any ACL document for any resource, and that you have Control access to the parent container).

This is out of scope for the current PR, because it is a complex feature and we would have to discuss the implications of implementing it, but I'd be interested to know if this is something that we might eventually want to expose, or if it is of a level of abstraction that requires too many assumptions on the desirable behaviour.

@Vinnl
Copy link
Contributor Author

Vinnl commented Jun 24, 2020

Yep, definitely something to think about, especially after we've seen what the Pod Manager team is going to do here. I've already added a ticket to at least copy over existing applicable rules, because I'm expecting at least that to be desired. This potential feature could build on that.

@Vinnl Vinnl merged commit 103e539 into master Jun 29, 2020
@Vinnl Vinnl deleted the feat/972-write-agent-acl branch June 29, 2020 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants