[ARVADOS] updated: 1.3.0-2669-gb612ef064
Git user
git at public.arvados.org
Mon Jun 15 22:18:31 UTC 2020
Summary of changes:
services/api/app/models/link.rb | 4 +-
services/api/app/models/user.rb | 13 +--
.../db/migrate/20200501150153_permission_table.rb | 31 +++---
services/api/db/structure.sql | 43 ++++----
.../20200501150153_permission_table_constants.rb | 9 +-
services/api/lib/update_permissions.rb | 13 ++-
services/api/test/unit/permission_test.rb | 119 +++++++++++++++++++++
7 files changed, 179 insertions(+), 53 deletions(-)
via b612ef0640ea45f03ad43ed4b124be1034d21071 (commit)
via 7105e7afc3436f95a09cc27e8a44e215a176dc38 (commit)
from f25b37a8e72716478e7cfae11ab5bf13b7051694 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit b612ef0640ea45f03ad43ed4b124be1034d21071
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Mon Jun 15 18:17:09 2020 -0400
16007: Handle overlapping permissions correctly
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index cd1ff40c2..21d89767c 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 d65cfb9c4..64facaa98 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 7b9a99813..0031bd766 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 a8885f584..affdf7e40 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 acf992432..6e43a628c 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 1d9c1006b..d6e653772 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 3f9408837..cb5ae7ba2 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
commit 7105e7afc3436f95a09cc27e8a44e215a176dc38
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Mon Jun 15 14:25:30 2020 -0400
16007: Add tests for overlapping permission links
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index d9383cf8e..3f9408837 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -455,4 +455,123 @@ class PermissionTest < ActiveSupport::TestCase
assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
end
+
+
+ test "add user to group, then change permission level" do
+ set_user_from_auth :admin
+ grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
+ col = Collection.create!(owner_uuid: grp.uuid)
+ assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
+ assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
+
+ l1 = Link.create!(tail_uuid: users(:active).uuid,
+ head_uuid: grp.uuid,
+ link_class: 'permission',
+ name: 'can_manage')
+ l2 = Link.create!(tail_uuid: grp.uuid,
+ head_uuid: users(:active).uuid,
+ link_class: 'permission',
+ name: 'can_read')
+
+ 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)
+
+ l1.name = 'can_read'
+ l1.save!
+
+ 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)
+
+ l1.name = 'can_write'
+ l1.save!
+
+ 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)
+ end
+
+
+ test "add user to group, then add overlapping permission link to group" do
+ set_user_from_auth :admin
+ grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
+ col = Collection.create!(owner_uuid: grp.uuid)
+ assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
+ assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
+
+ l1 = Link.create!(tail_uuid: users(:active).uuid,
+ head_uuid: grp.uuid,
+ link_class: 'permission',
+ name: 'can_manage')
+ l2 = Link.create!(tail_uuid: grp.uuid,
+ head_uuid: users(:active).uuid,
+ link_class: 'permission',
+ name: 'can_read')
+
+ 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)
+
+ l3 = Link.create!(tail_uuid: users(:active).uuid,
+ head_uuid: grp.uuid,
+ link_class: 'permission',
+ name: 'can_read')
+
+ 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)
+
+ l3.destroy!
+
+ 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)
+ end
+
+
+ test "add user to group, then add overlapping permission link to subproject" do
+ set_user_from_auth :admin
+ grp = Group.create!(owner_uuid: system_user_uuid, group_class: "project")
+ prj = Group.create!(owner_uuid: grp.uuid, group_class: "project")
+ assert_empty Group.readable_by(users(:active)).where(uuid: prj.uuid)
+ assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
+
+ l1 = Link.create!(tail_uuid: users(:active).uuid,
+ head_uuid: grp.uuid,
+ link_class: 'permission',
+ name: 'can_manage')
+ l2 = Link.create!(tail_uuid: grp.uuid,
+ head_uuid: users(:active).uuid,
+ link_class: 'permission',
+ name: 'can_read')
+
+ 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)
+
+ l3 = Link.create!(tail_uuid: grp.uuid,
+ head_uuid: prj.uuid,
+ link_class: 'permission',
+ name: 'can_read')
+
+ 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)
+
+ l3.destroy!
+
+ 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)
+ end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list