Feature #14874

[API] Protected collection property -- preserves original project owner through copy/move/modify operations

Added by Tom Clegg 5 months ago. Updated 24 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
06/18/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0
Release relationship:
Auto

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.


Subtasks

Task #15268: Review 14874-protected-collection-properties (arvados repo) & 14874-properties-editor-error-handling (wb2 repo)ResolvedPeter Amstutz


Related issues

Related to Arvados Workbench 2 - Bug #15407: Property keys on collections are getting translated from snake_case to camelCaseNew

Associated revisions

Revision e16542e3
Added by Lucas Di Pentima 27 days ago

Merge branch '14874-protected-collection-properties'
Closes #14874

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision cadfd226
Added by Lucas Di Pentima 27 days ago

Merge branch '14874-post-merge-fixes'
Refs #14874

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Tom Morris 5 months ago

  • Target version set to To Be Groomed

#2 Updated by Tom Clegg 5 months 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)

#3 Updated by Tom Clegg 5 months ago

  • Description updated (diff)

#4 Updated by Tom Clegg 5 months ago

  • Description updated (diff)

#5 Updated by Tom Clegg 5 months ago

  • Description updated (diff)

#6 Updated by Tom Clegg 5 months ago

  • Description updated (diff)

#7 Updated by Tom Clegg 5 months ago

  • Story points set to 2.0

#8 Updated by Tom Morris 4 months ago

  • Target version changed from To Be Groomed to Arvados Future Sprints

#9 Updated by Tom Morris 2 months ago

  • Target version changed from Arvados Future Sprints to 2019-06-05 Sprint

#10 Updated by Tom Morris 2 months ago

  • Assigned To set to Lucas Di Pentima

#11 Updated by Lucas Di Pentima about 2 months ago

  • Status changed from New to In Progress

#12 Updated by Lucas Di Pentima about 2 months ago

  • Target version changed from 2019-06-05 Sprint to 2019-06-19 Sprint

#13 Updated by Ward Vandewege about 1 month ago

  • Release set to 22

#14 Updated by Lucas Di Pentima about 1 month 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?)

#15 Updated by Peter Amstutz about 1 month 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.

#16 Updated by Lucas Di Pentima about 1 month ago

  • Target version changed from 2019-06-19 Sprint to 2019-07-03 Sprint

#17 Updated by Peter Amstutz about 1 month 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.

#18 Updated by Peter Amstutz about 1 month 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.

#19 Updated by Lucas Di Pentima about 1 month ago

Updates at e6eb33290 (arvados repo)
Test run: https://ci.curoverse.com/job/developer-run-tests/1335/

  • Renames Collections.DefaultProperties to Collections.ManagedProperties
  • Adds example admin scripts to documentation.

Pending: wb2 issue fix.

#20 Updated by Lucas Di Pentima about 1 month ago

  • Related to Bug #15407: Property keys on collections are getting translated from snake_case to camelCase added

#21 Updated by Peter Amstutz about 1 month ago

Lucas Di Pentima wrote:

Updates at e6eb33290 (arvados repo)
Test run: https://ci.curoverse.com/job/developer-run-tests/1335/

  • Renames Collections.DefaultProperties to Collections.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.

#22 Updated by Lucas Di Pentima about 1 month ago

Updates at 067bb74eb

  • Simplifies example scripts
  • Adds more descriptive comments on documentation

#23 Updated by Peter Amstutz 28 days 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?

#24 Updated by Peter Amstutz 27 days 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.

#25 Updated by Lucas Di Pentima 27 days ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved

#26 Updated by Lucas Di Pentima 27 days ago

Updates at f0475ef9a - branch 14874-post-merge-fixes
Test run: https://ci.curoverse.com/job/developer-run-tests/1345/

  • Adds Collections.ManagedProperties to Go SDK
  • Whitelists Collections.ManagedProperties.*.* so they can be accessed on the safe config export API.
  • Capitalizes ManagedProperties keys (Value, Protected & Function), with docs updates.

Also available in: Atom PDF