[ARVADOS] created: 2.1.0-1381-gd0cdf8567
Git user
git at public.arvados.org
Tue Sep 21 21:05:24 UTC 2021
at d0cdf85679a85998885c154d69dd317bd3396d7c (commit)
commit d0cdf85679a85998885c154d69dd317bd3396d7c
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Tue Sep 21 17:01:42 2021 -0400
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 at curii.com>
diff --git a/doc/api/methods/links.html.textile.liquid b/doc/api/methods/links.html.textile.liquid
index c71105c74..eceea296d 100644
--- 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 f54c4a9a5..384ffd64b 100644
--- 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 f2bae3a4b..af226cde8 100644
--- 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 cfd43f43b..9eae518c1 100644
--- 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
commit 64d7fc52356c128c367f18bcbe7a0c6f56a459a4
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Tue Sep 21 16:27:55 2021 -0400
18164: Add test for listing permission links
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index 66a62543b..cfd43f43b 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -347,6 +347,14 @@ class PermissionsTest < ActionDispatch::IntegrationTest
headers: auth(:active)
assert_response 404
+ get "/arvados/v1/links",
+ params: {
+ :filters => [["link_class", "=", "permission"], ["head_uuid", "=", groups(:public).uuid]].to_json
+ },
+ headers: auth(:active)
+ assert_response :success
+ assert_equal [], json_response['items']
+
# add some permissions, including can_manage
# permission for user :active
post "/arvados/v1/links",
@@ -379,6 +387,21 @@ class PermissionsTest < ActionDispatch::IntegrationTest
assert_response :success
can_write_uuid = json_response['uuid']
+ # Still should not be able read these permission links
+ get "/arvados/v1/permissions/#{groups(:public).uuid}",
+ params: nil,
+ headers: auth(:active)
+ assert_response 404
+
+ get "/arvados/v1/links",
+ params: {
+ :filters => [["link_class", "=", "permission"], ["head_uuid", "=", groups(:public).uuid]].to_json
+ },
+ headers: auth(:active)
+ assert_response :success
+ assert_equal [], json_response['items']
+
+ # Now add a can_manage link
post "/arvados/v1/links",
params: {
:format => :json,
@@ -405,6 +428,40 @@ class PermissionsTest < ActionDispatch::IntegrationTest
assert_includes perm_uuids, can_read_uuid, "can_read_uuid not found"
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.
+ get "/arvados/v1/links",
+ params: {
+ :filters => [["link_class", "=", "permission"], ["head_uuid", "=", groups(:public).uuid]].to_json
+ },
+ headers: auth(:active)
+ assert_response :success
+
+ perm_uuids = json_response['items'].map { |item| item['uuid'] }
+ assert_includes perm_uuids, can_read_uuid, "can_read_uuid not found"
+ 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
+ delete "/arvados/v1/links/#{can_manage_uuid}",
+ params: nil,
+ headers: auth(:active)
+ assert_response :success
+
+ # Should not be able read these permission links again
+ get "/arvados/v1/permissions/#{groups(:public).uuid}",
+ params: nil,
+ headers: auth(:active)
+ assert_response 404
+
+ get "/arvados/v1/links",
+ params: {
+ :filters => [["link_class", "=", "permission"], ["head_uuid", "=", groups(:public).uuid]].to_json
+ },
+ headers: auth(:active)
+ assert_response :success
+ assert_equal [], json_response['items']
end
test "get_permissions returns 404 for nonexistent uuid" do
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list