Project

General

Profile

Actions

Feature #19146

closed

Return can_manage and can_write alongside writable_by

Added by Peter Amstutz about 1 month ago. Updated 16 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
06/07/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

To correctly determine whether to display actions for sharing and project freezing in workbench, we need to know if a user has "can_manage" permission.

Proposal:

If the current user has can_manage permission to an object, the response includes a "can_manage: true" boolean field.

In addition, introduce a "can_write" boolean field.

The "writable_by" field is simplified to only include the user uuid if the user can_write, and be empty otherwise. This field will be considered deprecated. Add a configuration option to restore the original writable_by behavior which is default false, with a deprecation note.

In the case of a frozen project that the user owns, we expect the "can_write" field to be false, but the "can_manage" field to me true. Workbench is responsible for checking "can_write" for modification operations and "can_manage" for permission operations.


Subtasks 1 (0 open1 closed)

Task #19156: Review 19146-can-write-manageResolvedPeter Amstutz06/07/2022

Actions

Related issues

Related to Arvados - Feature #18692: Frozen projects workbench supportIn ProgressDaniel KutyƂa05/19/2022

Actions
Related to Arvados Epics - Story #18390: Frozen projectsIn Progress03/01/202207/31/2022

Actions
Related to Arvados - Feature #19194: Return can_manage and can_write for all object types (not just users/groups/projects)New

Actions
Related to Arvados - Feature #19196: Allow API select parameter to add/remove fields from the default setNew

Actions
Related to Arvados - Feature #19197: Optimize permission checks for can_write/can_manage fieldsNew

Actions
Actions #1

Updated by Peter Amstutz about 1 month ago

  • Target version changed from 2022-05-25 sprint to 2022-06-08 sprint
  • Description updated (diff)
Actions #2

Updated by Peter Amstutz about 1 month ago

Actions #3

Updated by Peter Amstutz about 1 month ago

Actions #4

Updated by Tom Clegg about 1 month ago

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.

Actions #5

Updated by Peter Amstutz about 1 month ago

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)

Actions #6

Updated by Peter Amstutz about 1 month ago

  • Subject changed from Return managed_by similar to writable_by to (design) Return managed_by similar to writable_by
Actions #7

Updated by Peter Amstutz about 1 month ago

  • Description updated (diff)
Actions #8

Updated by Tom Clegg about 1 month ago

  • Assigned To set to Tom Clegg
  • Status changed from New to In Progress
  • Subject changed from (design) Return managed_by similar to writable_by to Return can_manage and can_write alongside writable_by
Actions #9

Updated by Tom Clegg 27 days ago

I think it would make sense to deal with the added can_write/can_manage fields for group/user objects first in order to unblock the frozen projects UI, and save these aspects for a follow-up branch/issue:
  • 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")
Actions #10

Updated by Tom Clegg 27 days ago

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

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

Actions #12

Updated by Tom Clegg 25 days ago

  • Target version changed from 2022-06-08 sprint to 2022-06-22 Sprint
Actions #13

Updated by Peter Amstutz 23 days ago

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.

Actions #14

Updated by Tom Clegg 23 days ago

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

Actions #15

Updated by Peter Amstutz 22 days ago

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.

Actions #16

Updated by Tom Clegg 19 days ago

  • Related to Feature #19194: Return can_manage and can_write for all object types (not just users/groups/projects) added
Actions #17

Updated by Tom Clegg 19 days ago

  • Related to Feature #19196: Allow API select parameter to add/remove fields from the default set added
Actions #18

Updated by Tom Clegg 19 days ago

  • Related to Feature #19197: Optimize permission checks for can_write/can_manage fields added
Actions #19

Updated by Tom Clegg 19 days ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF