From a3aee2781cfcd006fa1b7ce3cfeeb1dd2d53c270 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Thu, 4 Jun 2020 16:58:18 -0400 Subject: [PATCH] 16007: Special handing for users with permissions on other users Revise & simplify permission traversal. Don't traverse users except when starting from the user (origin_uuid = starting_uuid). This avoids disasterous queries where we re-traverse other users "just in case" and end up recomputing the whole database. As a tradeoff, our epic readable_by query gets a touch more epic, as it now has to go to the permissions table to check if there are other user permissions the current user also is allowed to use. Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- services/api/app/models/arvados_model.rb | 11 +- services/api/app/models/user.rb | 11 +- .../20200501150153_permission_table.rb | 143 ++++---------- services/api/db/structure.sql | 176 +++++++----------- services/api/lib/update_permissions.rb | 40 +++- services/api/test/unit/permission_test.rb | 77 +++++--- services/api/test/unit/user_test.rb | 9 - 7 files changed, 212 insertions(+), 255 deletions(-) diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index cc6f7816aa..a27d6aba46 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -324,9 +324,14 @@ class ArvadosModel < ApplicationRecord # # see issue 13208 for details. + user_uuids_subquery = %{ +select target_uuid from materialized_permissions where user_uuid in (:user_uuids) +and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and perm_level >= 1 +} + # Match a direct read permission link from the user to the record uuid direct_check = "#{sql_table}.uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+ - "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 #{trashed_check})" + "WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 1 #{trashed_check})" # Match a read permission for the user to the record's # owner_uuid. This is so we can have a permissions table that @@ -347,7 +352,7 @@ class ArvadosModel < ApplicationRecord owner_check = "" if sql_table != "api_client_authorizations" and sql_table != "groups" then owner_check = "OR #{sql_table}.owner_uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+ - "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 #{trashed_check} AND traverse_owned) " + "WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 1 #{trashed_check} AND traverse_owned) " end links_cond = "" @@ -356,7 +361,7 @@ class ArvadosModel < ApplicationRecord # users some permission _or_ gives anyone else permission to # view one of the authorized users. links_cond = "OR (#{sql_table}.link_class IN (:permission_link_classes) AND "+ - "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))" + "(#{sql_table}.head_uuid IN (#{user_uuids_subquery}) OR #{sql_table}.tail_uuid IN (#{user_uuids_subquery})))" end sql_conds = "(#{direct_check} #{owner_check} #{links_cond}) #{exclude_trashed_records}" diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index cb7efe9cca..869ac88f4b 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -121,10 +121,15 @@ class User < ArvadosModel target_owner_uuid = target.owner_uuid if target.respond_to? :owner_uuid + user_uuids_subquery = %{ +select target_uuid from materialized_permissions where user_uuid = $1 +and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and perm_level >= #{VAL_FOR_PERM[action]} +} + unless ActiveRecord::Base.connection. exec_query(%{ SELECT 1 FROM #{PERMISSION_VIEW} - WHERE user_uuid = $1 and + WHERE user_uuid in (#{user_uuids_subquery}) and ((target_uuid = $2 and perm_level >= $3) or (target_uuid = $4 and perm_level >= $3 and traverse_owned)) }, @@ -438,8 +443,10 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2 end skip_check_permissions_against_full_refresh do update_permissions self.owner_uuid, self.uuid, 3 + update_permissions self.uuid, self.uuid, 3 + update_permissions new_user.owner_uuid, new_user.uuid, 3 end - update_permissions new_user.owner_uuid, new_user.uuid, 3 + update_permissions new_user.uuid, new_user.uuid, 3 end end diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb index 77944eb6ee..6dd2c29bdb 100644 --- a/services/api/db/migrate/20200501150153_permission_table.rb +++ b/services/api/db/migrate/20200501150153_permission_table.rb @@ -2,6 +2,8 @@ # # SPDX-License-Identifier: AGPL-3.0 +require 'update_permissions' + class PermissionTable < ActiveRecord::Migration[5.0] def up # This is a major migration. We are replacing the @@ -125,6 +127,8 @@ create view permission_graph_edges as select groups.owner_uuid as tail_uuid, groups.uuid as head_uuid, (3) as val from groups union all select users.owner_uuid as tail_uuid, users.uuid as head_uuid, (3) as val from users +union all + select users.uuid as tail_uuid, users.uuid as head_uuid, (3) as val from users union all select links.tail_uuid, links.head_uuid, @@ -133,69 +137,18 @@ union all WHEN links.name = 'can_login' THEN 1 WHEN links.name = 'can_write' THEN 2 WHEN links.name = 'can_manage' THEN 3 + ELSE 0 END as val from links where links.link_class='permission' } - ActiveRecord::Base.connection.execute %{ -create or replace function search_permission_graph (starting_uuid varchar(27), - starting_perm integer, - override_edge_tail varchar(27) default null, - override_edge_head varchar(27) default null, - override_edge_perm integer default null) - returns table (target_uuid varchar(27), val integer, traverse_owned bool) -STABLE -language SQL -as $$ -/* - From starting_uuid, perform a recursive self-join on the edges - to follow chains of permissions. This is a breadth-first search - of the permission graph. Permission is propagated across edges, - which may narrow the permission for subsequent links (eg I start - at can_manage but when traversing a can_read link everything - touched through that link will only be can_read). - - When revoking a permission, we follow the chain of permissions but - with a permissions level of 0. The update on the permissions table - has to happen _before_ the permission is actually removed, because - we need to be able to traverse the edge before it goes away. When - we do that, we also need to traverse it at the _new_ permission - level - this is what override_edge_tail/head/perm are for. - - Yields the set of objects that are potentially affected, and - their permission levels granted by having starting_perm on - starting_uuid. - - If starting_uuid is a user, this computes the entire set of - permissions for that user (because it returns everything that is - reachable by that user). - - Used by the compute_permission_subgraph function. -*/ -WITH RECURSIVE - traverse_graph(target_uuid, val, traverse_owned) as ( - values (starting_uuid, starting_perm, - should_traverse_owned(starting_uuid, starting_perm)) - union - (select edges.head_uuid, - least(edges.val, - traverse_graph.val, - case traverse_graph.traverse_owned - when true then null - else 0 - end, - case (edges.tail_uuid = override_edge_tail AND - edges.head_uuid = override_edge_head) - when true then override_edge_perm + override = %{, + case (edges.tail_uuid = perm_origin_uuid AND + edges.head_uuid = starting_uuid) + when true then starting_perm else null - end), - should_traverse_owned(edges.head_uuid, edges.val) - from permission_graph_edges as edges, traverse_graph - where traverse_graph.target_uuid = edges.tail_uuid)) - select target_uuid, max(val), bool_or(traverse_owned) from traverse_graph - group by (target_uuid); -$$; + end } ActiveRecord::Base.connection.execute %{ @@ -250,12 +203,13 @@ with permission change, using search_permission_graph. */ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select perm_origin_uuid, target_uuid, val, traverse_owned - from search_permission_graph(starting_uuid, - starting_perm, - perm_origin_uuid, - starting_uuid, - starting_perm)), + #{PERM_QUERY_TEMPLATE % {:base_case => %{ + values (perm_origin_uuid, starting_uuid, starting_perm, + should_traverse_owned(starting_uuid, starting_perm), + (perm_origin_uuid = starting_uuid or starting_uuid not like '_____-tpzed-_______________')) +}, +:override => override +} }), /* Finds other inbound edges that grant permissions on the objects in perm_from_start, and computes permissions that originate from @@ -267,50 +221,24 @@ with ownership. */ additional_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select edges.tail_uuid as perm_origin_uuid, ps.target_uuid, ps.val, - should_traverse_owned(ps.target_uuid, ps.val) - from permission_graph_edges as edges, - lateral search_permission_graph(edges.head_uuid, - edges.val, - perm_origin_uuid, - starting_uuid, - starting_perm) as ps + #{PERM_QUERY_TEMPLATE % {:base_case => %{ + select edges.tail_uuid as origin_uuid, edges.head_uuid as target_uuid, edges.val, + should_traverse_owned(edges.head_uuid, edges.val), + edges.head_uuid like '_____-j7d0g-_______________' + from permission_graph_edges as edges where (not (edges.tail_uuid = perm_origin_uuid and - edges.head_uuid = starting_uuid)) and - edges.tail_uuid not in (select target_uuid from perm_from_start) and - edges.head_uuid in (select target_uuid from perm_from_start)), + edges.head_uuid = starting_uuid)) and + edges.tail_uuid not in (select target_uuid from perm_from_start where target_uuid like '_____-j7d0g-_______________') and + edges.head_uuid in (select target_uuid from perm_from_start) +}, +:override => override +} }), /* Combines the permissions computed in the first two phases. */ - partial_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( + all_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( select * from perm_from_start union all select * from additional_perms - ), - - /* If there are any users in the set of potentially affected objects - and the user's owner was not traversed, recompute permissions for - that user. This is required because users always have permission - to themselves (identity property) which would be missing from the - permission set if the user was traversed while computing - permissions for another object. - */ - user_identity_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select users.uuid as perm_origin_uuid, ps.target_uuid, ps.val, ps.traverse_owned - from users, lateral search_permission_graph(users.uuid, - 3, - perm_origin_uuid, - starting_uuid, - starting_perm) as ps - where (users.owner_uuid not in (select target_uuid from partial_perms) or - users.owner_uuid = users.uuid) and - users.uuid in (select target_uuid from partial_perms) - ), - - /* Combines all the computed permissions into one table. */ - all_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select * from partial_perms - union - select * from user_identity_perms ) /* The actual query that produces rows to be added or removed @@ -351,10 +279,11 @@ with u.traverse_owned from all_perms as u, materialized_permissions as m where u.perm_origin_uuid = m.target_uuid AND m.traverse_owned + AND (m.user_uuid = m.target_uuid or m.target_uuid not like '_____-tpzed-_______________') union all - select perm_origin_uuid as user_uuid, target_uuid, val as perm_level, traverse_owned + select target_uuid as user_uuid, target_uuid, 3, true from all_perms - where all_perms.perm_origin_uuid like '_____-tpzed-_______________') as v + where all_perms.target_uuid like '_____-tpzed-_______________') as v group by v.user_uuid, v.target_uuid $$; } @@ -365,8 +294,11 @@ $$; # ActiveRecord::Base.connection.execute %{ INSERT INTO materialized_permissions -select users.uuid, g.target_uuid, g.val, g.traverse_owned -from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0 + #{PERM_QUERY_TEMPLATE % {:base_case => %{ + select uuid, uuid, 3, true, true from users +}, +:override => '' +} } } end @@ -376,7 +308,6 @@ from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0 ActiveRecord::Base.connection.execute "DROP function project_subtree_with_trash_at (varchar, timestamp);" ActiveRecord::Base.connection.execute "DROP function compute_trashed ();" - ActiveRecord::Base.connection.execute "DROP function search_permission_graph(varchar, integer, varchar, varchar, integer);" ActiveRecord::Base.connection.execute "DROP function compute_permission_subgraph (varchar, varchar, integer);" ActiveRecord::Base.connection.execute "DROP function should_traverse_owned(varchar, integer);" ActiveRecord::Base.connection.execute "DROP view permission_graph_edges;" diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql index eaa3d6299a..aa3ffae620 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -89,12 +89,35 @@ with permission change, using search_permission_graph. */ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select perm_origin_uuid, target_uuid, val, traverse_owned - from search_permission_graph(starting_uuid, - starting_perm, - perm_origin_uuid, - starting_uuid, - starting_perm)), + +WITH RECURSIVE + traverse_graph(origin_uuid, target_uuid, val, traverse_owned, starting_set) as ( + + values (perm_origin_uuid, starting_uuid, starting_perm, + should_traverse_owned(starting_uuid, starting_perm), + (perm_origin_uuid = starting_uuid or starting_uuid not like '_____-tpzed-_______________')) + + union + (select traverse_graph.origin_uuid, + edges.head_uuid, + least(edges.val, + traverse_graph.val + , + case (edges.tail_uuid = perm_origin_uuid AND + edges.head_uuid = starting_uuid) + when true then starting_perm + else null + end +), + should_traverse_owned(edges.head_uuid, edges.val), + false + from permission_graph_edges as edges, traverse_graph + where traverse_graph.target_uuid = edges.tail_uuid + and (edges.tail_uuid like '_____-j7d0g-_______________' or + traverse_graph.starting_set))) + select traverse_graph.origin_uuid, target_uuid, max(val) as val, bool_or(traverse_owned) as traverse_owned from traverse_graph + group by (traverse_graph.origin_uuid, target_uuid) +), /* Finds other inbound edges that grant permissions on the objects in perm_from_start, and computes permissions that originate from @@ -106,50 +129,46 @@ with ownership. */ additional_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select edges.tail_uuid as perm_origin_uuid, ps.target_uuid, ps.val, - should_traverse_owned(ps.target_uuid, ps.val) - from permission_graph_edges as edges, - lateral search_permission_graph(edges.head_uuid, - edges.val, - perm_origin_uuid, - starting_uuid, - starting_perm) as ps + +WITH RECURSIVE + traverse_graph(origin_uuid, target_uuid, val, traverse_owned, starting_set) as ( + + select edges.tail_uuid as origin_uuid, edges.head_uuid as target_uuid, edges.val, + should_traverse_owned(edges.head_uuid, edges.val), + edges.head_uuid like '_____-j7d0g-_______________' + from permission_graph_edges as edges where (not (edges.tail_uuid = perm_origin_uuid and - edges.head_uuid = starting_uuid)) and - edges.tail_uuid not in (select target_uuid from perm_from_start) and - edges.head_uuid in (select target_uuid from perm_from_start)), + edges.head_uuid = starting_uuid)) and + edges.tail_uuid not in (select target_uuid from perm_from_start where target_uuid like '_____-j7d0g-_______________') and + edges.head_uuid in (select target_uuid from perm_from_start) + + union + (select traverse_graph.origin_uuid, + edges.head_uuid, + least(edges.val, + traverse_graph.val + , + case (edges.tail_uuid = perm_origin_uuid AND + edges.head_uuid = starting_uuid) + when true then starting_perm + else null + end +), + should_traverse_owned(edges.head_uuid, edges.val), + false + from permission_graph_edges as edges, traverse_graph + where traverse_graph.target_uuid = edges.tail_uuid + and (edges.tail_uuid like '_____-j7d0g-_______________' or + traverse_graph.starting_set))) + select traverse_graph.origin_uuid, target_uuid, max(val) as val, bool_or(traverse_owned) as traverse_owned from traverse_graph + group by (traverse_graph.origin_uuid, target_uuid) +), /* Combines the permissions computed in the first two phases. */ - partial_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( + all_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( select * from perm_from_start union all select * from additional_perms - ), - - /* If there are any users in the set of potentially affected objects - and the user's owner was not traversed, recompute permissions for - that user. This is required because users always have permission - to themselves (identity property) which would be missing from the - permission set if the user was traversed while computing - permissions for another object. - */ - user_identity_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select users.uuid as perm_origin_uuid, ps.target_uuid, ps.val, ps.traverse_owned - from users, lateral search_permission_graph(users.uuid, - 3, - perm_origin_uuid, - starting_uuid, - starting_perm) as ps - where (users.owner_uuid not in (select target_uuid from partial_perms) or - users.owner_uuid = users.uuid) and - users.uuid in (select target_uuid from partial_perms) - ), - - /* Combines all the computed permissions into one table. */ - all_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select * from partial_perms - union - select * from user_identity_perms ) /* The actual query that produces rows to be added or removed @@ -190,10 +209,11 @@ with u.traverse_owned from all_perms as u, materialized_permissions as m where u.perm_origin_uuid = m.target_uuid AND m.traverse_owned + AND (m.user_uuid = m.target_uuid or m.target_uuid not like '_____-tpzed-_______________') union all - select perm_origin_uuid as user_uuid, target_uuid, val as perm_level, traverse_owned + select target_uuid as user_uuid, target_uuid, 3, true from all_perms - where all_perms.perm_origin_uuid like '_____-tpzed-_______________') as v + where all_perms.target_uuid like '_____-tpzed-_______________') as v group by v.user_uuid, v.target_uuid $$; @@ -242,63 +262,6 @@ WITH RECURSIVE $$; --- --- Name: search_permission_graph(character varying, integer, character varying, character varying, integer); Type: FUNCTION; Schema: public; Owner: - --- - -CREATE FUNCTION public.search_permission_graph(starting_uuid character varying, starting_perm integer, override_edge_tail character varying DEFAULT NULL::character varying, override_edge_head character varying DEFAULT NULL::character varying, override_edge_perm integer DEFAULT NULL::integer) RETURNS TABLE(target_uuid character varying, val integer, traverse_owned boolean) - LANGUAGE sql STABLE - AS $$ -/* - From starting_uuid, perform a recursive self-join on the edges - to follow chains of permissions. This is a breadth-first search - of the permission graph. Permission is propagated across edges, - which may narrow the permission for subsequent links (eg I start - at can_manage but when traversing a can_read link everything - touched through that link will only be can_read). - - When revoking a permission, we follow the chain of permissions but - with a permissions level of 0. The update on the permissions table - has to happen _before_ the permission is actually removed, because - we need to be able to traverse the edge before it goes away. When - we do that, we also need to traverse it at the _new_ permission - level - this is what override_edge_tail/head/perm are for. - - Yields the set of objects that are potentially affected, and - their permission levels granted by having starting_perm on - starting_uuid. - - If starting_uuid is a user, this computes the entire set of - permissions for that user (because it returns everything that is - reachable by that user). - - Used by the compute_permission_subgraph function. -*/ -WITH RECURSIVE - traverse_graph(target_uuid, val, traverse_owned) as ( - values (starting_uuid, starting_perm, - should_traverse_owned(starting_uuid, starting_perm)) - union - (select edges.head_uuid, - least(edges.val, - traverse_graph.val, - case traverse_graph.traverse_owned - when true then null - else 0 - end, - case (edges.tail_uuid = override_edge_tail AND - edges.head_uuid = override_edge_head) - when true then override_edge_perm - else null - end), - should_traverse_owned(edges.head_uuid, edges.val) - from permission_graph_edges as edges, traverse_graph - where traverse_graph.target_uuid = edges.tail_uuid)) - select target_uuid, max(val), bool_or(traverse_owned) from traverse_graph - group by (target_uuid); -$$; - - -- -- Name: should_traverse_owned(character varying, integer); Type: FUNCTION; Schema: public; Owner: - -- @@ -1092,6 +1055,11 @@ UNION ALL users.uuid AS head_uuid, 3 AS val FROM public.users +UNION ALL + SELECT users.uuid AS tail_uuid, + users.uuid AS head_uuid, + 3 AS val + FROM public.users UNION ALL SELECT links.tail_uuid, links.head_uuid, @@ -1100,7 +1068,7 @@ UNION ALL WHEN ((links.name)::text = 'can_login'::text) THEN 1 WHEN ((links.name)::text = 'can_write'::text) THEN 2 WHEN ((links.name)::text = 'can_manage'::text) THEN 3 - ELSE NULL::integer + ELSE 0 END AS val FROM public.links WHERE ((links.link_class)::text = 'permission'::text); diff --git a/services/api/lib/update_permissions.rb b/services/api/lib/update_permissions.rb index b2cf6595b8..33a8d97365 100644 --- a/services/api/lib/update_permissions.rb +++ b/services/api/lib/update_permissions.rb @@ -9,12 +9,15 @@ def refresh_permissions ActiveRecord::Base.transaction do ActiveRecord::Base.connection.execute("LOCK TABLE #{PERMISSION_VIEW}") ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW}") + ActiveRecord::Base.connection.execute %{ -INSERT INTO #{PERMISSION_VIEW} -select users.uuid, g.target_uuid, g.val, g.traverse_owned -from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0 +INSERT INTO materialized_permissions + #{PERM_QUERY_TEMPLATE % {:base_case => %{ + select uuid, uuid, 3, true, true from users }, - "refresh_permission_view.do" +:override => '' +} } +}, "refresh_permission_view.do" end end @@ -161,9 +164,12 @@ order by user_uuid, target_uuid }, "check_permissions_against_full_refresh.permission_table" q2 = ActiveRecord::Base.connection.exec_query %{ -select users.uuid as user_uuid, g.target_uuid, g.val as perm_level, g.traverse_owned -from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0 -order by users.uuid, target_uuid + select pq.origin_uuid as user_uuid, target_uuid, pq.val as perm_level, pq.traverse_owned from ( + #{PERM_QUERY_TEMPLATE % {:base_case => %{ + select uuid, uuid, 3, true, true from users +}, +:override => '' +} }) as pq order by origin_uuid, target_uuid }, "check_permissions_against_full_refresh.full_recompute" if q1.count != q2.count @@ -196,3 +202,23 @@ def skip_check_permissions_against_full_refresh Thread.current[:no_check_permissions_against_full_refresh] = check_perm_was end end + +PERM_QUERY_TEMPLATE = %{ +WITH RECURSIVE + traverse_graph(origin_uuid, target_uuid, val, traverse_owned, starting_set) as ( + %{base_case} + union + (select traverse_graph.origin_uuid, + edges.head_uuid, + least(edges.val, + traverse_graph.val + %{override}), + should_traverse_owned(edges.head_uuid, edges.val), + false + from permission_graph_edges as edges, traverse_graph + where traverse_graph.target_uuid = edges.tail_uuid + and (edges.tail_uuid like '_____-j7d0g-_______________' or + traverse_graph.starting_set))) + select traverse_graph.origin_uuid, target_uuid, max(val) as val, bool_or(traverse_owned) as traverse_owned from traverse_graph + group by (traverse_graph.origin_uuid, target_uuid) +} diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index a0478ec54a..d9383cf8e2 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -10,7 +10,7 @@ class PermissionTest < ActiveSupport::TestCase test "Grant permissions on an object I own" do set_user_from_auth :active_trustedclient - ob = Specimen.create + ob = Collection.create assert ob.save # Ensure I have permission to manage this group even when its owner changes @@ -24,7 +24,7 @@ class PermissionTest < ActiveSupport::TestCase test "Delete permission links when deleting an object" do set_user_from_auth :active_trustedclient - ob = Specimen.create! + ob = Collection.create! Link.create!(tail_uuid: users(:active).uuid, head_uuid: ob.uuid, link_class: 'permission', @@ -37,7 +37,7 @@ class PermissionTest < ActiveSupport::TestCase test "permission links owned by root" do set_user_from_auth :active_trustedclient - ob = Specimen.create! + ob = Collection.create! perm_link = Link.create!(tail_uuid: users(:active).uuid, head_uuid: ob.uuid, link_class: 'permission', @@ -48,18 +48,18 @@ class PermissionTest < ActiveSupport::TestCase test "readable_by" do set_user_from_auth :active_trustedclient - ob = Specimen.create! + ob = Collection.create! Link.create!(tail_uuid: users(:active).uuid, head_uuid: ob.uuid, link_class: 'permission', name: 'can_read') - assert Specimen.readable_by(users(:active)).where(uuid: ob.uuid).any?, "user does not have read permission" + assert Collection.readable_by(users(:active)).where(uuid: ob.uuid).any?, "user does not have read permission" end test "writable_by" do set_user_from_auth :active_trustedclient - ob = Specimen.create! + ob = Collection.create! Link.create!(tail_uuid: users(:active).uuid, head_uuid: ob.uuid, link_class: 'permission', @@ -67,6 +67,34 @@ class PermissionTest < ActiveSupport::TestCase assert ob.writable_by.include?(users(:active).uuid), "user does not have write permission" end + test "update permission link" do + set_user_from_auth :admin + + grp = Group.create! name: "blah project", group_class: "project" + ob = Collection.create! owner_uuid: grp.uuid + + assert !users(:active).can?(write: ob) + assert !users(:active).can?(read: ob) + + l1 = Link.create!(tail_uuid: users(:active).uuid, + head_uuid: grp.uuid, + link_class: 'permission', + name: 'can_write') + + assert users(:active).can?(write: ob) + assert users(:active).can?(read: ob) + + l1.update_attributes!(name: 'can_read') + + assert !users(:active).can?(write: ob) + assert users(:active).can?(read: ob) + + l1.destroy + + assert !users(:active).can?(write: ob) + assert !users(:active).can?(read: ob) + end + test "writable_by reports requesting user's own uuid for a writable project" do invited_to_write = users(:project_viewer) group = groups(:asubproject) @@ -124,16 +152,16 @@ class PermissionTest < ActiveSupport::TestCase 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) + owner_grp = Group.create!(owner_uuid: users(:active).uuid, group_class: "role") + sp_grp = Group.create!(group_class: "project") Link.create!(link_class: 'permission', name: 'can_manage', tail_uuid: owner_grp.uuid, head_uuid: sp_grp.uuid) + sp = Collection.create!(owner_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 @@ -149,7 +177,7 @@ class PermissionTest < ActiveSupport::TestCase skip "can_manage permission on a non-group object" do set_user_from_auth :admin - ob = Specimen.create! + ob = Collection.create! # grant can_manage permission to active perm_link = Link.create!(tail_uuid: users(:active).uuid, head_uuid: ob.uuid, @@ -170,7 +198,7 @@ class PermissionTest < ActiveSupport::TestCase test "user without can_manage permission may not modify permission link" do set_user_from_auth :admin - ob = Specimen.create! + ob = Collection.create! # grant can_manage permission to active perm_link = Link.create!(tail_uuid: users(:active).uuid, head_uuid: ob.uuid, @@ -192,7 +220,8 @@ class PermissionTest < ActiveSupport::TestCase 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! + g = Group.create! name: "minon project", group_class: "project" + Collection.create! owner_uuid: g.uuid end # Manager creates a group. (Make sure it doesn't magically give # anyone any additional permissions.) @@ -255,7 +284,7 @@ class PermissionTest < ActiveSupport::TestCase create(:permission_link, name: 'can_manage', tail_uuid: manager.uuid, head_uuid: minion.uuid) end - assert_empty(Specimen + assert_empty(Collection .readable_by(manager) .where(uuid: minions_specimen.uuid), "manager saw the minion's private stuff") @@ -273,7 +302,7 @@ class PermissionTest < ActiveSupport::TestCase act_as_user manager do # Now, manager can read and write Minion's stuff. - assert_not_empty(Specimen + assert_not_empty(Collection .readable_by(manager) .where(uuid: minions_specimen.uuid), "manager could not find minion's specimen by uuid") @@ -309,12 +338,12 @@ class PermissionTest < ActiveSupport::TestCase "#{a.first_name} should be able to see 'b' in the user list") a_specimen = act_as_user a do - Specimen.create! + Collection.create! end - assert_not_empty(Specimen.readable_by(a).where(uuid: a_specimen.uuid), - "A cannot read own Specimen, following test probably useless.") - assert_empty(Specimen.readable_by(b).where(uuid: a_specimen.uuid), - "B can read A's Specimen") + assert_not_empty(Collection.readable_by(a).where(uuid: a_specimen.uuid), + "A cannot read own Collection, following test probably useless.") + assert_empty(Collection.readable_by(b).where(uuid: a_specimen.uuid), + "B can read A's Collection") [a,b].each do |u| assert_empty(User.readable_by(u).where(uuid: other.uuid), "#{u.first_name} can see OTHER in the user list") @@ -341,13 +370,13 @@ class PermissionTest < ActiveSupport::TestCase test "cannot create with owner = unwritable user" do set_user_from_auth :rominiadmin assert_raises ArvadosModel::PermissionDeniedError, "created with owner = unwritable user" do - Specimen.create!(owner_uuid: users(:active).uuid) + Collection.create!(owner_uuid: users(:active).uuid) end end test "cannot change owner to unwritable user" do set_user_from_auth :rominiadmin - ob = Specimen.create! + ob = Collection.create! assert_raises ArvadosModel::PermissionDeniedError, "changed owner to unwritable user" do ob.update_attributes!(owner_uuid: users(:active).uuid) end @@ -356,13 +385,13 @@ class PermissionTest < ActiveSupport::TestCase test "cannot create with owner = unwritable group" do set_user_from_auth :rominiadmin assert_raises ArvadosModel::PermissionDeniedError, "created with owner = unwritable group" do - Specimen.create!(owner_uuid: groups(:aproject).uuid) + Collection.create!(owner_uuid: groups(:aproject).uuid) end end test "cannot change owner to unwritable group" do set_user_from_auth :rominiadmin - ob = Specimen.create! + ob = Collection.create! assert_raises ArvadosModel::PermissionDeniedError, "changed owner to unwritable group" do ob.update_attributes!(owner_uuid: groups(:aproject).uuid) end diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb index 596cd415fb..7fcd36d709 100644 --- a/services/api/test/unit/user_test.rb +++ b/services/api/test/unit/user_test.rb @@ -478,15 +478,6 @@ class UserTest < ActiveSupport::TestCase vm = VirtualMachine.create - # Set up the bogus Link - bad_uuid = 'zzzzz-tpzed-xyzxyzxyzxyzxyz' - - resp_link = Link.create ({tail_uuid: email, link_class: 'permission', - name: 'can_login', head_uuid: bad_uuid}) - resp_link.save(validate: false) - - verify_link resp_link, 'permission', 'can_login', email, bad_uuid - response = user.setup(repo_name: 'foo/testrepo', vm_uuid: vm.uuid) -- 2.30.2