From b612ef0640ea45f03ad43ed4b124be1034d21071 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Mon, 15 Jun 2020 18:17:09 -0400 Subject: [PATCH] 16007: Handle overlapping permissions correctly Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- services/api/app/models/link.rb | 4 +- services/api/app/models/user.rb | 13 +++--- .../20200501150153_permission_table.rb | 31 +++++++------ services/api/db/structure.sql | 43 +++++++++---------- ...200501150153_permission_table_constants.rb | 9 ++-- services/api/lib/update_permissions.rb | 13 ++++-- services/api/test/unit/permission_test.rb | 4 +- 7 files changed, 62 insertions(+), 55 deletions(-) diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index cd1ff40c22..21d89767c7 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -74,13 +74,13 @@ class Link < ArvadosModel def call_update_permissions if self.link_class == 'permission' - update_permissions tail_uuid, head_uuid, PERM_LEVEL[name] + update_permissions tail_uuid, head_uuid, PERM_LEVEL[name], self.uuid end end def clear_permissions if self.link_class == 'permission' - update_permissions tail_uuid, head_uuid, REVOKE_PERM + update_permissions tail_uuid, head_uuid, REVOKE_PERM, self.uuid end end diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index d65cfb9c4f..64facaa98e 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -51,7 +51,7 @@ class User < ArvadosModel (not user.username_was.nil?) } before_destroy :clear_permissions - after_destroy :check_permissions + after_destroy :remove_self_from_permissions has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid has_many :repositories, foreign_key: :owner_uuid, primary_key: :uuid @@ -157,11 +157,11 @@ SELECT 1 FROM #{PERMISSION_VIEW} end def clear_permissions - update_permissions self.owner_uuid, self.uuid, REVOKE_PERM - MaterializedPermission.where("user_uuid = ? or target_uuid = ?", uuid, uuid).delete_all + MaterializedPermission.where("user_uuid = ? and target_uuid != ?", uuid, uuid).delete_all end - def check_permissions + def remove_self_from_permissions + MaterializedPermission.where("target_uuid = ?", uuid).delete_all check_permissions_against_full_refresh end @@ -371,6 +371,7 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2 raise "cannot merge to an already merged user" if new_user.redirect_to_user_uuid self.clear_permissions + new_user.clear_permissions # If 'self' is a remote user, don't transfer authorizations # (i.e. ability to access the account) to the new user, because @@ -447,11 +448,11 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2 update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil) end skip_check_permissions_against_full_refresh do - update_permissions self.owner_uuid, self.uuid, CAN_MANAGE_PERM update_permissions self.uuid, self.uuid, CAN_MANAGE_PERM + update_permissions new_user.uuid, new_user.uuid, CAN_MANAGE_PERM update_permissions new_user.owner_uuid, new_user.uuid, CAN_MANAGE_PERM end - update_permissions new_user.uuid, new_user.uuid, CAN_MANAGE_PERM + update_permissions self.owner_uuid, self.uuid, CAN_MANAGE_PERM end end diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb index 7b9a998139..0031bd766c 100644 --- a/services/api/db/migrate/20200501150153_permission_table.rb +++ b/services/api/db/migrate/20200501150153_permission_table.rb @@ -108,11 +108,14 @@ $$; # force query of all edges. ActiveRecord::Base.connection.execute %{ create view permission_graph_edges as - select groups.owner_uuid as tail_uuid, groups.uuid as head_uuid, (3) as val from groups + select groups.owner_uuid as tail_uuid, groups.uuid as head_uuid, + (3) as val, groups.uuid as edge_id from groups union all - select users.owner_uuid as tail_uuid, users.uuid as head_uuid, (3) as val from users + select users.owner_uuid as tail_uuid, users.uuid as head_uuid, + (3) as val, users.uuid as edge_id from users union all - select users.uuid as tail_uuid, users.uuid as head_uuid, (3) as val from users + select users.uuid as tail_uuid, users.uuid as head_uuid, + (3) as val, '' as edge_id from users union all select links.tail_uuid, links.head_uuid, @@ -122,7 +125,8 @@ union all WHEN links.name = 'can_write' THEN 2 WHEN links.name = 'can_manage' THEN 3 ELSE 0 - END as val + END as val, + links.uuid as edge_id from links where links.link_class='permission' } @@ -130,11 +134,10 @@ union all # Code fragment that is used below. This is used to ensure that # the permission edge passed into compute_permission_subgraph # takes precedence over an existing edge in the "edges" view. - override = %{, - case (edges.tail_uuid = perm_origin_uuid AND - edges.head_uuid = starting_uuid) + edge_perm = %{ +case (edges.edge_id = perm_edge_id) when true then starting_perm - else null + else edges.val end } @@ -149,7 +152,8 @@ union all ActiveRecord::Base.connection.execute %{ create or replace function compute_permission_subgraph (perm_origin_uuid varchar(27), starting_uuid varchar(27), - starting_perm integer) + starting_perm integer, + perm_edge_id varchar(27)) returns table (user_uuid varchar(27), target_uuid varchar(27), val integer, traverse_owned bool) STABLE language SQL @@ -182,7 +186,7 @@ with should_traverse_owned(starting_uuid, starting_perm), (perm_origin_uuid = starting_uuid or starting_uuid not like '_____-tpzed-_______________')) }, -:override => override +:edge_perm => edge_perm } }), /* Find other inbound edges that grant permissions to 'targets' in @@ -207,12 +211,11 @@ with 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 + where edges.edge_id != perm_edge_id 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 +:edge_perm => edge_perm } }), /* Combine the permissions computed in the first two phases. */ @@ -278,7 +281,7 @@ $$; drop_table :trashed_groups ActiveRecord::Base.connection.execute "DROP function project_subtree_with_trash_at (varchar, timestamp);" - ActiveRecord::Base.connection.execute "DROP function compute_permission_subgraph (varchar, varchar, integer);" + ActiveRecord::Base.connection.execute "DROP function compute_permission_subgraph (varchar, varchar, integer, varchar);" 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 a8885f584a..affdf7e401 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -39,10 +39,10 @@ CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA public; -- --- Name: compute_permission_subgraph(character varying, character varying, integer); Type: FUNCTION; Schema: public; Owner: - +-- Name: compute_permission_subgraph(character varying, character varying, integer, character varying); Type: FUNCTION; Schema: public; Owner: - -- -CREATE FUNCTION public.compute_permission_subgraph(perm_origin_uuid character varying, starting_uuid character varying, starting_perm integer) RETURNS TABLE(user_uuid character varying, target_uuid character varying, val integer, traverse_owned boolean) +CREATE FUNCTION public.compute_permission_subgraph(perm_origin_uuid character varying, starting_uuid character varying, starting_perm integer, perm_edge_id character varying) RETURNS TABLE(user_uuid character varying, target_uuid character varying, val integer, traverse_owned boolean) LANGUAGE sql STABLE AS $$ @@ -79,15 +79,13 @@ WITH RECURSIVE 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) + least( +case (edges.edge_id = perm_edge_id) when true then starting_perm - else null + else edges.val end -), +, + traverse_graph.val), should_traverse_owned(edges.head_uuid, edges.val), false from permission_graph_edges as edges, traverse_graph @@ -123,23 +121,20 @@ WITH RECURSIVE 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 + where edges.edge_id != perm_edge_id 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) + least( +case (edges.edge_id = perm_edge_id) when true then starting_perm - else null + else edges.val end -), +, + traverse_graph.val), should_traverse_owned(edges.head_uuid, edges.val), false from permission_graph_edges as edges, traverse_graph @@ -1014,17 +1009,20 @@ CREATE TABLE public.users ( CREATE VIEW public.permission_graph_edges AS SELECT groups.owner_uuid AS tail_uuid, groups.uuid AS head_uuid, - 3 AS val + 3 AS val, + groups.uuid AS edge_id FROM public.groups UNION ALL SELECT users.owner_uuid AS tail_uuid, users.uuid AS head_uuid, - 3 AS val + 3 AS val, + users.uuid AS edge_id FROM public.users UNION ALL SELECT users.uuid AS tail_uuid, users.uuid AS head_uuid, - 3 AS val + 3 AS val, + ''::character varying AS edge_id FROM public.users UNION ALL SELECT links.tail_uuid, @@ -1035,7 +1033,8 @@ UNION ALL WHEN ((links.name)::text = 'can_write'::text) THEN 2 WHEN ((links.name)::text = 'can_manage'::text) THEN 3 ELSE 0 - END AS val + END AS val, + links.uuid AS edge_id FROM public.links WHERE ((links.link_class)::text = 'permission'::text); diff --git a/services/api/lib/20200501150153_permission_table_constants.rb b/services/api/lib/20200501150153_permission_table_constants.rb index acf992432d..6e43a628c7 100644 --- a/services/api/lib/20200501150153_permission_table_constants.rb +++ b/services/api/lib/20200501150153_permission_table_constants.rb @@ -28,7 +28,7 @@ TRASHED_GROUPS = "trashed_groups" # going with the brute force approach of inlining the whole thing. # # The two substitutions are "base_case" which determines the initial -# set of permission origins and "override" which is used to ensure +# set of permission origins and "edge_perm" which is used to ensure # that the new permission takes precedence over the one in the edges # table (but some queries don't need that.) # @@ -39,9 +39,8 @@ WITH RECURSIVE union (select traverse_graph.origin_uuid, edges.head_uuid, - least(edges.val, - traverse_graph.val - %{override}), + least(%{edge_perm}, + traverse_graph.val), should_traverse_owned(edges.head_uuid, edges.val), false from permission_graph_edges as edges, traverse_graph @@ -79,7 +78,7 @@ INSERT INTO materialized_permissions #{PERM_QUERY_TEMPLATE % {:base_case => %{ select uuid, uuid, 3, true, true from users }, -:override => '' +:edge_perm => 'edges.val' } } }, "refresh_permission_view.do" end diff --git a/services/api/lib/update_permissions.rb b/services/api/lib/update_permissions.rb index 1d9c1006b8..d6e6537725 100644 --- a/services/api/lib/update_permissions.rb +++ b/services/api/lib/update_permissions.rb @@ -7,7 +7,7 @@ require '20200501150153_permission_table_constants' REVOKE_PERM = 0 CAN_MANAGE_PERM = 3 -def update_permissions perm_origin_uuid, starting_uuid, perm_level +def update_permissions perm_origin_uuid, starting_uuid, perm_level, edge_id=nil # # Update a subset of the permission table affected by adding or # removing a particular permission relationship (ownership or a @@ -49,6 +49,10 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level # see db/migrate/20200501150153_permission_table.rb for details on # how the permissions are computed. + if edge_id.nil? + edge_id = starting_uuid + end + ActiveRecord::Base.transaction do # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE @@ -95,12 +99,13 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}" ActiveRecord::Base.connection.exec_query %{ create temporary table #{temptable_perms} on commit drop -as select * from compute_permission_subgraph($1, $2, $3) +as select * from compute_permission_subgraph($1, $2, $3, $4) }, 'update_permissions.select', [[nil, perm_origin_uuid], [nil, starting_uuid], - [nil, perm_level]] + [nil, perm_level], + [nil, edge_id]] ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;" @@ -146,7 +151,7 @@ order by user_uuid, target_uuid #{PERM_QUERY_TEMPLATE % {:base_case => %{ select uuid, uuid, 3, true, true from users }, -:override => '' +:edge_perm => 'edges.val' } }) as pq order by origin_uuid, target_uuid }, "check_permissions_against_full_refresh.full_recompute" diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index 3f9408837a..cb5ae7ba2f 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -532,7 +532,7 @@ class PermissionTest < ActiveSupport::TestCase assert Collection.readable_by(users(:active)).where(uuid: col.uuid).first assert users(:active).can?(read: col.uuid) assert users(:active).can?(write: col.uuid) - assert !users(:active).can?(manage: col.uuid) + assert users(:active).can?(manage: col.uuid) end @@ -572,6 +572,6 @@ class PermissionTest < ActiveSupport::TestCase assert Group.readable_by(users(:active)).where(uuid: prj.uuid).first assert users(:active).can?(read: prj.uuid) assert users(:active).can?(write: prj.uuid) - assert !users(:active).can?(manage: prj.uuid) + assert users(:active).can?(manage: prj.uuid) end end -- 2.30.2