From: Tom Clegg Date: Fri, 25 Mar 2022 18:21:59 +0000 (-0400) Subject: 18865: Merge branch 'main' X-Git-Tag: 2.5.0~227^2~3 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/4b0200c3f6f70e90534a2f623cdf631b3a75ec6d?hp=ff635ccb09b0b79663b0220e16b8a0ef00997f5d 18865: Merge branch 'main' Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb index 384ffd64b7..14e010640e 100644 --- a/services/api/app/controllers/arvados/v1/links_controller.rb +++ b/services/api/app/controllers/arvados/v1/links_controller.rb @@ -26,8 +26,8 @@ class Arvados::V1::LinksController < ApplicationController def get_permissions if current_user.andand.can?(manage: @object) # find all links and return them - @objects = Link.where(link_class: "permission", - head_uuid: params[:uuid]) + @objects = Link.unscoped.where(link_class: "permission", + head_uuid: params[:uuid]) @offset = 0 @limit = @objects.count render_list @@ -39,14 +39,32 @@ class Arvados::V1::LinksController < ApplicationController protected def find_object_by_uuid + if params[:id] && params[:id].match(/\D/) + params[:uuid] = params.delete :id + end if action_name == 'get_permissions' # get_permissions accepts a UUID for any kind of object. @object = ArvadosModel::resource_class_for_uuid(params[:uuid]) .readable_by(*@read_users) .where(uuid: params[:uuid]) .first - else + elsif !current_user super + else + # The usual permission-filtering index query is unnecessarily + # inefficient, and doesn't match all permission links that + # should be visible (see #18865). 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. + @object = Link.unscoped.where(uuid: params[:uuid]).first + if @object.link_class != 'permission' + super + elsif @object && + current_user.uuid != @object.tail_uuid && + !current_user.can?(manage: @object.head_uuid) + @object = nil + end end end @@ -86,6 +104,30 @@ class Arvados::V1::LinksController < ApplicationController k end end + + # If the provided filters are enough to limit the results to + # permission links with specific head_uuids or + # tail_uuid=current_user, bypass the normal readable_by query + # (which doesn't match all can_manage-able items, see #18865) -- + # just ensure the current user actually has can_manage permission + # for the provided head_uuids, removing any that don't. At that + # point the caller's filters are an effective permission filter. + if @filters.include?(['link_class', '=', 'permission']) + @filters.map do |k| + if k[0] == 'tail_uuid' && k[1] == '=' && k[2] == current_user.uuid + @objects = Link.unscoped + elsif k[0] == 'head_uuid' + if k[1] == '=' && current_user.can?(manage: k[2]) + @objects = Link.unscoped + elsif k[1] == 'in' + k[2].select! do |head_uuid| + current_user.can?(manage: head_uuid) + end + @objects = Link.unscoped + end + end + end + end end end diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb index 9eae518c1d..65f5adc1d1 100644 --- a/services/api/test/integration/permissions_test.rb +++ b/services/api/test/integration/permissions_test.rb @@ -451,35 +451,226 @@ class PermissionsTest < ActionDispatch::IntegrationTest # Should be able to read links directly too get "/arvados/v1/links/#{can_read_uuid}", - params: {}, headers: auth(:active) assert_response :success + ### Create some objects of different types (other than projects) + ### inside a subproject inside the shared project, and share those + ### individual objects with a 3rd user ("spectator"). + post '/arvados/v1/groups', + params: { + group: { + owner_uuid: groups(:public).uuid, + name: 'permission test subproject', + group_class: 'project', + }, + }, + headers: auth(:admin) + assert_response :success + subproject_uuid = json_response['uuid'] + + test_types = ['collection', 'workflow', 'container_request'] + test_type_create_attrs = { + 'container_request' => { + command: ["echo", "foo"], + container_image: links(:docker_image_collection_tag).name, + cwd: "/tmp", + environment: {}, + mounts: {"/out" => {kind: "tmp", capacity: 1000000}}, + output_path: "/out", + runtime_constraints: {"vcpus" => 1, "ram" => 2}, + }, + } + + test_object = {} + test_object_perm_link = {} + test_types.each do |test_type| + post "/arvados/v1/#{test_type}s", + params: { + test_type.to_sym => { + owner_uuid: subproject_uuid, + name: "permission test #{test_type} in subproject", + }.merge(test_type_create_attrs[test_type] || {}).to_json, + }, + headers: auth(:admin) + assert_response :success + test_object[test_type] = json_response + + post '/arvados/v1/links', + params: { + link: { + tail_uuid: users(:spectator).uuid, + link_class: 'permission', + name: 'can_read', + head_uuid: test_object[test_type]['uuid'], + } + }, + headers: auth(:admin) + assert_response :success + test_object_perm_link[test_type] = json_response + end + + # The "active-can_manage-project" permission should cause the + # "spectator-can_read-object" links to be visible to the "active" + # user. + test_types.each do |test_type| + get "/arvados/v1/permissions/#{test_object[test_type]['uuid']}", + headers: auth(:active) + assert_response :success + perm_uuids = json_response['items'].map { |item| item['uuid'] } + assert_includes perm_uuids, test_object_perm_link[test_type]['uuid'], "can_read_uuid not found" + + get "/arvados/v1/links/#{test_object_perm_link[test_type]['uuid']}", + headers: auth(:active) + assert_response :success + + [ + ['head_uuid', '=', test_object[test_type]['uuid']], + ['head_uuid', 'in', [test_object[test_type]['uuid']]], + ['head_uuid', 'in', [users(:admin).uuid, test_object[test_type]['uuid']]], + ].each do |filter| + get "/arvados/v1/links", + params: { + filters: ([['link_class', '=', 'permission'], filter]).to_json, + }, + headers: auth(:active) + assert_response :success + assert_not_empty json_response['items'], "could not find can_read link using index with filter #{filter}" + assert_equal test_object_perm_link[test_type]['uuid'], json_response['items'][0]['uuid'] + end + + # The "spectator-can_read-object" link should be visible to the + # subject user ("spectator") in a filter query, even without + # can_manage permission on the target object. + [ + ['tail_uuid', '=', users(:spectator).uuid], + ].each do |filter| + get "/arvados/v1/links", + params: { + filters: ([['link_class', '=', 'permission'], filter]).to_json, + }, + headers: auth(:spectator) + assert_response :success + perm_uuids = json_response['items'].map { |item| item['uuid'] } + assert_includes perm_uuids, test_object_perm_link[test_type]['uuid'], "could not find can_read link using index with filter #{filter}" + end + end + ### 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, + test_types.each do |test_type| + get "/arvados/v1/permissions/#{groups(:public).uuid}", + headers: auth(:active) + assert_response 404 + + get "/arvados/v1/permissions/#{test_object[test_type]['uuid']}", + 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'] + + [ + ['head_uuid', '=', test_object[test_type]['uuid']], + ['head_uuid', 'in', [users(:admin).uuid, test_object[test_type]['uuid']]], + ['head_uuid', 'in', []], + ].each do |filter| + get "/arvados/v1/links", + params: { + :filters => [["link_class", "=", "permission"], filter].to_json + }, + headers: auth(:active) + assert_response :success + assert_equal [], json_response['items'] + end + + # Should not be able to read links directly either + get "/arvados/v1/links/#{can_read_uuid}", + headers: auth(:active) + assert_response 404 + + test_types.each do |test_type| + get "/arvados/v1/links/#{test_object_perm_link[test_type]['uuid']}", + headers: auth(:active) + assert_response 404 + end + end + + ### Create a collection, and share it with a direct permission + ### link (as opposed to sharing its parent project) + post "/arvados/v1/collections", + params: { + collection: { + name: 'permission test', + } + }, + headers: auth(:admin) + assert_response :success + collection_uuid = json_response['uuid'] + post "/arvados/v1/links", + params: { + link: { + tail_uuid: users(:spectator).uuid, + link_class: 'permission', + name: 'can_read', + head_uuid: collection_uuid, + properties: {} + } + }, + headers: auth(:admin) + assert_response :success + can_read_collection_uuid = json_response['uuid'] + + # Should not be able read the permission link via permissions API, + # because permission is only can_read, not can_manage + get "/arvados/v1/permissions/#{collection_uuid}", headers: auth(:active) assert_response 404 - get "/arvados/v1/links", - params: { - :filters => [["link_class", "=", "permission"], ["head_uuid", "=", groups(:public).uuid]].to_json - }, + # Should not be able to read the permission link directly, for + # same reason + get "/arvados/v1/links/#{can_read_collection_uuid}", headers: auth(:active) + assert_response 404 + + ### Now add a can_manage link + post "/arvados/v1/links", + params: { + link: { + tail_uuid: users(:active).uuid, + link_class: 'permission', + name: 'can_manage', + head_uuid: collection_uuid, + properties: {} + } + }, + headers: auth(:admin) assert_response :success - assert_equal [], json_response['items'] + can_manage_collection_uuid = json_response['uuid'] - # Should not be able to read links directly either - get "/arvados/v1/links/#{can_read_uuid}", - params: {}, + # Should be able read both permission links via permissions API + get "/arvados/v1/permissions/#{collection_uuid}", headers: auth(:active) - assert_response 404 + assert_response :success + perm_uuids = json_response['items'].map { |item| item['uuid'] } + assert_includes perm_uuids, can_read_collection_uuid, "can_read_uuid not found" + assert_includes perm_uuids, can_manage_collection_uuid, "can_manage_uuid not found" + + # Should be able to read both permission links directly + [can_read_collection_uuid, can_manage_collection_uuid].each do |uuid| + get "/arvados/v1/links/#{uuid}", + headers: auth(:active) + assert_response :success + end end test "get_permissions returns 404 for nonexistent uuid" do