18164: Improve permission query for links

* Fixed so that permission links where the user can_manage the head_uuid are
visible to the user

* Add get_permissions to the API documentation for links

* Do not consider 'resource' a permission link class (as far as I know
this has never been used)

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

diff --git a/doc/api/methods/links.html.textile.liquid b/doc/api/methods/links.html.textile.liquid
index c71105c74569a88b26c594b32482a696e1ab0165..eceea296da72a17313567a38f4c6616be4a8aa81 100644 (file)
--- a/doc/api/methods/links.html.textile.liquid
+++ b/doc/api/methods/links.html.textile.liquid
@@ -143,3 +143,13 @@ table(table table-bordered table-condensed).
 |_. Argument |_. Type |_. Description |_. Location |_. Example |
 {background:#ccffcc}.|uuid|string|The UUID of the Link in question.|path||
 |link|object||query||
+
+h3. get_permissions
+
+Get all permission links that point directly to given UUID (in the head_uuid field).  The requesting user must have @can_manage@ permission or be an admin.
+
+Arguments:
+
+table(table table-bordered table-condensed).
+|_. Argument |_. Type |_. Description |_. Location |_. Example |
+{background:#ccffcc}.|uuid|string|The UUID of the object.|path||

diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb
index f54c4a9a519c563ca8fc08e9bb480b254e616608..384ffd64b7080190c21a4ec46f2ab7ea31d97c83 100644 (file)
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -47,17 +47,6 @@ class Arvados::V1::LinksController < ApplicationController
         .first
     else
       super
-      if @object.nil?
-        # Normally group permission links are not readable_by users.
-        # Make an exception for users with permission to manage the group.
-        # FIXME: Solve this more generally - see the controller tests.
-        link = Link.find_by_uuid(params[:uuid])
-        if (not link.nil?) and
-            (link.link_class == "permission") and
-            (@read_users.any? { |u| u.can?(manage: link.head_uuid) })
-          @object = link
-        end
-      end
     end
   end
 

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index f2bae3a4b5b2c79a9a3aacc882b7e570d713bc92..af226cde821399a0d115fdddfc698ec57b39ace9 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -386,11 +386,17 @@ class ArvadosModel < ApplicationRecord
 
       links_cond = ""
       if sql_table == "links"
-        # Match any permission link that gives one of the authorized
-        # users some permission _or_ gives anyone else permission to
-        # view one of the authorized users.
+        # 1) Match permission links incoming or outgoing on the
+        # user, i.e. granting permission on the user, or granting
+        # permission to the user.
+        #
+        # 2) Match permission links which grant permission on an
+        # object that this user can_manage.
+        #
         links_cond = "OR (#{sql_table}.link_class IN (:permission_link_classes) AND "+
-                       "(#{sql_table}.head_uuid IN (#{user_uuids_subquery}) OR #{sql_table}.tail_uuid IN (#{user_uuids_subquery})))"
+                     "   ((#{sql_table}.head_uuid IN (#{user_uuids_subquery}) OR #{sql_table}.tail_uuid IN (#{user_uuids_subquery})) OR " +
+                     "    #{sql_table}.head_uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+
+                     "    WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 3))) "
       end
 
       sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) #{trashed_check.empty? ? "" : "AND"} #{trashed_check}"
@@ -408,7 +414,7 @@ class ArvadosModel < ApplicationRecord
 
     self.where(sql_conds,
                user_uuids: all_user_uuids.collect{|c| c["target_uuid"]},
-               permission_link_classes: ['permission', 'resources'])
+               permission_link_classes: ['permission'])
   end
 
   def save_with_unique_name!

diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index cfd43f43b40dc95c18fdefe41a2490464182ddd3..9eae518c1d0f7ffd0700fc110de375754713fdc1 100644 (file)
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -355,8 +355,8 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_equal [], json_response['items']
 
-    # add some permissions, including can_manage
-    # permission for user :active
+    ### add some permissions, including can_manage
+    ### permission for user :active
     post "/arvados/v1/links",
       params: {
         :format => :json,
@@ -401,7 +401,13 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_equal [], json_response['items']
 
-    # Now add a can_manage link
+    # Shouldn't be able to read links directly either
+    get "/arvados/v1/links/#{can_read_uuid}",
+        params: {},
+      headers: auth(:active)
+    assert_response 404
+
+    ### Now add a can_manage link
     post "/arvados/v1/links",
       params: {
         :format => :json,
@@ -417,8 +423,8 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     assert_response :success
     can_manage_uuid = json_response['uuid']
 
-    # Now user :active should be able to retrieve permissions
-    # on group :public.
+    # user :active should be able to retrieve permissions
+    # on group :public using get_permissions
     get("/arvados/v1/permissions/#{groups(:public).uuid}",
       params: { :format => :json },
       headers: auth(:active))
@@ -429,8 +435,8 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     assert_includes perm_uuids, can_write_uuid, "can_write_uuid not found"
     assert_includes perm_uuids, can_manage_uuid, "can_manage_uuid not found"
 
-    # Now user :active should be able to retrieve permissions
-    # on group :public.
+    # user :active should be able to retrieve permissions
+    # on group :public using link list
     get "/arvados/v1/links",
         params: {
           :filters => [["link_class", "=", "permission"], ["head_uuid", "=", groups(:public).uuid]].to_json
@@ -443,7 +449,13 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     assert_includes perm_uuids, can_write_uuid, "can_write_uuid not found"
     assert_includes perm_uuids, can_manage_uuid, "can_manage_uuid not found"
 
-    # Now delete the can_manage link
+    # Should be able to read links directly too
+    get "/arvados/v1/links/#{can_read_uuid}",
+        params: {},
+      headers: auth(:active)
+    assert_response :success
+
+    ### Now delete the can_manage link
     delete "/arvados/v1/links/#{can_manage_uuid}",
       params: nil,
       headers: auth(:active)
@@ -462,6 +474,12 @@ class PermissionsTest < ActionDispatch::IntegrationTest
       headers: auth(:active)
     assert_response :success
     assert_equal [], json_response['items']
+
+    # Should not be able to read links directly either
+    get "/arvados/v1/links/#{can_read_uuid}",
+        params: {},
+      headers: auth(:active)
+    assert_response 404
   end
 
   test "get_permissions returns 404 for nonexistent uuid" do