Add Valkyrie-native permission handling #3938
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
no-reply
force-pushed
the
permissions
branch
3 times, most recently
from
3ec2129
to
ee34081
Compare
no-reply
changed the title
no-reply
force-pushed
the
permissions
branch
from
d53db92
to
6bd0985
Compare
tpendragon
reviewed
Sep 11, 2019
| # metadata. This approach also allows policies to be persisted using a | ||
| # different database or adapter than is used for curatorial objects. | ||
| class Permission < Valkyrie::Resource | ||
| include Dry::Equalizer(:access_to, :agent, :mode) |
There was a problem hiding this comment.
This is neat, didn't know about this.
There was a problem hiding this comment.
pretty cool, right!?
mcritchlow
reviewed
Sep 11, 2019
| attrs[:updated_at] = modified_date | ||
|
|
||
| attrs[:permissions] = permissions.map do |permission| | ||
| agent = permission.type == 'group' ? "group/#{permission.agent_name}" : permission.agent_name |
There was a problem hiding this comment.
Would it make sense to consider having the group/ prefix be a constant? The usage seems pretty much isolated to private methods in PermissionManager and PermissionValue#result. Just a thought.
There was a problem hiding this comment.
yeah, i think it would be a good idea. i didn't pursue it during development because it wasn't clear to me where that constant should be namespaced.
i think it's still not clear, but this is all static enough now that we could probably just pick a place and have it be fine. ideas?
There was a problem hiding this comment.
I wasn't sure either.. I'm tempted to say Hyrax::Group::PREFIX, or something along those lines. At least to me that reads fairly directly and seems in context. That's my gut reaction, I suppose.
There was a problem hiding this comment.
oh, that seems pretty good. i'll go with that.
no-reply
force-pushed
the
permissions
branch
3 times, most recently
from
5c2fdd9
to
4f09b08
Compare
elrayle
previously approved these changes
Sep 16, 2019
added 13 commits
September 16, 2019 15:09
The persister spec has a hard expectation on a specific array, but the test really doesn't care about enumerable format or order. Make this more tolerant by using the `contain_exactly` matcher.
Some of these tests are order dependent, and fail on repeated runs of just the ModelTransformer tests. Cleaning up the repository ensures they work as expected regardless of test order.
If we want to check what object a permission is applied to, there's now a matcher for that. `expect(permission).to grant_permission(:read).on(object_id).to(agent.id)`
This is the beginning of a larger effort to establish Valkyrie-native permission management/ACLs.
Adds support for editing ACLs for a given resource using Valkyrie interfaces.
Adds a simple DSL for adding and removing permissions by mode/user pairs. This reduces the overhead/possibility for error in manually creating ACL policies by directly managing `Hyrax::Permission` objects.
This model wraps `Permission` data for an individual object. For many Valkyrie adapters, this will make it possible for grouped permissions to be saved as a single unit.
The previous approach for saving valkyrie objects with FactoryBot's `#to_create` would save them fine, but then returned an unsaved object instance (i.e. one without an `#id` or create/modified dates). Using a dedicated strategy gives us a bit more control over what happens when we save a valkyrie object. This approach can also be extended to better support "associations" and similar for Valkyrie resources. See: https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#custom-strategies
Ensure `#access_to` persists, and is saved on ActiveFedora/Hydra Access Controls `Permission` objects when saved from native `valkyrie` using Wings.
Given a resource, we want to be able to find an AccessControl object for it. For a typical valkyrie adapter, this is an inverse reference lookup on the `AccessControl#access_to` property. An initial implementation does a broad reverse lookup on `#access_to`, then filters the result classes. It assumes the first `AccessControl` found is the correct one. For Wings, the story is a bit different. `Hydra::AccessControl` resources don't know what their target is. When rebuilding valkyrie resources from `Hydra::AccessControl`/AF objects, we inspect the permissions to assign `#access_to`, but we can't use this for lookup. Instead, we can rely on the `ActiveFedora`/`Hydra::AccessControls` pattern of looking up the `Hydra::AccessControl` object from its target (via its `belongs_to` reference).
Integrate `Hyrax::AccessControl` into the `AccessControlList` service. This service now saves an `AccessControl` when its save method is called. Rather than tracking changes manually in the `AccessControlList` service, we can now use `Valkyrie::ChangeSet` to track them directly against an `Hyrax::AccessControl` object.
added 7 commits
September 16, 2019 15:09
Allow a `Hyrax::Resource` to cache a `PermissionManager` (and therefore an ACL `ChangeSet`). Delegate current `#read_groups=` and similar management to it, and also use it in `VisibilityReader` and `VisibilityWriter`. This allows each Resource to maintain its own current permission state across the various access lenses (visibility, `*_users/*groups`, and permissions) in a single place. When we convert from an `ActiveFedora` object, we ensure that the current permissions are copied down to the Resource. Notably we don't do this in the other direction; our implementation of permissions in Valkyrie treats them as separate resources, pointing back to repository Objects. Changing an ACL, and then saving the object it points to has no effect: you need to save the ACL explictly.
`Enumerator#+` was added in Ruby 2.6, so `+=` won't work in Ruby 2.5.
This attribute should have a type that indicates that the members of the set should all be Permission objects.
This avoids relying on hard coded strings to understand group names as handled by Valkyrie adapters.
no-reply
force-pushed
the
permissions
branch
from
4f09b08
to
c83462b
Compare
Valkyrie 2.0.0 treats more values as frozen, so we can't do `#map!` in permission arrays. Instead of converting inline, return a new permission array when converting permission values.
no-reply
force-pushed
the
permissions
branch
from
e69ee69
to
4669b46
Compare
elrayle
approved these changes
Sep 17, 2019
|
👍 |
|
👍 |
|
awesome work 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
valkyrie-native approach for permission handling.Valkyrie::Resource::AccessControlsis insufficient, since Hyrax previously had support for arbitrary access modes (especially:Hydra::AccessControlsheavily used:discoverwhich isn't present inValkyrie::Resource::AccessControls). The need to round-trip arbitrary access list entries motivated this re-implementation, as a per-requisite to #3584.In the Valkyrie native implementation Hyrax permissions are managed via Access Control List style permissions. Legacy Hyrax models powered by
ActiveFedoralinked the ACLs from the repository object itself (as anacl:accessControllink to a container). Valkyrie models jettison that approach in favor of relying on links back from the permissions usingaccess_to. As was the case in the past implementation, we include an object to represent the access list itself (Hyrax::AccessControl). This object's#access_tois the way Hyrax discovers list entries--it MUST match between theAccessControland its individualPermissions.The effect of this change is that our
AccessControlobjects are detached fromHyrax::Resourcethey can (and usually should) be edited and persisted independently from the resource itself.Some utilitiy methods are provided for ergonomics in transitioning from
ActiveFedora: the#visibilityaccessor, and the#*_usersand#*_groupaccessors. The main purpose of these is to provide a cached ACL attached to a given Resource instance. However, these will likely be deprecated in the future, and it's advisable to avoid them in favor ofHyrax::AccessControlList,Hyrax::PermissionManagerand/orHyrax::VisibilityWriter(which provide their underlying implementations).In support of this work, three explicitly different projections on the base-truth ACLs are implemented:
visibilitysupports the generic "public", "institutional", and "restricted" access groups available in the UI.*_users/*_groupssupports setting mode/agent pairs with a simple DSL viaHyrax::PermissionManagerHyrax::AccessControlListprovides a DSL for managing ACL entries directly.The relationship between these three approaches is designed to have a clear dependency chain down to the base
Permissiondata.@samvera/hyrax-code-reviewers