From 6f70a514652050bde05301a4715be8769f213ac6 Mon Sep 17 00:00:00 2001 From: Tim Pierce Date: Thu, 26 Jun 2014 14:17:48 -0400 Subject: [PATCH] 2873: add /permissions API method The /permissions/:uuid method will return a list of all permissions that the current user is allowed to see on the given uuid. * New method LinksController::get_permissions, with a route from /arvados/v1/permissions. * LinksController overrides find_object_by_uuid to permit looking up a uuid in any class, when called by get_permissions. * Moved link permission checking to Link.ensure_owner_uuid_is_permitted. * Use current_user.can? to check the user's permission on head_uuid. Removed unnecessary owns? and can_manage? code. * Unit tests: * test/integration/permissions_test.rb: added tests: * "get_permissions returns list" * "get_permissions returns 404 for nonexistent uuid" * "get_permissions returns 403 if user lacks manage permission" * test/unit/link.rb: test that only permission and name links have their ownership changed upon save. * test/unit/permission_test.rb: test the following scenario: when user "active" owns a group G which can_manage another group H, then active user is permitted to create permission links directly on objects in group H. Refs #2873. 2873: perms --- .../arvados/v1/links_controller.rb | 28 ++++++++ services/api/app/models/arvados_model.rb | 12 ++-- services/api/app/models/link.rb | 31 +++++---- services/api/app/models/user.rb | 58 ++++------------ services/api/config/routes.rb | 1 + .../api/test/integration/permissions_test.rb | 66 +++++++++++++++++++ services/api/test/unit/link_test.rb | 4 ++ services/api/test/unit/permission_test.rb | 29 +++++++- 8 files changed, 159 insertions(+), 70 deletions(-) diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb index 188ecfc1a0..c09ebb43a9 100644 --- a/services/api/app/controllers/arvados/v1/links_controller.rb +++ b/services/api/app/controllers/arvados/v1/links_controller.rb @@ -18,8 +18,36 @@ class Arvados::V1::LinksController < ApplicationController super end + def get_permissions + if current_user.can?(manage: @object) + # find all links and return them + @where = { link_class: "permission", head_uuid: params[:uuid] } + @offset = 0 + @orders = [] + @filters = [] + @objects = nil + find_objects_for_index + render_list + else + render :json => { errors: ['Forbidden'] }.to_json, status: 403 + end + end + protected + # Override find_object_by_uuid: the get_permissions method may be + # called on a uuid belonging to any class. + def find_object_by_uuid + if action_name == 'get_permissions' + @objects = ArvadosModel::resource_class_for_uuid(params[:uuid]) + .readable_by(*@read_users) + .where(uuid: params[:uuid]) + @object = @objects.first + else + super + end + end + # Overrides ApplicationController load_where_param def load_where_param super diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index a6f2e41676..ce5f1614ce 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -219,14 +219,6 @@ class ArvadosModel < ActiveRecord::Base end if new_record? return true - elsif respond_to? :link_class and link_class == 'permission' - # Users are permitted to modify permission links themselves - # if they have "manage" permission on the destination object. - if current_user.can_manage? head_uuid - return true - else - raise PermissionDeniedError - end elsif current_user.uuid == self.owner_uuid_was or current_user.uuid == self.uuid or current_user.can? write: self.owner_uuid_was @@ -462,6 +454,10 @@ class ArvadosModel < ActiveRecord::Base nil end + def self.lookup_by_uuid(uuid) + ArvadosModel.resource_class_for_uuid(uuid).find_by_uuid(uuid) + end + def log_start_state @old_etag = etag @old_attributes = logged_attributes diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index 575f32c585..3de536ef07 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -5,7 +5,6 @@ class Link < ArvadosModel serialize :properties, Hash before_create :permission_to_attach_to_objects before_update :permission_to_attach_to_objects - before_save :permission_link_ownership, if: "link_class == 'permission'" after_update :maybe_invalidate_permissions_cache after_create :maybe_invalidate_permissions_cache after_destroy :maybe_invalidate_permissions_cache @@ -53,15 +52,8 @@ class Link < ArvadosModel return true if current_user.is_admin # All users can grant permissions on objects they own or can manage - return true if current_user.can_manage? head_uuid - - # Users with "can_grant" permission on an object can grant - # permissions on that object - has_grant_permission = self.class. - where('link_class=? AND name=? AND tail_uuid=? AND head_uuid=?', - 'permission', 'can_grant', current_user.uuid, self.head_uuid). - count > 0 - return true if has_grant_permission + head_obj = ArvadosModel.lookup_by_uuid(head_uuid) + return true if current_user.can?(manage: head_obj) # Default = deny. false @@ -96,9 +88,20 @@ class Link < ArvadosModel end end - # permission_link_ownership: ensure that permission links are - # owned by root. - def permission_link_ownership - self.owner_uuid = system_user_uuid + # A user is permitted to create, update or modify a permission link + # if and only if they have "manage" permission on the destination + # object. + # All other links are treated as regular ArvadosModel objects. + # + def ensure_owner_uuid_is_permitted + if link_class == 'permission' + ob = ArvadosModel.lookup_by_uuid(head_uuid) + raise PermissionDeniedError unless current_user.can?(manage: ob) + # All permission links should be owned by the system user. + self.owner_uuid = system_user_uuid + return true + else + super + end end end diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index f1b5c44407..2ef56bf7e5 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -71,19 +71,30 @@ class User < ArvadosModel # Return a hash of {group_uuid: perm_hash} where perm_hash[:read] # and perm_hash[:write] are true if this user can read and write # objects owned by group_uuid. + # + # The permission graph is built by repeatedly enumerating all + # permission links reachable from self.uuid, and then calling + # search_permissions def group_permissions Rails.cache.fetch "groups_for_user_#{self.uuid}" do permissions_from = {} todo = {self.uuid => true} done = {} + # Build the equivalence class of permissions starting with + # self.uuid. On each iteration of this loop, todo contains + # the next set of uuids in the permission equivalence class + # to evaluate. while !todo.empty? lookup_uuids = todo.keys lookup_uuids.each do |uuid| done[uuid] = true end todo = {} newgroups = [] + # include all groups owned by the current set of uuids. Group.where('owner_uuid in (?)', lookup_uuids).each do |group| newgroups << [group.owner_uuid, group.uuid, 'can_manage'] end + # add any permission links from the current lookup_uuids to a + # User or Group. Link.where('tail_uuid in (?) and link_class = ? and (head_uuid like ? or head_uuid like ?)', lookup_uuids, 'permission', @@ -179,55 +190,8 @@ class User < ArvadosModel self.save! end - def owns? object_uuid - return User.find_user_owning(object_uuid).andand.uuid == uuid - end - - def can_manage? object_uuid - is_admin or - owns?(object_uuid) or - has_permission?(:can_manage, object_uuid) - end - protected - # Returns the first User found in the ownership path for obj_uuid. - # If obj_uuid is not owned by any user, returns nil. - # - # TODO(twp): this code largely stolen from - # ArvadosModel::ensure_ownership_path_leads_to_user. See if we can - # refactor these methods to share more code. - # - def self.find_user_owning obj_uuid - uuid_in_path = {obj_uuid => true} - # Walk up the owner_uuid chain for obj_uuid until one of these - # conditions is met: - # - the owner_uuid belongs to the User class - # - no owner_uuid is found (no User owns this object) - # - we discover an ownership cycle (a fatal consistency error) - # - x = obj_uuid - while (owner_class = ArvadosModel.resource_class_for_uuid(x)) != User - begin - if !owner_class.respond_to? :find_by_uuid - raise ActiveRecord::RecordNotFound.new - else - x = owner_class.find_by_uuid(x).owner_uuid - end - rescue ActiveRecord::RecordNotFound => e - # errors.add :owner_uuid, "is not owned by any user: #{e}" - return nil - end - # If there is an ownership cycle, we can conclude that - # no User owns this object. - if uuid_in_path[x] - return nil - end - uuid_in_path[x] = true - end - return owner_class.find_by_uuid(x) - end - def ensure_ownership_path_leads_to_user true end diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb index e4d2975a57..70934553f2 100644 --- a/services/api/config/routes.rb +++ b/services/api/config/routes.rb @@ -56,6 +56,7 @@ Server::Application.routes.draw do get 'logins', on: :member get 'get_all_logins', on: :collection end + get '/permissions/:uuid', :to => 'links#get_permissions' end end diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb index 2ebd62bc8b..f83e8c374f 100644 --- a/services/api/test/integration/permissions_test.rb +++ b/services/api/test/integration/permissions_test.rb @@ -283,4 +283,70 @@ class PermissionsTest < ActionDispatch::IntegrationTest end end + test "get_permissions returns list" do + # add some permissions + post "/arvados/v1/links", { + :format => :json, + :link => { + tail_uuid: users(:spectator).uuid, + link_class: 'permission', + name: 'can_read', + head_uuid: collections(:foo_file).uuid, + properties: {} + } + }, auth(:admin) + assert_response :success + can_read_uuid = json_response['uuid'] + + post "/arvados/v1/links", { + :format => :json, + :link => { + tail_uuid: users(:active).uuid, + link_class: 'permission', + name: 'can_write', + head_uuid: collections(:foo_file).uuid, + properties: {} + } + }, auth(:admin) + assert_response :success + can_write_uuid = json_response['uuid'] + + post "/arvados/v1/links", { + :format => :json, + :link => { + tail_uuid: users(:inactive).uuid, + link_class: 'permission', + name: 'can_manage', + head_uuid: collections(:foo_file).uuid, + properties: {} + } + }, auth(:admin) + assert_response :success + can_manage_uuid = json_response['uuid'] + + get "/arvados/v1/permissions/#{collections(:foo_file).uuid}", { + :format => :json, + }, auth(:admin) + assert_response :success + + perm_uuids = json_response['items'].map { |item| item['uuid'] } + assert perm_uuids.include?(can_read_uuid), "can_read_uuid not found" + assert perm_uuids.include?(can_write_uuid), "can_write_uuid not found" + assert perm_uuids.include?(can_manage_uuid), "can_manage_uuid not found" + end + + test "get_permissions returns 404 for nonexistent uuid" do + nonexistent = Collection.generate_uuid + # make sure it really doesn't exist + get "/arvados/v1/collections/#{nonexistent}", { :format => :json }, auth(:admin) + assert_response 404 + + get "/arvados/v1/permissions/#{nonexistent}", { :format => :json }, auth(:active) + assert_response 404 + end + + test "get_permissions returns 403 if user lacks manage permission" do + get "/arvados/v1/permissions/#{collections(:foo_file).uuid}", { :format => :json }, auth(:active) + assert_response 403 + end end diff --git a/services/api/test/unit/link_test.rb b/services/api/test/unit/link_test.rb index 56a38045e4..e40326504a 100644 --- a/services/api/test/unit/link_test.rb +++ b/services/api/test/unit/link_test.rb @@ -13,6 +13,7 @@ class LinkTest < ActiveSupport::TestCase link_class: 'name', name: 'foo') assert a.valid?, a.errors.to_s + assert_equal groups(:aproject).uuid, a.owner_uuid assert_raises ActiveRecord::RecordNotUnique do b = Link.create!(tail_uuid: groups(:aproject).uuid, head_uuid: specimens(:owned_by_active_user).uuid, @@ -27,11 +28,13 @@ class LinkTest < ActiveSupport::TestCase link_class: 'name', name: 'foo') assert a.valid?, a.errors.to_s + assert_equal groups(:aproject).uuid, a.owner_uuid b = Link.create!(tail_uuid: groups(:asubproject).uuid, head_uuid: specimens(:owned_by_active_user).uuid, link_class: 'name', name: 'foo') assert b.valid?, b.errors.to_s + assert_equal groups(:asubproject).uuid, b.owner_uuid assert_not_equal(a.uuid, b.uuid, "created two links and got the same uuid back.") end @@ -52,6 +55,7 @@ class LinkTest < ActiveSupport::TestCase head_uuid: ob.uuid, link_class: 'test', name: 'test') + assert_equal users(:admin).uuid, link.owner_uuid assert_raises(ActiveRecord::DeleteRestrictionError, "should not delete #{ob.uuid} with link #{link.uuid}") do ob.destroy diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index 23f5f756f5..748c7907a2 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -63,7 +63,34 @@ class PermissionTest < ActiveSupport::TestCase assert ob.writable_by.include?(users(:active).uuid), "user does not have write permission" end - test "user with can_manage permission allowed to modify permission link" do + test "user owns group, group can_manage object's group, user can add permissions" do + set_user_from_auth :admin + + owner_grp = Group.create!(owner_uuid: users(:active).uuid) + + sp_grp = Group.create! + sp = Specimen.create!(owner_uuid: sp_grp.uuid) + + manage_perm = Link.create!(link_class: 'permission', + name: 'can_manage', + tail_uuid: owner_grp.uuid, + head_uuid: sp_grp.uuid) + + # active user owns owner_grp, which has can_manage permission on sp_grp + # user should be able to add permissions on sp. + set_user_from_auth :active_trustedclient + test_perm = Link.create(tail_uuid: users(:active).uuid, + head_uuid: sp.uuid, + link_class: 'permission', + name: 'can_write') + test_uuid = test_perm.uuid + assert test_perm.save, "could not save new permission on target object" + assert test_perm.destroy, "could not delete new permission on target object" + end + + # TODO(twp): fix bug #3091, which should fix this test. + test "can_manage permission on a non-group object" do + skip set_user_from_auth :admin ob = Specimen.create! -- 2.30.2