Arvados

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.

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

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?

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

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.

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

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