[ARVADOS] updated: 1.3.0-2615-gb879b9cd1
Git user
git at public.arvados.org
Fri May 29 02:31:40 UTC 2020
Summary of changes:
build/run-build-packages-one-target.sh | 16 ++
doc/admin/federation.html.textile.liquid | 2 +-
.../arvados-on-kubernetes-GKE.html.textile.liquid | 6 +-
...ados-on-kubernetes-minikube.html.textile.liquid | 6 +-
sdk/python/arvados/keep.py | 4 +
sdk/python/arvados/util.py | 5 +-
.../db/migrate/20200501150153_permission_table.rb | 308 ++++++++++++---------
services/api/db/structure.sql | 175 +++++++++++-
services/api/test/unit/permission_test.rb | 36 +++
9 files changed, 401 insertions(+), 157 deletions(-)
discards a4ade714a38980e239e9bc01244cba6b33575206 (commit)
discards c36d5cb91099d79184b85388829d637f842be34e (commit)
discards a5c857c5ab354d3b0e6a51653d0f1f21c108e131 (commit)
discards dc6710242d28803f085e92acd2ff6ad313a51860 (commit)
discards 2353613d6a767eade497ba33808574835837526f (commit)
via b879b9cd18ddba6ba87b65f81eba676114478a06 (commit)
via 0cff40366028dc1fbaf186e3109cde6379942381 (commit)
via fc5742654641a10e765ed81d25ca44cb47976d02 (commit)
via decdb6f9c5961573dc1b0d0aafd3450c517d3ae3 (commit)
via f87e2011a2f8048927cffe329fd2f7119bc08f46 (commit)
via 74be9180943834aebba7683418b4c9700ef16192 (commit)
via 6ba9fdc006d023fe1a56cd28fb87a54dff5265d8 (commit)
via 84c91acae26c09d7c9195eeb34e872964c0ccbc3 (commit)
via 6090fe3accf8a305a78e888a35fc6c882f8d13d0 (commit)
via f98e61d49ee0219dab4de3e32d789b4f25cdef68 (commit)
via 1115cfbeca79c55c9060c752b04e602b13d6a343 (commit)
via abdcc90b12348e7406abb63a9583653375f1c729 (commit)
via 5fcca42249b8b35f50beb9ed4c51d090d76c1767 (commit)
via fd43686beea061253fc1f936b14d9fa601e73f02 (commit)
This update added new revisions after undoing existing revisions. That is
to say, the old revision is not a strict subset of the new revision. This
situation occurs when you --force push a change and generate a repository
containing something like this:
* -- * -- B -- O -- O -- O (a4ade714a38980e239e9bc01244cba6b33575206)
\
N -- N -- N (b879b9cd18ddba6ba87b65f81eba676114478a06)
When this happens we assume that you've already had alert emails for all
of the O revisions, and so we here report only the revisions in the N
branch from the common base, B.
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 b879b9cd18ddba6ba87b65f81eba676114478a06
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu May 28 22:22:57 2020 -0400
16007: Add comment about override_edge_* parameters
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index 4d7bc25ba..77944eb6e 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -156,6 +156,13 @@ as $$
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.
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 508d90757..eaa3d6299 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -257,6 +257,13 @@ CREATE FUNCTION public.search_permission_graph(starting_uuid character varying,
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.
commit 0cff40366028dc1fbaf186e3109cde6379942381
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu May 28 21:41:46 2020 -0400
16007: Ensure that updated permission edge overrides edges view
An edge originating from a user can be traversed more than once, if
that edge is the same as the one being updated, ensure that it uses
the updated permission level and not the permission from the edges
view. Necessary when revoking permissions.
Also moved comments into the body of the postgres functions to bring
them closer to the code, this also has the convenient effect of having
the comments appear in structure.sql function definitions so as to be
easier for future developers to find.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index ccb32c883..4d7bc25ba 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -30,22 +30,21 @@ class PermissionTable < ActiveRecord::Migration[5.0]
end
add_index :trashed_groups, :group_uuid, :unique => true
- #
- # Starting from a project, recursively traverse all the projects
- # underneath it and return a set of project uuids and trash_at
- # times (may be null). The initial trash_at can be a timestamp or
- # null. The trash_at time propagates downward to groups it owns,
- # i.e. when a group is trashed, everything underneath it in the
- # ownership hierarchy is also considered trashed. However, this
- # is fact is recorded in the trashed_groups table, not by updating
- # trash_at field in the groups table.
- #
ActiveRecord::Base.connection.execute %{
create or replace function project_subtree_with_trash_at (starting_uuid varchar(27), starting_trash_at timestamp)
returns table (target_uuid varchar(27), trash_at timestamp)
STABLE
language SQL
as $$
+/* Starting from a project, recursively traverse all the projects
+ underneath it and return a set of project uuids and trash_at times
+ (may be null). The initial trash_at can be a timestamp or null.
+ The trash_at time propagates downward to groups it owns, i.e. when a
+ group is trashed, everything underneath it in the ownership
+ hierarchy is also considered trashed. However, this is fact is
+ recorded in the trashed_groups table, not by updating trash_at field
+ in the groups table.
+*/
WITH RECURSIVE
project_subtree(uuid, trash_at) as (
values (starting_uuid, starting_trash_at)
@@ -57,15 +56,16 @@ WITH RECURSIVE
$$;
}
- # Helper function to populate trashed_groups table. This starts
- # with each group owned by a user and computes the subtree under
- # that group to find any groups that are trashed.
ActiveRecord::Base.connection.execute %{
create or replace function compute_trashed ()
returns table (uuid varchar(27), trash_at timestamp)
STABLE
language SQL
as $$
+/* Helper function to populate trashed_groups table. This starts with
+ each group owned by a user and computes the subtree under that
+ group to find any groups that are trashed.
+*/
select ps.target_uuid as group_uuid, ps.trash_at from groups,
lateral project_subtree_with_trash_at(groups.uuid, groups.trash_at) ps
where groups.owner_uuid like '_____-tpzed-_______________'
@@ -77,7 +77,6 @@ $$;
# incrementally. See app/models/group.rb#update_trash
ActiveRecord::Base.connection.execute("INSERT INTO trashed_groups select * from compute_trashed()")
-
# The table to store the flattened permissions. This is almost
# exactly the same as the old materalized_permission_view except
# that the target_owner_uuid colunm in the view is now just a
@@ -96,10 +95,6 @@ $$;
add_index :materialized_permissions, [:user_uuid, :target_uuid], unique: true, name: 'permission_user_target'
add_index :materialized_permissions, [:target_uuid], unique: false, name: 'permission_target'
- # Helper function. Determines if permission on an object implies
- # transitive permission to things the object owns. This is always
- # true for groups, but only true for users when the permission
- # level is can_manage.
ActiveRecord::Base.connection.execute %{
create or replace function should_traverse_owned (starting_uuid varchar(27),
starting_perm integer)
@@ -107,6 +102,11 @@ create or replace function should_traverse_owned (starting_uuid varchar(27),
IMMUTABLE
language SQL
as $$
+/* Helper function. Determines if permission on an object implies
+ transitive permission to things the object owns. This is always
+ true for groups, but only true for users when the permission level
+ is can_manage.
+*/
select starting_uuid like '_____-j7d0g-_______________' or
(starting_uuid like '_____-tpzed-_______________' and starting_perm >= 3);
$$;
@@ -138,40 +138,51 @@ union all
where links.link_class='permission'
}
- # 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).
- #
- # 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 compute_permission_subgraph below.
ActiveRecord::Base.connection.execute %{
create or replace function search_permission_graph (starting_uuid varchar(27),
- starting_perm integer)
+ 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).
+
+ 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),
+ 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))
@@ -180,100 +191,6 @@ WITH RECURSIVE
$$;
}
- # This is the key function.
- #
- # perm_origin_uuid: The object that 'gets' or 'has' the permission.
- #
- # starting_uuid: The starting object the permission applies to.
- #
- # starting_perm: The permission that perm_origin_uuid 'has' on starting_uuid
- # One of 1, 2, 3 for can_read, can_write, can_manage
- # respectively, or 0 to revoke permissions.
- #
- # This function is broken up into a number of phases.
- #
- # 1. perm_from_start: Gets the initial set of objects potentially
- # affected by the permission change, using
- # search_permission_graph.
- #
- # 2. additional_perms: Finds other inbound edges that grant
- # permissions on the objects in perm_from_start, and computes
- # permissions that originate from those. This is required to
- # handle the case where there is more than one path through which
- # a user gets permission to an object. For example, a user owns a
- # project and also shares it can_read with a group the user
- # belongs to, adding the can_read link must not overwrite the
- # existing can_manage permission granted by ownership.
- #
- # 3. partial_perms: Combine the permissions computed in the first two phases.
- #
- # 4. user_identity_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.
- #
- # 5. all_perms: Combines perm_from_start, additional_perms, and user_identity_perms.
- #
- # 6. The actual query that produces rows to be added or removed
- # from the materialized_permissions table. This is the clever
- # bit.
- #
- # Key insights:
- #
- # * Permissions are transitive (with some special cases involving
- # users, this is controlled by the traverse_owned flag).
- #
- # * A user object can only gain permissions via an inbound edge,
- # or appearing in the graph.
- #
- # * The materialized_permissions table includes the permission
- # each user has on the tail end of each inbound edge.
- #
- # * The all_perms subquery has permissions for each object in the
- # subgraph reachable from certain origin (tail end of an edge).
- #
- # * Therefore, for each user, we can compute user permissions on
- # each object in subgraph by determining the permission the user
- # has on each origin (tail end of an edge), joining that with the
- # perm_origin_uuid column of all_perms, and taking the least() of
- # the origin edge or all_perms val (because of the "least
- # permission on the path" rule). If an object was reachable by
- # more than one path (appears with more than one origin), we take
- # the max() of the computed permissions.
- #
- # Finally, because users always have permission on themselves, the
- # query also makes sure those permission rows are always
- # returned.
- #
- # Notes on query optimization:
- #
- # Each clause in a "with" statement is called a "common table
- # expression" or CTE.
- #
- # In Postgres, they are evaluated in sequence and results of each
- # CTE is stored in a temporary table. This means Postgres does
- # not propagate constraints from later queries to earlier CTEs.
- #
- # This is a problem if, for example, a later CTE only needs to
- # choose 10 items out of a set of 1000000 from an earlier CTE,
- # because it will always compute all 1000000 rows even if the
- # query on the 1000000 rows could have been constrained. This is
- # why permission_graph_edges is a view and not a CTE -- views are
- # inlined so and can be optimized using external constraints.
- #
- # The query optimizer does sort the temporary tables for later use
- # in joins.
- #
- # Final note, this query would have been almost impossible to
- # write (and certainly impossible to read) without using SQL
- # "with" and CTEs but unfortunately it also stumbles into a
- # frustrating Postgres optimizer bug, see
- # lib/refresh_permission_view.rb#update_permissions for details
- # and a partial workaround.
- #
ActiveRecord::Base.connection.execute %{
create or replace function compute_permission_subgraph (perm_origin_uuid varchar(27),
starting_uuid varchar(27),
@@ -282,40 +199,144 @@ returns table (user_uuid varchar(27), target_uuid varchar(27), val integer, trav
STABLE
language SQL
as $$
+/* perm_origin_uuid: The object that 'gets' or 'has' the permission.
+
+ starting_uuid: The starting object the permission applies to.
+
+ starting_perm: The permission that perm_origin_uuid 'has' on
+ starting_uuid One of 1, 2, 3 for can_read,
+ can_write, can_manage respectively, or 0 to revoke
+ permissions.
+
+ This function is broken up into a number of clauses, described
+ below.
+
+ Note on query optimization:
+
+ Each clause in a "with" statement is called a "common table
+ expression" or CTE.
+
+ In Postgres, they are evaluated in sequence and results of each CTE
+ is stored in a temporary table. This means Postgres does not
+ propagate constraints from later subqueries to earlier subqueries
+ when they are CTEs.
+
+ This is a problem if, for example, a later subquery chooses 10
+ items out of a set of 1000000 defined by an earlier subquery,
+ because it will always compute all 1000000 rows even if the query
+ on the 1000000 rows could have been constrained. This is why
+ permission_graph_edges is a view -- views are inlined so and can be
+ optimized using external constraints.
+
+ The query optimizer does sort the temporary tables for later use in
+ joins.
+
+ Final note, this query would have been almost impossible to write
+ (and certainly impossible to read) without splitting it up using
+ SQL "with" but unfortunately it also stumbles into a frustrating
+ Postgres optimizer bug, see
+ lib/refresh_permission_view.rb#update_permissions
+ for details and a partial workaround.
+*/
with
+ /* Gets the initial set of objects potentially affected by the
+ 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)),
+ from search_permission_graph(starting_uuid,
+ starting_perm,
+ perm_origin_uuid,
+ starting_uuid,
+ starting_perm)),
+ /* Finds other inbound edges that grant permissions on the objects
+ in perm_from_start, and computes permissions that originate from
+ those. This is required to handle the case where there is more
+ than one path through which a user gets permission to an object.
+ For example, a user owns a project and also shares it can_read
+ with a group the user belongs to, adding the can_read link must
+ not overwrite the existing can_manage permission granted by
+ 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) as ps
+ from permission_graph_edges as edges,
+ lateral search_permission_graph(edges.head_uuid,
+ edges.val,
+ perm_origin_uuid,
+ starting_uuid,
+ starting_perm) as ps
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)),
+ /* Combines the permissions computed in the first two phases. */
partial_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) as ps
+ 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
+ from the materialized_permissions table. This is the clever
+ bit.
+
+ Key insights:
+
+ * Permissions are transitive (with some special cases involving
+ users, this is controlled by the traverse_owned flag).
+
+ * A user object can only gain permissions via an inbound edge,
+ or appearing in the graph.
+
+ * The materialized_permissions table includes the permission
+ each user has on the tail end of each inbound edge.
+
+ * The all_perms subquery has permissions for each object in the
+ subgraph reachable from certain origin (tail end of an edge).
+
+ * Therefore, for each user, we can compute user permissions on
+ each object in subgraph by determining the permission the user
+ has on each origin (tail end of an edge), joining that with the
+ perm_origin_uuid column of all_perms, and taking the least() of
+ the origin edge or all_perms val (because of the "least
+ permission on the path" rule). If an object was reachable by
+ more than one path (appears with more than one origin), we take
+ the max() of the computed permissions.
+
+ * Finally, because users always have permission on themselves, the
+ query also makes sure those permission rows are always
+ returned.
+ */
select v.user_uuid, v.target_uuid, max(v.perm_level), bool_or(v.traverse_owned) from
(select m.user_uuid,
u.target_uuid,
@@ -348,7 +369,7 @@ 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);"
+ 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 c4bf90dca..508d90757 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -45,40 +45,144 @@ CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA public;
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)
LANGUAGE sql STABLE
AS $$
+/* perm_origin_uuid: The object that 'gets' or 'has' the permission.
+
+ starting_uuid: The starting object the permission applies to.
+
+ starting_perm: The permission that perm_origin_uuid 'has' on
+ starting_uuid One of 1, 2, 3 for can_read,
+ can_write, can_manage respectively, or 0 to revoke
+ permissions.
+
+ This function is broken up into a number of clauses, described
+ below.
+
+ Note on query optimization:
+
+ Each clause in a "with" statement is called a "common table
+ expression" or CTE.
+
+ In Postgres, they are evaluated in sequence and results of each CTE
+ is stored in a temporary table. This means Postgres does not
+ propagate constraints from later subqueries to earlier subqueries
+ when they are CTEs.
+
+ This is a problem if, for example, a later subquery chooses 10
+ items out of a set of 1000000 defined by an earlier subquery,
+ because it will always compute all 1000000 rows even if the query
+ on the 1000000 rows could have been constrained. This is why
+ permission_graph_edges is a view -- views are inlined so and can be
+ optimized using external constraints.
+
+ The query optimizer does sort the temporary tables for later use in
+ joins.
+
+ Final note, this query would have been almost impossible to write
+ (and certainly impossible to read) without splitting it up using
+ SQL "with" but unfortunately it also stumbles into a frustrating
+ Postgres optimizer bug, see
+ lib/refresh_permission_view.rb#update_permissions
+ for details and a partial workaround.
+*/
with
+ /* Gets the initial set of objects potentially affected by the
+ 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)),
-
+ from search_permission_graph(starting_uuid,
+ starting_perm,
+ perm_origin_uuid,
+ starting_uuid,
+ starting_perm)),
+
+ /* Finds other inbound edges that grant permissions on the objects
+ in perm_from_start, and computes permissions that originate from
+ those. This is required to handle the case where there is more
+ than one path through which a user gets permission to an object.
+ For example, a user owns a project and also shares it can_read
+ with a group the user belongs to, adding the can_read link must
+ not overwrite the existing can_manage permission granted by
+ 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) as ps
+ from permission_graph_edges as edges,
+ lateral search_permission_graph(edges.head_uuid,
+ edges.val,
+ perm_origin_uuid,
+ starting_uuid,
+ starting_perm) as ps
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)),
+ /* Combines the permissions computed in the first two phases. */
partial_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) as ps
+ 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
+ from the materialized_permissions table. This is the clever
+ bit.
+
+ Key insights:
+
+ * Permissions are transitive (with some special cases involving
+ users, this is controlled by the traverse_owned flag).
+
+ * A user object can only gain permissions via an inbound edge,
+ or appearing in the graph.
+
+ * The materialized_permissions table includes the permission
+ each user has on the tail end of each inbound edge.
+
+ * The all_perms subquery has permissions for each object in the
+ subgraph reachable from certain origin (tail end of an edge).
+
+ * Therefore, for each user, we can compute user permissions on
+ each object in subgraph by determining the permission the user
+ has on each origin (tail end of an edge), joining that with the
+ perm_origin_uuid column of all_perms, and taking the least() of
+ the origin edge or all_perms val (because of the "least
+ permission on the path" rule). If an object was reachable by
+ more than one path (appears with more than one origin), we take
+ the max() of the computed permissions.
+
+ * Finally, because users always have permission on themselves, the
+ query also makes sure those permission rows are always
+ returned.
+ */
select v.user_uuid, v.target_uuid, max(v.perm_level), bool_or(v.traverse_owned) from
(select m.user_uuid,
u.target_uuid,
@@ -101,6 +205,10 @@ $$;
CREATE FUNCTION public.compute_trashed() RETURNS TABLE(uuid character varying, trash_at timestamp without time zone)
LANGUAGE sql STABLE
AS $$
+/* Helper function to populate trashed_groups table. This starts with
+ each group owned by a user and computes the subtree under that
+ group to find any groups that are trashed.
+*/
select ps.target_uuid as group_uuid, ps.trash_at from groups,
lateral project_subtree_with_trash_at(groups.uuid, groups.trash_at) ps
where groups.owner_uuid like '_____-tpzed-_______________'
@@ -114,6 +222,15 @@ $$;
CREATE FUNCTION public.project_subtree_with_trash_at(starting_uuid character varying, starting_trash_at timestamp without time zone) RETURNS TABLE(target_uuid character varying, trash_at timestamp without time zone)
LANGUAGE sql STABLE
AS $$
+/* Starting from a project, recursively traverse all the projects
+ underneath it and return a set of project uuids and trash_at times
+ (may be null). The initial trash_at can be a timestamp or null.
+ The trash_at time propagates downward to groups it owns, i.e. when a
+ group is trashed, everything underneath it in the ownership
+ hierarchy is also considered trashed. However, this is fact is
+ recorded in the trashed_groups table, not by updating trash_at field
+ in the groups table.
+*/
WITH RECURSIVE
project_subtree(uuid, trash_at) as (
values (starting_uuid, starting_trash_at)
@@ -126,23 +243,47 @@ $$;
--
--- Name: search_permission_graph(character varying, integer); Type: FUNCTION; Schema: public; Owner: -
+-- 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) RETURNS TABLE(target_uuid character varying, val integer, traverse_owned boolean)
+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).
+
+ 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),
+ 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))
@@ -158,6 +299,11 @@ $$;
CREATE FUNCTION public.should_traverse_owned(starting_uuid character varying, starting_perm integer) RETURNS boolean
LANGUAGE sql IMMUTABLE
AS $$
+/* Helper function. Determines if permission on an object implies
+ transitive permission to things the object owns. This is always
+ true for groups, but only true for users when the permission level
+ is can_manage.
+*/
select starting_uuid like '_____-j7d0g-_______________' or
(starting_uuid like '_____-tpzed-_______________' and starting_perm >= 3);
$$;
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 18d2fbbcb..a0478ec54 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -390,4 +390,40 @@ class PermissionTest < ActiveSupport::TestCase
assert_not_empty container_logs(:running_older, :anonymous)
end
+
+ test "add user to group, then remove them" 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_read')
+ l2 = Link.create!(tail_uuid: grp.uuid,
+ head_uuid: users(:active).uuid,
+ link_class: 'permission',
+ name: 'can_read')
+
+ l3 = Link.create!(tail_uuid: users(:project_viewer).uuid,
+ head_uuid: grp.uuid,
+ link_class: 'permission',
+ name: 'can_read')
+ l4 = Link.create!(tail_uuid: grp.uuid,
+ head_uuid: users(:project_viewer).uuid,
+ link_class: 'permission',
+ name: 'can_read')
+
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).first
+ assert User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid).first
+
+ l1.destroy
+ l2.destroy
+
+ assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
+ assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
+
+ end
end
commit fc5742654641a10e765ed81d25ca44cb47976d02
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu May 28 15:35:29 2020 -0400
16007: Enable permission correctness checking (only for tests)
* Explicitly set up a transaction in update_permissions
* Rename refresh_permission_view.rb -> update_permissions.rb
* Add skip_check_permissions_against_full_refresh
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/controllers/database_controller.rb b/services/api/app/controllers/database_controller.rb
index 24c6cf5a7..5c4cf7bc1 100644
--- a/services/api/app/controllers/database_controller.rb
+++ b/services/api/app/controllers/database_controller.rb
@@ -75,9 +75,9 @@ class DatabaseController < ApplicationController
raise
end
- require 'refresh_permission_view'
+ require 'update_permissions'
- refresh_permission_view
+ refresh_permissions
refresh_trashed
# Done.
diff --git a/services/api/app/models/database_seeds.rb b/services/api/app/models/database_seeds.rb
index 0fea2cf7b..39f491503 100644
--- a/services/api/app/models/database_seeds.rb
+++ b/services/api/app/models/database_seeds.rb
@@ -2,7 +2,7 @@
#
# SPDX-License-Identifier: AGPL-3.0
-require 'refresh_permission_view'
+require 'update_permissions'
class DatabaseSeeds
extend CurrentApiClient
@@ -14,7 +14,7 @@ class DatabaseSeeds
anonymous_group_read_permission
anonymous_user
empty_collection
- refresh_permission_view
+ refresh_permissions
refresh_trashed
end
end
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 9b8a9877e..485205f1e 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -80,7 +80,7 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
def before_ownership_change
if owner_uuid_changed? and !self.owner_uuid_was.nil?
MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
- update_permissions self.owner_uuid, self.uuid, 0
+ update_permissions self.owner_uuid_was, self.uuid, 0
end
end
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 33e18b296..da4ca6c87 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -86,7 +86,7 @@ class Link < ArvadosModel
def check_permissions
if self.link_class == 'permission'
- #check_permissions_against_full_refresh
+ check_permissions_against_full_refresh
end
end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index d48dde25a..cb7efe9cc 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -436,7 +436,9 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
if redirect_to_new_user
update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil)
end
- update_permissions self.owner_uuid, self.uuid, 3, false
+ skip_check_permissions_against_full_refresh do
+ update_permissions self.owner_uuid, self.uuid, 3
+ end
update_permissions new_user.owner_uuid, new_user.uuid, 3
end
end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 5f71554ff..c4bf90dca 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -46,9 +46,9 @@ CREATE FUNCTION public.compute_permission_subgraph(perm_origin_uuid character va
LANGUAGE sql STABLE
AS $$
with
-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_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)),
additional_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
select edges.tail_uuid as perm_origin_uuid, ps.target_uuid, ps.val,
@@ -156,7 +156,7 @@ $$;
--
CREATE FUNCTION public.should_traverse_owned(starting_uuid character varying, starting_perm integer) RETURNS boolean
- LANGUAGE sql STABLE
+ LANGUAGE sql IMMUTABLE
AS $$
select starting_uuid like '_____-j7d0g-_______________' or
(starting_uuid like '_____-tpzed-_______________' and starting_perm >= 3);
diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/update_permissions.rb
similarity index 52%
rename from services/api/lib/refresh_permission_view.rb
rename to services/api/lib/update_permissions.rb
index 826c44c3b..b2cf6595b 100644
--- a/services/api/lib/refresh_permission_view.rb
+++ b/services/api/lib/update_permissions.rb
@@ -5,7 +5,7 @@
PERMISSION_VIEW = "materialized_permissions"
TRASHED_GROUPS = "trashed_groups"
-def refresh_permission_view
+def refresh_permissions
ActiveRecord::Base.transaction do
ActiveRecord::Base.connection.execute("LOCK TABLE #{PERMISSION_VIEW}")
ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW}")
@@ -26,7 +26,7 @@ def refresh_trashed
end
end
-def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
+def update_permissions perm_origin_uuid, starting_uuid, perm_level
#
# Update a subset of the permission table affected by adding or
# removing a particular permission relationship (ownership or a
@@ -45,7 +45,7 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
# can_manage=3
# or call with perm_level=0 to revoke permissions
#
- # check: for testing/debugging only, compare the result of the
+ # check: for testing/debugging, compare the result of the
# incremental update against a full table recompute. Throws an
# error if the contents are not identical (ie they produce different
# permission results)
@@ -68,60 +68,62 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
# see db/migrate/20200501150153_permission_table.rb for details on
# how the permissions are computed.
- # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE
- # ROW EXCLUSIVE, EXCLUSIVE, and ACCESS EXCLUSIVE lock modes. This
- # mode protects a table against concurrent data changes."
- ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE"
-
- # Workaround for
- # BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE
- # https://www.postgresql.org/message-id/152395805004.19366.3107109716821067806@wrigleys.postgresql.org
- #
- # For a crucial join in the compute_permission_subgraph() query, the
- # planner mis-estimates the number of rows in a Common Table
- # Expression (CTE, this is a subquery in a WITH clause) and as a
- # result it chooses the wrong join order. The join starts with the
- # permissions table because it mistakenly thinks
- # count(materalized_permissions) < count(new computed permissions)
- # when actually it is the other way around.
- #
- # Because of the incorrect join order, it choose the wrong join
- # strategy (merge join, which works best when two tables are roughly
- # the same size). As a workaround, we can tell it not to use that
- # join strategy, this causes it to pick hash join instead, which
- # turns out to be a bit better. However, because the join order is
- # still wrong, we don't get the full benefit of the index.
- #
- # This is very unfortunate because it makes the query performance
- # dependent on the size of the materalized_permissions table, when
- # the goal of this design was to make permission updates scale-free
- # and only depend on the number of permissions affected and not the
- # total table size. In several hours of researching I wasn't able
- # to find a way to force the correct join order, so I'm calling it
- # here and I have to move on.
- #
- # This is apparently addressed in Postgres 12, but I developed &
- # tested this on Postgres 9.6, so in the future we should reevaluate
- # the performance & query plan on Postgres 12.
- #
- # https://git.furworks.de/opensourcemirror/postgresql/commit/a314c34079cf06d05265623dd7c056f8fa9d577f
- #
- # Disable merge join for just this query (also local for this transaction), then reenable it.
- ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;"
+ ActiveRecord::Base.transaction do
- temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}"
- ActiveRecord::Base.connection.exec_query %{
+ # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE
+ # ROW EXCLUSIVE, EXCLUSIVE, and ACCESS EXCLUSIVE lock modes. This
+ # mode protects a table against concurrent data changes."
+ ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE"
+
+ # Workaround for
+ # BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE
+ # https://www.postgresql.org/message-id/152395805004.19366.3107109716821067806@wrigleys.postgresql.org
+ #
+ # For a crucial join in the compute_permission_subgraph() query, the
+ # planner mis-estimates the number of rows in a Common Table
+ # Expression (CTE, this is a subquery in a WITH clause) and as a
+ # result it chooses the wrong join order. The join starts with the
+ # permissions table because it mistakenly thinks
+ # count(materalized_permissions) < count(new computed permissions)
+ # when actually it is the other way around.
+ #
+ # Because of the incorrect join order, it choose the wrong join
+ # strategy (merge join, which works best when two tables are roughly
+ # the same size). As a workaround, we can tell it not to use that
+ # join strategy, this causes it to pick hash join instead, which
+ # turns out to be a bit better. However, because the join order is
+ # still wrong, we don't get the full benefit of the index.
+ #
+ # This is very unfortunate because it makes the query performance
+ # dependent on the size of the materalized_permissions table, when
+ # the goal of this design was to make permission updates scale-free
+ # and only depend on the number of permissions affected and not the
+ # total table size. In several hours of researching I wasn't able
+ # to find a way to force the correct join order, so I'm calling it
+ # here and I have to move on.
+ #
+ # This is apparently addressed in Postgres 12, but I developed &
+ # tested this on Postgres 9.6, so in the future we should reevaluate
+ # the performance & query plan on Postgres 12.
+ #
+ # https://git.furworks.de/opensourcemirror/postgresql/commit/a314c34079cf06d05265623dd7c056f8fa9d577f
+ #
+ # Disable merge join for just this query (also local for this transaction), then reenable it.
+ ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;"
+
+ 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)
},
- 'update_permissions.select',
- [[nil, perm_origin_uuid],
- [nil, starting_uuid],
- [nil, perm_level]]
+ 'update_permissions.select',
+ [[nil, perm_origin_uuid],
+ [nil, starting_uuid],
+ [nil, perm_level]]
- ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;"
+ ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;"
- ActiveRecord::Base.connection.exec_delete %{
+ ActiveRecord::Base.connection.exec_delete %{
delete from #{PERMISSION_VIEW} where
target_uuid in (select target_uuid from #{temptable_perms}) and
not exists (select 1 from #{temptable_perms}
@@ -129,27 +131,29 @@ delete from #{PERMISSION_VIEW} where
user_uuid=#{PERMISSION_VIEW}.user_uuid and
val>0)
},
- "update_permissions.delete"
+ "update_permissions.delete"
- ActiveRecord::Base.connection.exec_query %{
+ ActiveRecord::Base.connection.exec_query %{
insert into #{PERMISSION_VIEW} (user_uuid, target_uuid, perm_level, traverse_owned)
select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms} where val>0
on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_level, traverse_owned=EXCLUDED.traverse_owned;
},
- "update_permissions.insert"
+ "update_permissions.insert"
- if check and perm_level>0
- check_permissions_against_full_refresh
+ if perm_level>0
+ check_permissions_against_full_refresh
+ end
end
end
def check_permissions_against_full_refresh
- #
- # For debugging, this checks contents of the
- # incrementally-updated 'materialized_permission' against a
- # from-scratch permission refresh.
- #
+ # No-op except when running tests
+ return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh]
+
+ # For checking correctness of the incremental permission updates.
+ # Check contents of the current 'materialized_permission' table
+ # against a from-scratch permission refresh.
q1 = ActiveRecord::Base.connection.exec_query %{
select user_uuid, target_uuid, perm_level, traverse_owned from #{PERMISSION_VIEW}
@@ -182,3 +186,13 @@ order by users.uuid, target_uuid
end
end
end
+
+def skip_check_permissions_against_full_refresh
+ check_perm_was = Thread.current[:no_check_permissions_against_full_refresh]
+ Thread.current[:no_check_permissions_against_full_refresh] = true
+ begin
+ yield
+ ensure
+ Thread.current[:no_check_permissions_against_full_refresh] = check_perm_was
+ end
+end
diff --git a/services/api/test/performance/permission_test.rb b/services/api/test/performance/permission_test.rb
index 0fea3b135..d0e6413b1 100644
--- a/services/api/test/performance/permission_test.rb
+++ b/services/api/test/performance/permission_test.rb
@@ -40,7 +40,7 @@ class PermissionPerfTest < ActionDispatch::IntegrationTest
end
end
end
- refresh_permission_view
+ refresh_permissions
end
end)
end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 12f729e33..c99a57aaf 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -2,7 +2,7 @@
#
# SPDX-License-Identifier: AGPL-3.0
-require 'refresh_permission_view'
+require 'update_permissions'
ENV["RAILS_ENV"] = "test"
unless ENV["NO_COVERAGE_TEST"]
@@ -209,5 +209,5 @@ class ActionDispatch::IntegrationTest
end
# Ensure permissions are computed from the test fixtures.
-refresh_permission_view
+refresh_permissions
refresh_trashed
diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb
index 2bdc198a3..ca02e2db5 100644
--- a/services/api/test/unit/owner_test.rb
+++ b/services/api/test/unit/owner_test.rb
@@ -87,9 +87,11 @@ class OwnerTest < ActiveSupport::TestCase
assert_equal(true, Specimen.where(owner_uuid: o.uuid).any?,
"need something to be owned by #{o.uuid} for this test")
- assert_raises(ActiveRecord::DeleteRestrictionError,
- "should not delete #{ofixt} that owns objects") do
- o.destroy
+ skip_check_permissions_against_full_refresh do
+ assert_raises(ActiveRecord::DeleteRestrictionError,
+ "should not delete #{ofixt} that owns objects") do
+ o.destroy
+ end
end
end
@@ -108,9 +110,14 @@ class OwnerTest < ActiveSupport::TestCase
assert User.where(uuid: o.uuid).any?, "new User should really be in DB"
assert_equal(true, o.update_attributes(owner_uuid: o.uuid),
"setting owner to self should work")
- assert(o.destroy, "should delete User that owns self")
+
+ skip_check_permissions_against_full_refresh do
+ assert(o.destroy, "should delete User that owns self")
+ end
+
assert_equal(false, User.where(uuid: o.uuid).any?,
"#{o.uuid} should not be in DB after deleting")
+ check_permissions_against_full_refresh
end
test "change uuid of User that owns self" do
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 5d25361ed..596cd415f 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -168,9 +168,6 @@ class UserTest < ActiveSupport::TestCase
act_as_system_user do
users(:admin).update_attributes!(is_admin: false)
end
- # need to manually call clear_permissions because we used 'delete' instead of 'destory'
- # we use delete here because 'destroy' will throw an error
- #users(:admin).clear_permissions
@all_users = User.where("uuid not like '%-000000000000000'").where(:is_admin => true)
assert_equal 0, @all_users.count, "No admin users should exist (except for the system user)"
end
commit decdb6f9c5961573dc1b0d0aafd3450c517d3ae3
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Tue May 26 15:12:56 2020 -0400
16007: refresh_trashed uses a transaction
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/refresh_permission_view.rb
index c0d5c1b69..826c44c3b 100644
--- a/services/api/lib/refresh_permission_view.rb
+++ b/services/api/lib/refresh_permission_view.rb
@@ -19,9 +19,11 @@ from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0
end
def refresh_trashed
+ ActiveRecord::Base.transaction do
ActiveRecord::Base.connection.execute("LOCK TABLE #{TRASHED_GROUPS}")
- ActiveRecord::Base.connection.execute("DELETE FROM #{TRASHED_GROUPS}")
- ActiveRecord::Base.connection.execute("INSERT INTO #{TRASHED_GROUPS} select * from compute_trashed()")
+ ActiveRecord::Base.connection.execute("DELETE FROM #{TRASHED_GROUPS}")
+ ActiveRecord::Base.connection.execute("INSERT INTO #{TRASHED_GROUPS} select * from compute_trashed()")
+ end
end
def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
commit f87e2011a2f8048927cffe329fd2f7119bc08f46
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Tue May 26 14:11:24 2020 -0400
16007: More code comment detail about compute_permission_subgraph query
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index d62141a76..ccb32c883 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -221,13 +221,58 @@ $$;
# from the materialized_permissions table. This is the clever
# bit.
#
- # Key insight: because permissions are transitive (unless
- # traverse_owned is false), by knowing the permissions granted
- # from all the "origins" (perm_origin_uuid, tail_uuid of links
- # where head_uuid is in our potentially affected set, etc) we can
- # join with the materialized_permissions table to get user
- # permissions on those origins, and apply that to the whole graph
- # of objects reached through that origin.
+ # Key insights:
+ #
+ # * Permissions are transitive (with some special cases involving
+ # users, this is controlled by the traverse_owned flag).
+ #
+ # * A user object can only gain permissions via an inbound edge,
+ # or appearing in the graph.
+ #
+ # * The materialized_permissions table includes the permission
+ # each user has on the tail end of each inbound edge.
+ #
+ # * The all_perms subquery has permissions for each object in the
+ # subgraph reachable from certain origin (tail end of an edge).
+ #
+ # * Therefore, for each user, we can compute user permissions on
+ # each object in subgraph by determining the permission the user
+ # has on each origin (tail end of an edge), joining that with the
+ # perm_origin_uuid column of all_perms, and taking the least() of
+ # the origin edge or all_perms val (because of the "least
+ # permission on the path" rule). If an object was reachable by
+ # more than one path (appears with more than one origin), we take
+ # the max() of the computed permissions.
+ #
+ # Finally, because users always have permission on themselves, the
+ # query also makes sure those permission rows are always
+ # returned.
+ #
+ # Notes on query optimization:
+ #
+ # Each clause in a "with" statement is called a "common table
+ # expression" or CTE.
+ #
+ # In Postgres, they are evaluated in sequence and results of each
+ # CTE is stored in a temporary table. This means Postgres does
+ # not propagate constraints from later queries to earlier CTEs.
+ #
+ # This is a problem if, for example, a later CTE only needs to
+ # choose 10 items out of a set of 1000000 from an earlier CTE,
+ # because it will always compute all 1000000 rows even if the
+ # query on the 1000000 rows could have been constrained. This is
+ # why permission_graph_edges is a view and not a CTE -- views are
+ # inlined so and can be optimized using external constraints.
+ #
+ # The query optimizer does sort the temporary tables for later use
+ # in joins.
+ #
+ # Final note, this query would have been almost impossible to
+ # write (and certainly impossible to read) without using SQL
+ # "with" and CTEs but unfortunately it also stumbles into a
+ # frustrating Postgres optimizer bug, see
+ # lib/refresh_permission_view.rb#update_permissions for details
+ # and a partial workaround.
#
ActiveRecord::Base.connection.execute %{
create or replace function compute_permission_subgraph (perm_origin_uuid varchar(27),
@@ -286,6 +331,10 @@ with
$$;
}
+ #
+ # Populate the materialized_permissions by traversing permissions
+ # starting at each user.
+ #
ActiveRecord::Base.connection.execute %{
INSERT INTO materialized_permissions
select users.uuid, g.target_uuid, g.val, g.traverse_owned
commit 74be9180943834aebba7683418b4c9700ef16192
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Fri May 22 15:00:22 2020 -0400
16007: Lots and lots lots of method documentation via code comments.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index e168cc608..cc6f7816a 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -285,6 +285,9 @@ class ArvadosModel < ApplicationRecord
sql_conds = nil
user_uuids = users_list.map { |u| u.uuid }
+ # For details on how the trashed_groups table is constructed, see
+ # see db/migrate/20200501150153_permission_table.rb
+
exclude_trashed_records = ""
if !include_trash and (sql_table == "groups" or sql_table == "collections") then
# Only include records that are not trashed
@@ -306,6 +309,13 @@ class ArvadosModel < ApplicationRecord
trashed_check = "AND target_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} where trash_at <= statement_timestamp())"
end
+ # The core of the permission check is a join against the
+ # materialized_permissions table to determine if the user has at
+ # least read permission to either the object itself or its
+ # direct owner. See
+ # db/migrate/20200501150153_permission_table.rb for details on
+ # how the permissions are computed.
+
# Note: it is possible to combine the direct_check and
# owner_check into a single EXISTS() clause, however it turns
# out query optimizer doesn't like it and forces a sequential
@@ -318,7 +328,22 @@ class ArvadosModel < ApplicationRecord
direct_check = "#{sql_table}.uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+
"WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 #{trashed_check})"
- # Match a read permission link from the user to the record's owner_uuid
+ # Match a read permission for the user to the record's
+ # owner_uuid. This is so we can have a permissions table that
+ # mostly consists of users and groups (projects are a type of
+ # group) and not have to compute and list user permission to
+ # every single object in the system.
+ #
+ # Don't do this for API keys (special behavior) or groups
+ # (already covered by direct_check).
+ #
+ # The traverse_owned flag indicates whether the permission to
+ # read an object also implies transitive permission to read
+ # things the object owns. The situation where this is important
+ # are determining if we can read an object owned by another
+ # user. This makes it possible to have permission to read the
+ # user record without granting permission to read things the
+ # other user owns.
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} "+
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index 51216b9c2..d62141a76 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -4,18 +4,42 @@
class PermissionTable < ActiveRecord::Migration[5.0]
def up
+ # This is a major migration. We are replacing the
+ # materialized_permission_view, which is fully recomputed any time
+ # a permission changes (and becomes very expensive as the number
+ # of users/groups becomes large), with a new strategy that only
+ # recomputes permissions for the subset of objects that are
+ # potentially affected by the addition or removal of a permission
+ # relationship (i.e. ownership or a permission link).
+ #
+ # This also disentangles the concept of "trashed groups" from the
+ # permissions system. Updating trashed items follows a similar
+ # (but less complicated) strategy to updating permissions, so it
+ # may be helpful to look at that first.
+ #
+
ActiveRecord::Base.connection.execute "DROP MATERIALIZED VIEW IF EXISTS materialized_permission_view;"
drop_table :permission_refresh_lock
- create_table :materialized_permissions, :id => false do |t|
- t.string :user_uuid
- t.string :target_uuid
- t.integer :perm_level
- t.boolean :traverse_owned
+ # This table stores the set of trashed groups and their trash_at
+ # time. Used to exclude trashed projects and their contents when
+ # getting object listings.
+ create_table :trashed_groups, :id => false do |t|
+ t.string :group_uuid
+ t.datetime :trash_at
end
- add_index :materialized_permissions, [:user_uuid, :target_uuid], unique: true, name: 'permission_user_target'
- add_index :materialized_permissions, [:target_uuid], unique: false, name: 'permission_target'
+ add_index :trashed_groups, :group_uuid, :unique => true
+ #
+ # Starting from a project, recursively traverse all the projects
+ # underneath it and return a set of project uuids and trash_at
+ # times (may be null). The initial trash_at can be a timestamp or
+ # null. The trash_at time propagates downward to groups it owns,
+ # i.e. when a group is trashed, everything underneath it in the
+ # ownership hierarchy is also considered trashed. However, this
+ # is fact is recorded in the trashed_groups table, not by updating
+ # trash_at field in the groups table.
+ #
ActiveRecord::Base.connection.execute %{
create or replace function project_subtree_with_trash_at (starting_uuid varchar(27), starting_trash_at timestamp)
returns table (target_uuid varchar(27), trash_at timestamp)
@@ -33,12 +57,9 @@ WITH RECURSIVE
$$;
}
- create_table :trashed_groups, :id => false do |t|
- t.string :group_uuid
- t.datetime :trash_at
- end
- add_index :trashed_groups, :group_uuid, :unique => true
-
+ # Helper function to populate trashed_groups table. This starts
+ # with each group owned by a user and computes the subtree under
+ # that group to find any groups that are trashed.
ActiveRecord::Base.connection.execute %{
create or replace function compute_trashed ()
returns table (uuid varchar(27), trash_at timestamp)
@@ -51,13 +72,39 @@ select ps.target_uuid as group_uuid, ps.trash_at from groups,
$$;
}
+ # Now populate the table. For a non-test databse this is the only
+ # time this ever happens, after this the trash table is updated
+ # incrementally. See app/models/group.rb#update_trash
ActiveRecord::Base.connection.execute("INSERT INTO trashed_groups select * from compute_trashed()")
+
+ # The table to store the flattened permissions. This is almost
+ # exactly the same as the old materalized_permission_view except
+ # that the target_owner_uuid colunm in the view is now just a
+ # boolean traverse_owned (the column was only ever tested for null
+ # or non-null).
+ #
+ # For details on how this table is used to apply permissions to
+ # queries, see app/models/arvados_model.rb#readable_by
+ #
+ create_table :materialized_permissions, :id => false do |t|
+ t.string :user_uuid
+ t.string :target_uuid
+ t.integer :perm_level
+ t.boolean :traverse_owned
+ end
+ add_index :materialized_permissions, [:user_uuid, :target_uuid], unique: true, name: 'permission_user_target'
+ add_index :materialized_permissions, [:target_uuid], unique: false, name: 'permission_target'
+
+ # Helper function. Determines if permission on an object implies
+ # transitive permission to things the object owns. This is always
+ # true for groups, but only true for users when the permission
+ # level is can_manage.
ActiveRecord::Base.connection.execute %{
create or replace function should_traverse_owned (starting_uuid varchar(27),
starting_perm integer)
returns bool
-STABLE
+IMMUTABLE
language SQL
as $$
select starting_uuid like '_____-j7d0g-_______________' or
@@ -65,6 +112,14 @@ select starting_uuid like '_____-j7d0g-_______________' or
$$;
}
+ # Merge all permission relationships into a single view. This
+ # consists of: groups (projects) owning things, users owning
+ # things, and explicit permission links.
+ #
+ # Fun fact, a SQL view gets inlined into the query where it is
+ # used, this enables the query planner to inject constraints, so
+ # when using the view we only look up edges we plan to traverse
+ # and avoid a brute force computation 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
@@ -83,16 +138,22 @@ union all
where links.link_class='permission'
}
- # Get a set of permission by searching the graph and following
- # ownership and permission links.
- #
- # edges() - a subselect with the union of ownership and permission links
- #
- # traverse_graph() - recursive query, from the starting node,
- # self-join with edges to find outgoing permissions.
- # Re-runs the query on new rows until there are no more results.
- # This accomplishes a breadth-first search of the permission graph.
- #
+ # 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).
+ #
+ # 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 compute_permission_subgraph below.
ActiveRecord::Base.connection.execute %{
create or replace function search_permission_graph (starting_uuid varchar(27),
starting_perm integer)
@@ -119,6 +180,55 @@ WITH RECURSIVE
$$;
}
+ # This is the key function.
+ #
+ # perm_origin_uuid: The object that 'gets' or 'has' the permission.
+ #
+ # starting_uuid: The starting object the permission applies to.
+ #
+ # starting_perm: The permission that perm_origin_uuid 'has' on starting_uuid
+ # One of 1, 2, 3 for can_read, can_write, can_manage
+ # respectively, or 0 to revoke permissions.
+ #
+ # This function is broken up into a number of phases.
+ #
+ # 1. perm_from_start: Gets the initial set of objects potentially
+ # affected by the permission change, using
+ # search_permission_graph.
+ #
+ # 2. additional_perms: Finds other inbound edges that grant
+ # permissions on the objects in perm_from_start, and computes
+ # permissions that originate from those. This is required to
+ # handle the case where there is more than one path through which
+ # a user gets permission to an object. For example, a user owns a
+ # project and also shares it can_read with a group the user
+ # belongs to, adding the can_read link must not overwrite the
+ # existing can_manage permission granted by ownership.
+ #
+ # 3. partial_perms: Combine the permissions computed in the first two phases.
+ #
+ # 4. user_identity_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.
+ #
+ # 5. all_perms: Combines perm_from_start, additional_perms, and user_identity_perms.
+ #
+ # 6. The actual query that produces rows to be added or removed
+ # from the materialized_permissions table. This is the clever
+ # bit.
+ #
+ # Key insight: because permissions are transitive (unless
+ # traverse_owned is false), by knowing the permissions granted
+ # from all the "origins" (perm_origin_uuid, tail_uuid of links
+ # where head_uuid is in our potentially affected set, etc) we can
+ # join with the materialized_permissions table to get user
+ # permissions on those origins, and apply that to the whole graph
+ # of objects reached through that origin.
+ #
ActiveRecord::Base.connection.execute %{
create or replace function compute_permission_subgraph (perm_origin_uuid varchar(27),
starting_uuid varchar(27),
@@ -128,9 +238,9 @@ STABLE
language SQL
as $$
with
-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_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)),
additional_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
select edges.tail_uuid as perm_origin_uuid, ps.target_uuid, ps.val,
diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/refresh_permission_view.rb
index 3901a1f22..c0d5c1b69 100644
--- a/services/api/lib/refresh_permission_view.rb
+++ b/services/api/lib/refresh_permission_view.rb
@@ -19,29 +19,92 @@ from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0
end
def refresh_trashed
+ ActiveRecord::Base.connection.execute("LOCK TABLE #{TRASHED_GROUPS}")
ActiveRecord::Base.connection.execute("DELETE FROM #{TRASHED_GROUPS}")
ActiveRecord::Base.connection.execute("INSERT INTO #{TRASHED_GROUPS} select * from compute_trashed()")
end
def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
- # Update a subset of the permission graph
- # perm_level is the inherited permission
+ #
+ # Update a subset of the permission table affected by adding or
+ # removing a particular permission relationship (ownership or a
+ # permission link).
+ #
+ # perm_origin_uuid: This is the object that 'gets' the permission.
+ # It is the owner_uuid or tail_uuid.
+ #
+ # starting_uuid: The object we are computing permission for (or head_uuid)
+ #
+ # perm_level: The level of permission that perm_origin_uuid gets for starting_uuid.
+ #
# perm_level is a number from 0-3
# can_read=1
# can_write=2
# can_manage=3
- # call with perm_level=0 to revoke permissions
+ # or call with perm_level=0 to revoke permissions
#
- # 1. Compute set (group, permission) implied by traversing
- # graph starting at this group
- # 2. Find links from outside the graph that point inside
- # 3. For each starting uuid, get the set of permissions from the
- # materialized permission table
- # 3. Delete permissions from table not in our computed subset.
- # 4. Upsert each permission in our subset (user, group, val)
+ # check: for testing/debugging only, compare the result of the
+ # incremental update against a full table recompute. Throws an
+ # error if the contents are not identical (ie they produce different
+ # permission results)
+ # Theory of operation
+ #
+ # Give a change in a specific permission relationship, we recompute
+ # the set of permissions (for all users) that could possibly be
+ # affected by that relationship. For example, if a project is
+ # shared with another user, we recompute all permissions for all
+ # projects in the hierarchy. This returns a set of updated
+ # permissions, which we stash in a temporary table.
+ #
+ # Then, for each user_uuid/target_uuid in the updated permissions
+ # result set we insert/update a permission row in
+ # materialized_permissions, and delete any rows that exist in
+ # materialized_permissions that are not in the result set or have
+ # perm_level=0.
+ #
+ # see db/migrate/20200501150153_permission_table.rb for details on
+ # how the permissions are computed.
+
+ # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE
+ # ROW EXCLUSIVE, EXCLUSIVE, and ACCESS EXCLUSIVE lock modes. This
+ # mode protects a table against concurrent data changes."
ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE"
+ # Workaround for
+ # BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE
+ # https://www.postgresql.org/message-id/152395805004.19366.3107109716821067806@wrigleys.postgresql.org
+ #
+ # For a crucial join in the compute_permission_subgraph() query, the
+ # planner mis-estimates the number of rows in a Common Table
+ # Expression (CTE, this is a subquery in a WITH clause) and as a
+ # result it chooses the wrong join order. The join starts with the
+ # permissions table because it mistakenly thinks
+ # count(materalized_permissions) < count(new computed permissions)
+ # when actually it is the other way around.
+ #
+ # Because of the incorrect join order, it choose the wrong join
+ # strategy (merge join, which works best when two tables are roughly
+ # the same size). As a workaround, we can tell it not to use that
+ # join strategy, this causes it to pick hash join instead, which
+ # turns out to be a bit better. However, because the join order is
+ # still wrong, we don't get the full benefit of the index.
+ #
+ # This is very unfortunate because it makes the query performance
+ # dependent on the size of the materalized_permissions table, when
+ # the goal of this design was to make permission updates scale-free
+ # and only depend on the number of permissions affected and not the
+ # total table size. In several hours of researching I wasn't able
+ # to find a way to force the correct join order, so I'm calling it
+ # here and I have to move on.
+ #
+ # This is apparently addressed in Postgres 12, but I developed &
+ # tested this on Postgres 9.6, so in the future we should reevaluate
+ # the performance & query plan on Postgres 12.
+ #
+ # https://git.furworks.de/opensourcemirror/postgresql/commit/a314c34079cf06d05265623dd7c056f8fa9d577f
+ #
+ # Disable merge join for just this query (also local for this transaction), then reenable it.
ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;"
temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}"
commit 6ba9fdc006d023fe1a56cd28fb87a54dff5265d8
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Fri May 1 14:26:35 2020 -0400
16007: Use incremental updates instead of materialized view for permissions
Separate 'trashed' from 'permissions' and remove 'trashed' from
permission computation. Add postgres functions for computing trash
and update trashed_groups incrementally. Make sure trash table gets
refreshed on database reset. readable_by() now checks trash_at timestamp.
Drop materialized view and replace with a table that is updated
incrementally. Add postgres functions for computing permissions.
Initialize materialized_permissions from search_permission_graph.
Call refresh_permissions in database_seeds. Add index on
materialized_permissions.target_uuid.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/controllers/database_controller.rb b/services/api/app/controllers/database_controller.rb
index d6045a5dc..24c6cf5a7 100644
--- a/services/api/app/controllers/database_controller.rb
+++ b/services/api/app/controllers/database_controller.rb
@@ -78,6 +78,7 @@ class DatabaseController < ApplicationController
require 'refresh_permission_view'
refresh_permission_view
+ refresh_trashed
# Done.
send_json success: true
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 816dbf475..e168cc608 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -287,8 +287,8 @@ class ArvadosModel < ApplicationRecord
exclude_trashed_records = ""
if !include_trash and (sql_table == "groups" or sql_table == "collections") then
- # Only include records that are not explicitly trashed
- exclude_trashed_records = "AND #{sql_table}.is_trashed = false"
+ # Only include records that are not trashed
+ exclude_trashed_records = "AND (#{sql_table}.trash_at is NULL or #{sql_table}.trash_at > statement_timestamp())"
end
if users_list.select { |u| u.is_admin }.any?
@@ -296,14 +296,14 @@ class ArvadosModel < ApplicationRecord
if !include_trash
if sql_table != "api_client_authorizations"
# Only include records where the owner is not trashed
- sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+
- "WHERE trashed = 1) #{exclude_trashed_records}"
+ sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} "+
+ "where trash_at <= statement_timestamp()) #{exclude_trashed_records}"
end
end
else
trashed_check = ""
if !include_trash then
- trashed_check = "AND trashed = 0"
+ trashed_check = "AND target_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} where trash_at <= statement_timestamp())"
end
# Note: it is possible to combine the direct_check and
@@ -322,7 +322,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 target_owner_uuid IS NOT NULL) "
+ "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 #{trashed_check} AND traverse_owned) "
end
links_cond = ""
diff --git a/services/api/app/models/database_seeds.rb b/services/api/app/models/database_seeds.rb
index 6e7ab9b07..0fea2cf7b 100644
--- a/services/api/app/models/database_seeds.rb
+++ b/services/api/app/models/database_seeds.rb
@@ -2,6 +2,8 @@
#
# SPDX-License-Identifier: AGPL-3.0
+require 'refresh_permission_view'
+
class DatabaseSeeds
extend CurrentApiClient
def self.install
@@ -12,5 +14,7 @@ class DatabaseSeeds
anonymous_group_read_permission
anonymous_user
empty_collection
+ refresh_permission_view
+ refresh_trashed
end
end
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 1f2b0d8b7..9b8a9877e 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -17,9 +17,15 @@ class Group < ArvadosModel
attribute :properties, :jsonbHash, default: {}
validate :ensure_filesystem_compatible_name
- after_create :invalidate_permissions_cache
- after_update :maybe_invalidate_permissions_cache
before_create :assign_name
+ after_create :after_ownership_change
+ after_create :update_trash
+
+ before_update :before_ownership_change
+ after_update :after_ownership_change
+
+ after_update :update_trash
+ before_destroy :clear_permissions_and_trash
api_accessible :user, extend: :common do |t|
t.add :name
@@ -38,18 +44,58 @@ class Group < ArvadosModel
super if group_class == 'project'
end
- def maybe_invalidate_permissions_cache
- if uuid_changed? or owner_uuid_changed? or is_trashed_changed?
- # This can change users' permissions on other groups as well as
- # this one.
- invalidate_permissions_cache
+ def update_trash
+ if trash_at_changed? or owner_uuid_changed?
+ # The group was added or removed from the trash.
+ #
+ # Strategy:
+ # Compute project subtree, propagating trash_at to subprojects
+ # Remove groups that don't belong from trash
+ # Add/update groups that do belong in the trash
+
+ temptable = "group_subtree_#{rand(2**64).to_s(10)}"
+ ActiveRecord::Base.connection.exec_query %{
+create temporary table #{temptable} on commit drop
+as select * from project_subtree_with_trash_at($1, LEAST($2, $3)::timestamp)
+},
+ 'Group.update_trash.select',
+ [[nil, self.uuid],
+ [nil, TrashedGroup.find_by_group_uuid(self.owner_uuid).andand.trash_at],
+ [nil, self.trash_at]]
+
+ ActiveRecord::Base.connection.exec_delete %{
+delete from trashed_groups where group_uuid in (select target_uuid from #{temptable} where trash_at is NULL);
+},
+ "Group.update_trash.delete"
+
+ ActiveRecord::Base.connection.exec_query %{
+insert into trashed_groups (group_uuid, trash_at)
+ select target_uuid as group_uuid, trash_at from #{temptable} where trash_at is not NULL
+on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
+},
+ "Group.update_trash.insert"
+ end
+ end
+
+ def before_ownership_change
+ if owner_uuid_changed? and !self.owner_uuid_was.nil?
+ MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
+ update_permissions self.owner_uuid, self.uuid, 0
+ end
+ end
+
+ def after_ownership_change
+ if owner_uuid_changed?
+ update_permissions self.owner_uuid, self.uuid, 3
end
end
- def invalidate_permissions_cache
- # Ensure a new group can be accessed by the appropriate users
- # immediately after being created.
- User.invalidate_permissions_cache self.async_permissions_update
+ def clear_permissions_and_trash
+ MaterializedPermission.where(target_uuid: uuid).delete_all
+ ActiveRecord::Base.connection.exec_delete %{
+delete from trashed_groups where group_uuid=$1
+}, "Group.clear_permissions_and_trash", [[nil, self.uuid]]
+
end
def assign_name
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index ad7800fe6..33e18b296 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -11,12 +11,13 @@ class Link < ArvadosModel
# already know how to properly treat them.
attribute :properties, :jsonbHash, default: {}
+ validate :name_links_are_obsolete
before_create :permission_to_attach_to_objects
before_update :permission_to_attach_to_objects
- after_update :maybe_invalidate_permissions_cache
- after_create :maybe_invalidate_permissions_cache
- after_destroy :maybe_invalidate_permissions_cache
- validate :name_links_are_obsolete
+ after_update :call_update_permissions
+ after_create :call_update_permissions
+ before_destroy :clear_permissions
+ after_destroy :check_permissions
api_accessible :user, extend: :common do |t|
t.add :tail_uuid
@@ -64,15 +65,28 @@ class Link < ArvadosModel
false
end
- def maybe_invalidate_permissions_cache
+ PERM_LEVEL = {
+ 'can_read' => 1,
+ 'can_login' => 1,
+ 'can_write' => 2,
+ 'can_manage' => 3,
+ }
+
+ def call_update_permissions
+ if self.link_class == 'permission'
+ update_permissions tail_uuid, head_uuid, PERM_LEVEL[name]
+ end
+ end
+
+ def clear_permissions
+ if self.link_class == 'permission'
+ update_permissions tail_uuid, head_uuid, 0
+ end
+ end
+
+ def check_permissions
if self.link_class == 'permission'
- # Clearing the entire permissions cache can generate many
- # unnecessary queries if many active users are not affected by
- # this change. In such cases it would be better to search cached
- # permissions for head_uuid and tail_uuid, and invalidate the
- # cache for only those users. (This would require a browseable
- # cache.)
- User.invalidate_permissions_cache
+ #check_permissions_against_full_refresh
end
end
diff --git a/services/api/app/models/materialized_permission.rb b/services/api/app/models/materialized_permission.rb
new file mode 100644
index 000000000..24ba6737a
--- /dev/null
+++ b/services/api/app/models/materialized_permission.rb
@@ -0,0 +1,6 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class MaterializedPermission < ApplicationRecord
+end
diff --git a/services/api/app/models/trashed_group.rb b/services/api/app/models/trashed_group.rb
new file mode 100644
index 000000000..5c85946ba
--- /dev/null
+++ b/services/api/app/models/trashed_group.rb
@@ -0,0 +1,6 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class TrashedGroup < ApplicationRecord
+end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index c3641b64e..d48dde25a 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -3,7 +3,6 @@
# SPDX-License-Identifier: AGPL-3.0
require 'can_be_an_owner'
-require 'refresh_permission_view'
class User < ArvadosModel
include HasUuid
@@ -28,25 +27,31 @@ class User < ArvadosModel
user.username.nil? and user.username_changed?
}
before_update :setup_on_activate
+
before_create :check_auto_admin
before_create :set_initial_username, :if => Proc.new { |user|
user.username.nil? and user.email
}
+ after_create :after_ownership_change
after_create :setup_on_activate
after_create :add_system_group_permission_link
- after_create :invalidate_permissions_cache
after_create :auto_setup_new_user, :if => Proc.new { |user|
Rails.configuration.Users.AutoSetupNewUsers and
(user.uuid != system_user_uuid) and
(user.uuid != anonymous_user_uuid)
}
after_create :send_admin_notifications
+
+ before_update :before_ownership_change
+ after_update :after_ownership_change
after_update :send_profile_created_notification
after_update :sync_repository_names, :if => Proc.new { |user|
(user.uuid != system_user_uuid) and
user.username_changed? and
(not user.username_was.nil?)
}
+ before_destroy :clear_permissions
+ after_destroy :check_permissions
has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid
has_many :repositories, foreign_key: :owner_uuid, primary_key: :uuid
@@ -77,6 +82,12 @@ class User < ArvadosModel
{read: true, write: true},
{read: true, write: true, manage: true}]
+ VAL_FOR_PERM =
+ {:read => 1,
+ :write => 2,
+ :manage => 3}
+
+
def full_name
"#{first_name} #{last_name}".strip
end
@@ -88,7 +99,7 @@ class User < ArvadosModel
end
def groups_i_can(verb)
- my_groups = self.group_permissions.select { |uuid, mask| mask[verb] }.keys
+ my_groups = self.group_permissions(VAL_FOR_PERM[verb]).keys
if verb == :read
my_groups << anonymous_group_uuid
end
@@ -107,60 +118,61 @@ class User < ArvadosModel
end
end
next if target_uuid == self.uuid
- next if (group_permissions[target_uuid] and
- group_permissions[target_uuid][action])
- if target.respond_to? :owner_uuid
- next if target.owner_uuid == self.uuid
- next if (group_permissions[target.owner_uuid] and
- group_permissions[target.owner_uuid][action])
- end
- sufficient_perms = case action
- when :manage
- ['can_manage']
- when :write
- ['can_manage', 'can_write']
- when :read
- ['can_manage', 'can_write', 'can_read']
- else
- # (Skip this kind of permission opportunity
- # if action is an unknown permission type)
- end
- if sufficient_perms
- # Check permission links with head_uuid pointing directly at
- # the target object. If target is a Group, this is redundant
- # and will fail except [a] if permission caching is broken or
- # [b] during a race condition, where a permission link has
- # *just* been added.
- if Link.where(link_class: 'permission',
- name: sufficient_perms,
- tail_uuid: groups_i_can(action) + [self.uuid],
- head_uuid: target_uuid).any?
- next
- end
+
+ target_owner_uuid = target.owner_uuid if target.respond_to? :owner_uuid
+
+ unless ActiveRecord::Base.connection.
+ exec_query(%{
+SELECT 1 FROM #{PERMISSION_VIEW}
+ WHERE user_uuid = $1 and
+ ((target_uuid = $2 and perm_level >= $3)
+ or (target_uuid = $4 and perm_level >= $3 and traverse_owned))
+},
+ # "name" arg is a query label that appears in logs:
+ "user_can_query",
+ [[nil, self.uuid],
+ [nil, target_uuid],
+ [nil, VAL_FOR_PERM[action]],
+ [nil, target_owner_uuid]]
+ ).any?
+ return false
end
- return false
end
true
end
- def self.invalidate_permissions_cache(async=false)
- refresh_permission_view(async)
+ def before_ownership_change
+ if owner_uuid_changed? and !self.owner_uuid_was.nil?
+ MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
+ update_permissions self.owner_uuid_was, self.uuid, 0
+ end
+ end
+
+ def after_ownership_change
+ if owner_uuid_changed?
+ update_permissions self.owner_uuid, self.uuid, 3
+ end
end
- def invalidate_permissions_cache
- User.invalidate_permissions_cache
+ def clear_permissions
+ update_permissions self.owner_uuid, self.uuid, 0
+ MaterializedPermission.where("user_uuid = ? or target_uuid = ?", uuid, uuid).delete_all
+ end
+
+ def check_permissions
+ check_permissions_against_full_refresh
end
# Return a hash of {user_uuid: group_perms}
def self.all_group_permissions
all_perms = {}
ActiveRecord::Base.connection.
- exec_query("SELECT user_uuid, target_owner_uuid, perm_level, trashed
+ exec_query("SELECT user_uuid, target_uuid, perm_level
FROM #{PERMISSION_VIEW}
- WHERE target_owner_uuid IS NOT NULL",
+ WHERE traverse_owned",
# "name" arg is a query label that appears in logs:
- "all_group_permissions",
- ).rows.each do |user_uuid, group_uuid, max_p_val, trashed|
+ "all_group_permissions").
+ rows.each do |user_uuid, group_uuid, max_p_val|
all_perms[user_uuid] ||= {}
all_perms[user_uuid][group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
end
@@ -170,18 +182,20 @@ 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.
- def group_permissions
- group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
+ def group_permissions(level=1)
+ group_perms = {}
ActiveRecord::Base.connection.
- exec_query("SELECT target_owner_uuid, perm_level, trashed
- FROM #{PERMISSION_VIEW}
- WHERE user_uuid = $1
- AND target_owner_uuid IS NOT NULL",
+ exec_query(%{
+SELECT target_uuid, perm_level
+ FROM #{PERMISSION_VIEW}
+ WHERE user_uuid = $1 and perm_level >= $2
+},
# "name" arg is a query label that appears in logs:
- "group_permissions for #{uuid}",
+ "group_permissions_for_user",
# "binds" arg is an array of [col_id, value] for '$1' vars:
- [[nil, uuid]],
- ).rows.each do |group_uuid, max_p_val, trashed|
+ [[nil, uuid],
+ [nil, level]]).
+ rows.each do |group_uuid, max_p_val|
group_perms[group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
end
group_perms
@@ -309,6 +323,18 @@ class User < ArvadosModel
self.uuid = new_uuid
save!(validate: false)
change_all_uuid_refs(old_uuid: old_uuid, new_uuid: new_uuid)
+ ActiveRecord::Base.connection.exec_update %{
+update #{PERMISSION_VIEW} set user_uuid=$1 where user_uuid = $2
+},
+ 'User.update_uuid.update_permissions_user_uuid',
+ [[nil, new_uuid],
+ [nil, old_uuid]]
+ ActiveRecord::Base.connection.exec_update %{
+update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
+},
+ 'User.update_uuid.update_permissions_target_uuid',
+ [[nil, new_uuid],
+ [nil, old_uuid]]
end
end
@@ -334,6 +360,8 @@ class User < ArvadosModel
raise "user does not exist" if !new_user
raise "cannot merge to an already merged user" if new_user.redirect_to_user_uuid
+ self.clear_permissions
+
# If 'self' is a remote user, don't transfer authorizations
# (i.e. ability to access the account) to the new user, because
# that gives the remote site the ability to access the 'new'
@@ -408,7 +436,8 @@ class User < ArvadosModel
if redirect_to_new_user
update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil)
end
- invalidate_permissions_cache
+ update_permissions self.owner_uuid, self.uuid, 3, false
+ update_permissions new_user.owner_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
new file mode 100644
index 000000000..51216b9c2
--- /dev/null
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -0,0 +1,263 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class PermissionTable < ActiveRecord::Migration[5.0]
+ def up
+ ActiveRecord::Base.connection.execute "DROP MATERIALIZED VIEW IF EXISTS materialized_permission_view;"
+ drop_table :permission_refresh_lock
+
+ create_table :materialized_permissions, :id => false do |t|
+ t.string :user_uuid
+ t.string :target_uuid
+ t.integer :perm_level
+ t.boolean :traverse_owned
+ end
+ add_index :materialized_permissions, [:user_uuid, :target_uuid], unique: true, name: 'permission_user_target'
+ add_index :materialized_permissions, [:target_uuid], unique: false, name: 'permission_target'
+
+ ActiveRecord::Base.connection.execute %{
+create or replace function project_subtree_with_trash_at (starting_uuid varchar(27), starting_trash_at timestamp)
+returns table (target_uuid varchar(27), trash_at timestamp)
+STABLE
+language SQL
+as $$
+WITH RECURSIVE
+ project_subtree(uuid, trash_at) as (
+ values (starting_uuid, starting_trash_at)
+ union
+ select groups.uuid, LEAST(project_subtree.trash_at, groups.trash_at)
+ from groups join project_subtree on (groups.owner_uuid = project_subtree.uuid)
+ )
+ select uuid, trash_at from project_subtree;
+$$;
+}
+
+ create_table :trashed_groups, :id => false do |t|
+ t.string :group_uuid
+ t.datetime :trash_at
+ end
+ add_index :trashed_groups, :group_uuid, :unique => true
+
+ ActiveRecord::Base.connection.execute %{
+create or replace function compute_trashed ()
+returns table (uuid varchar(27), trash_at timestamp)
+STABLE
+language SQL
+as $$
+select ps.target_uuid as group_uuid, ps.trash_at from groups,
+ lateral project_subtree_with_trash_at(groups.uuid, groups.trash_at) ps
+ where groups.owner_uuid like '_____-tpzed-_______________'
+$$;
+}
+
+ ActiveRecord::Base.connection.execute("INSERT INTO trashed_groups select * from compute_trashed()")
+
+ ActiveRecord::Base.connection.execute %{
+create or replace function should_traverse_owned (starting_uuid varchar(27),
+ starting_perm integer)
+ returns bool
+STABLE
+language SQL
+as $$
+select starting_uuid like '_____-j7d0g-_______________' or
+ (starting_uuid like '_____-tpzed-_______________' and starting_perm >= 3);
+$$;
+}
+
+ 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
+union all
+ select users.owner_uuid as tail_uuid, users.uuid as head_uuid, (3) as val from users
+union all
+ select links.tail_uuid,
+ links.head_uuid,
+ CASE
+ WHEN links.name = 'can_read' THEN 1
+ WHEN links.name = 'can_login' THEN 1
+ WHEN links.name = 'can_write' THEN 2
+ WHEN links.name = 'can_manage' THEN 3
+ END as val
+ from links
+ where links.link_class='permission'
+}
+
+ # Get a set of permission by searching the graph and following
+ # ownership and permission links.
+ #
+ # edges() - a subselect with the union of ownership and permission links
+ #
+ # traverse_graph() - recursive query, from the starting node,
+ # self-join with edges to find outgoing permissions.
+ # Re-runs the query on new rows until there are no more results.
+ # This accomplishes a breadth-first search of the permission graph.
+ #
+ ActiveRecord::Base.connection.execute %{
+create or replace function search_permission_graph (starting_uuid varchar(27),
+ starting_perm integer)
+ returns table (target_uuid varchar(27), val integer, traverse_owned bool)
+STABLE
+language SQL
+as $$
+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),
+ 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);
+$$;
+}
+
+ ActiveRecord::Base.connection.execute %{
+create or replace function compute_permission_subgraph (perm_origin_uuid varchar(27),
+ starting_uuid varchar(27),
+ starting_perm integer)
+returns table (user_uuid varchar(27), target_uuid varchar(27), val integer, traverse_owned bool)
+STABLE
+language SQL
+as $$
+with
+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)),
+
+ 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) as ps
+ 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)),
+
+ partial_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
+ select * from perm_from_start
+ union all
+ select * from additional_perms
+ ),
+
+ 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) 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)
+ ),
+
+ all_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
+ select * from partial_perms
+ union
+ select * from user_identity_perms
+ )
+
+ select v.user_uuid, v.target_uuid, max(v.perm_level), bool_or(v.traverse_owned) from
+ (select m.user_uuid,
+ u.target_uuid,
+ least(u.val, m.perm_level) as perm_level,
+ u.traverse_owned
+ from all_perms as u, materialized_permissions as m
+ where u.perm_origin_uuid = m.target_uuid AND m.traverse_owned
+ union all
+ select perm_origin_uuid as user_uuid, target_uuid, val as perm_level, traverse_owned
+ from all_perms
+ where all_perms.perm_origin_uuid like '_____-tpzed-_______________') as v
+ group by v.user_uuid, v.target_uuid
+$$;
+ }
+
+ 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
+}
+ end
+
+ def down
+ drop_table :materialized_permissions
+ drop_table :trashed_groups
+
+ 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);"
+ 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;"
+
+ ActiveRecord::Base.connection.execute(%{
+CREATE MATERIALIZED VIEW materialized_permission_view AS
+ WITH RECURSIVE perm_value(name, val) AS (
+ VALUES ('can_read'::text,(1)::smallint), ('can_login'::text,1), ('can_write'::text,2), ('can_manage'::text,3)
+ ), perm_edges(tail_uuid, head_uuid, val, follow, trashed) AS (
+ SELECT links.tail_uuid,
+ links.head_uuid,
+ pv.val,
+ ((pv.val = 3) OR (groups.uuid IS NOT NULL)) AS follow,
+ (0)::smallint AS trashed,
+ (0)::smallint AS followtrash
+ FROM ((public.links
+ LEFT JOIN perm_value pv ON ((pv.name = (links.name)::text)))
+ LEFT JOIN public.groups ON (((pv.val < 3) AND ((groups.uuid)::text = (links.head_uuid)::text))))
+ WHERE ((links.link_class)::text = 'permission'::text)
+ UNION ALL
+ SELECT groups.owner_uuid,
+ groups.uuid,
+ 3,
+ true AS bool,
+ CASE
+ WHEN ((groups.trash_at IS NOT NULL) AND (groups.trash_at < clock_timestamp())) THEN 1
+ ELSE 0
+ END AS "case",
+ 1
+ FROM public.groups
+ ), perm(val, follow, user_uuid, target_uuid, trashed) AS (
+ SELECT (3)::smallint AS val,
+ true AS follow,
+ (users.uuid)::character varying(32) AS user_uuid,
+ (users.uuid)::character varying(32) AS target_uuid,
+ (0)::smallint AS trashed
+ FROM public.users
+ UNION
+ SELECT (LEAST((perm_1.val)::integer, edges.val))::smallint AS val,
+ edges.follow,
+ perm_1.user_uuid,
+ (edges.head_uuid)::character varying(32) AS target_uuid,
+ ((GREATEST((perm_1.trashed)::integer, edges.trashed) * edges.followtrash))::smallint AS trashed
+ FROM (perm perm_1
+ JOIN perm_edges edges ON ((perm_1.follow AND ((edges.tail_uuid)::text = (perm_1.target_uuid)::text))))
+ )
+ SELECT perm.user_uuid,
+ perm.target_uuid,
+ max(perm.val) AS perm_level,
+ CASE perm.follow
+ WHEN true THEN perm.target_uuid
+ ELSE NULL::character varying
+ END AS target_owner_uuid,
+ max(perm.trashed) AS trashed
+ FROM perm
+ GROUP BY perm.user_uuid, perm.target_uuid,
+ CASE perm.follow
+ WHEN true THEN perm.target_uuid
+ ELSE NULL::character varying
+ END
+ WITH NO DATA;
+}
+ )
+
+ add_index :materialized_permission_view, [:trashed, :target_uuid], name: 'permission_target_trashed'
+ add_index :materialized_permission_view, [:user_uuid, :trashed, :perm_level], name: 'permission_target_user_trashed_level'
+ create_table :permission_refresh_lock
+
+ ActiveRecord::Base.connection.execute 'REFRESH MATERIALIZED VIEW materialized_permission_view;'
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 88cd0baa2..5f71554ff 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -38,6 +38,131 @@ CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA public;
-- COMMENT ON EXTENSION pg_trgm IS 'text similarity measurement and index searching based on trigrams';
+--
+-- Name: compute_permission_subgraph(character varying, character varying, integer); 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)
+ LANGUAGE sql STABLE
+ AS $$
+with
+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)),
+
+ 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) as ps
+ 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)),
+
+ partial_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
+ select * from perm_from_start
+ union all
+ select * from additional_perms
+ ),
+
+ 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) 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)
+ ),
+
+ all_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
+ select * from partial_perms
+ union
+ select * from user_identity_perms
+ )
+
+ select v.user_uuid, v.target_uuid, max(v.perm_level), bool_or(v.traverse_owned) from
+ (select m.user_uuid,
+ u.target_uuid,
+ least(u.val, m.perm_level) as perm_level,
+ u.traverse_owned
+ from all_perms as u, materialized_permissions as m
+ where u.perm_origin_uuid = m.target_uuid AND m.traverse_owned
+ union all
+ select perm_origin_uuid as user_uuid, target_uuid, val as perm_level, traverse_owned
+ from all_perms
+ where all_perms.perm_origin_uuid like '_____-tpzed-_______________') as v
+ group by v.user_uuid, v.target_uuid
+$$;
+
+
+--
+-- Name: compute_trashed(); Type: FUNCTION; Schema: public; Owner: -
+--
+
+CREATE FUNCTION public.compute_trashed() RETURNS TABLE(uuid character varying, trash_at timestamp without time zone)
+ LANGUAGE sql STABLE
+ AS $$
+select ps.target_uuid as group_uuid, ps.trash_at from groups,
+ lateral project_subtree_with_trash_at(groups.uuid, groups.trash_at) ps
+ where groups.owner_uuid like '_____-tpzed-_______________'
+$$;
+
+
+--
+-- Name: project_subtree_with_trash_at(character varying, timestamp without time zone); Type: FUNCTION; Schema: public; Owner: -
+--
+
+CREATE FUNCTION public.project_subtree_with_trash_at(starting_uuid character varying, starting_trash_at timestamp without time zone) RETURNS TABLE(target_uuid character varying, trash_at timestamp without time zone)
+ LANGUAGE sql STABLE
+ AS $$
+WITH RECURSIVE
+ project_subtree(uuid, trash_at) as (
+ values (starting_uuid, starting_trash_at)
+ union
+ select groups.uuid, LEAST(project_subtree.trash_at, groups.trash_at)
+ from groups join project_subtree on (groups.owner_uuid = project_subtree.uuid)
+ )
+ select uuid, trash_at from project_subtree;
+$$;
+
+
+--
+-- Name: search_permission_graph(character varying, integer); Type: FUNCTION; Schema: public; Owner: -
+--
+
+CREATE FUNCTION public.search_permission_graph(starting_uuid character varying, starting_perm integer) RETURNS TABLE(target_uuid character varying, val integer, traverse_owned boolean)
+ LANGUAGE sql STABLE
+ AS $$
+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),
+ 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: -
+--
+
+CREATE FUNCTION public.should_traverse_owned(starting_uuid character varying, starting_perm integer) RETURNS boolean
+ LANGUAGE sql STABLE
+ AS $$
+select starting_uuid like '_____-j7d0g-_______________' or
+ (starting_uuid like '_____-tpzed-_______________' and starting_perm >= 3);
+$$;
+
+
SET default_tablespace = '';
SET default_with_oids = false;
@@ -719,93 +844,17 @@ ALTER SEQUENCE public.logs_id_seq OWNED BY public.logs.id;
--
--- Name: users; Type: TABLE; Schema: public; Owner: -
+-- Name: materialized_permissions; Type: TABLE; Schema: public; Owner: -
--
-CREATE TABLE public.users (
- id integer NOT NULL,
- uuid character varying(255),
- owner_uuid character varying(255) NOT NULL,
- created_at timestamp without time zone NOT NULL,
- modified_by_client_uuid character varying(255),
- modified_by_user_uuid character varying(255),
- modified_at timestamp without time zone,
- email character varying(255),
- first_name character varying(255),
- last_name character varying(255),
- identity_url character varying(255),
- is_admin boolean,
- prefs text,
- updated_at timestamp without time zone NOT NULL,
- default_owner_uuid character varying(255),
- is_active boolean DEFAULT false,
- username character varying(255),
- redirect_to_user_uuid character varying
+CREATE TABLE public.materialized_permissions (
+ user_uuid character varying,
+ target_uuid character varying,
+ perm_level integer,
+ traverse_owned boolean
);
---
--- Name: materialized_permission_view; Type: MATERIALIZED VIEW; Schema: public; Owner: -
---
-
-CREATE MATERIALIZED VIEW public.materialized_permission_view AS
- WITH RECURSIVE perm_value(name, val) AS (
- VALUES ('can_read'::text,(1)::smallint), ('can_login'::text,1), ('can_write'::text,2), ('can_manage'::text,3)
- ), perm_edges(tail_uuid, head_uuid, val, follow, trashed) AS (
- SELECT links.tail_uuid,
- links.head_uuid,
- pv.val,
- ((pv.val = 3) OR (groups.uuid IS NOT NULL)) AS follow,
- (0)::smallint AS trashed,
- (0)::smallint AS followtrash
- FROM ((public.links
- LEFT JOIN perm_value pv ON ((pv.name = (links.name)::text)))
- LEFT JOIN public.groups ON (((pv.val < 3) AND ((groups.uuid)::text = (links.head_uuid)::text))))
- WHERE ((links.link_class)::text = 'permission'::text)
- UNION ALL
- SELECT groups.owner_uuid,
- groups.uuid,
- 3,
- true AS bool,
- CASE
- WHEN ((groups.trash_at IS NOT NULL) AND (groups.trash_at < clock_timestamp())) THEN 1
- ELSE 0
- END AS "case",
- 1
- FROM public.groups
- ), perm(val, follow, user_uuid, target_uuid, trashed) AS (
- SELECT (3)::smallint AS val,
- true AS follow,
- (users.uuid)::character varying(32) AS user_uuid,
- (users.uuid)::character varying(32) AS target_uuid,
- (0)::smallint AS trashed
- FROM public.users
- UNION
- SELECT (LEAST((perm_1.val)::integer, edges.val))::smallint AS val,
- edges.follow,
- perm_1.user_uuid,
- (edges.head_uuid)::character varying(32) AS target_uuid,
- ((GREATEST((perm_1.trashed)::integer, edges.trashed) * edges.followtrash))::smallint AS trashed
- FROM (perm perm_1
- JOIN perm_edges edges ON ((perm_1.follow AND ((edges.tail_uuid)::text = (perm_1.target_uuid)::text))))
- )
- SELECT perm.user_uuid,
- perm.target_uuid,
- max(perm.val) AS perm_level,
- CASE perm.follow
- WHEN true THEN perm.target_uuid
- ELSE NULL::character varying
- END AS target_owner_uuid,
- max(perm.trashed) AS trashed
- FROM perm
- GROUP BY perm.user_uuid, perm.target_uuid,
- CASE perm.follow
- WHEN true THEN perm.target_uuid
- ELSE NULL::character varying
- END
- WITH NO DATA;
-
-
--
-- Name: nodes; Type: TABLE; Schema: public; Owner: -
--
@@ -851,31 +900,57 @@ ALTER SEQUENCE public.nodes_id_seq OWNED BY public.nodes.id;
--
--- Name: permission_refresh_lock; Type: TABLE; Schema: public; Owner: -
+-- Name: users; Type: TABLE; Schema: public; Owner: -
--
-CREATE TABLE public.permission_refresh_lock (
- id integer NOT NULL
+CREATE TABLE public.users (
+ id integer NOT NULL,
+ uuid character varying(255),
+ owner_uuid character varying(255) NOT NULL,
+ created_at timestamp without time zone NOT NULL,
+ modified_by_client_uuid character varying(255),
+ modified_by_user_uuid character varying(255),
+ modified_at timestamp without time zone,
+ email character varying(255),
+ first_name character varying(255),
+ last_name character varying(255),
+ identity_url character varying(255),
+ is_admin boolean,
+ prefs text,
+ updated_at timestamp without time zone NOT NULL,
+ default_owner_uuid character varying(255),
+ is_active boolean DEFAULT false,
+ username character varying(255),
+ redirect_to_user_uuid character varying
);
--
--- Name: permission_refresh_lock_id_seq; Type: SEQUENCE; Schema: public; Owner: -
---
-
-CREATE SEQUENCE public.permission_refresh_lock_id_seq
- START WITH 1
- INCREMENT BY 1
- NO MINVALUE
- NO MAXVALUE
- CACHE 1;
-
-
---
--- Name: permission_refresh_lock_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
+-- Name: permission_graph_edges; Type: VIEW; Schema: public; Owner: -
--
-ALTER SEQUENCE public.permission_refresh_lock_id_seq OWNED BY public.permission_refresh_lock.id;
+CREATE VIEW public.permission_graph_edges AS
+ SELECT groups.owner_uuid AS tail_uuid,
+ groups.uuid AS head_uuid,
+ 3 AS val
+ FROM public.groups
+UNION ALL
+ SELECT users.owner_uuid AS tail_uuid,
+ users.uuid AS head_uuid,
+ 3 AS val
+ FROM public.users
+UNION ALL
+ SELECT links.tail_uuid,
+ links.head_uuid,
+ CASE
+ WHEN ((links.name)::text = 'can_read'::text) THEN 1
+ 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
+ END AS val
+ FROM public.links
+ WHERE ((links.link_class)::text = 'permission'::text);
--
@@ -1079,6 +1154,16 @@ CREATE SEQUENCE public.traits_id_seq
ALTER SEQUENCE public.traits_id_seq OWNED BY public.traits.id;
+--
+-- Name: trashed_groups; Type: TABLE; Schema: public; Owner: -
+--
+
+CREATE TABLE public.trashed_groups (
+ group_uuid character varying,
+ trash_at timestamp without time zone
+);
+
+
--
-- Name: users_id_seq; Type: SEQUENCE; Schema: public; Owner: -
--
@@ -1277,13 +1362,6 @@ ALTER TABLE ONLY public.logs ALTER COLUMN id SET DEFAULT nextval('public.logs_id
ALTER TABLE ONLY public.nodes ALTER COLUMN id SET DEFAULT nextval('public.nodes_id_seq'::regclass);
---
--- Name: permission_refresh_lock id; Type: DEFAULT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public.permission_refresh_lock ALTER COLUMN id SET DEFAULT nextval('public.permission_refresh_lock_id_seq'::regclass);
-
-
--
-- Name: pipeline_instances id; Type: DEFAULT; Schema: public; Owner: -
--
@@ -1468,14 +1546,6 @@ ALTER TABLE ONLY public.nodes
ADD CONSTRAINT nodes_pkey PRIMARY KEY (id);
---
--- Name: permission_refresh_lock permission_refresh_lock_pkey; Type: CONSTRAINT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public.permission_refresh_lock
- ADD CONSTRAINT permission_refresh_lock_pkey PRIMARY KEY (id);
-
-
--
-- Name: pipeline_instances pipeline_instances_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--
@@ -2513,6 +2583,13 @@ CREATE INDEX index_traits_on_owner_uuid ON public.traits USING btree (owner_uuid
CREATE UNIQUE INDEX index_traits_on_uuid ON public.traits USING btree (uuid);
+--
+-- Name: index_trashed_groups_on_group_uuid; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE UNIQUE INDEX index_trashed_groups_on_group_uuid ON public.trashed_groups USING btree (group_uuid);
+
+
--
-- Name: index_users_on_created_at; Type: INDEX; Schema: public; Owner: -
--
@@ -2703,17 +2780,17 @@ CREATE INDEX nodes_search_index ON public.nodes USING btree (uuid, owner_uuid, m
--
--- Name: permission_target_trashed; Type: INDEX; Schema: public; Owner: -
+-- Name: permission_target; Type: INDEX; Schema: public; Owner: -
--
-CREATE INDEX permission_target_trashed ON public.materialized_permission_view USING btree (trashed, target_uuid);
+CREATE INDEX permission_target ON public.materialized_permissions USING btree (target_uuid);
--
--- Name: permission_target_user_trashed_level; Type: INDEX; Schema: public; Owner: -
+-- Name: permission_user_target; Type: INDEX; Schema: public; Owner: -
--
-CREATE INDEX permission_target_user_trashed_level ON public.materialized_permission_view USING btree (user_uuid, trashed, perm_level);
+CREATE UNIQUE INDEX permission_user_target ON public.materialized_permissions USING btree (user_uuid, target_uuid);
--
@@ -3024,6 +3101,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20190523180148'),
('20190808145904'),
('20190809135453'),
-('20190905151603');
+('20190905151603'),
+('20200501150153');
diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/refresh_permission_view.rb
index 5d6081f26..3901a1f22 100644
--- a/services/api/lib/refresh_permission_view.rb
+++ b/services/api/lib/refresh_permission_view.rb
@@ -2,39 +2,118 @@
#
# SPDX-License-Identifier: AGPL-3.0
-PERMISSION_VIEW = "materialized_permission_view"
+PERMISSION_VIEW = "materialized_permissions"
+TRASHED_GROUPS = "trashed_groups"
-def do_refresh_permission_view
+def refresh_permission_view
ActiveRecord::Base.transaction do
- ActiveRecord::Base.connection.execute("LOCK TABLE permission_refresh_lock")
- ActiveRecord::Base.connection.execute("REFRESH MATERIALIZED VIEW #{PERMISSION_VIEW}")
+ 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
+},
+ "refresh_permission_view.do"
end
end
-def refresh_permission_view(async=false)
- if async and Rails.configuration.API.AsyncPermissionsUpdateInterval > 0
- exp = Rails.configuration.API.AsyncPermissionsUpdateInterval.seconds
- need = false
- Rails.cache.fetch('AsyncRefreshPermissionView', expires_in: exp) do
- need = true
- end
- if need
- # Schedule a new permission update and return immediately
- Thread.new do
- Thread.current.abort_on_exception = false
- begin
- sleep(exp)
- Rails.cache.delete('AsyncRefreshPermissionView')
- do_refresh_permission_view
- rescue => e
- Rails.logger.error "Updating permission view: #{e}\n#{e.backtrace.join("\n\t")}"
- ensure
- ActiveRecord::Base.connection.close
- end
+def refresh_trashed
+ ActiveRecord::Base.connection.execute("DELETE FROM #{TRASHED_GROUPS}")
+ ActiveRecord::Base.connection.execute("INSERT INTO #{TRASHED_GROUPS} select * from compute_trashed()")
+end
+
+def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
+ # Update a subset of the permission graph
+ # perm_level is the inherited permission
+ # perm_level is a number from 0-3
+ # can_read=1
+ # can_write=2
+ # can_manage=3
+ # call with perm_level=0 to revoke permissions
+ #
+ # 1. Compute set (group, permission) implied by traversing
+ # graph starting at this group
+ # 2. Find links from outside the graph that point inside
+ # 3. For each starting uuid, get the set of permissions from the
+ # materialized permission table
+ # 3. Delete permissions from table not in our computed subset.
+ # 4. Upsert each permission in our subset (user, group, val)
+
+ ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE"
+
+ ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;"
+
+ 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)
+},
+ 'update_permissions.select',
+ [[nil, perm_origin_uuid],
+ [nil, starting_uuid],
+ [nil, perm_level]]
+
+ ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;"
+
+ ActiveRecord::Base.connection.exec_delete %{
+delete from #{PERMISSION_VIEW} where
+ target_uuid in (select target_uuid from #{temptable_perms}) and
+ not exists (select 1 from #{temptable_perms}
+ where target_uuid=#{PERMISSION_VIEW}.target_uuid and
+ user_uuid=#{PERMISSION_VIEW}.user_uuid and
+ val>0)
+},
+ "update_permissions.delete"
+
+ ActiveRecord::Base.connection.exec_query %{
+insert into #{PERMISSION_VIEW} (user_uuid, target_uuid, perm_level, traverse_owned)
+ select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms} where val>0
+on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_level, traverse_owned=EXCLUDED.traverse_owned;
+},
+ "update_permissions.insert"
+
+ if check and perm_level>0
+ check_permissions_against_full_refresh
+ end
+end
+
+
+def check_permissions_against_full_refresh
+ #
+ # For debugging, this checks contents of the
+ # incrementally-updated 'materialized_permission' against a
+ # from-scratch permission refresh.
+ #
+
+ q1 = ActiveRecord::Base.connection.exec_query %{
+select user_uuid, target_uuid, perm_level, traverse_owned from #{PERMISSION_VIEW}
+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
+}, "check_permissions_against_full_refresh.full_recompute"
+
+ if q1.count != q2.count
+ puts "Didn't match incremental+: #{q1.count} != full refresh-: #{q2.count}"
+ end
+
+ if q1.count > q2.count
+ q1.each_with_index do |r, i|
+ if r != q2[i]
+ puts "+#{r}\n-#{q2[i]}"
+ raise "Didn't match"
end
- true
end
else
- do_refresh_permission_view
+ q2.each_with_index do |r, i|
+ if r != q1[i]
+ puts "+#{q1[i]}\n-#{r}"
+ raise "Didn't match"
+ end
+ end
end
end
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 92a1ced52..22c999ecd 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -60,6 +60,7 @@ testusergroup_admins:
uuid: zzzzz-j7d0g-48foin4vonvc2at
owner_uuid: zzzzz-tpzed-000000000000000
name: Administrators of a subset of users
+ group_class: role
aproject:
uuid: zzzzz-j7d0g-v955i6s2oi1cbso
@@ -143,6 +144,7 @@ active_user_has_can_manage:
uuid: zzzzz-j7d0g-ptt1ou6a9lxrv07
owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
name: Active user has can_manage
+ group_class: project
# Group for testing granting permission between users who share a group.
group_for_sharing_tests:
@@ -343,4 +345,4 @@ trashed_on_next_sweep:
trash_at: 2001-01-01T00:00:00Z
delete_at: 2038-03-01T00:00:00Z
is_trashed: false
- modified_at: 2001-01-01T00:00:00Z
\ No newline at end of file
+ modified_at: 2001-01-01T00:00:00Z
diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml
index 57633a312..14630d9ef 100644
--- a/services/api/test/fixtures/users.yml
+++ b/services/api/test/fixtures/users.yml
@@ -418,3 +418,17 @@ double_redirects_to_active:
organization: example.com
role: Computational biologist
getting_started_shown: 2015-03-26 12:34:56.789000000 Z
+
+has_can_login_permission:
+ owner_uuid: zzzzz-tpzed-000000000000000
+ uuid: zzzzz-tpzed-xabcdjxw79nv3jz
+ email: can-login-user at arvados.local
+ modified_by_client_uuid: zzzzz-ozdt8-teyxzyd8qllg11h
+ modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ first_name: Can_login
+ last_name: User
+ identity_url: https://can-login-user.openid.local
+ is_active: true
+ is_admin: false
+ modified_at: 2015-03-26 12:34:56.789000000 Z
+ username: can-login-user
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 30ab89c7e..2b5e8d5a9 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -505,9 +505,19 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
### trashed project tests ###
- [:active, :admin].each do |auth|
+ #
+ # The structure is
+ #
+ # trashed_project (zzzzz-j7d0g-trashedproject1)
+ # trashed_subproject (zzzzz-j7d0g-trashedproject2)
+ # trashed_subproject3 (zzzzz-j7d0g-trashedproject3)
+ # zzzzz-xvhdp-cr5trashedcontr
+
+ [:active,
+ :admin].each do |auth|
# project: to query, to untrash, is visible, parent contents listing success
- [[:trashed_project, [], false, true],
+ [
+ [:trashed_project, [], false, true],
[:trashed_project, [:trashed_project], true, true],
[:trashed_subproject, [], false, false],
[:trashed_subproject, [:trashed_project], true, true],
diff --git a/services/api/test/integration/groups_test.rb b/services/api/test/integration/groups_test.rb
index eb97fc1f4..445670a3d 100644
--- a/services/api/test/integration/groups_test.rb
+++ b/services/api/test/integration/groups_test.rb
@@ -193,11 +193,15 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
assert_response :success
end
- test "create request with async=true defers permissions update" do
+ test "create request with async=true does not defer permissions update" do
Rails.configuration.API.AsyncPermissionsUpdateInterval = 1 # second
name = "Random group #{rand(1000)}"
assert_equal nil, Group.find_by_name(name)
+ # Following the implementation of incremental permission updates
+ # (#16007) the async flag is now a no-op. Permission changes are
+ # visible immediately.
+
# Trigger the asynchronous permission update by using async=true parameter.
post "/arvados/v1/groups",
params: {
@@ -209,7 +213,7 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
headers: auth(:active)
assert_response 202
- # The group exists on the database, but it's not accessible yet.
+ # The group exists in the database
assert_not_nil Group.find_by_name(name)
get "/arvados/v1/groups",
params: {
@@ -218,7 +222,7 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
},
headers: auth(:active)
assert_response 200
- assert_equal 0, json_response['items_available']
+ assert_equal 1, json_response['items_available']
# Wait a bit and try again.
sleep(1)
diff --git a/services/api/test/performance/permission_test.rb b/services/api/test/performance/permission_test.rb
index a0605f97e..0fea3b135 100644
--- a/services/api/test/performance/permission_test.rb
+++ b/services/api/test/performance/permission_test.rb
@@ -40,7 +40,7 @@ class PermissionPerfTest < ActionDispatch::IntegrationTest
end
end
end
- User.invalidate_permissions_cache
+ refresh_permission_view
end
end)
end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 5747a85cf..12f729e33 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -2,6 +2,8 @@
#
# SPDX-License-Identifier: AGPL-3.0
+require 'refresh_permission_view'
+
ENV["RAILS_ENV"] = "test"
unless ENV["NO_COVERAGE_TEST"]
begin
@@ -207,4 +209,5 @@ class ActionDispatch::IntegrationTest
end
# Ensure permissions are computed from the test fixtures.
-User.invalidate_permissions_cache
+refresh_permission_view
+refresh_trashed
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index bf1ba517e..addea8306 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -1000,6 +1000,19 @@ class CollectionTest < ActiveSupport::TestCase
test "delete referring links in SweepTrashedObjects" do
uuid = collections(:trashed_on_next_sweep).uuid
act_as_system_user do
+ assert_raises ActiveRecord::RecordInvalid do
+ # Cannot create because :trashed_on_next_sweep is already trashed
+ Link.create!(head_uuid: uuid,
+ tail_uuid: system_user_uuid,
+ link_class: 'whatever',
+ name: 'something')
+ end
+
+ # Bump trash_at to now + 1 minute
+ Collection.where(uuid: uuid).
+ update(trash_at: db_current_time + (1).minute)
+
+ # Not considered trashed now
Link.create!(head_uuid: uuid,
tail_uuid: system_user_uuid,
link_class: 'whatever',
diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb
index 528c6d253..2bdc198a3 100644
--- a/services/api/test/unit/owner_test.rb
+++ b/services/api/test/unit/owner_test.rb
@@ -70,8 +70,12 @@ class OwnerTest < ActiveSupport::TestCase
"new #{o_class} should really be in DB")
old_uuid = o.uuid
new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9])
- assert(o.update_attributes(uuid: new_uuid),
- "should change #{o_class} uuid from #{old_uuid} to #{new_uuid}")
+ if o.respond_to? :update_uuid
+ o.update_uuid(new_uuid: new_uuid)
+ else
+ assert(o.update_attributes(uuid: new_uuid),
+ "should change #{o_class} uuid from #{old_uuid} to #{new_uuid}")
+ end
assert_equal(false, o_class.where(uuid: old_uuid).any?,
"#{old_uuid} should disappear when renamed to #{new_uuid}")
end
@@ -116,8 +120,8 @@ class OwnerTest < ActiveSupport::TestCase
"setting owner to self should work")
old_uuid = o.uuid
new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9])
- assert(o.update_attributes(uuid: new_uuid),
- "should change uuid of User that owns self")
+ o.update_uuid(new_uuid: new_uuid)
+ o = User.find_by_uuid(new_uuid)
assert_equal(false, User.where(uuid: old_uuid).any?,
"#{old_uuid} should not be in DB after deleting")
assert_equal(true, User.where(uuid: new_uuid).any?,
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 260795c12..5d25361ed 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -165,7 +165,12 @@ class UserTest < ActiveSupport::TestCase
if auto_admin_first_user_config
# This test requires no admin users exist (except for the system user)
- users(:admin).delete
+ act_as_system_user do
+ users(:admin).update_attributes!(is_admin: false)
+ end
+ # need to manually call clear_permissions because we used 'delete' instead of 'destory'
+ # we use delete here because 'destroy' will throw an error
+ #users(:admin).clear_permissions
@all_users = User.where("uuid not like '%-000000000000000'").where(:is_admin => true)
assert_equal 0, @all_users.count, "No admin users should exist (except for the system user)"
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list