From: Tom Clegg Date: Fri, 22 Aug 2014 00:16:47 +0000 (-0400) Subject: 3171: Do not follow permission graph through a User, unless permission on the User... X-Git-Tag: 1.1.0~2286^2~6 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/42a27c7cabdacb00bd5a9ba06443c8a72322738b?ds=sidebyside 3171: Do not follow permission graph through a User, unless permission on the User is can_manage. Restore usual permission model to user lookups. Add tests. --- diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb index 271299b6c9..0afb4e506c 100644 --- a/services/api/app/controllers/arvados/v1/users_controller.rb +++ b/services/api/app/controllers/arvados/v1/users_controller.rb @@ -136,14 +136,16 @@ class Arvados::V1::UsersController < ApplicationController } end - def find_objects_for_index + def apply_filters if (action_name == "index") and (not @read_users.any? { |u| u.is_admin }) - # Non-admin index returns very basic information about all active users. - # We ignore where and filters params to avoid leaking information. - @where = {} - @filters = [] - @select = ["uuid", "is_active", "email", "first_name", "last_name"] - @objects = model_class.where(is_active: true) + # Non-admin index returns very basic information about readable users. + safe_attrs = ["uuid", "is_active", "email", "first_name", "last_name"] + if @select + @select = @select & safe_attrs + else + @select = safe_attrs + end + @filters += [['is_active', '=', true]] end super end diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index 3058081c1e..f4a7de29e2 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -89,8 +89,9 @@ class Link < ArvadosModel end # A user is permitted to create, update or modify a permission link - # if and only if they have "manage" permission on the destination - # object. + # if and only if they have "manage" permission on the object + # indicated by the permission link's head_uuid. + # # All other links are treated as regular ArvadosModel objects. # def ensure_owner_uuid_is_permitted @@ -105,14 +106,4 @@ class Link < ArvadosModel end end - # A user can give all other users permissions on projects. - def skip_uuid_read_permission_check - skipped_attrs = super - if link_class == "permission" and - (ArvadosModel.resource_class_for_uuid(head_uuid) == Group) and - (ArvadosModel.resource_class_for_uuid(tail_uuid) == User) - skipped_attrs << "tail_uuid" - end - skipped_attrs - end end diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index 19e84dca7c..4df38ced67 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -103,12 +103,13 @@ class User < ArvadosModel 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, + # add any permission links from the current lookup_uuids to a Group. + Link.where('link_class = ? and tail_uuid in (?) and ' \ + '(head_uuid like ? or (name = ? and head_uuid like ?))', 'permission', + lookup_uuids, Group.uuid_like_pattern, + 'can_manage', User.uuid_like_pattern).each do |link| newgroups << [link.tail_uuid, link.head_uuid, link.name] end diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index 24399f500e..315f2bd472 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -132,10 +132,105 @@ class PermissionTest < ActiveSupport::TestCase end end + test "manager user gets permission to minions' articles via can_manage link" do + manager = create :active_user, first_name: "Manage", last_name: "Er" + minion = create :active_user, first_name: "Min", last_name: "Ion" + minions_specimen = act_as_user minion do + Specimen.create! + end + # Manager creates a group. (Make sure it doesn't magically give + # anyone any additional permissions.) + g = nil + act_as_user manager do + g = create :group, name: "NoBigSecret Lab" + assert_empty(User.readable_by(manager).where(uuid: minion.uuid), + "saw a user I shouldn't see") + assert_raises(ArvadosModel::PermissionDeniedError, + ActiveRecord::RecordInvalid, + "gave can_read permission to a user I shouldn't see") do + create(:permission_link, + name: 'can_read', tail_uuid: minion.uuid, head_uuid: g.uuid) + end + %w(can_manage can_write can_read).each do |perm_type| + assert_raises(ArvadosModel::PermissionDeniedError, + ActiveRecord::RecordInvalid, + "escalated privileges") do + create(:permission_link, + name: perm_type, tail_uuid: g.uuid, head_uuid: minion.uuid) + end + end + assert_empty(User.readable_by(manager).where(uuid: minion.uuid), + "manager saw minion too soon") + assert_empty(User.readable_by(minion).where(uuid: manager.uuid), + "minion saw manager too soon") + assert_empty(Group.readable_by(minion).where(uuid: g.uuid), + "minion saw manager's new NoBigSecret Lab group too soon") + + # Manager declares everybody on the system should be able to see + # the NoBigSecret Lab group. + create(:permission_link, + name: 'can_read', + tail_uuid: 'zzzzz-j7d0g-fffffffffffffff', + head_uuid: g.uuid) + # ...but nobody has joined the group yet. Manager still can't see + # minion. + assert_empty(User.readable_by(manager).where(uuid: minion.uuid), + "manager saw minion too soon") + end + + act_as_user minion do + # Minion can see the group. + assert_not_empty(Group.readable_by(minion).where(uuid: g.uuid), + "minion could not see the NoBigSecret Lab group") + # Minion joins the group. + create(:permission_link, + name: 'can_read', + tail_uuid: g.uuid, + head_uuid: minion.uuid) + end + + act_as_user manager do + # Now, manager can see minion. + assert_not_empty(User.readable_by(manager).where(uuid: minion.uuid), + "manager could not see minion") + # But cannot obtain further privileges this way. + assert_raises(ArvadosModel::PermissionDeniedError, + "escalated privileges") do + create(:permission_link, + name: 'can_manage', tail_uuid: manager.uuid, head_uuid: minion.uuid) + end + assert_empty(Specimen + .readable_by(manager) + .where(uuid: minions_specimen.uuid), + "manager saw the minion's private stuff") + assert_raises(ArvadosModel::PermissionDeniedError, + "manager could update minion's private stuff") do + minions_specimen.update_attributes(properties: {'x' => 'y'}) + end + end + + act_as_system_user do + # Root can give Manager more privileges over Minion. + create(:permission_link, + name: 'can_manage', tail_uuid: g.uuid, head_uuid: minion.uuid) + end + + act_as_user manager do + # Now, manager can read and write Minion's stuff. + assert_not_empty(Specimen + .readable_by(manager) + .where(uuid: minions_specimen.uuid), + "manager could not find minion's specimen by uuid") + assert_equal(true, + minions_specimen.update_attributes(properties: {'x' => 'y'}), + "manager could not update minion's specimen object") + end + end + test "users with bidirectional read permission in group can see each other, but cannot see each other's private articles" do - a = create :active_user first_name: "A" - b = create :active_user first_name: "B" - other = create :active_user first_name: "OTHER" + a = create :active_user, first_name: "A" + b = create :active_user, first_name: "B" + other = create :active_user, first_name: "OTHER" act_as_system_user do g = create :group [a,b].each do |u| @@ -161,15 +256,16 @@ class PermissionTest < ActiveSupport::TestCase assert_raises ArvadosModel::PermissionDeniedError, "wrote without perm" do other.update_attributes!(prefs: {'pwned' => true}) end - assert_equal true, u.update_attributes!(prefs: {'thisisme' => true}) + assert_equal(true, u.update_attributes!(prefs: {'thisisme' => true}), + "#{u.first_name} can't update its own prefs") end act_as_user other do - ([other, a, b] - [u]).each do |x| - assert_raises ArvadosModel::PermissionDeniedError, "wrote without perm" do - x.update_attributes!(prefs: {'pwned' => true}) - end + assert_raises(ArvadosModel::PermissionDeniedError, + "OTHER wrote #{u.first_name} without perm") do + u.update_attributes!(prefs: {'pwned' => true}) end - assert_equal true, other.update_attributes!(prefs: {'thisisme' => true}) + assert_equal(true, other.update_attributes!(prefs: {'thisisme' => true}), + "OTHER can't update its own prefs") end end end