[ARVADOS] created: d03fcecb39335ac364e3f6f11cdfdc668fbda559
Git user
git at public.curoverse.com
Thu Sep 7 15:29:13 EDT 2017
at d03fcecb39335ac364e3f6f11cdfdc668fbda559 (commit)
commit d03fcecb39335ac364e3f6f11cdfdc668fbda559
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Sep 7 13:39:12 2017 -0400
12032: Add comments to migration. Also special case api_client_authorizations
in readable_by.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index f6e914d..94c2cc5 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -263,16 +263,11 @@ class ArvadosModel < ActiveRecord::Base
if users_list.select { |u| u.is_admin }.any?
if !include_trash
# exclude rows that are explicitly trashed.
- if self.column_names.include? "owner_uuid"
+ if sql_table != "api_client_authorizations"
sql_conds.push "NOT EXISTS(SELECT 1
FROM permission_view
WHERE trashed = 1 AND
(#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"
- else
- sql_conds.push "NOT EXISTS(SELECT 1
- FROM permission_view
- WHERE trashed = 1 AND
- (#{sql_table}.uuid = target_uuid))"
end
end
else
@@ -286,7 +281,7 @@ class ArvadosModel < ActiveRecord::Base
trashed_check = "trashed = 0"
end
- if self.column_names.include? "owner_uuid" and sql_table != "groups"
+ if sql_table != "api_client_authorizations" and sql_table != "groups"
owner_check = "OR (target_uuid = #{sql_table}.owner_uuid AND target_owner_uuid IS NOT NULL)"
sql_conds.push "#{sql_table}.owner_uuid IN (:user_uuids)"
else
diff --git a/services/api/db/migrate/20170906224040_materialized_permission_view.rb b/services/api/db/migrate/20170906224040_materialized_permission_view.rb
index de0814e..8e5df35 100644
--- a/services/api/db/migrate/20170906224040_materialized_permission_view.rb
+++ b/services/api/db/migrate/20170906224040_materialized_permission_view.rb
@@ -1,10 +1,38 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
class MaterializedPermissionView < ActiveRecord::Migration
@@idxtables = [:collections, :container_requests, :groups, :jobs, :links, :pipeline_instances, :pipeline_templates, :repositories, :users, :virtual_machines, :workflows]
def up
+
+ #
+ # Construct a materialized view for permissions. This is a view which is
+ # derived from querying other tables, but is saved to a static table itself
+ # so that it can be indexed and queried efficiently without rerunning the
+ # query. The view is updated using "REFRESH MATERIALIZED VIEW" which is
+ # executed after an operation invalidates the permission graph.
+ #
+
ActiveRecord::Base.connection.execute(
- "CREATE MATERIALIZED VIEW permission_view AS
+"-- constructing perm_edges
+-- 1. get the list of all permission links,
+-- 2. any can_manage link or permission link to a group means permission should "follow through"
+-- (as a special case, can_manage links to a user grant access to everything owned by the user,
+-- unlike can_read or can_write which only grant access to the user record)
+-- 3. add all owner->owned relationships between groups as can_manage edges
+--
+-- constructing permissions
+-- 1. base case: start with set of all users as the working set
+-- 2. recursive case:
+-- join with edges where the tail is in the working set and "follow" is true
+-- produce a new working set with the head (target) of each edge
+-- set permission to the least permission encountered on the path
+-- propagate trashed flag down
+
+CREATE MATERIALIZED VIEW permission_view AS
WITH RECURSIVE
perm_value (name, val) AS (
VALUES
@@ -58,6 +86,19 @@ SELECT user_uuid,
add_index :permission_view, [:trashed, :target_uuid], name: 'permission_target_trashed'
add_index :permission_view, [:user_uuid, :trashed, :perm_level], name: 'permission_target_user_trashed_level'
+ # Indexes on the other tables are essential to for the query planner to
+ # construct an efficient join with permission_view.
+ #
+ # Our default query uses "ORDER BY modified_by desc, uuid"
+ #
+ # It turns out the existing simple index on modified_by can't be used
+ # because of the additional ordering on "uuid".
+ #
+ # To be able to utilize the index, the index ordering has to match the
+ # ORDER BY clause. For more detail see:
+ #
+ # https://www.postgresql.org/docs/9.3/static/indexes-ordering.html
+ #
@@idxtables.each do |table|
ActiveRecord::Base.connection.execute("CREATE INDEX index_#{table.to_s}_on_modified_at_uuid ON #{table.to_s} USING btree (modified_at desc, uuid asc)")
end
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
deleted file mode 100644
index 71ac8c4..0000000
--- a/services/api/lib/create_permission_view.sql
+++ /dev/null
@@ -1,69 +0,0 @@
--- Copyright (C) The Arvados Authors. All rights reserved.
---
--- SPDX-License-Identifier: AGPL-3.0
-
--- constructing perm_edges
--- 1. get the list of all permission links,
--- 2. any can_manage link or permission link to a group means permission should "follow through"
--- (as a special case, can_manage links to a user grant access to everything owned by the user,
--- unlike can_read or can_write which only grant access to the user record)
--- 3. add all owner->owned relationships between groups as can_manage edges
---
--- constructing permissions
--- 1. base case: start with set of all users as the working set
--- 2. recursive case:
--- join with edges where the tail is in the working set and "follow" is true
--- produce a new working set with the head (target) of each edge
--- set permission to the least permission encountered on the path
--- propagate trashed flag down
-
-CREATE TEMPORARY VIEW permission_view AS
-WITH RECURSIVE
-perm_value (name, val) AS (
- VALUES
- ('can_read', 1::smallint),
- ('can_login', 1),
- ('can_write', 2),
- ('can_manage', 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
- FROM links
- LEFT JOIN perm_value pv ON pv.name = links.name
- LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
- WHERE links.link_class = 'permission'
- UNION ALL
- SELECT owner_uuid, uuid, 3, true,
- CASE WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1 ELSE 0 END
- FROM groups
- ),
-perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
- SELECT 3::smallint AS val,
- false AS follow,
- users.uuid::varchar(32) AS user_uuid,
- users.uuid::varchar(32) AS target_uuid,
- 0::smallint AS trashed,
- true AS startnode
- FROM users
- UNION
- SELECT LEAST(perm.val, edges.val)::smallint AS val,
- edges.follow AS follow,
- perm.user_uuid::varchar(32) AS user_uuid,
- edges.head_uuid::varchar(32) AS target_uuid,
- GREATEST(perm.trashed, edges.trashed)::smallint AS trashed,
- false AS startnode
- FROM perm
- INNER JOIN perm_edges edges
- ON (perm.startnode or perm.follow) AND edges.tail_uuid = perm.target_uuid
-)
-SELECT user_uuid,
- target_uuid,
- MAX(val) AS perm_level,
- CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid,
- MAX(trashed) AS trashed
- FROM perm
- GROUP BY user_uuid, target_uuid, target_owner_uuid;
commit 50a40811dfd36b8bf265860bb3d4016811447e78
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Sep 7 10:59:42 2017 -0400
12032: Update structure.sql
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 77c833a..0128dbf 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -33,6 +33,74 @@ SET default_tablespace = '';
SET default_with_oids = false;
--
+-- Name: groups; Type: TABLE; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE TABLE groups (
+ id integer NOT NULL,
+ uuid character varying(255),
+ owner_uuid character varying(255),
+ 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,
+ name character varying(255) NOT NULL,
+ description character varying(524288),
+ updated_at timestamp without time zone NOT NULL,
+ group_class character varying(255),
+ trash_at timestamp without time zone,
+ is_trashed boolean DEFAULT false NOT NULL,
+ delete_at timestamp without time zone
+);
+
+
+--
+-- Name: links; Type: TABLE; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE TABLE links (
+ id integer NOT NULL,
+ uuid character varying(255),
+ owner_uuid character varying(255),
+ 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,
+ tail_uuid character varying(255),
+ link_class character varying(255),
+ name character varying(255),
+ head_uuid character varying(255),
+ properties text,
+ updated_at timestamp without time zone NOT NULL
+);
+
+
+--
+-- Name: users; Type: TABLE; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE TABLE 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)
+);
+
+
+--
-- Name: api_client_authorizations; Type: TABLE; Schema: public; Owner: -; Tablespace:
--
@@ -377,28 +445,6 @@ ALTER SEQUENCE containers_id_seq OWNED BY containers.id;
--
--- Name: groups; Type: TABLE; Schema: public; Owner: -; Tablespace:
---
-
-CREATE TABLE groups (
- id integer NOT NULL,
- uuid character varying(255),
- owner_uuid character varying(255),
- 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,
- name character varying(255) NOT NULL,
- description character varying(524288),
- updated_at timestamp without time zone NOT NULL,
- group_class character varying(255),
- trash_at timestamp without time zone,
- is_trashed boolean DEFAULT false NOT NULL,
- delete_at timestamp without time zone
-);
-
-
---
-- Name: groups_id_seq; Type: SEQUENCE; Schema: public; Owner: -
--
@@ -665,27 +711,6 @@ ALTER SEQUENCE keep_services_id_seq OWNED BY keep_services.id;
--
--- Name: links; Type: TABLE; Schema: public; Owner: -; Tablespace:
---
-
-CREATE TABLE links (
- id integer NOT NULL,
- uuid character varying(255),
- owner_uuid character varying(255),
- 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,
- tail_uuid character varying(255),
- link_class character varying(255),
- name character varying(255),
- head_uuid character varying(255),
- properties text,
- updated_at timestamp without time zone NOT NULL
-);
-
-
---
-- Name: links_id_seq; Type: SEQUENCE; Schema: public; Owner: -
--
@@ -705,11 +730,23 @@ ALTER SEQUENCE links_id_seq OWNED BY links.id;
--
+-- Name: logs_id_seq; Type: SEQUENCE; Schema: public; Owner: -
+--
+
+CREATE SEQUENCE logs_id_seq
+ START WITH 1
+ INCREMENT BY 1
+ NO MINVALUE
+ NO MAXVALUE
+ CACHE 1;
+
+
+--
-- Name: logs; Type: TABLE; Schema: public; Owner: -; Tablespace:
--
CREATE TABLE logs (
- id integer NOT NULL,
+ id integer DEFAULT nextval('logs_id_seq'::regclass) NOT NULL,
uuid character varying(255),
owner_uuid character varying(255),
modified_by_client_uuid character varying(255),
@@ -727,25 +764,6 @@ CREATE TABLE logs (
--
--- Name: logs_id_seq; Type: SEQUENCE; Schema: public; Owner: -
---
-
-CREATE SEQUENCE logs_id_seq
- START WITH 1
- INCREMENT BY 1
- NO MINVALUE
- NO MAXVALUE
- CACHE 1;
-
-
---
--- Name: logs_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
---
-
-ALTER SEQUENCE logs_id_seq OWNED BY logs.id;
-
-
---
-- Name: nodes; Type: TABLE; Schema: public; Owner: -; Tablespace:
--
@@ -1089,31 +1107,6 @@ ALTER SEQUENCE traits_id_seq OWNED BY traits.id;
--
--- Name: users; Type: TABLE; Schema: public; Owner: -; Tablespace:
---
-
-CREATE TABLE 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)
-);
-
-
---
-- Name: users_id_seq; Type: SEQUENCE; Schema: public; Owner: -
--
@@ -1315,13 +1308,6 @@ ALTER TABLE ONLY links ALTER COLUMN id SET DEFAULT nextval('links_id_seq'::regcl
-- Name: id; Type: DEFAULT; Schema: public; Owner: -
--
-ALTER TABLE ONLY logs ALTER COLUMN id SET DEFAULT nextval('logs_id_seq'::regclass);
-
-
---
--- Name: id; Type: DEFAULT; Schema: public; Owner: -
---
-
ALTER TABLE ONLY nodes ALTER COLUMN id SET DEFAULT nextval('nodes_id_seq'::regclass);
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 3687819..ddc40a4 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -692,17 +692,17 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
test "move trashed subproject to new owner #{auth}" do
authorize_with auth
+ assert_nil Group.readable_by(users(auth)).where(uuid: groups(:trashed_subproject).uuid).first
put :update, {
id: groups(:trashed_subproject).uuid,
group: {
owner_uuid: users(:active).uuid
},
- include_trashed: true,
+ include_trash: true,
format: :json,
}
- # Currently fails but might want to change it in the future
- # so leave the test here.
- assert_response 404
+ assert_response :success
+ assert_not_nil Group.readable_by(users(auth)).where(uuid: groups(:trashed_subproject).uuid).first
end
end
commit 4e82a971807db9386ba0693cd98c5f7e433e8a40
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Sep 6 23:41:34 2017 -0400
12032: Added migration for materialized view
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/db/migrate/20170906224040_materialized_permission_view.rb b/services/api/db/migrate/20170906224040_materialized_permission_view.rb
new file mode 100644
index 0000000..de0814e
--- /dev/null
+++ b/services/api/db/migrate/20170906224040_materialized_permission_view.rb
@@ -0,0 +1,74 @@
+class MaterializedPermissionView < ActiveRecord::Migration
+
+ @@idxtables = [:collections, :container_requests, :groups, :jobs, :links, :pipeline_instances, :pipeline_templates, :repositories, :users, :virtual_machines, :workflows]
+
+ def up
+ ActiveRecord::Base.connection.execute(
+ "CREATE MATERIALIZED VIEW permission_view AS
+WITH RECURSIVE
+perm_value (name, val) AS (
+ VALUES
+ ('can_read', 1::smallint),
+ ('can_login', 1),
+ ('can_write', 2),
+ ('can_manage', 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
+ FROM links
+ LEFT JOIN perm_value pv ON pv.name = links.name
+ LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
+ WHERE links.link_class = 'permission'
+ UNION ALL
+ SELECT owner_uuid, uuid, 3, true,
+ CASE WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1 ELSE 0 END
+ FROM groups
+ ),
+perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
+ SELECT 3::smallint AS val,
+ false AS follow,
+ users.uuid::varchar(32) AS user_uuid,
+ users.uuid::varchar(32) AS target_uuid,
+ 0::smallint AS trashed,
+ true AS startnode
+ FROM users
+ UNION
+ SELECT LEAST(perm.val, edges.val)::smallint AS val,
+ edges.follow AS follow,
+ perm.user_uuid::varchar(32) AS user_uuid,
+ edges.head_uuid::varchar(32) AS target_uuid,
+ GREATEST(perm.trashed, edges.trashed)::smallint AS trashed,
+ false AS startnode
+ FROM perm
+ INNER JOIN perm_edges edges
+ ON (perm.startnode or perm.follow) AND edges.tail_uuid = perm.target_uuid
+)
+SELECT user_uuid,
+ target_uuid,
+ MAX(val) AS perm_level,
+ CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid,
+ MAX(trashed) AS trashed
+ FROM perm
+ GROUP BY user_uuid, target_uuid, target_owner_uuid;
+")
+ add_index :permission_view, [:trashed, :target_uuid], name: 'permission_target_trashed'
+ add_index :permission_view, [:user_uuid, :trashed, :perm_level], name: 'permission_target_user_trashed_level'
+
+ @@idxtables.each do |table|
+ ActiveRecord::Base.connection.execute("CREATE INDEX index_#{table.to_s}_on_modified_at_uuid ON #{table.to_s} USING btree (modified_at desc, uuid asc)")
+ end
+ end
+
+ def down
+ remove_index :permission_view, name: 'permission_target_trashed'
+ remove_index :permission_view, name: 'permission_target_user_trashed_level'
+ @@idxtables.each do |table|
+ ActiveRecord::Base.connection.execute("DROP INDEX index_#{table.to_s}_on_modified_at_uuid")
+ end
+ ActiveRecord::Base.connection.execute("DROP MATERIALIZED VIEW IF EXISTS permission_view")
+ end
+end
commit 3188061bf90b0789e6563e2ba5b10fbae2bfc075
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Sep 6 23:12:41 2017 -0400
12032: Ensure that permission_view is invalidated & refreshed
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index f5d20b1..f6e914d 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -260,55 +260,52 @@ class ArvadosModel < ActiveRecord::Base
sql_conds = []
user_uuids = users_list.map { |u| u.uuid }
- User.install_view('permission')
-
- # Check if any of the users are admin.
if users_list.select { |u| u.is_admin }.any?
if !include_trash
# exclude rows that are explicitly trashed.
if self.column_names.include? "owner_uuid"
- sql_conds += ["NOT EXISTS(SELECT 1
+ sql_conds.push "NOT EXISTS(SELECT 1
FROM permission_view
WHERE trashed = 1 AND
- (#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"]
+ (#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"
else
- sql_conds += ["NOT EXISTS(SELECT 1
+ sql_conds.push "NOT EXISTS(SELECT 1
FROM permission_view
WHERE trashed = 1 AND
- (#{sql_table}.uuid = target_uuid))"]
+ (#{sql_table}.uuid = target_uuid))"
end
end
else
# Can read object (evidently a group or user) whose UUID is listed
# explicitly in user_uuids.
- sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
+ sql_conds.push "#{sql_table}.uuid IN (:user_uuids)"
if include_trash
- trashed_check = ""
+ trashed_check = "true"
else
- trashed_check = "trashed = 0 AND"
+ trashed_check = "trashed = 0"
end
- perm_check = "perm_level >= 1"
-
if self.column_names.include? "owner_uuid" and sql_table != "groups"
- owner_check = "OR #{sql_table}.owner_uuid IN (:user_uuids) OR (target_uuid = #{sql_table}.owner_uuid AND target_owner_uuid IS NOT NULL)"
+ owner_check = "OR (target_uuid = #{sql_table}.owner_uuid AND target_owner_uuid IS NOT NULL)"
+ sql_conds.push "#{sql_table}.owner_uuid IN (:user_uuids)"
else
owner_check = ""
end
- sql_conds += ["EXISTS(SELECT 1 FROM permission_view "+
- "WHERE user_uuid IN (:user_uuids) AND #{trashed_check} #{perm_check} AND (target_uuid = #{sql_table}.uuid #{owner_check}))"]
+ sql_conds.push "EXISTS(SELECT 1 FROM permission_view "+
+ "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trashed_check} AND (target_uuid = #{sql_table}.uuid #{owner_check}))"
if sql_table == "links"
# Match any permission link that gives one of the authorized
# users some permission _or_ gives anyone else permission to
# view one of the authorized users.
- sql_conds += ["(#{sql_table}.link_class IN (:permission_link_classes) AND "+
- "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"]
+ sql_conds.push "(#{sql_table}.link_class IN (:permission_link_classes) AND "+
+ "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"
end
end
+ User.fresh_permission_view
query_on.where(sql_conds.join(' OR '),
user_uuids: user_uuids,
permission_link_classes: ['permission', 'resources'])
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 861800d..fe18367 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -27,7 +27,7 @@ class Group < ArvadosModel
end
def maybe_invalidate_permissions_cache
- if uuid_changed? or owner_uuid_changed?
+ 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
diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index 99d0e28..8664448 100644
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -68,7 +68,7 @@ class Log < ArvadosModel
end
user_uuids = users_list.map { |u| u.uuid }
- User.install_view('permission')
+ User.fresh_permission_view
joins("LEFT JOIN container_requests ON container_requests.container_uuid=logs.object_uuid").
where("EXISTS(SELECT target_uuid FROM permission_view "+
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 9f053c0..2daa37e 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -35,6 +35,7 @@ class User < ArvadosModel
user.username.nil? and user.email
}
after_create :add_system_group_permission_link
+ after_create :invalidate_permissions_cache
after_create :auto_setup_new_user, :if => Proc.new { |user|
Rails.configuration.auto_setup_new_users and
(user.uuid != system_user_uuid) and
@@ -150,10 +151,13 @@ class User < ArvadosModel
end
end
+ def invalidate_permissions_cache(timestamp=nil)
+ User.invalidate_permissions_cache
+ end
+
# Return a hash of {user_uuid: group_perms}
def self.all_group_permissions
all_perms = {}
- User.install_view('permission')
ActiveRecord::Base.connection.
exec_query('SELECT user_uuid, target_owner_uuid, perm_level, trashed
FROM permission_view
@@ -172,7 +176,7 @@ class User < ArvadosModel
# objects owned by group_uuid.
def calculate_group_permissions
group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
- User.install_view('permission')
+ User.fresh_permission_view
ActiveRecord::Base.connection.
exec_query('SELECT target_owner_uuid, perm_level, trashed
FROM permission_view
@@ -207,6 +211,14 @@ class User < ArvadosModel
r
end
+ def self.fresh_permission_view
+ r = Rails.cache.read "#{PERM_CACHE_PREFIX}_fresh"
+ if r.nil?
+ ActiveRecord::Base.connection.exec_query('REFRESH MATERIALIZED VIEW permission_view')
+ Rails.cache.write "#{PERM_CACHE_PREFIX}_fresh", 1, expires_in: PERM_CACHE_TTL
+ end
+ end
+
# create links
def setup(openid_prefix:, repo_name: nil, vm_uuid: nil)
oid_login_perm = create_oid_login_perm openid_prefix
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 9656e72..77c833a 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -790,6 +790,68 @@ ALTER SEQUENCE nodes_id_seq OWNED BY nodes.id;
--
+-- Name: permission_view; Type: MATERIALIZED VIEW; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE MATERIALIZED VIEW 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
+ FROM ((links
+ LEFT JOIN perm_value pv ON ((pv.name = (links.name)::text)))
+ LEFT JOIN 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"
+ FROM groups
+ ), perm(val, follow, user_uuid, target_uuid, trashed, startnode) AS (
+ SELECT (3)::smallint AS val,
+ false AS follow,
+ (users.uuid)::character varying(32) AS user_uuid,
+ (users.uuid)::character varying(32) AS target_uuid,
+ (0)::smallint AS trashed,
+ true AS startnode
+ FROM 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))::smallint AS trashed,
+ false AS startnode
+ FROM (perm perm_1
+ JOIN perm_edges edges ON (((perm_1.startnode OR 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: pipeline_instances; Type: TABLE; Schema: public; Owner: -; Tablespace:
--
@@ -872,6 +934,30 @@ ALTER SEQUENCE pipeline_templates_id_seq OWNED BY pipeline_templates.id;
--
+-- Name: read_permissions; Type: VIEW; Schema: public; Owner: -
+--
+
+CREATE VIEW read_permissions AS
+ WITH RECURSIVE read_permissions(follow, user_uuid, readable_uuid) AS (
+ SELECT true AS bool,
+ users.uuid,
+ users.uuid
+ FROM users
+ UNION
+ SELECT (((links.name)::text = 'can_manage'::text) OR ((links.head_uuid)::text ~~ 'su92l-j7d0g-%'::text)) AS follow,
+ rp.user_uuid,
+ links.head_uuid
+ FROM read_permissions rp,
+ links
+ WHERE (rp.follow AND ((links.tail_uuid)::text = (rp.readable_uuid)::text))
+ )
+ SELECT read_permissions.follow,
+ read_permissions.user_uuid,
+ read_permissions.readable_uuid
+ FROM read_permissions;
+
+
+--
-- Name: repositories; Type: TABLE; Schema: public; Owner: -; Tablespace:
--
@@ -908,6 +994,18 @@ ALTER SEQUENCE repositories_id_seq OWNED BY repositories.id;
--
+-- Name: rp_cache; Type: MATERIALIZED VIEW; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE MATERIALIZED VIEW rp_cache AS
+ SELECT read_permissions.follow,
+ read_permissions.user_uuid,
+ read_permissions.readable_uuid
+ FROM read_permissions
+ WITH NO DATA;
+
+
+--
-- Name: schema_migrations; Type: TABLE; Schema: public; Owner: -; Tablespace:
--
@@ -1673,6 +1771,13 @@ CREATE INDEX index_collections_on_modified_at ON collections USING btree (modifi
--
+-- Name: index_collections_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_collections_on_modified_at_uuid ON collections USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_collections_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -1722,6 +1827,13 @@ CREATE UNIQUE INDEX index_commits_on_repository_name_and_sha1 ON commits USING b
--
+-- Name: index_container_requests_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_container_requests_on_modified_at_uuid ON container_requests USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_container_requests_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -1792,6 +1904,13 @@ CREATE INDEX index_groups_on_modified_at ON groups USING btree (modified_at);
--
+-- Name: index_groups_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_groups_on_modified_at_uuid ON groups USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_groups_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -1911,6 +2030,13 @@ CREATE INDEX index_jobs_on_modified_at ON jobs USING btree (modified_at);
--
+-- Name: index_jobs_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_jobs_on_modified_at_uuid ON jobs USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_jobs_on_output; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2030,6 +2156,13 @@ CREATE INDEX index_links_on_modified_at ON links USING btree (modified_at);
--
+-- Name: index_links_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_links_on_modified_at_uuid ON links USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_links_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2170,6 +2303,13 @@ CREATE INDEX index_pipeline_instances_on_modified_at ON pipeline_instances USING
--
+-- Name: index_pipeline_instances_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_pipeline_instances_on_modified_at_uuid ON pipeline_instances USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_pipeline_instances_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2198,6 +2338,13 @@ CREATE INDEX index_pipeline_templates_on_modified_at ON pipeline_templates USING
--
+-- Name: index_pipeline_templates_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_pipeline_templates_on_modified_at_uuid ON pipeline_templates USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_pipeline_templates_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2212,6 +2359,13 @@ CREATE UNIQUE INDEX index_pipeline_templates_on_uuid ON pipeline_templates USING
--
+-- Name: index_repositories_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_repositories_on_modified_at_uuid ON repositories USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_repositories_on_name; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2296,6 +2450,13 @@ CREATE INDEX index_users_on_modified_at ON users USING btree (modified_at);
--
+-- Name: index_users_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_users_on_modified_at_uuid ON users USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_users_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2324,6 +2485,13 @@ CREATE INDEX index_virtual_machines_on_hostname ON virtual_machines USING btree
--
+-- Name: index_virtual_machines_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_virtual_machines_on_modified_at_uuid ON virtual_machines USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_virtual_machines_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2338,6 +2506,13 @@ CREATE UNIQUE INDEX index_virtual_machines_on_uuid ON virtual_machines USING btr
--
+-- Name: index_workflows_on_modified_at_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_workflows_on_modified_at_uuid ON workflows USING btree (modified_at DESC, uuid);
+
+
+--
-- Name: index_workflows_on_owner_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2415,6 +2590,20 @@ CREATE INDEX nodes_search_index ON nodes USING btree (uuid, owner_uuid, modified
--
+-- Name: permission_target_trashed; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX permission_target_trashed ON permission_view USING btree (trashed, target_uuid);
+
+
+--
+-- Name: permission_target_user_trashed_level; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX permission_target_user_trashed_level ON permission_view USING btree (user_uuid, trashed, perm_level);
+
+
+--
-- Name: pipeline_instances_full_text_search_idx; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2464,6 +2653,13 @@ CREATE INDEX specimens_search_index ON specimens USING btree (uuid, owner_uuid,
--
+-- Name: test_1; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX test_1 ON collections USING btree (id) WHERE (delete_at IS NULL);
+
+
+--
-- Name: traits_search_index; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -2817,3 +3013,5 @@ INSERT INTO schema_migrations (version) VALUES ('20170628185847');
INSERT INTO schema_migrations (version) VALUES ('20170824202826');
+INSERT INTO schema_migrations (version) VALUES ('20170906224040');
+
commit 26a45f49291391c3ed791ac59d4949df0d93ebfc
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Sep 6 16:01:36 2017 -0400
12032: Fix owner_check to include user_uuids and target_owner_uuid
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 10996d4..f5d20b1 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -292,7 +292,7 @@ class ArvadosModel < ActiveRecord::Base
perm_check = "perm_level >= 1"
if self.column_names.include? "owner_uuid" and sql_table != "groups"
- owner_check = "OR target_uuid = #{sql_table}.owner_uuid"
+ owner_check = "OR #{sql_table}.owner_uuid IN (:user_uuids) OR (target_uuid = #{sql_table}.owner_uuid AND target_owner_uuid IS NOT NULL)"
else
owner_check = ""
end
commit 44666813323b5940cfa25f5d44638c74052cde4c
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Sep 6 14:54:50 2017 -0400
12032: Make queries to groups table a special case, streamlines permission logic.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index d656970..10996d4 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -282,39 +282,24 @@ class ArvadosModel < ActiveRecord::Base
# Can read object (evidently a group or user) whose UUID is listed
# explicitly in user_uuids.
sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
+
if include_trash
- not_trashed = ""
- not_null = "IS NOT NULL"
+ trashed_check = ""
else
- not_trashed = "0 IN"
- not_null = ""
+ trashed_check = "trashed = 0 AND"
end
perm_check = "perm_level >= 1"
- if self.column_names.include? "owner_uuid"
- # to be readable:
- # row(s) exists granting readable permission to uuid or owner, and none are trashed
- sql_conds += ["#{not_trashed} (SELECT MAX(trashed) FROM permission_view "+
- "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
- "(#{sql_table}.uuid = target_uuid OR (#{sql_table}.owner_uuid = target_uuid "+
- "AND target_owner_uuid IS NOT NULL))) #{not_null}"]
- # reader is owner, and item is not trashed
- not_trashed_clause = if include_trash then
- ""
- else
- "AND 1 NOT IN (SELECT MAX(trashed) FROM permission_view "+
- "WHERE #{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid)"
- end
- sql_conds += ["(#{sql_table}.owner_uuid IN (:user_uuids) #{not_trashed_clause})"]
+ if self.column_names.include? "owner_uuid" and sql_table != "groups"
+ owner_check = "OR target_uuid = #{sql_table}.owner_uuid"
else
- # to be readable:
- # * a non-trash row exists with readable permission uuid
- sql_conds += ["#{not_trashed} (SELECT MAX(trashed) FROM permission_view "+
- "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
- "#{sql_table}.uuid = target_uuid) #{not_null}"]
+ owner_check = ""
end
+ sql_conds += ["EXISTS(SELECT 1 FROM permission_view "+
+ "WHERE user_uuid IN (:user_uuids) AND #{trashed_check} #{perm_check} AND (target_uuid = #{sql_table}.uuid #{owner_check}))"]
+
if sql_table == "links"
# Match any permission link that gives one of the authorized
# users some permission _or_ gives anyone else permission to
diff --git a/services/api/test/performance/permission_test.rb b/services/api/test/performance/permission_test.rb
index 2db42bc..e4a3b0a 100644
--- a/services/api/test/performance/permission_test.rb
+++ b/services/api/test/performance/permission_test.rb
@@ -48,6 +48,7 @@ class PermissionPerfTest < ActionDispatch::IntegrationTest
(0..4).each do
puts(Benchmark.measure do
get '/arvados/v1/groups', {format: :json, limit: 1000}, auth(:permission_perftest)
+ assert json_response['items_available'] >= n
end)
end
end
commit 66f5e61f6f026c285b0120cf0187cdf6df6b53e9
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 31 14:58:43 2017 -0400
12032: Add copyright notice
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/db/migrate/20170824202826_trashable_groups.rb b/services/api/db/migrate/20170824202826_trashable_groups.rb
index 6d2a09a..17fc31f 100644
--- a/services/api/db/migrate/20170824202826_trashable_groups.rb
+++ b/services/api/db/migrate/20170824202826_trashable_groups.rb
@@ -1,3 +1,7 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
class TrashableGroups < ActiveRecord::Migration
def up
add_column :groups, :trash_at, :datetime
commit 51a3a54e86aa5b78dac747489944de05d7cb6843
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 31 14:50:29 2017 -0400
12032: Bug fix so include_trash still respects permissions.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 0312d95..3803d37 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -28,7 +28,7 @@ class Arvados::V1::CollectionsController < ApplicationController
def find_objects_for_index
if params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name)
- @objects = Collection.unscoped.readable_by(*@read_users)
+ @objects = Collection.readable_by(*@read_users, {include_trash: true, query_on: Collection.unscoped})
end
super
end
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 4a4f7ec..d656970 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -282,29 +282,37 @@ class ArvadosModel < ActiveRecord::Base
# Can read object (evidently a group or user) whose UUID is listed
# explicitly in user_uuids.
sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
- trashed_check = if include_trash then "EXISTS" else "0 IN " end
+ if include_trash
+ not_trashed = ""
+ not_null = "IS NOT NULL"
+ else
+ not_trashed = "0 IN"
+ not_null = ""
+ end
+
perm_check = "perm_level >= 1"
if self.column_names.include? "owner_uuid"
# to be readable:
# row(s) exists granting readable permission to uuid or owner, and none are trashed
- sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+ sql_conds += ["#{not_trashed} (SELECT MAX(trashed) FROM permission_view "+
"WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
- "(#{sql_table}.uuid = target_uuid OR (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL)))"]
+ "(#{sql_table}.uuid = target_uuid OR (#{sql_table}.owner_uuid = target_uuid "+
+ "AND target_owner_uuid IS NOT NULL))) #{not_null}"]
# reader is owner, and item is not trashed
- not_trashed_check = if include_trash then
+ not_trashed_clause = if include_trash then
""
else
"AND 1 NOT IN (SELECT MAX(trashed) FROM permission_view "+
"WHERE #{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid)"
end
- sql_conds += ["(#{sql_table}.owner_uuid IN (:user_uuids) #{not_trashed_check})"]
+ sql_conds += ["(#{sql_table}.owner_uuid IN (:user_uuids) #{not_trashed_clause})"]
else
# to be readable:
# * a non-trash row exists with readable permission uuid
- sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+ sql_conds += ["#{not_trashed} (SELECT MAX(trashed) FROM permission_view "+
"WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
- "#{sql_table}.uuid = target_uuid) "]
+ "#{sql_table}.uuid = target_uuid) #{not_null}"]
end
if sql_table == "links"
diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 4cf8778..47f0887 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -1112,7 +1112,7 @@ EOS
test 'cannot index collection in trashed subproject' do
authorize_with :active
- get :index
+ get :index, { limit: 1000 }
assert_response :success
item_uuids = json_response['items'].map do |item|
item['uuid']
@@ -1123,7 +1123,20 @@ EOS
test 'can index collection in untrashed subproject' do
authorize_with :active
Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
- get :index
+ get :index, { limit: 1000 }
+ assert_response :success
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_includes(item_uuids, collections(:collection_in_trashed_subproject).uuid)
+ end
+
+ test 'can index trashed subproject collection with include_trash' do
+ authorize_with :active
+ get :index, {
+ include_trash: true,
+ limit: 1000
+ }
assert_response :success
item_uuids = json_response['items'].map do |item|
item['uuid']
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 5db8475..3687819 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -689,5 +689,21 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_response :success
assert_match /^trashed subproject 3 \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
end
+
+ test "move trashed subproject to new owner #{auth}" do
+ authorize_with auth
+ put :update, {
+ id: groups(:trashed_subproject).uuid,
+ group: {
+ owner_uuid: users(:active).uuid
+ },
+ include_trashed: true,
+ format: :json,
+ }
+ # Currently fails but might want to change it in the future
+ # so leave the test here.
+ assert_response 404
+ end
+
end
end
commit ef3c1bf7fd89b27a1aed3b0604fee649f26ba6e3
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 31 12:10:11 2017 -0400
12032: Test for ensure_unique_name when untrashing groups.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/doc/api/methods/collections.html.textile.liquid b/doc/api/methods/collections.html.textile.liquid
index ce1e0c7..d753f09 100644
--- a/doc/api/methods/collections.html.textile.liquid
+++ b/doc/api/methods/collections.html.textile.liquid
@@ -32,6 +32,9 @@ table(table table-bordered table-condensed).
|replication_desired|number|Minimum storage replication level desired for each data block referenced by this collection. A value of @null@ signifies that the site default replication level (typically 2) is desired.|@2@|
|replication_confirmed|number|Replication level most recently confirmed by the storage system. This field is null when a collection is first created, and is reset to null when the manifest_text changes in a way that introduces a new data block. An integer value indicates the replication level of the _least replicated_ data block in the collection.|@2@, null|
|replication_confirmed_at|datetime|When replication_confirmed was confirmed. If replication_confirmed is null, this field is also null.||
+|trash_at|datetime|If @trash_at@ is non-null and in the past, this collection will be hidden from API calls. May be untrashed.||
+|delete_at|datetime|If @delete_at@ is non-null and in the past, the collection may be permanently deleted.||
+|is_trashed|datetime|True if @trash_at@ is in the past, false if not.||
h3. Conditions of creating a Collection
@@ -61,7 +64,7 @@ table(table table-bordered table-condensed).
h3. delete
-Delete an existing Collection.
+Put a Collection in the trash. This sets the @trash_at@ field to @now@ and @delete_at@ field to @now@ + token TTL. A trashed group is invisible to most API calls unless the @include_trash@ parameter is true.
Arguments:
@@ -97,3 +100,14 @@ table(table table-bordered table-condensed).
|_. Argument |_. Type |_. Description |_. Location |_. Example |
{background:#ccffcc}.|uuid|string|The UUID of the Collection in question.|path||
|collection|object||query||
+
+h3. untrash
+
+Remove a Collection from the trash. This sets the @trash_at@ and @delete_at@ fields to @null at .
+
+Arguments:
+
+table(table table-bordered table-condensed).
+|_. Argument |_. Type |_. Description |_. Location |_. Example |
+{background:#ccffcc}.|uuid|string|The UUID of the Collection to untrash.|path||
+|ensure_unique_name|boolean (default false)|Rename collection uniquely if untrashing it would fail with a unique name conflict.|query||
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index 5c8dd4b..2716056 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -29,6 +29,9 @@ table(table table-bordered table-condensed).
null|
|description|text|||
|writable_by|array|List of UUID strings identifying Users and other Groups that have write permission for this Group. Only users who are allowed to administer the Group will receive a full list. Other users will receive a partial list that includes the Group's owner_uuid and (if applicable) their own user UUID.||
+|trash_at|datetime|If @trash_at@ is non-null and in the past, this group and all objects directly or indirectly owned by the group will be hidden from API calls. May be untrashed.||
+|delete_at|datetime|If @delete_at@ is non-null and in the past, the group and all objects directly or indirectly owned by the group may be permanently deleted.||
+|is_trashed|datetime|True if @trash_at@ is in the past, false if not.||
h2. Methods
@@ -66,7 +69,7 @@ table(table table-bordered table-condensed).
h3. delete
-Delete an existing Group.
+Put a Group in the trash. This sets the @trash_at@ field to @now@ and @delete_at@ field to @now@ + token TTL. A trashed group is invisible to most API calls unless the @include_trash@ parameter is true. All objects directly or indirectly owned by the Group are considered trashed as well.
Arguments:
@@ -110,3 +113,14 @@ table(table table-bordered table-condensed).
|_. Argument |_. Type |_. Description |_. Location |_. Example |
{background:#ccffcc}.|uuid|string|The UUID of the Group in question.|path||
|group|object||query||
+
+h3. untrash
+
+Remove a Group from the trash. This sets the @trash_at@ and @delete_at@ fields to @null at .
+
+Arguments:
+
+table(table table-bordered table-condensed).
+|_. Argument |_. Type |_. Description |_. Location |_. Example |
+{background:#ccffcc}.|uuid|string|The UUID of the Group to untrash.|path||
+|ensure_unique_name|boolean (default false)|Rename project uniquely if untrashing it would fail with a unique name conflict.|query||
diff --git a/services/api/db/migrate/20170824202826_trashable_groups.rb b/services/api/db/migrate/20170824202826_trashable_groups.rb
index b7e3373..6d2a09a 100644
--- a/services/api/db/migrate/20170824202826_trashable_groups.rb
+++ b/services/api/db/migrate/20170824202826_trashable_groups.rb
@@ -1,7 +1,38 @@
class TrashableGroups < ActiveRecord::Migration
- def change
+ def up
add_column :groups, :trash_at, :datetime
- add_column :groups, :delete_at, :datetime
+ add_index(:groups, :trash_at)
+
add_column :groups, :is_trashed, :boolean, null: false, default: false
+ add_index(:groups, :is_trashed)
+
+ add_column :groups, :delete_at, :datetime
+ add_index(:groups, :delete_at)
+
+ Group.reset_column_information
+ add_index(:groups, [:owner_uuid, :name],
+ unique: true,
+ where: 'is_trashed = false',
+ name: 'index_groups_on_owner_uuid_and_name')
+ remove_index(:groups,
+ name: 'groups_owner_uuid_name_unique')
+ end
+
+ def down
+ Group.transaction do
+ add_index(:groups, [:owner_uuid, :name], unique: true,
+ name: 'groups_owner_uuid_name_unique')
+ remove_index(:groups,
+ name: 'index_groups_on_owner_uuid_and_name')
+
+ remove_index(:groups, :delete_at)
+ remove_column(:groups, :delete_at)
+
+ remove_index(:groups, :is_trashed)
+ remove_column(:groups, :is_trashed)
+
+ remove_index(:groups, :trash_at)
+ remove_column(:groups, :trash_at)
+ end
end
end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 90aa0a3..9656e72 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -393,8 +393,8 @@ CREATE TABLE groups (
updated_at timestamp without time zone NOT NULL,
group_class character varying(255),
trash_at timestamp without time zone,
- delete_at timestamp without time zone,
- is_trashed boolean DEFAULT false NOT NULL
+ is_trashed boolean DEFAULT false NOT NULL,
+ delete_at timestamp without time zone
);
@@ -1547,13 +1547,6 @@ CREATE INDEX groups_full_text_search_idx ON groups USING gin (to_tsvector('engli
--
--- Name: groups_owner_uuid_name_unique; Type: INDEX; Schema: public; Owner: -; Tablespace:
---
-
-CREATE UNIQUE INDEX groups_owner_uuid_name_unique ON groups USING btree (owner_uuid, name);
-
-
---
-- Name: groups_search_index; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -1771,6 +1764,13 @@ CREATE INDEX index_groups_on_created_at ON groups USING btree (created_at);
--
+-- Name: index_groups_on_delete_at; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_groups_on_delete_at ON groups USING btree (delete_at);
+
+
+--
-- Name: index_groups_on_group_class; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -1778,6 +1778,13 @@ CREATE INDEX index_groups_on_group_class ON groups USING btree (group_class);
--
+-- Name: index_groups_on_is_trashed; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_groups_on_is_trashed ON groups USING btree (is_trashed);
+
+
+--
-- Name: index_groups_on_modified_at; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
@@ -1792,6 +1799,20 @@ CREATE INDEX index_groups_on_owner_uuid ON groups USING btree (owner_uuid);
--
+-- Name: index_groups_on_owner_uuid_and_name; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE UNIQUE INDEX index_groups_on_owner_uuid_and_name ON groups USING btree (owner_uuid, name) WHERE (is_trashed = false);
+
+
+--
+-- Name: index_groups_on_trash_at; Type: INDEX; Schema: public; Owner: -; Tablespace:
+--
+
+CREATE INDEX index_groups_on_trash_at ON groups USING btree (trash_at);
+
+
+--
-- Name: index_groups_on_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace:
--
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 b80fcb1..5db8475 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -673,5 +673,21 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert !Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
end
+ test "untrash project with name conflict #{auth}" do
+ authorize_with auth
+ [:trashed_project].each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ gc = Group.create!({owner_uuid: "zzzzz-j7d0g-trashedproject1",
+ name: "trashed subproject 3",
+ group_class: "project"})
+ post :untrash, {
+ id: groups(:trashed_subproject3).uuid,
+ format: :json,
+ ensure_unique_name: true
+ }
+ assert_response :success
+ assert_match /^trashed subproject 3 \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
+ end
end
end
commit bb19b61be5926453b3316c26543401f9e5eda871
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 31 10:37:13 2017 -0400
12032: Refactor readable_by to minimize subqueries.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 46b96a9..d09283d 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -180,7 +180,7 @@ class ApplicationController < ActionController::Base
end
def find_objects_for_index
- @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name))})
+ @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || 'untrash' == action_name)})
apply_where_limit_order_params
end
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 9e0cdf8..8e195ff 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -163,13 +163,14 @@ class Arvados::V1::GroupsController < ApplicationController
end
end.compact
- if klass == Collection and params[:include_trash]
- @objects = klass.unscoped.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
- order(request_order).where(where_conds)
- else
- @objects = klass.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
- order(request_order).where(where_conds)
- end
+ query_on = if klass == Collection and params[:include_trash]
+ klass.unscoped
+ else
+ klass
+ end
+ @objects = query_on.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
+ order(request_order).where(where_conds)
+
klass_limit = limit_all - all_objects.count
@limit = klass_limit
apply_where_limit_order_params klass
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 518d4c8..4a4f7ec 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -265,52 +265,46 @@ class ArvadosModel < ActiveRecord::Base
# Check if any of the users are admin.
if users_list.select { |u| u.is_admin }.any?
if !include_trash
- # exclude rows that are trashed.
+ # exclude rows that are explicitly trashed.
if self.column_names.include? "owner_uuid"
- sql_conds += ["NOT EXISTS(SELECT target_uuid
+ sql_conds += ["NOT EXISTS(SELECT 1
FROM permission_view
WHERE trashed = 1 AND
(#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"]
else
- sql_conds += ["NOT EXISTS(SELECT target_uuid
+ sql_conds += ["NOT EXISTS(SELECT 1
FROM permission_view
WHERE trashed = 1 AND
(#{sql_table}.uuid = target_uuid))"]
end
end
else
- trash_clause = if !include_trash then "trashed = 0 AND" else "" end
-
# Can read object (evidently a group or user) whose UUID is listed
# explicitly in user_uuids.
sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
-
- direct_permission_check = "EXISTS(SELECT 1 FROM permission_view
- WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
- (#{sql_table}.uuid = target_uuid))"
+ trashed_check = if include_trash then "EXISTS" else "0 IN " end
+ perm_check = "perm_level >= 1"
if self.column_names.include? "owner_uuid"
- # if an explicit permission row exists for the uuid in question, apply
- # the "direct_permission_check"
- # if not, check for permission to read the owner instead
- sql_conds += ["CASE
- WHEN EXISTS(select 1 FROM permission_view where target_uuid = #{sql_table}.uuid)
- THEN #{direct_permission_check}
- ELSE EXISTS(SELECT 1 FROM permission_view
- WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
- (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL))
- END"]
- # Can also read if one of the users is the owner of the object.
- trash_clause = if !include_trash
- "1 NOT IN (SELECT trashed
- FROM permission_view
- WHERE #{sql_table}.uuid = target_uuid) AND"
- else
- ""
- end
- sql_conds += ["(#{trash_clause} #{sql_table}.owner_uuid IN (:user_uuids))"]
+ # to be readable:
+ # row(s) exists granting readable permission to uuid or owner, and none are trashed
+ sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+ "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
+ "(#{sql_table}.uuid = target_uuid OR (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL)))"]
+ # reader is owner, and item is not trashed
+ not_trashed_check = if include_trash then
+ ""
+ else
+ "AND 1 NOT IN (SELECT MAX(trashed) FROM permission_view "+
+ "WHERE #{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid)"
+ end
+ sql_conds += ["(#{sql_table}.owner_uuid IN (:user_uuids) #{not_trashed_check})"]
else
- sql_conds += [direct_permission_check]
+ # to be readable:
+ # * a non-trash row exists with readable permission uuid
+ sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+ "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
+ "#{sql_table}.uuid = target_uuid) "]
end
if sql_table == "links"
diff --git a/services/api/test/integration/groups_test.rb b/services/api/test/integration/groups_test.rb
index 6226ffd..c4ab3cf 100644
--- a/services/api/test/integration/groups_test.rb
+++ b/services/api/test/integration/groups_test.rb
@@ -99,7 +99,8 @@ class GroupsTest < ActionDispatch::IntegrationTest
test "group contents with include trash collections" do
get "/arvados/v1/groups/contents", {
include_trash: "true",
- filters: [["uuid", "is_a", "arvados#collection"]].to_json
+ filters: [["uuid", "is_a", "arvados#collection"]].to_json,
+ limit: 1000
}, auth(:active)
assert_response 200
@@ -108,4 +109,17 @@ class GroupsTest < ActionDispatch::IntegrationTest
assert_includes coll_uuids, collections(:foo_collection_in_aproject).uuid
assert_includes coll_uuids, collections(:expired_collection).uuid
end
+
+ test "group contents without trash collections" do
+ get "/arvados/v1/groups/contents", {
+ filters: [["uuid", "is_a", "arvados#collection"]].to_json,
+ limit: 1000
+ }, auth(:active)
+ assert_response 200
+
+ coll_uuids = []
+ json_response['items'].each { |c| coll_uuids << c['uuid'] }
+ assert_includes coll_uuids, collections(:foo_collection_in_aproject).uuid
+ assert_not_includes coll_uuids, collections(:expired_collection).uuid
+ end
end
commit 8e7a4a55686fc9f9aa80723bfd6b3c0944802317
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Aug 30 13:13:29 2017 -0400
12032: Log.readable_by uses permission_view
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index 7f2d3ef..99d0e28 100644
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -67,17 +67,14 @@ class Log < ArvadosModel
return self
end
user_uuids = users_list.map { |u| u.uuid }
- uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
- uuid_list.uniq!
- permitted = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (:uuids))"
+
+ User.install_view('permission')
+
joins("LEFT JOIN container_requests ON container_requests.container_uuid=logs.object_uuid").
- where("logs.object_uuid IN #{permitted} OR "+
- "container_requests.uuid IN (:uuids) OR "+
- "container_requests.owner_uuid IN (:uuids) OR "+
- "logs.object_uuid IN (:uuids) OR "+
- "logs.owner_uuid IN (:uuids) OR "+
- "logs.object_owner_uuid IN (:uuids)",
- uuids: uuid_list)
+ where("EXISTS(SELECT target_uuid FROM permission_view "+
+ "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND "+
+ "target_uuid IN (container_requests.uuid, container_requests.owner_uuid, logs.object_uuid, logs.owner_uuid, logs.object_owner_uuid))",
+ user_uuids: user_uuids)
end
protected
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 0e2db76..9f053c0 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -152,8 +152,8 @@ class User < ArvadosModel
# Return a hash of {user_uuid: group_perms}
def self.all_group_permissions
- install_view('permission')
all_perms = {}
+ User.install_view('permission')
ActiveRecord::Base.connection.
exec_query('SELECT user_uuid, target_owner_uuid, perm_level, trashed
FROM permission_view
@@ -171,9 +171,8 @@ class User < ArvadosModel
# and perm_hash[:write] are true if this user can read and write
# objects owned by group_uuid.
def calculate_group_permissions
- self.class.install_view('permission')
-
group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
+ User.install_view('permission')
ActiveRecord::Base.connection.
exec_query('SELECT target_owner_uuid, perm_level, trashed
FROM permission_view
commit d39674945d6d69d63866f613210fbc3eb569bc67
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Tue Aug 29 09:08:19 2017 -0400
12032: Controller support for group trash.
Conttroller support and testing for contents, index, show, trash, untrash,
containers.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index a3d8f08..46b96a9 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -180,7 +180,7 @@ class ApplicationController < ActionController::Base
end
def find_objects_for_index
- @objects ||= model_class.readable_by(*@read_users)
+ @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name))})
apply_where_limit_order_params
end
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 87d88fe..0312d95 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -3,9 +3,11 @@
# SPDX-License-Identifier: AGPL-3.0
require "arvados/keep"
+require "trashable"
class Arvados::V1::CollectionsController < ApplicationController
include DbCurrentTime
+ include TrashableController
def self._index_requires_parameters
(super rescue {}).
@@ -16,7 +18,6 @@ class Arvados::V1::CollectionsController < ApplicationController
})
end
-
def create
if resource_attrs[:uuid] and (loc = Keep::Locator.parse(resource_attrs[:uuid]))
resource_attrs[:portable_data_hash] = loc.to_s
@@ -58,39 +59,6 @@ class Arvados::V1::CollectionsController < ApplicationController
end
end
- def destroy
- if !@object.is_trashed
- @object.update_attributes!(trash_at: db_current_time)
- end
- earliest_delete = (@object.trash_at +
- Rails.configuration.blob_signature_ttl.seconds)
- if @object.delete_at > earliest_delete
- @object.update_attributes!(delete_at: earliest_delete)
- end
- show
- end
-
- def trash
- if !@object.is_trashed
- @object.update_attributes!(trash_at: db_current_time)
- end
- show
- end
-
- def untrash
- if @object.is_trashed
- @object.trash_at = nil
-
- if params[:ensure_unique_name]
- @object.save_with_unique_name!
- else
- @object.save!
- end
- else
- raise InvalidStateTransitionError
- end
- show
- end
def find_collections(visited, sp, &b)
case sp
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 75ef095..9e0cdf8 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -2,7 +2,19 @@
#
# SPDX-License-Identifier: AGPL-3.0
+require "trashable"
+
class Arvados::V1::GroupsController < ApplicationController
+ include TrashableController
+
+ def self._index_requires_parameters
+ (super rescue {}).
+ merge({
+ include_trash: {
+ type: 'boolean', required: false, description: "Include items whose is_trashed attribute is true."
+ },
+ })
+ end
def self._contents_requires_parameters
params = _index_requires_parameters.
@@ -152,10 +164,10 @@ class Arvados::V1::GroupsController < ApplicationController
end.compact
if klass == Collection and params[:include_trash]
- @objects = klass.unscoped.readable_by(*@read_users).
+ @objects = klass.unscoped.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
order(request_order).where(where_conds)
else
- @objects = klass.readable_by(*@read_users).
+ @objects = klass.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
order(request_order).where(where_conds)
end
klass_limit = limit_all - all_objects.count
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index ccd8f82..518d4c8 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -254,7 +254,8 @@ class ArvadosModel < ActiveRecord::Base
# Collect the UUIDs of the authorized users.
sql_table = kwargs.fetch(:table_name, table_name)
- include_trashed = kwargs.fetch(:include_trashed, false)
+ include_trash = kwargs.fetch(:include_trash, false)
+ query_on = kwargs.fetch(:query_on, self)
sql_conds = []
user_uuids = users_list.map { |u| u.uuid }
@@ -263,49 +264,53 @@ class ArvadosModel < ActiveRecord::Base
# Check if any of the users are admin.
if users_list.select { |u| u.is_admin }.any?
- if !include_trashed
+ if !include_trash
# exclude rows that are trashed.
if self.column_names.include? "owner_uuid"
sql_conds += ["NOT EXISTS(SELECT target_uuid
- FROM permission_view pv
+ FROM permission_view
WHERE trashed = 1 AND
- (#{sql_table}.uuid = pv.target_uuid OR #{sql_table}.owner_uuid = pv.target_uuid))"]
+ (#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"]
else
sql_conds += ["NOT EXISTS(SELECT target_uuid
- FROM permission_view pv
+ FROM permission_view
WHERE trashed = 1 AND
- (#{sql_table}.uuid = pv.target_uuid))"]
+ (#{sql_table}.uuid = target_uuid))"]
end
end
else
- # Match objects which appear in the permission view
- trash_clause = if !include_trashed then "trashed = 0 AND" else "" end
+ trash_clause = if !include_trash then "trashed = 0 AND" else "" end
- # Match any object (evidently a group or user) whose UUID is
- # listed explicitly in user_uuids.
+ # Can read object (evidently a group or user) whose UUID is listed
+ # explicitly in user_uuids.
sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
+ direct_permission_check = "EXISTS(SELECT 1 FROM permission_view
+ WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
+ (#{sql_table}.uuid = target_uuid))"
+
if self.column_names.include? "owner_uuid"
- sql_conds += ["EXISTS(SELECT target_uuid
- FROM permission_view pv
+ # if an explicit permission row exists for the uuid in question, apply
+ # the "direct_permission_check"
+ # if not, check for permission to read the owner instead
+ sql_conds += ["CASE
+ WHEN EXISTS(select 1 FROM permission_view where target_uuid = #{sql_table}.uuid)
+ THEN #{direct_permission_check}
+ ELSE EXISTS(SELECT 1 FROM permission_view
WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
- (#{sql_table}.uuid = pv.target_uuid OR
- (#{sql_table}.owner_uuid = pv.target_uuid AND pv.target_owner_uuid is NOT NULL)))"]
- # Match any object whose owner is listed explicitly in
- # user_uuids.
- trash_clause = if !include_trashed
+ (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL))
+ END"]
+ # Can also read if one of the users is the owner of the object.
+ trash_clause = if !include_trash
"1 NOT IN (SELECT trashed
- FROM permission_view pv
- WHERE #{sql_table}.uuid = pv.target_uuid) AND"
+ FROM permission_view
+ WHERE #{sql_table}.uuid = target_uuid) AND"
else
""
end
sql_conds += ["(#{trash_clause} #{sql_table}.owner_uuid IN (:user_uuids))"]
else
- sql_conds += ["EXISTS(SELECT target_uuid
- FROM permission_view pv
- WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
- (#{sql_table}.uuid = pv.target_uuid))"]
+ sql_conds += [direct_permission_check]
end
if sql_table == "links"
@@ -317,9 +322,9 @@ class ArvadosModel < ActiveRecord::Base
end
end
- where(sql_conds.join(' OR '),
- user_uuids: user_uuids,
- permission_link_classes: ['permission', 'resources'])
+ query_on.where(sql_conds.join(' OR '),
+ user_uuids: user_uuids,
+ permission_link_classes: ['permission', 'resources'])
end
def save_with_unique_name!
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 36c87af..d61ba75 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -297,17 +297,15 @@ class Container < ArvadosModel
end
def self.readable_by(*users_list)
- if users_list.select { |u| u.is_admin }.any?
- return self
+ # Load optional keyword arguments, if they exist.
+ if users_list.last.is_a? Hash
+ kwargs = users_list.pop
+ else
+ kwargs = {}
end
- user_uuids = users_list.map { |u| u.uuid }
- uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
- uuid_list.uniq!
- permitted = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (:uuids))"
- joins(:container_requests).
- where("container_requests.uuid IN #{permitted} OR "+
- "container_requests.owner_uuid IN (:uuids)",
- uuids: uuid_list)
+ kwargs[:query_on] = joins(:container_requests)
+ users_list.push kwargs
+ ContainerRequest.readable_by(*users_list)
end
def final?
diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index 73f143e..7f2d3ef 100644
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -60,6 +60,9 @@ class Log < ArvadosModel
end
def self.readable_by(*users_list)
+ if users_list.last.is_a? Hash
+ users_list.pop
+ end
if users_list.select { |u| u.is_admin }.any?
return self
end
diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb
index 2e9d618..5b6fe80 100644
--- a/services/api/config/routes.rb
+++ b/services/api/config/routes.rb
@@ -30,6 +30,8 @@ Server::Application.routes.draw do
resources :groups do
get 'contents', on: :collection
get 'contents', on: :member
+ post 'trash', on: :member
+ post 'untrash', on: :member
end
resources :humans
resources :job_tasks
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 639610a..71ac8c4 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -37,9 +37,8 @@ perm_edges (tail_uuid, head_uuid, val, follow, trashed) AS (
LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
WHERE links.link_class = 'permission'
UNION ALL
- SELECT owner_uuid, uuid, 3, true, CASE
- WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1
- ELSE 0 END
+ SELECT owner_uuid, uuid, 3, true,
+ CASE WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1 ELSE 0 END
FROM groups
),
perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
diff --git a/services/api/lib/trashable.rb b/services/api/lib/trashable.rb
index 1e2f466..38ebaf7 100644
--- a/services/api/lib/trashable.rb
+++ b/services/api/lib/trashable.rb
@@ -89,3 +89,40 @@ module Trashable
true
end
end
+
+module TrashableController
+ def destroy
+ if !@object.is_trashed
+ @object.update_attributes!(trash_at: db_current_time)
+ end
+ earliest_delete = (@object.trash_at +
+ Rails.configuration.blob_signature_ttl.seconds)
+ if @object.delete_at > earliest_delete
+ @object.update_attributes!(delete_at: earliest_delete)
+ end
+ show
+ end
+
+ def trash
+ if !@object.is_trashed
+ @object.update_attributes!(trash_at: db_current_time)
+ end
+ show
+ end
+
+ def untrash
+ if @object.is_trashed
+ @object.trash_at = nil
+
+ if params[:ensure_unique_name]
+ @object.save_with_unique_name!
+ else
+ @object.save!
+ end
+ else
+ raise InvalidStateTransitionError
+ end
+ show
+ end
+
+end
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 1cb5817..2411831 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -313,6 +313,13 @@ trashed_subproject:
owner_uuid: zzzzz-j7d0g-trashedproject1
name: trashed subproject
group_class: project
+ is_trashed: false
+
+trashed_subproject3:
+ uuid: zzzzz-j7d0g-trashedproject3
+ owner_uuid: zzzzz-j7d0g-trashedproject1
+ name: trashed subproject 3
+ group_class: project
trash_at: 2001-01-01T00:00:00Z
delete_at: 2038-03-01T00:00:00Z
is_trashed: true
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 043d665..b80fcb1 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -533,111 +533,145 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
### trashed project tests ###
[:active, :admin].each do |auth|
- test "trashed project is hidden in contents (#{auth})" do
- authorize_with auth
- get :contents, {
- id: users(:active).uuid,
- format: :json,
- }
- item_uuids = json_response['items'].map do |item|
- item['uuid']
+ # project: to query, to untrash, is visible, parent contents listing success
+ [[:trashed_project, [], false, true],
+ [:trashed_project, [:trashed_project], true, true],
+ [:trashed_subproject, [], false, false],
+ [:trashed_subproject, [:trashed_project], true, true],
+ [:trashed_subproject3, [:trashed_project], false, true],
+ [:trashed_subproject3, [:trashed_subproject3], false, false],
+ [:trashed_subproject3, [:trashed_project, :trashed_subproject3], true, true],
+ ].each do |project, untrash, visible, success|
+
+ test "contents listing #{project} #{untrash} as #{auth}" do
+ authorize_with auth
+ untrash.each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ get :contents, {
+ id: groups(project).owner_uuid,
+ format: :json
+ }
+ if success
+ assert_response :success
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ if visible
+ assert_includes(item_uuids, groups(project).uuid)
+ else
+ assert_not_includes(item_uuids, groups(project).uuid)
+ end
+ else
+ assert_response 404
+ end
end
- assert_not_includes(item_uuids, groups(:trashed_project).uuid)
- end
- test "untrashed project is visible in contents (#{auth})" do
- authorize_with auth
- Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
- get :contents, {
- id: users(:active).uuid,
- format: :json,
- }
- item_uuids = json_response['items'].map do |item|
- item['uuid']
+ test "contents of #{project} #{untrash} as #{auth}" do
+ authorize_with auth
+ untrash.each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ get :contents, {
+ id: groups(project).uuid,
+ format: :json
+ }
+ if visible
+ assert_response :success
+ else
+ assert_response 404
+ end
end
- assert_includes(item_uuids, groups(:trashed_project).uuid)
- end
- test "trashed project is hidden in index (#{auth})" do
- authorize_with :active
- get :index, {
- format: :json,
- }
- item_uuids = json_response['items'].map do |item|
- item['uuid']
+ test "index #{project} #{untrash} as #{auth}" do
+ authorize_with auth
+ untrash.each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ get :index, {
+ format: :json,
+ }
+ assert_response :success
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ if visible
+ assert_includes(item_uuids, groups(project).uuid)
+ else
+ assert_not_includes(item_uuids, groups(project).uuid)
+ end
end
- assert_not_includes(item_uuids, groups(:trashed_project).uuid)
- assert_not_includes(item_uuids, groups(:trashed_subproject).uuid)
- end
- test "untrashed project is visible in index (#{auth})" do
- authorize_with :active
- Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
- get :index, {
- format: :json,
- }
- item_uuids = json_response['items'].map do |item|
- item['uuid']
+ test "show #{project} #{untrash} as #{auth}" do
+ authorize_with auth
+ untrash.each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ get :show, {
+ id: groups(project).uuid,
+ format: :json
+ }
+ if visible
+ assert_response :success
+ else
+ assert_response 404
+ end
end
- assert_includes(item_uuids, groups(:trashed_project).uuid)
- assert_includes(item_uuids, groups(:trashed_subproject).uuid)
- end
- test "cannot get contents of trashed project (#{auth})" do
- authorize_with :active
- get :contents, {
- id: groups(:trashed_project).uuid,
- format: :json,
- }
- assert_response 404
- end
+ test "show include_trash #{project} #{untrash} as #{auth}" do
+ authorize_with auth
+ untrash.each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ get :show, {
+ id: groups(project).uuid,
+ format: :json,
+ include_trash: true
+ }
+ assert_response :success
+ end
- test "cannot get contents of trashed subproject (#{auth})" do
- authorize_with :active
- get :contents, {
- id: groups(:trashed_subproject).uuid,
- format: :json,
- }
- assert_response 404
+ test "index include_trash #{project} #{untrash} as #{auth}" do
+ authorize_with auth
+ untrash.each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ get :index, {
+ format: :json,
+ include_trash: true
+ }
+ assert_response :success
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_includes(item_uuids, groups(project).uuid)
+ end
end
- test "can get contents of untrashed project (#{auth})" do
- authorize_with :active
- Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
- get :contents, {
+ test "delete project #{auth}" do
+ authorize_with auth
+ [:trashed_project].each do |pr|
+ Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+ end
+ assert !Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
+ post :destroy, {
id: groups(:trashed_project).uuid,
format: :json,
}
- assert_response 200
- end
-
- test "can get contents of untrashed subproject (#{auth})" do
- authorize_with :active
- Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
- get :contents, {
- id: groups(:trashed_subproject).uuid,
- format: :json,
- }
- assert_response 200
+ assert_response :success
+ assert Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
end
- test "cannot show trashed project (#{auth})" do
- authorize_with :active
- get :show, {
+ test "untrash project #{auth}" do
+ authorize_with auth
+ assert Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
+ post :untrash, {
id: groups(:trashed_project).uuid,
format: :json,
}
- assert_response 404
+ assert_response :success
+ assert !Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
end
- test "cannot show trashed subproject (#{auth})" do
- authorize_with :active
- get :show, {
- id: groups(:trashed_subproject).uuid,
- format: :json,
- }
- assert_response 404
- end
end
-
end
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index e2c8829..4672acd 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -69,30 +69,87 @@ class GroupTest < ActiveSupport::TestCase
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
g_foo.update! is_trashed: true
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
+ assert Collection.readable_by(users(:active), {:include_trash => true}).where(uuid: col.uuid).any?
g_foo.update! is_trashed: false
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
end
+ test "delete group" do
+ set_user_from_auth :active_trustedclient
- test "delete group propagates to subgroups" do
+ g_foo = Group.create!(name: "foo")
+ g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
+ g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
+ g_foo.update! is_trashed: true
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+
+ assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_bar.uuid).any?
+ assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_baz.uuid).any?
+ end
+
+
+ test "delete subgroup" do
set_user_from_auth :active_trustedclient
g_foo = Group.create!(name: "foo")
g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
- col = Collection.create!(owner_uuid: g_bar.uuid)
+ g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
- assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
+ g_bar.update! is_trashed: true
+
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+
+ assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_bar.uuid).any?
+ assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_baz.uuid).any?
+ end
+
+ test "delete subsubgroup" do
+ set_user_from_auth :active_trustedclient
+
+ g_foo = Group.create!(name: "foo")
+ g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
+ g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
+ g_baz.update! is_trashed: true
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+ assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_baz.uuid).any?
+ end
+
+
+ test "delete group propagates to subgroups" do
+ set_user_from_auth :active_trustedclient
+
+ g_foo = groups(:trashed_project)
+ g_bar = groups(:trashed_subproject)
+ g_baz = groups(:trashed_subproject3)
+ col = collections(:collection_in_trashed_subproject)
- g_foo.update! is_trashed: true
assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
set_user_from_auth :admin
assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
set_user_from_auth :active_trustedclient
@@ -100,6 +157,12 @@ class GroupTest < ActiveSupport::TestCase
assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+
+ # this one should still be deleted.
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+
+ g_baz.update! is_trashed: false
+ assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
end
end
commit 906482f112d67d5fb06a3f85397c4cf0bd1d3db5
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Mon Aug 28 09:57:05 2017 -0400
12032: Adding controller tests
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index c9783d0..1cb5817 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -310,7 +310,7 @@ trashed_project:
trashed_subproject:
uuid: zzzzz-j7d0g-trashedproject2
- owner_uuid:
+ owner_uuid: zzzzz-j7d0g-trashedproject1
name: trashed subproject
group_class: project
trash_at: 2001-01-01T00:00:00Z
diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 56b0790..4cf8778 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -1090,4 +1090,44 @@ EOS
assert_nil json_response['delete_at']
assert_match /^same name for trashed and persisted collections \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
end
+
+ test 'cannot show collection in trashed subproject' do
+ authorize_with :active
+ get :show, {
+ id: collections(:collection_in_trashed_subproject).uuid,
+ format: :json
+ }
+ assert_response 404
+ end
+
+ test 'can show collection in untrashed subproject' do
+ authorize_with :active
+ Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+ get :show, {
+ id: collections(:collection_in_trashed_subproject).uuid,
+ format: :json,
+ }
+ assert_response :success
+ end
+
+ test 'cannot index collection in trashed subproject' do
+ authorize_with :active
+ get :index
+ assert_response :success
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_not_includes(item_uuids, collections(:collection_in_trashed_subproject).uuid)
+ end
+
+ test 'can index collection in untrashed subproject' do
+ authorize_with :active
+ Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+ get :index
+ assert_response :success
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_includes(item_uuids, collections(:collection_in_trashed_subproject).uuid)
+ end
end
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 5ae1e9a..043d665 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -529,4 +529,115 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_includes(owners, groups(:aproject).uuid)
assert_includes(owners, groups(:asubproject).uuid)
end
+
+ ### trashed project tests ###
+
+ [:active, :admin].each do |auth|
+ test "trashed project is hidden in contents (#{auth})" do
+ authorize_with auth
+ get :contents, {
+ id: users(:active).uuid,
+ format: :json,
+ }
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_not_includes(item_uuids, groups(:trashed_project).uuid)
+ end
+
+ test "untrashed project is visible in contents (#{auth})" do
+ authorize_with auth
+ Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+ get :contents, {
+ id: users(:active).uuid,
+ format: :json,
+ }
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_includes(item_uuids, groups(:trashed_project).uuid)
+ end
+
+ test "trashed project is hidden in index (#{auth})" do
+ authorize_with :active
+ get :index, {
+ format: :json,
+ }
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_not_includes(item_uuids, groups(:trashed_project).uuid)
+ assert_not_includes(item_uuids, groups(:trashed_subproject).uuid)
+ end
+
+ test "untrashed project is visible in index (#{auth})" do
+ authorize_with :active
+ Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+ get :index, {
+ format: :json,
+ }
+ item_uuids = json_response['items'].map do |item|
+ item['uuid']
+ end
+ assert_includes(item_uuids, groups(:trashed_project).uuid)
+ assert_includes(item_uuids, groups(:trashed_subproject).uuid)
+ end
+
+ test "cannot get contents of trashed project (#{auth})" do
+ authorize_with :active
+ get :contents, {
+ id: groups(:trashed_project).uuid,
+ format: :json,
+ }
+ assert_response 404
+ end
+
+ test "cannot get contents of trashed subproject (#{auth})" do
+ authorize_with :active
+ get :contents, {
+ id: groups(:trashed_subproject).uuid,
+ format: :json,
+ }
+ assert_response 404
+ end
+
+ test "can get contents of untrashed project (#{auth})" do
+ authorize_with :active
+ Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+ get :contents, {
+ id: groups(:trashed_project).uuid,
+ format: :json,
+ }
+ assert_response 200
+ end
+
+ test "can get contents of untrashed subproject (#{auth})" do
+ authorize_with :active
+ Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+ get :contents, {
+ id: groups(:trashed_subproject).uuid,
+ format: :json,
+ }
+ assert_response 200
+ end
+
+ test "cannot show trashed project (#{auth})" do
+ authorize_with :active
+ get :show, {
+ id: groups(:trashed_project).uuid,
+ format: :json,
+ }
+ assert_response 404
+ end
+
+ test "cannot show trashed subproject (#{auth})" do
+ authorize_with :active
+ get :show, {
+ id: groups(:trashed_subproject).uuid,
+ format: :json,
+ }
+ assert_response 404
+ end
+ end
+
end
commit e034e0fa88432078649cfa8c9110bc54bf1f1baa
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Fri Aug 25 14:23:30 2017 -0400
12032: Add test fixtures
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index e6195d7..639610a 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -37,7 +37,10 @@ perm_edges (tail_uuid, head_uuid, val, follow, trashed) AS (
LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
WHERE links.link_class = 'permission'
UNION ALL
- SELECT owner_uuid, uuid, 3, true, CASE is_trashed WHEN true THEN 1 ELSE 0 END FROM groups
+ SELECT owner_uuid, uuid, 3, true, CASE
+ WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1
+ ELSE 0 END
+ FROM groups
),
perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
SELECT 3::smallint AS val,
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 7777f74..8023503 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -703,6 +703,17 @@ same_name_as_trashed_coll_to_test_name_conflict_on_untrash:
manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:file1 0:0:file2\n"
name: same name for trashed and persisted collections
+collection_in_trashed_subproject:
+ uuid: zzzzz-4zz18-trashedproj2col
+ portable_data_hash: 80cf6dd2cf079dd13f272ec4245cb4a8+48
+ owner_uuid: zzzzz-j7d0g-trashedproject2
+ created_at: 2014-02-03T17:22:54Z
+ modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+ modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+ modified_at: 2014-02-03T17:22:54Z
+ updated_at: 2014-02-03T17:22:54Z
+ manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:file1 0:0:file2\n"
+ name: collection in trashed subproject
# Test Helper trims the rest of the file
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 90d087f..c9783d0 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -298,3 +298,21 @@ starred_and_shared_active_user_project:
name: Starred and shared active user project
description: Starred and shared active user project
group_class: project
+
+trashed_project:
+ uuid: zzzzz-j7d0g-trashedproject1
+ owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ name: trashed project
+ group_class: project
+ trash_at: 2001-01-01T00:00:00Z
+ delete_at: 2038-03-01T00:00:00Z
+ is_trashed: true
+
+trashed_subproject:
+ uuid: zzzzz-j7d0g-trashedproject2
+ owner_uuid:
+ name: trashed subproject
+ group_class: project
+ trash_at: 2001-01-01T00:00:00Z
+ delete_at: 2038-03-01T00:00:00Z
+ is_trashed: true
commit 70eff8f88e5d4aa2be1aa9a50de8aa9570bd3743
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 24 21:58:38 2017 -0400
12032: Fix hiding trashed groups directly owned by user.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 5a9d43f..ccd8f82 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -293,7 +293,14 @@ class ArvadosModel < ActiveRecord::Base
(#{sql_table}.owner_uuid = pv.target_uuid AND pv.target_owner_uuid is NOT NULL)))"]
# Match any object whose owner is listed explicitly in
# user_uuids.
- sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
+ trash_clause = if !include_trashed
+ "1 NOT IN (SELECT trashed
+ FROM permission_view pv
+ WHERE #{sql_table}.uuid = pv.target_uuid) AND"
+ else
+ ""
+ end
+ sql_conds += ["(#{trash_clause} #{sql_table}.owner_uuid IN (:user_uuids))"]
else
sql_conds += ["EXISTS(SELECT target_uuid
FROM permission_view pv
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 11f546b..e2c8829 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -90,10 +90,12 @@ class GroupTest < ActiveSupport::TestCase
assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
- assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_foo.uuid).any?
- assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_bar.uuid).any?
- assert Collection.readable_by(users(:active), {:include_trashed => true}).where(uuid: col.uuid).any?
+ set_user_from_auth :admin
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
+ set_user_from_auth :active_trustedclient
g_foo.update! is_trashed: false
assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
commit de75d524b5a436545bf58c743f4b1058aca7507b
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 24 17:09:49 2017 -0400
12032: Tests for group delete behavior.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 0dd2a73..861800d 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -3,12 +3,15 @@
# SPDX-License-Identifier: AGPL-3.0
require 'can_be_an_owner'
+require 'trashable'
class Group < ArvadosModel
include HasUuid
include KindAndEtag
include CommonApiTemplate
include CanBeAnOwner
+ include Trashable
+
after_create :invalidate_permissions_cache
after_update :maybe_invalidate_permissions_cache
before_create :assign_name
@@ -18,6 +21,9 @@ class Group < ArvadosModel
t.add :group_class
t.add :description
t.add :writable_by
+ t.add :delete_at
+ t.add :trash_at
+ t.add :is_trashed
end
def maybe_invalidate_permissions_cache
diff --git a/services/api/db/migrate/20170824202826_trashable_groups.rb b/services/api/db/migrate/20170824202826_trashable_groups.rb
new file mode 100644
index 0000000..b7e3373
--- /dev/null
+++ b/services/api/db/migrate/20170824202826_trashable_groups.rb
@@ -0,0 +1,7 @@
+class TrashableGroups < ActiveRecord::Migration
+ def change
+ add_column :groups, :trash_at, :datetime
+ add_column :groups, :delete_at, :datetime
+ add_column :groups, :is_trashed, :boolean, null: false, default: false
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 7e6bb1d..90aa0a3 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -391,7 +391,10 @@ CREATE TABLE groups (
name character varying(255) NOT NULL,
description character varying(524288),
updated_at timestamp without time zone NOT NULL,
- group_class character varying(255)
+ group_class character varying(255),
+ trash_at timestamp without time zone,
+ delete_at timestamp without time zone,
+ is_trashed boolean DEFAULT false NOT NULL
);
@@ -2791,3 +2794,5 @@ INSERT INTO schema_migrations (version) VALUES ('20170419175801');
INSERT INTO schema_migrations (version) VALUES ('20170628185847');
+INSERT INTO schema_migrations (version) VALUES ('20170824202826');
+
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 82839e8..e6195d7 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -37,7 +37,7 @@ perm_edges (tail_uuid, head_uuid, val, follow, trashed) AS (
LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
WHERE links.link_class = 'permission'
UNION ALL
- SELECT owner_uuid, uuid, 3, true, 0::smallint FROM groups
+ SELECT owner_uuid, uuid, 3, true, CASE is_trashed WHEN true THEN 1 ELSE 0 END FROM groups
),
perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
SELECT 3::smallint AS val,
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index a1123b1..11f546b 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -60,4 +60,44 @@ class GroupTest < ActiveSupport::TestCase
assert g_foo.errors.messages[:owner_uuid].join(" ").match(/ownership cycle/)
end
+ test "delete group hides contents" do
+ set_user_from_auth :active_trustedclient
+
+ g_foo = Group.create!(name: "foo")
+ col = Collection.create!(owner_uuid: g_foo.uuid)
+
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+ g_foo.update! is_trashed: true
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
+ g_foo.update! is_trashed: false
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+ end
+
+
+ test "delete group propagates to subgroups" do
+ set_user_from_auth :active_trustedclient
+
+ g_foo = Group.create!(name: "foo")
+ g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
+ col = Collection.create!(owner_uuid: g_bar.uuid)
+
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+
+ g_foo.update! is_trashed: true
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
+
+ assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_bar.uuid).any?
+ assert Collection.readable_by(users(:active), {:include_trashed => true}).where(uuid: col.uuid).any?
+
+ g_foo.update! is_trashed: false
+ assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+ assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+ assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+ end
+
end
commit 07036ebe470ba95e66c4679dc4c2ee6f76ccf222
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 24 16:22:12 2017 -0400
12032: Additional refactoring of readable_by. Refactor "trashable" into module.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 457b1bf..5a9d43f 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -254,7 +254,7 @@ class ArvadosModel < ActiveRecord::Base
# Collect the UUIDs of the authorized users.
sql_table = kwargs.fetch(:table_name, table_name)
- include_trashed = kwargs.fetch(:include_trashed, 0)
+ include_trashed = kwargs.fetch(:include_trashed, false)
sql_conds = []
user_uuids = users_list.map { |u| u.uuid }
@@ -263,57 +263,56 @@ class ArvadosModel < ActiveRecord::Base
# Check if any of the users are admin.
if users_list.select { |u| u.is_admin }.any?
- # For admins, only filter on "trashed"
- # sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
- # FROM permission_view
- # WHERE trashed in (:include_trashed)
- # GROUP BY user_uuid, target_uuid)"]
-
- # if self.column_names.include? 'owner_uuid'
- # sql_conds[0] += "AND #{sql_table}.owner_uuid in (SELECT target_uuid
- # FROM permission_view
- # WHERE trashed in (:include_trashed)
- # GROUP BY user_uuid, target_uuid)"
- # end
- return where({})
+ if !include_trashed
+ # exclude rows that are trashed.
+ if self.column_names.include? "owner_uuid"
+ sql_conds += ["NOT EXISTS(SELECT target_uuid
+ FROM permission_view pv
+ WHERE trashed = 1 AND
+ (#{sql_table}.uuid = pv.target_uuid OR #{sql_table}.owner_uuid = pv.target_uuid))"]
+ else
+ sql_conds += ["NOT EXISTS(SELECT target_uuid
+ FROM permission_view pv
+ WHERE trashed = 1 AND
+ (#{sql_table}.uuid = pv.target_uuid))"]
+ end
+ end
else
+ # Match objects which appear in the permission view
+ trash_clause = if !include_trashed then "trashed = 0 AND" else "" end
+
# Match any object (evidently a group or user) whose UUID is
# listed explicitly in user_uuids.
- sql_conds += ["#{sql_table}.uuid in (:user_uuids)"]
-
- # Match any object whose owner is listed explicitly in
- # user_uuids.
- sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
-
- # At least read permission from user_uuid to target_uuid of object
- sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
- FROM permission_view
- WHERE user_uuid in (:user_uuids) and perm_level >= 1 and trashed = (:include_trashed)
- GROUP BY user_uuid, target_uuid)"]
-
- if self.column_names.include? 'owner_uuid'
- # At least read permission from user_uuid to target_uuid that owns object
- sql_conds += ["#{sql_table}.owner_uuid in (SELECT target_uuid
- FROM permission_view
- WHERE user_uuid in (:user_uuids) and
- target_owner_uuid IS NOT NULL and
- perm_level >= 1 and trashed = (:include_trashed)
- GROUP BY user_uuid, target_uuid)"]
+ sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
+
+ if self.column_names.include? "owner_uuid"
+ sql_conds += ["EXISTS(SELECT target_uuid
+ FROM permission_view pv
+ WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
+ (#{sql_table}.uuid = pv.target_uuid OR
+ (#{sql_table}.owner_uuid = pv.target_uuid AND pv.target_owner_uuid is NOT NULL)))"]
+ # Match any object whose owner is listed explicitly in
+ # user_uuids.
+ sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
+ else
+ sql_conds += ["EXISTS(SELECT target_uuid
+ FROM permission_view pv
+ WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
+ (#{sql_table}.uuid = pv.target_uuid))"]
end
if sql_table == "links"
# Match any permission link that gives one of the authorized
# users some permission _or_ gives anyone else permission to
# view one of the authorized users.
- sql_conds += ["(#{sql_table}.link_class in (:permission_link_classes) AND "+
+ sql_conds += ["(#{sql_table}.link_class IN (:permission_link_classes) AND "+
"(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"]
end
end
where(sql_conds.join(' OR '),
user_uuids: user_uuids,
- permission_link_classes: ['permission', 'resources'],
- include_trashed: include_trashed)
+ permission_link_classes: ['permission', 'resources'])
end
def save_with_unique_name!
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 6f13a63..ce7e9fe 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -4,6 +4,7 @@
require 'arvados/keep'
require 'sweep_trashed_collections'
+require 'trashable'
class Collection < ArvadosModel
extend CurrentApiClient
@@ -11,20 +12,16 @@ class Collection < ArvadosModel
include HasUuid
include KindAndEtag
include CommonApiTemplate
+ include Trashable
serialize :properties, Hash
- before_validation :set_validation_timestamp
before_validation :default_empty_manifest
before_validation :check_encoding
before_validation :check_manifest_validity
before_validation :check_signatures
before_validation :strip_signatures_and_update_replication_confirmed
- before_validation :ensure_trash_at_not_in_past
- before_validation :sync_trash_state
- before_validation :default_trash_interval
validate :ensure_pdh_matches_manifest_text
- validate :validate_trash_and_delete_timing
before_save :set_file_names
# Query only untrashed collections by default.
@@ -486,81 +483,4 @@ class Collection < ArvadosModel
super
end
- # Use a single timestamp for all validations, even though each
- # validation runs at a different time.
- def set_validation_timestamp
- @validation_timestamp = db_current_time
- end
-
- # If trash_at is being changed to a time in the past, change it to
- # now. This allows clients to say "expires {client-current-time}"
- # without failing due to clock skew, while avoiding odd log entries
- # like "expiry date changed to {1 year ago}".
- def ensure_trash_at_not_in_past
- if trash_at_changed? && trash_at
- self.trash_at = [@validation_timestamp, trash_at].max
- end
- end
-
- # Caller can move into/out of trash by setting/clearing is_trashed
- # -- however, if the caller also changes trash_at, then any changes
- # to is_trashed are ignored.
- def sync_trash_state
- if is_trashed_changed? && !trash_at_changed?
- if is_trashed
- self.trash_at = @validation_timestamp
- else
- self.trash_at = nil
- self.delete_at = nil
- end
- end
- self.is_trashed = trash_at && trash_at <= @validation_timestamp || false
- true
- end
-
- def default_trash_interval
- if trash_at_changed? && !delete_at_changed?
- # If trash_at is updated without touching delete_at,
- # automatically update delete_at to a sensible value.
- if trash_at.nil?
- self.delete_at = nil
- else
- self.delete_at = trash_at + Rails.configuration.default_trash_lifetime.seconds
- end
- elsif !trash_at || !delete_at || trash_at > delete_at
- # Not trash, or bogus arguments? Just validate in
- # validate_trash_and_delete_timing.
- elsif delete_at_changed? && delete_at >= trash_at
- # Fix delete_at if needed, so it's not earlier than the expiry
- # time on any permission tokens that might have been given out.
-
- # In any case there are no signatures expiring after now+TTL.
- # Also, if the existing trash_at time has already passed, we
- # know we haven't given out any signatures since then.
- earliest_delete = [
- @validation_timestamp,
- trash_at_was,
- ].compact.min + Rails.configuration.blob_signature_ttl.seconds
-
- # The previous value of delete_at is also an upper bound on the
- # longest-lived permission token. For example, if TTL=14,
- # trash_at_was=now-7, delete_at_was=now+7, then it is safe to
- # set trash_at=now+6, delete_at=now+8.
- earliest_delete = [earliest_delete, delete_at_was].compact.min
-
- # If delete_at is too soon, use the earliest possible time.
- if delete_at < earliest_delete
- self.delete_at = earliest_delete
- end
- end
- end
-
- def validate_trash_and_delete_timing
- if trash_at.nil? != delete_at.nil?
- errors.add :delete_at, "must be set if trash_at is set, and must be nil otherwise"
- elsif delete_at && delete_at < trash_at
- errors.add :delete_at, "must not be earlier than trash_at"
- end
- true
- end
end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index c8c9bfb..0e2db76 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -155,10 +155,9 @@ class User < ArvadosModel
install_view('permission')
all_perms = {}
ActiveRecord::Base.connection.
- exec_query('SELECT user_uuid, target_owner_uuid, max(perm_level), max(trashed)
+ exec_query('SELECT user_uuid, target_owner_uuid, perm_level, trashed
FROM permission_view
- WHERE target_owner_uuid IS NOT NULL
- GROUP BY user_uuid, target_owner_uuid',
+ WHERE target_owner_uuid IS NOT NULL',
# "name" arg is a query label that appears in logs:
"all_group_permissions",
).rows.each do |user_uuid, group_uuid, max_p_val, trashed|
@@ -176,11 +175,10 @@ class User < ArvadosModel
group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
ActiveRecord::Base.connection.
- exec_query('SELECT target_owner_uuid, max(perm_level), max(trashed)
+ exec_query('SELECT target_owner_uuid, perm_level, trashed
FROM permission_view
WHERE user_uuid = $1
- AND target_owner_uuid IS NOT NULL
- GROUP BY target_owner_uuid',
+ AND target_owner_uuid IS NOT NULL',
# "name" arg is a query label that appears in logs:
"group_permissions for #{uuid}",
# "binds" arg is an array of [col_id, value] for '$1' vars:
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 10c4b27..82839e8 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -60,7 +60,8 @@ perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
)
SELECT user_uuid,
target_uuid,
- val AS perm_level,
+ MAX(val) AS perm_level,
CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid,
- trashed
- FROM perm;
+ MAX(trashed) AS trashed
+ FROM perm
+ GROUP BY user_uuid, target_uuid, target_owner_uuid;
diff --git a/services/api/lib/trashable.rb b/services/api/lib/trashable.rb
new file mode 100644
index 0000000..1e2f466
--- /dev/null
+++ b/services/api/lib/trashable.rb
@@ -0,0 +1,91 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+module Trashable
+ def self.included(base)
+ base.before_validation :set_validation_timestamp
+ base.before_validation :ensure_trash_at_not_in_past
+ base.before_validation :sync_trash_state
+ base.before_validation :default_trash_interval
+ base.validate :validate_trash_and_delete_timing
+ end
+
+ # Use a single timestamp for all validations, even though each
+ # validation runs at a different time.
+ def set_validation_timestamp
+ @validation_timestamp = db_current_time
+ end
+
+ # If trash_at is being changed to a time in the past, change it to
+ # now. This allows clients to say "expires {client-current-time}"
+ # without failing due to clock skew, while avoiding odd log entries
+ # like "expiry date changed to {1 year ago}".
+ def ensure_trash_at_not_in_past
+ if trash_at_changed? && trash_at
+ self.trash_at = [@validation_timestamp, trash_at].max
+ end
+ end
+
+ # Caller can move into/out of trash by setting/clearing is_trashed
+ # -- however, if the caller also changes trash_at, then any changes
+ # to is_trashed are ignored.
+ def sync_trash_state
+ if is_trashed_changed? && !trash_at_changed?
+ if is_trashed
+ self.trash_at = @validation_timestamp
+ else
+ self.trash_at = nil
+ self.delete_at = nil
+ end
+ end
+ self.is_trashed = trash_at && trash_at <= @validation_timestamp || false
+ true
+ end
+
+ def default_trash_interval
+ if trash_at_changed? && !delete_at_changed?
+ # If trash_at is updated without touching delete_at,
+ # automatically update delete_at to a sensible value.
+ if trash_at.nil?
+ self.delete_at = nil
+ else
+ self.delete_at = trash_at + Rails.configuration.default_trash_lifetime.seconds
+ end
+ elsif !trash_at || !delete_at || trash_at > delete_at
+ # Not trash, or bogus arguments? Just validate in
+ # validate_trash_and_delete_timing.
+ elsif delete_at_changed? && delete_at >= trash_at
+ # Fix delete_at if needed, so it's not earlier than the expiry
+ # time on any permission tokens that might have been given out.
+
+ # In any case there are no signatures expiring after now+TTL.
+ # Also, if the existing trash_at time has already passed, we
+ # know we haven't given out any signatures since then.
+ earliest_delete = [
+ @validation_timestamp,
+ trash_at_was,
+ ].compact.min + Rails.configuration.blob_signature_ttl.seconds
+
+ # The previous value of delete_at is also an upper bound on the
+ # longest-lived permission token. For example, if TTL=14,
+ # trash_at_was=now-7, delete_at_was=now+7, then it is safe to
+ # set trash_at=now+6, delete_at=now+8.
+ earliest_delete = [earliest_delete, delete_at_was].compact.min
+
+ # If delete_at is too soon, use the earliest possible time.
+ if delete_at < earliest_delete
+ self.delete_at = earliest_delete
+ end
+ end
+ end
+
+ def validate_trash_and_delete_timing
+ if trash_at.nil? != delete_at.nil?
+ errors.add :delete_at, "must be set if trash_at is set, and must be nil otherwise"
+ elsif delete_at && delete_at < trash_at
+ errors.add :delete_at, "must not be earlier than trash_at"
+ end
+ true
+ end
+end
commit dc38303d784362e361f8221c88171527f1c04759
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Thu Aug 24 12:03:09 2017 -0400
12032: Use permission_view in subquery to filter objects readable by user.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/controllers/arvados/v1/repositories_controller.rb b/services/api/app/controllers/arvados/v1/repositories_controller.rb
index a28cee2..b88e10c 100644
--- a/services/api/app/controllers/arvados/v1/repositories_controller.rb
+++ b/services/api/app/controllers/arvados/v1/repositories_controller.rb
@@ -46,7 +46,7 @@ class Arvados::V1::RepositoriesController < ApplicationController
# group also has access to the repository. Access level is
# min(group-to-repo permission, user-to-group permission).
user_aks.each do |user_uuid, _|
- perm_mask = all_group_permissions[user_uuid][perm.tail_uuid]
+ perm_mask = all_group_permissions[user_uuid].andand[perm.tail_uuid]
if not perm_mask
next
elsif perm_mask[:manage] and perm.name == 'can_manage'
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index ea69735..457b1bf 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -252,49 +252,68 @@ class ArvadosModel < ActiveRecord::Base
kwargs = {}
end
- # Check if any of the users are admin. If so, we're done.
- if users_list.select { |u| u.is_admin }.any?
- # Return existing relation with no new filters.
- return where({})
- end
-
# Collect the UUIDs of the authorized users.
- user_uuids = users_list.map { |u| u.uuid }
-
- # Collect the UUIDs of all groups readable by any of the
- # authorized users. If one of these (or the UUID of one of the
- # authorized users themselves) is an object's owner_uuid, that
- # object is readable.
- owner_uuids = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
- owner_uuids.uniq!
-
- sql_conds = []
sql_table = kwargs.fetch(:table_name, table_name)
+ include_trashed = kwargs.fetch(:include_trashed, 0)
- # Match any object (evidently a group or user) whose UUID is
- # listed explicitly in owner_uuids.
- sql_conds += ["#{sql_table}.uuid in (:owner_uuids)"]
+ sql_conds = []
+ user_uuids = users_list.map { |u| u.uuid }
- # Match any object whose owner is listed explicitly in
- # owner_uuids.
- sql_conds += ["#{sql_table}.owner_uuid IN (:owner_uuids)"]
+ User.install_view('permission')
- # Match the head of any permission link whose tail is listed
- # explicitly in owner_uuids.
- sql_conds += ["#{sql_table}.uuid IN (SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (:owner_uuids))"]
+ # Check if any of the users are admin.
+ if users_list.select { |u| u.is_admin }.any?
+ # For admins, only filter on "trashed"
+ # sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
+ # FROM permission_view
+ # WHERE trashed in (:include_trashed)
+ # GROUP BY user_uuid, target_uuid)"]
+
+ # if self.column_names.include? 'owner_uuid'
+ # sql_conds[0] += "AND #{sql_table}.owner_uuid in (SELECT target_uuid
+ # FROM permission_view
+ # WHERE trashed in (:include_trashed)
+ # GROUP BY user_uuid, target_uuid)"
+ # end
+ return where({})
+ else
+ # Match any object (evidently a group or user) whose UUID is
+ # listed explicitly in user_uuids.
+ sql_conds += ["#{sql_table}.uuid in (:user_uuids)"]
+
+ # Match any object whose owner is listed explicitly in
+ # user_uuids.
+ sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
+
+ # At least read permission from user_uuid to target_uuid of object
+ sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
+ FROM permission_view
+ WHERE user_uuid in (:user_uuids) and perm_level >= 1 and trashed = (:include_trashed)
+ GROUP BY user_uuid, target_uuid)"]
+
+ if self.column_names.include? 'owner_uuid'
+ # At least read permission from user_uuid to target_uuid that owns object
+ sql_conds += ["#{sql_table}.owner_uuid in (SELECT target_uuid
+ FROM permission_view
+ WHERE user_uuid in (:user_uuids) and
+ target_owner_uuid IS NOT NULL and
+ perm_level >= 1 and trashed = (:include_trashed)
+ GROUP BY user_uuid, target_uuid)"]
+ end
- if sql_table == "links"
- # Match any permission link that gives one of the authorized
- # users some permission _or_ gives anyone else permission to
- # view one of the authorized users.
- sql_conds += ["(#{sql_table}.link_class in (:permission_link_classes) AND "+
- "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"]
+ if sql_table == "links"
+ # Match any permission link that gives one of the authorized
+ # users some permission _or_ gives anyone else permission to
+ # view one of the authorized users.
+ sql_conds += ["(#{sql_table}.link_class in (:permission_link_classes) AND "+
+ "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"]
+ end
end
where(sql_conds.join(' OR '),
- owner_uuids: owner_uuids,
user_uuids: user_uuids,
- permission_link_classes: ['permission', 'resources'])
+ permission_link_classes: ['permission', 'resources'],
+ include_trashed: include_trashed)
end
def save_with_unique_name!
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index f093410..c8c9bfb 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -155,14 +155,14 @@ class User < ArvadosModel
install_view('permission')
all_perms = {}
ActiveRecord::Base.connection.
- exec_query('SELECT user_uuid, target_owner_uuid, max(perm_level)
+ exec_query('SELECT user_uuid, target_owner_uuid, max(perm_level), max(trashed)
FROM permission_view
WHERE target_owner_uuid IS NOT NULL
GROUP BY user_uuid, target_owner_uuid',
# "name" arg is a query label that appears in logs:
"all_group_permissions",
- ).rows.each do |user_uuid, group_uuid, max_p_val|
- all_perms[user_uuid] ||= {}
+ ).rows.each do |user_uuid, group_uuid, max_p_val, trashed|
+ all_perms[user_uuid] ||= {user_uuid => {:read => true, :write => true, :manage => true}}
all_perms[user_uuid][group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
end
all_perms
@@ -174,9 +174,9 @@ class User < ArvadosModel
def calculate_group_permissions
self.class.install_view('permission')
- group_perms = {}
+ group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
ActiveRecord::Base.connection.
- exec_query('SELECT target_owner_uuid, max(perm_level)
+ exec_query('SELECT target_owner_uuid, max(perm_level), max(trashed)
FROM permission_view
WHERE user_uuid = $1
AND target_owner_uuid IS NOT NULL
@@ -185,7 +185,7 @@ class User < ArvadosModel
"group_permissions for #{uuid}",
# "binds" arg is an array of [col_id, value] for '$1' vars:
[[nil, uuid]],
- ).rows.each do |group_uuid, max_p_val|
+ ).rows.each do |group_uuid, max_p_val, trashed|
group_perms[group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
end
Rails.cache.write "#{PERM_CACHE_PREFIX}#{self.uuid}", group_perms, expires_in: PERM_CACHE_TTL
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 6e24d30..10c4b27 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -2,6 +2,21 @@
--
-- SPDX-License-Identifier: AGPL-3.0
+-- constructing perm_edges
+-- 1. get the list of all permission links,
+-- 2. any can_manage link or permission link to a group means permission should "follow through"
+-- (as a special case, can_manage links to a user grant access to everything owned by the user,
+-- unlike can_read or can_write which only grant access to the user record)
+-- 3. add all owner->owned relationships between groups as can_manage edges
+--
+-- constructing permissions
+-- 1. base case: start with set of all users as the working set
+-- 2. recursive case:
+-- join with edges where the tail is in the working set and "follow" is true
+-- produce a new working set with the head (target) of each edge
+-- set permission to the least permission encountered on the path
+-- propagate trashed flag down
+
CREATE TEMPORARY VIEW permission_view AS
WITH RECURSIVE
perm_value (name, val) AS (
@@ -11,35 +26,41 @@ perm_value (name, val) AS (
('can_write', 2),
('can_manage', 3)
),
-perm_edges (tail_uuid, head_uuid, val, follow) AS (
+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
+ (pv.val = 3 OR groups.uuid IS NOT NULL) AS follow,
+ 0::smallint AS trashed
FROM links
LEFT JOIN perm_value pv ON pv.name = links.name
LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
WHERE links.link_class = 'permission'
UNION ALL
- SELECT owner_uuid, uuid, 3, true FROM groups
+ SELECT owner_uuid, uuid, 3, true, 0::smallint FROM groups
),
-perm (val, follow, user_uuid, target_uuid) AS (
+perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
SELECT 3::smallint AS val,
- true AS follow,
+ false AS follow,
users.uuid::varchar(32) AS user_uuid,
- users.uuid::varchar(32) AS target_uuid
+ users.uuid::varchar(32) AS target_uuid,
+ 0::smallint AS trashed,
+ true AS startnode
FROM users
UNION
- SELECT LEAST(perm.val, edges.val)::smallint AS val,
- edges.follow AS follow,
- perm.user_uuid::varchar(32) AS user_uuid,
- edges.head_uuid::varchar(32) AS target_uuid
+ SELECT LEAST(perm.val, edges.val)::smallint AS val,
+ edges.follow AS follow,
+ perm.user_uuid::varchar(32) AS user_uuid,
+ edges.head_uuid::varchar(32) AS target_uuid,
+ GREATEST(perm.trashed, edges.trashed)::smallint AS trashed,
+ false AS startnode
FROM perm
INNER JOIN perm_edges edges
- ON perm.follow AND edges.tail_uuid = perm.target_uuid
+ ON (perm.startnode or perm.follow) AND edges.tail_uuid = perm.target_uuid
)
SELECT user_uuid,
target_uuid,
val AS perm_level,
- CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid
+ CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid,
+ trashed
FROM perm;
commit e3c4b6b12c72140aa7c3df6d5f98199476b8b3ba
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Sep 6 11:35:55 2017 -0400
12032: Benchmark for group permissions.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index 87d6a44..23e63f2 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -309,3 +309,10 @@ not_running_container_auth:
user: active
api_token: 4kg6k6lzmp9kj6bpkcoxie963cmvjahbt2fod9zru30k1jqdmj
expires_at: 2038-01-01 00:00:00
+
+permission_perftest:
+ uuid: zzzzz-gj3su-077z32anoj93boo
+ api_client: untrusted
+ user: permission_perftest
+ api_token: 3kg6k6lzmp9kjabonentustoecn5bahbt2fod9zru30k1jqdmi
+ expires_at: 2038-01-01 00:00:00
diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml
index 3daaaa8..8087952 100644
--- a/services/api/test/fixtures/users.yml
+++ b/services/api/test/fixtures/users.yml
@@ -333,3 +333,19 @@ fuse:
organization: example.com
role: IT
getting_started_shown: 2015-03-26 12:34:56.789000000 Z
+
+permission_perftest:
+ owner_uuid: zzzzz-tpzed-000000000000000
+ uuid: zzzzz-tpzed-permissionptest
+ email: fuse at arvados.local
+ first_name: FUSE
+ last_name: User
+ identity_url: https://fuse.openid.local
+ is_active: true
+ is_admin: false
+ username: perftest
+ prefs:
+ profile:
+ organization: example.com
+ role: IT
+ getting_started_shown: 2015-03-26 12:34:56.789000000 Z
diff --git a/services/api/test/performance/permission_test.rb b/services/api/test/performance/permission_test.rb
new file mode 100644
index 0000000..2db42bc
--- /dev/null
+++ b/services/api/test/performance/permission_test.rb
@@ -0,0 +1,54 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'test_helper'
+require 'benchmark'
+
+
+def create_eight parent
+ uuids = []
+ values = []
+ (0..8).each do
+ uuid = Group.generate_uuid
+ values.push "('#{uuid}', '#{parent}', now(), now(), '#{uuid}')"
+ uuids.push uuid
+ end
+ ActiveRecord::Base.connection.execute("INSERT INTO groups (uuid, owner_uuid, created_at, updated_at, name) VALUES #{values.join ','}")
+ uuids
+end
+
+class PermissionPerfTest < ActionDispatch::IntegrationTest
+ def test_groups_index
+ n = 0
+ act_as_system_user do
+ puts("Time spent creating records:", Benchmark.measure do
+ ActiveRecord::Base.transaction do
+ root = Group.create!(owner_uuid: users(:permission_perftest).uuid)
+ n += 1
+ a = create_eight root.uuid
+ n += 8
+ a.each do |p1|
+ b = create_eight p1
+ n += 8
+ b.each do |p2|
+ c = create_eight p2
+ n += 8
+ c.each do |p3|
+ d = create_eight p3
+ n += 8
+ end
+ end
+ end
+ end
+ end)
+ end
+ puts "created #{n}"
+ puts "Time spent getting group index:"
+ (0..4).each do
+ puts(Benchmark.measure do
+ get '/arvados/v1/groups', {format: :json, limit: 1000}, auth(:permission_perftest)
+ end)
+ end
+ end
+end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list