Bug #18865

Regular users cannot read, update or delete permission links pointing to objects they can_manage that are not users or groups

Added by Peter Amstutz 2 months ago. Updated about 2 months ago.

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

100%

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

Description

The fix in #18164 to make permission links correctly visible through the "links" API was incomplete.

It assumes that the head_uuid is always in the permission table.

This is correct for users and groups, which are listed exhaustively in the permission table.

If you own another type of object (like a collection) and create a permission link to it, under the current query logic you will not be able to see the permission link. That is because no explicit row exists for the owner group to the collection, the can_manage permission is implied.

The query logic needs to somehow incorporate a check for can_manage on the owner_uuid where target_uuid is not a user or group.

(This is annoying because it's probably going to require another table join).

Start by writing a test case.


Subtasks

Task #18875: Review 18865-collection-permission-linksResolvedPeter Amstutz

Associated revisions

Revision 3c73e233
Added by Tom Clegg about 2 months ago

Merge branch '18865-collection-permission-links'

fixes #18865

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision f2861b55 (diff)
Added by Tom Clegg about 2 months ago

18865: Cherry-pick '18865-collection-permission-links' merge

3c73e2337ed73cd44e9bcc2d38a4dd742637ad19 onto 2.4-release

refs #18865

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Peter Amstutz 2 months ago

  • Category deleted (API)
  • Description updated (diff)
  • Project changed from Arvados Private to Arvados

#2 Updated by Peter Amstutz 2 months ago

  • Target version changed from 2022-03-16 sprint to 2022-03-30 Sprint

#3 Updated by Peter Amstutz 2 months ago

  • Description updated (diff)

#4 Updated by Peter Amstutz 2 months ago

  • Release set to 46

#5 Updated by Peter Amstutz 2 months ago

  • Assigned To set to Tom Clegg

#6 Updated by Tom Clegg 2 months ago

  • Status changed from New to In Progress
  • Category set to API

18865-collection-permission-links @ fca6909a08d0140b99be11b872d0b519f2ae7f59 -- developer-run-tests: #2981

I tried writing a test for this, but it already passes.
  • admin creates a collection and shares it with a regular user using a direct link (tail=user, head=collection, can_read)
  • user cannot see the permission link
  • admin shares the collection using a direct link (tail=user, head=collection, can_manage)
  • user can see both permission links via arvados/v1/permissions and arvados/v1/links endpoints

Did I misinterpret the problem, or mess up the test? Or do we already have the desired behavior?

#7 Updated by Peter Amstutz 2 months ago

Try this:

  1. create a project
  2. create a collection inside the project
  3. grant can_manage on the project to user A
  4. user A creates a permission link from user B to the collection
  5. user A lists permission links with tail_uuid=collection

What should happen:

because user A has can_manage permission on the collection, user A sees the permission link from user B to the collection.

What actually happens (according to bug reports / my testing):

user A sees nothing, because the query on materialized_permissions to determine which permission links the user is allowed to see does not take into account transitive permissions.

#8 Updated by Tom Clegg 2 months ago

Aha. Yes, with that setup, I have a failing test. I figured I might as well introduce an intermediate subproject to make it a little more generalized, so we have

                        admin(user)
active ---can_manage--> ╰project
                         ╰subproject
 spectator ---can_read--> ╰collection

...and check that "active" can see the "can_read" link via both "permissions" and "links" APIs.

18865-collection-permission-links @ 697f6fcaaf91f6d1acb1927efd5465a52b8829e5

#9 Updated by Peter Amstutz about 2 months ago

Tom Clegg wrote:

Aha. Yes, with that setup, I have a failing test. I figured I might as well introduce an intermediate subproject to make it a little more generalized, so we have

[...]

...and check that "active" can see the "can_read" link via both "permissions" and "links" APIs.

18865-collection-permission-links @ 697f6fcaaf91f6d1acb1927efd5465a52b8829e5

The test should include any other record types that are intended to support direct permission links. Workflow records maybe, anything else?

#10 Updated by Tom Clegg about 2 months ago

I came up with three possible approaches:
  1. Extend the generic "readable_by" permission query to join/subquery all of the relevant tables (collections, container_requests, workflows) in order to get each link target's owner_uuid and look that up in the materialized view (this approach seems difficult and prone to performance catastrophes)
  2. Copy/sync all single-object permission links into the materialized view from now on, so the existing readable_by query would find them (this seems daunting too, although it does have the advantage that the generic permission filter could be simplified a bit)
  3. Modify the "get permission links for specified object", "get link by uuid", and "list links filtered by head and/or tail" APIs to skip the generic readable_by query entirely. Instead, check manage permission on the specified target (or target of specified link, in the "get link by uuid" case), and depending on the outcome, either return all the links, or none

One shortcoming of the option 3 is that "list all permission links that I can see" still doesn't return these hard-to-find links in question. But I'm not sure anyone will ever care -- realistically the caller knows which target object(s) they are interested in.

So, for the sake of expediency and simplicity/maintainability I went ahead with option 3. If the above shortcoming is a problem, I think option 2 would be my next choice.

Even "get all permission links that I can see just by virtue of the tail_uuid being my own user uuid" might not be especially useful, but it's pretty easy to implement, so I did.

The code doesn't refer to any particular types but as requested I made the test cover multiple object types (collections, workflows, container_requests) to be sure.

18865-collection-permission-links @ d1b1a36ca2c71c7bc60f603d43755ba828af3666 -- developer-run-tests: #2992
retry wb1 developer-run-tests-apps-workbench-integration: #3194

#11 Updated by Peter Amstutz about 2 months ago

  • Target version changed from 2022-03-30 Sprint to 2022-04-13 Sprint

#12 Updated by Peter Amstutz about 2 months ago

I came up with three possible approaches:

1. Extend the generic "readable_by" permission query to join/subquery all of the relevant tables (collections, container_requests, workflows) in order to get each link target's owner_uuid and look that up in the materialized view (this approach seems difficult and prone to performance catastrophes)

I had a similar thought, this is "most correct" but the query would be kind of a disaster.

2. Copy/sync all single-object permission links into the materialized view from now on, so the existing readable_by query would find them (this seems daunting too, although it does have the advantage that the generic permission filter could be simplified a bit)

The permission table can already have millions of rows (it's roughly the cross product of all users and all groups), and this would make it two to four orders of magnitude larger, so, no.

3. Modify the "get permission links for specified object", "get link by uuid", and "list links filtered by head and/or tail" APIs to skip the generic readable_by query entirely. Instead, check manage permission on the specified target (or target of specified link, in the "get link by uuid" case), and depending on the outcome, either return all the links, or none

This sounds like a good compromise.

1.

   def find_object_by_uuid
+    if params[:id] && params[:id].match(/\D/)
+      params[:uuid] = params.delete :id
+    end
    if action_name == 'get_permissions'

What's going on here?

2.

      if @object && @object.link_class != 'permission'
        # Throw this out and re-fetch using generic permission query
        @object = nil

I think you meant "generic link query" in the comment, right?

3.

      # Instead, we look up the link
      # by UUID, then check whether (a) its tail_uuid is the current
      # user or (b) its head_uuid is an object the current_user
      # can_manage.
...
     elsif @object &&
         current_user.uuid != @object.tail_uuid &&
         !current_user.can?(manage: @object.head_uuid)
        @object = nil
      end

The comment describes the exact opposite boolean expression to the one written in code. (I understand what it's doing and it is correct, but if I didn't understand what you were trying to do it would appear to be a bug).

4.

            k[2].select! do |head_uuid|
              current_user.can?(manage: head_uuid)
            end

Maybe include a comment that we're doing a destructive in-place modification on the 3rd parameter of the filter clause (it's pretty subtle).

Rest LGTM.

#13 Updated by Tom Clegg about 2 months ago

Peter Amstutz wrote:

   def find_object_by_uuid
+    if params[:id] && params[:id].match(/\D/)
+      params[:uuid] = params.delete :id
+    end
    if action_name == 'get_permissions'

What's going on here?

This is the same code we use in ApplicationController.find_object_by_uuid so the rest of the code can use params[:uuid] without paying attention to whether it came through a generic Rails route like "/arvados/v1/links/:id" or through a custom route like "get /permissions/:uuid" or ...?uuid=foo etc.

I think you meant "generic link query" in the comment, right?

I meant the generic SQL query we use to filter by permission. Updated comment to "generic permission-filtering query."

The comment describes the exact opposite boolean expression to the one written in code. (I understand what it's doing and it is correct, but if I didn't understand what you were trying to do it would appear to be a bug).

Rearranged this and added comments to (hopefully) make it less confusing.

Maybe include a comment that we're doing a destructive in-place modification on the 3rd parameter of the filter clause (it's pretty subtle).

Added.

Merged main.

18865-collection-permission-links @ 3961f5bd4349fca6769fd4263d2f7fbe54e8bcaa -- developer-run-tests: #3020

#14 Updated by Peter Amstutz about 2 months ago

Tom Clegg wrote:

Peter Amstutz wrote:

[...]

What's going on here?

This is the same code we use in ApplicationController.find_object_by_uuid so the rest of the code can use params[:uuid] without paying attention to whether it came through a generic Rails route like "/arvados/v1/links/:id" or through a custom route like "get /permissions/:uuid" or ...?uuid=foo etc.

I think you meant "generic link query" in the comment, right?

I meant the generic SQL query we use to filter by permission. Updated comment to "generic permission-filtering query."

The comment describes the exact opposite boolean expression to the one written in code. (I understand what it's doing and it is correct, but if I didn't understand what you were trying to do it would appear to be a bug).

Rearranged this and added comments to (hopefully) make it less confusing.

Maybe include a comment that we're doing a destructive in-place modification on the 3rd parameter of the filter clause (it's pretty subtle).

Added.

Merged main.

18865-collection-permission-links @ 3961f5bd4349fca6769fd4263d2f7fbe54e8bcaa -- developer-run-tests: #3020

Much clearer now, thank you.

LGTM

#15 Updated by Tom Clegg about 2 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF