Arvados

According to the code comments,

  # If current user can manage the object, return an array of uuids of                                                                                       
  # users and groups that have permission to write the object. The                                                                                           
  # first two elements are always [self.owner_uuid, current user's                                                                                           
  # uuid].                                                                                                                                                   
  #                                                                                                                                                          
  # If current user can write but not manage the object, return                                                                                              
  # [self.owner_uuid, current user's uuid].                                                                                                                  
  #                                                                                                                                                          
  # If current user cannot write this object, just return                                                                                                    
  # [self.owner_uuid].                                                                                                                                       
  def writable_by

In the "current user can manage" case, the list of writers returned is incomplete: it only reflects direct permission links with tail_uuid = this object. Users/groups that have permission via this object's parent/ancestor are not included.

Looking up these permission links for each group/project returned in each get/list request seems wasteful. (Does anything rely on this?)

Rather than replicate this (seemingly incomplete and inefficient) behavior at the "manage" level, perhaps it would be better to add two boolean fields that give the specific information needed for UI: "can_write" and "can_manage".

Ideally we can migrate callers to use can_write/can_manage instead of writable_by, or at least have the caller select writable_by explicitly when needed.

Having the can_write/can_manage fields un-selected by default might also be worthwhile.

To be maintain backwards compatibility, we might want to continue returning "writable_by" but maybe it only returns your uuid (or not). Which I believe is currently how it's been working if you have write permission but not manage permission. We should review how it is currently used by both workbenches. The most difficult question is if there are other end user integrations that rely on it.

We could add a boolean "can_write" and have a configuration option that enables/disables returning legacy "writable_by".

Having a plain boolean "can_manage" makes sense.

Adding an extra parameter to return permission info could work. Although to avoid a proliferation of special parameters, a thought I just had is to extend our "select" syntax to be able to add or remove fields from the default fields instead of having to be explicit:

["+can_write"] (default fields, plus can_write)

["-mounts"] (default fields, but exclude mounts)

• return can_write/can_manage for all objects, not just groups and users
• optimize lookups so we don't need N permission queries to return a list of N objects
• alternate select syntax ("+attr", "-attr")

19146-can-write-manage @ 74323ae3de455071de4fce0c2e2ee79a5650a040 -- developer-run-tests: #3165

todo: test list-groups, get-user, list-users responses

19146-can-write-manage @ 9551b59d3aab67f77240b90bbb550faec6b2a7d9 -- developer-run-tests: #3167

Tom Clegg wrote:

19146-can-write-manage @ 9551b59d3aab67f77240b90bbb550faec6b2a7d9 -- developer-run-tests: #3167

Instead of incorporating a frozen project check into can_write, could we rebase on main, which has 19145-admin-write-frozen merged, and only use User.can? method? This seems like the sort of thing where DRY applies and as much as possible, permission checks should funnel through one method so we don't end up with different methods with different behavior.

If there's a desire to avoid a database query in the direct owner / admin cases, that optimization can be moved into the can? method if it isn't there already.

Added the "skip some db queries when target itself is frozen" optimization to User.can?(), added a comment explaining the additional frozen check in can_write, and deleted the unnecessary special cases from the can_write/can_manage methods.

19146-can-write-manage @ 2e727c5d2d000faa6f1d9a566dc59568f1b276fe -- developer-run-tests: #3171

Tom Clegg wrote:

Added the "skip some db queries when target itself is frozen" optimization to User.can?(), added a comment explaining the additional frozen check in can_write, and deleted the unnecessary special cases from the can_write/can_manage methods.

19146-can-write-manage @ 2e727c5d2d000faa6f1d9a566dc59568f1b276fe -- developer-run-tests: #3171

Thanks, this is certainly cleaner. LGTM.