Project

General

Profile

Actions

Bug #18865

closed

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 about 2 years ago. Updated almost 2 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
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 1 (0 open1 closed)

Task #18875: Review 18865-collection-permission-linksResolvedPeter Amstutz03/23/2022Actions
Actions #1

Updated by Peter Amstutz about 2 years ago

  • Category deleted (API)
  • Description updated (diff)
  • Project changed from 35 to Arvados
Actions #2

Updated by Peter Amstutz about 2 years ago

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

Updated by Peter Amstutz about 2 years ago

  • Description updated (diff)
Actions #4

Updated by Peter Amstutz about 2 years ago

  • Release set to 46
Actions #5

Updated by Peter Amstutz about 2 years ago

  • Assigned To set to Tom Clegg
Actions #6

Updated by Tom Clegg about 2 years 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?

Actions #7

Updated by Peter Amstutz about 2 years 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.

Actions #8

Updated by Tom Clegg about 2 years 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

Actions #9

Updated by Peter Amstutz about 2 years 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?

Actions #10

Updated by Tom Clegg about 2 years 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

Actions #11

Updated by Peter Amstutz almost 2 years ago

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

Updated by Peter Amstutz almost 2 years 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.

Actions #13

Updated by Tom Clegg almost 2 years 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

Actions #14

Updated by Peter Amstutz almost 2 years 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

Actions #15

Updated by Tom Clegg almost 2 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF