Feature #14874
closed
[API] Protected collection property -- preserves original project owner through copy/move/modify operations
Description
This feature enables a site to use a collection properties entry for tracking ownership/responsibility for file data found in collections ("who was responsible for uploading these files to Arvados?") even after the collections are modified and copied/moved from one project to another.
The feature will be activated by configuring two new API behaviors.
# In application.yml / cluster configuration file:
collection_properties:
responsible_person_uuid: {function: original_owner, protected: true}
foo_bar: {value: baz, protected: false}
When creating a new collection, if the caller has not provided values for these keys in the properties hash, they are automatically populated with the UUID of the user who owns [the parent of …] the containing project.
When updating a collection, a protected entry in the properties hash cannot be changed by a non-admin user, even the collection's owner. Attempting to do so results in a 403 error.
The default is the empty set (no special behavior).
Supporting this, we should provide some example admin scripts:- List UUIDs/names of collections with no responsible_person_uuid property value
- Update the responsible_person_uuid property from nil to X on all collections in the project hierarchy rooted at P, where P is a user UUID or a group UUID.
- Update the responsible_person_uuid property from X to Y on all collections where it is X.
We should also confirm that Workbench/Workbench2 preserve the properties hash when copying a collection.
Related issues
Updated by Tom Clegg over 5 years ago
- Subject changed from [API] Preserve collection's original owner "data owner" in subsequent copy/move/modify operations to [API] Protected collection property -- preserves original project owner through copy/move/modify operations
- Description updated (diff)
Updated by Tom Morris over 5 years ago
- Target version changed from To Be Groomed to Arvados Future Sprints
Updated by Tom Morris over 5 years ago
- Target version changed from Arvados Future Sprints to 2019-06-05 Sprint
Updated by Lucas Di Pentima over 5 years ago
- Status changed from New to In Progress
Updated by Lucas Di Pentima over 5 years ago
- Target version changed from 2019-06-05 Sprint to 2019-06-19 Sprint
Updated by
Updated by Lucas Di Pentima over 5 years ago
Updates at a981ba700 - branch 14874-protected-collection-properties
Test run: https://ci.curoverse.com/job/developer-run-tests/1321/
- Adds Collections.DefaultProperties config section to arvados config
- Adds behaviors described on this story (default value & original_owner)
- Adds documentation to the Admin section
- Fixes workbench's tag editor to handle protected tags errors
Updates at commit:f6129f70 - branch 14874-properties-editor-error-handling (workbench2's repo):
- Fixes protected error handling on tag editor
- Modifies the way the update data is sent to the API server by only sending the properties field
Pending: Admin example scripts (should those be added as documentation?)
Updated by Peter Amstutz over 5 years ago
Lucas Di Pentima wrote:
Updates at a981ba700 - branch
14874-protected-collection-properties
Test run: https://ci.curoverse.com/job/developer-run-tests/1321/
- Adds Collections.DefaultProperties config section to arvados config
- Adds behaviors described on this story (default value & original_owner)
- Adds documentation to the Admin section
- Fixes workbench's tag editor to handle protected tags errors
Updates at commit:f6129f70 - branch
14874-properties-editor-error-handling(workbench2's repo):
- Fixes protected error handling on tag editor
- Modifies the way the update data is sent to the API server by only sending the properties field
Pending: Admin example scripts (should those be added as documentation?)
"If there is a need to avoid prevent a non-admin user to modify from modifying a specific property"
The documentation should describe updating config.yml (Collections.DefaultProperties) not application.yml (collection_default_properties).
Instead of "Collections.DefaultProperties" maybe it should be "Collections.ManagedProperties" ?
Still need to play with the feature a bit more.
Updated by Lucas Di Pentima over 5 years ago
- Target version changed from 2019-06-19 Sprint to 2019-07-03 Sprint
Updated by Peter Amstutz over 5 years ago
Things that I manually tested that worked:
- I created a collection in workbench2 and it had the expected initial properties set on it
- In Workbench1 I can edit/delete/add non-protected properties, and trying to modify or delete the protected property displays an error.
- I tested copying the collection in both workbench1 and workbench2 and the properties were correctly copied.
Things to fix:
- I think you mentioned this in chat -- something is turning 'responsible_person_uuid' to 'responsiblePersonUuid'. We should fix that.
- In Workbench2, when I try to delete a non-protected property on a collection with a protected property, it will report "Protected property cannot be updated" for the protected property. I get the same error when I try to add a tag.
Updated by Peter Amstutz over 5 years ago
From chat: we tracked down the use of mapKeys on responses from the API server. The method applies recursively, and has the effect of corrupting the keys of any field containing a JSON object such as properties, mounts, etc containing punctuation such as "_" or "/".
That explains why wb2 reports 'Protected property cannot be updated: responsible_person_uuid' because it was mangled into 'responsiblePersonUuid' so when it attempts to write it back, the protected field is indeed no longer there.
This method should not apply recursively.
Updated by Lucas Di Pentima over 5 years ago
Updates at e6eb33290 (arvados repo)
Test run: https://ci.curoverse.com/job/developer-run-tests/1335/
- Renames
Collections.DefaultPropertiestoCollections.ManagedProperties - Adds example admin scripts to documentation.
Pending: wb2 issue fix.
Updated by Lucas Di Pentima over 5 years ago
- Related to Bug #15407: [WB2] Property keys on collections are getting translated from snake_case to camelCase added
Updated by Peter Amstutz over 5 years ago
Lucas Di Pentima wrote:
Updates at e6eb33290 (arvados repo)
Test run: https://ci.curoverse.com/job/developer-run-tests/1335/
- Renames
Collections.DefaultPropertiestoCollections.ManagedProperties- Adds example admin scripts to documentation.
These look fine. You could simplify them a bit using arvados.util.list_all().
I am generally reluctant to add code to the documentation where the user is expected to directly copy and paste, it is an awkward category that is more than a small example but less than a fully supported admin tool. I don't know if we will have met the customer need until they actually have a chance to try it.
LGTM.
Updated by Lucas Di Pentima over 5 years ago
Updates at 067bb74eb
- Simplifies example scripts
- Adds more descriptive comments on documentation
Updated by Peter Amstutz over 5 years ago
Lucas Di Pentima wrote:
Updates at 067bb74eb
- Simplifies example scripts
- Adds more descriptive comments on documentation
I know we're trying to wrap this story up but it just occurred to me that new API call(s) "apply default properties" and/or "reset default properties" might be a better approach than reimplementing the "root owner of project" logic in Python. What do you think?
Updated by Peter Amstutz over 5 years ago
Peter Amstutz wrote:
Lucas Di Pentima wrote:
Updates at 067bb74eb
- Simplifies example scripts
- Adds more descriptive comments on documentation
I know we're trying to wrap this story up but it just occurred to me that new API call(s) "apply default properties" and/or "reset default properties" might be a better approach than reimplementing the "root owner of project" logic in Python. What do you think?
From discussion in chat, we don't want to add new API design to this ticket, so this LGTM.
Updated by Lucas Di Pentima over 5 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|e16542e3cb4fddf05c407cb013c9e1573eb9c289.
Updated by Lucas Di Pentima over 5 years ago
Updates at f0475ef9a - branch 14874-post-merge-fixes
Test run: https://ci.curoverse.com/job/developer-run-tests/1345/
- Adds
Collections.ManagedPropertiesto Go SDK - Whitelists
Collections.ManagedProperties.*.*so they can be accessed on the safe config export API. - Capitalizes
ManagedPropertieskeys (Value,Protected&Function), with docs updates.