16007: Name our custom queries consistently
authorPeter Amstutz <peter.amstutz@curii.com>
Wed, 13 May 2020 20:31:34 +0000 (16:31 -0400)
committerPeter Amstutz <peter.amstutz@curii.com>
Wed, 13 May 2020 20:31:34 +0000 (16:31 -0400)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

services/api/app/models/group.rb
services/api/app/models/link.rb
services/api/app/models/user.rb
services/api/db/migrate/20200501150153_permission_table.rb
services/api/db/structure.sql
services/api/lib/refresh_permission_view.rb
services/api/test/integration/groups_test.rb
services/api/test/unit/owner_test.rb

index 1d69300b6068a422a2b4952b0ba95729bf08f1a1..9b8a9877e1b607e53865e4bc4fea083a8dc6fa0f 100644 (file)
@@ -58,41 +58,43 @@ class Group < ArvadosModel
 create temporary table #{temptable} on commit drop
 as select * from project_subtree_with_trash_at($1, LEAST($2, $3)::timestamp)
 },
-                                               'Group.get_subtree',
+                                               'Group.update_trash.select',
                                                [[nil, self.uuid],
                                                 [nil, TrashedGroup.find_by_group_uuid(self.owner_uuid).andand.trash_at],
                                                 [nil, self.trash_at]]
 
-      ActiveRecord::Base.connection.exec_query %{
+      ActiveRecord::Base.connection.exec_delete %{
 delete from trashed_groups where group_uuid in (select target_uuid from #{temptable} where trash_at is NULL);
-}
+},
+                                            "Group.update_trash.delete"
 
       ActiveRecord::Base.connection.exec_query %{
 insert into trashed_groups (group_uuid, trash_at)
   select target_uuid as group_uuid, trash_at from #{temptable} where trash_at is not NULL
 on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
-}
+},
+                                            "Group.update_trash.insert"
     end
   end
 
   def before_ownership_change
     if owner_uuid_changed? and !self.owner_uuid_was.nil?
       MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
-      User.update_permissions self.owner_uuid, self.uuid, 0
+      update_permissions self.owner_uuid, self.uuid, 0
     end
   end
 
   def after_ownership_change
     if owner_uuid_changed?
-      User.update_permissions self.owner_uuid, self.uuid, 3
+      update_permissions self.owner_uuid, self.uuid, 3
     end
   end
 
   def clear_permissions_and_trash
     MaterializedPermission.where(target_uuid: uuid).delete_all
-    ActiveRecord::Base.connection.exec_query %{
+    ActiveRecord::Base.connection.exec_delete %{
 delete from trashed_groups where group_uuid=$1
-}, "Group.clear_trash", [[nil, self.uuid]]
+}, "Group.clear_permissions_and_trash", [[nil, self.uuid]]
 
   end
 
index 2aadb374d2cc9df091415560a5598112f3fa9042..f6210fbb5208f0ea28a8c078431d7c0422d93b1a 100644 (file)
@@ -14,8 +14,8 @@ class Link < ArvadosModel
   validate :name_links_are_obsolete
   before_create :permission_to_attach_to_objects
   before_update :permission_to_attach_to_objects
-  after_update :update_permissions
-  after_create :update_permissions
+  after_update :call_update_permissions
+  after_create :call_update_permissions
   before_destroy :clear_permissions
   after_destroy :clear_permissions
 
@@ -72,21 +72,21 @@ class Link < ArvadosModel
     'can_manage' => 3,
   }
 
-  def update_permissions
+  def call_update_permissions
     if self.link_class == 'permission'
-      User.update_permissions tail_uuid, head_uuid, PERM_LEVEL[name]
+      update_permissions tail_uuid, head_uuid, PERM_LEVEL[name]
     end
   end
 
   def clear_permissions
     if self.link_class == 'permission'
-      User.update_permissions tail_uuid, head_uuid, 0, false
+      update_permissions tail_uuid, head_uuid, 0, false
     end
   end
 
   def check_permissions
     if self.link_class == 'permission'
-      User.update_permissions tail_uuid, head_uuid, 0, true
+      update_permissions tail_uuid, head_uuid, 0, true
     end
   end
 
index 4df2039d83f61c102510acf8b53018a7f3e7e923..9afe445821c73a9a133f0d77b49ddd09c899641e 100644 (file)
@@ -143,242 +143,31 @@ SELECT 1 FROM #{PERMISSION_VIEW}
   def before_ownership_change
     if owner_uuid_changed? and !self.owner_uuid_was.nil?
       MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
-      User.update_permissions self.owner_uuid_was, self.uuid, 0, false
+      update_permissions self.owner_uuid_was, self.uuid, 0, false
     end
   end
 
   def after_ownership_change
-
-#       puts "Update permissions for #{uuid}"
-#     User.printdump %{
-# select * from materialized_permissions where user_uuid='#{uuid}'
-# }
-    puts "-1- #{uuid_changed?} and #{uuid} and #{uuid_was}"
-    puts "-2- #{owner_uuid_changed?} and #{owner_uuid} and #{owner_uuid_was}"
-
-    unless owner_uuid_changed?
-      return
+    if owner_uuid_changed?
+      update_permissions self.owner_uuid, self.uuid, 3
     end
-
-#     if !uuid_was.nil? and uuid_changed?
-#       puts "Cleaning house #{uuid_was}"
-#     ActiveRecord::Base.connection.exec_query %{
-# update #{PERMISSION_VIEW} set user_uuid=$1 where user_uuid = $2
-# },
-#                                              'Change user uuid',
-#                                              [[nil, uuid],
-#                                               [nil, uuid_was]]
-#     ActiveRecord::Base.connection.exec_query %{
-# update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
-# },
-#                                              'Change user uuid',
-#                                              [[nil, uuid],
-#                                               [nil, uuid_was]]
-#     end
-
-    User.update_permissions self.owner_uuid, self.uuid, 3
-
-#   puts "post-update"
-#    User.printdump %{
-# select * from materialized_permissions where user_uuid='#{uuid}'
-# }
-#    puts "<<<"
   end
 
   def clear_permissions
     MaterializedPermission.where("user_uuid = ? or target_uuid = ?", uuid, uuid).delete_all
   end
 
-  def self.printdump qr
-    q1 = ActiveRecord::Base.connection.exec_query qr
-    q1.each do |r|
-      puts r
-    end
-  end
-
   def recompute_permissions
-    ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW} where user_uuid='#{uuid}'")
-    ActiveRecord::Base.connection.execute %{
+    ActiveRecord::Base.connection.exec_delete("DELETE FROM #{PERMISSION_VIEW} where user_uuid=$1",
+                                          "User.recompute_permissions.delete_user_uuid",
+                                          [[nil, uuid]])
+    ActiveRecord::Base.connection.exec_insert %{
 INSERT INTO #{PERMISSION_VIEW}
-select '#{uuid}', g.target_uuid, g.val, g.traverse_owned
-from search_permission_graph('#{uuid}', 3) as g
-}
-  end
-
-  def self.update_permissions perm_origin_uuid, starting_uuid, perm_level, check=true
-    # Update a subset of the permission graph
-    # perm_level is the inherited permission
-    # perm_level is a number from 0-3
-    #   can_read=1
-    #   can_write=2
-    #   can_manage=3
-    #   call with perm_level=0 to revoke permissions
-    #
-    # 1. Compute set (group, permission) implied by traversing
-    #    graph starting at this group
-    # 2. Find links from outside the graph that point inside
-    # 3. For each starting uuid, get the set of permissions from the
-    #    materialized permission table
-    # 3. Delete permissions from table not in our computed subset.
-    # 4. Upsert each permission in our subset (user, group, val)
-
-    ## testinging
-    puts "\n__ update_permissions __"
-#     puts "What's in there now for #{starting_uuid}"
-#     printdump %{
-# select * from materialized_permissions where user_uuid='#{starting_uuid}'
-# }
-
-    puts "search_permission_graph #{perm_origin_uuid} #{starting_uuid}, #{perm_level}"
-    printdump %{
-select '#{perm_origin_uuid}'::varchar as perm_origin_uuid, target_uuid, val, traverse_owned from search_permission_graph('#{starting_uuid}', #{perm_level})
-}
-
-#     puts "other_links #{perm_origin_uuid} #{starting_uuid}, #{perm_level}"
-#     printdump %{
-# with
-# perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-#   select '#{perm_origin_uuid}'::varchar, target_uuid, val, traverse_owned
-#     from search_permission_graph('#{starting_uuid}'::varchar, #{perm_level}))
-
-#     select links.tail_uuid as perm_origin_uuid, links.head_uuid, links.name
-#       from links
-#       where links.link_class='permission' and
-#         links.tail_uuid not in (select target_uuid from perm_from_start where traverse_owned) and
-#         links.tail_uuid != '#{perm_origin_uuid}' and
-#         links.head_uuid in (select target_uuid from perm_from_start)
-# }
-
-    puts "additional_perms #{perm_origin_uuid} #{starting_uuid}, #{perm_level}"
-    printdump %{
-with
-perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-  select '#{perm_origin_uuid}'::varchar, target_uuid, val, traverse_owned
-    from search_permission_graph('#{starting_uuid}'::varchar, #{perm_level})),
-
-  edges(tail_uuid, head_uuid, val) as (
-        select * from permission_graph_edges()),
-
-  additional_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-    select edges.tail_uuid as perm_origin_uuid, ps.target_uuid, ps.val,
-           should_traverse_owned(ps.target_uuid, ps.val)
-      from edges, lateral search_permission_graph(edges.head_uuid, edges.val) as ps
-      where (not (edges.tail_uuid = '#{perm_origin_uuid}' and
-                  edges.head_uuid = '#{starting_uuid}')) and
-            edges.tail_uuid not in (select target_uuid from perm_from_start) and
-            edges.head_uuid in (select target_uuid from perm_from_start)),
-
-  partial_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-      select * from perm_from_start
-    union
-      select * from additional_perms
-  ),
-
-  user_identity_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-    select users.uuid as perm_origin_uuid, ps.target_uuid, ps.val, ps.traverse_owned
-      from users, lateral search_permission_graph(users.uuid, 3) as ps
-      where (users.owner_uuid not in (select target_uuid from partial_perms) or
-             users.owner_uuid = users.uuid) and
-      users.uuid in (select target_uuid from partial_perms)
-  ),
-
-  all_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-      select * from additional_perms
-    union
-      select * from user_identity_perms
-  )
-
-  select * from all_perms order by perm_origin_uuid, target_uuid
-}
-
-    puts "Perms out"
-    printdump %{
-select * from compute_permission_subgraph('#{perm_origin_uuid}', '#{starting_uuid}', '#{perm_level}')
-order by user_uuid, target_uuid
-}
-    ## end
-
-    temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}"
-    ActiveRecord::Base.connection.exec_query %{
-create temporary table #{temptable_perms} on commit drop
-as select * from compute_permission_subgraph($1, $2, $3)
+select $1::varchar, g.target_uuid, g.val, g.traverse_owned
+from search_permission_graph($1::varchar, 3) as g
 },
-                                             'Group.search_permissions',
-                                             [[nil, perm_origin_uuid],
-                                              [nil, starting_uuid],
-                                              [nil, perm_level]]
-
-#     q1 = ActiveRecord::Base.connection.exec_query %{
-# select * from #{temptable_perms} order by user_uuid, target_uuid
-# }
-#     puts "recomputed perms was #{perm_origin_uuid} #{starting_uuid}, #{perm_level}"
-#     q1.each do |r|
-#       puts r
-#     end
-#     puts "<<<<"
-
-    ActiveRecord::Base.connection.exec_query %{
-delete from materialized_permissions where
-  target_uuid in (select target_uuid from #{temptable_perms}) and
-  not exists (select 1 from #{temptable_perms}
-              where target_uuid=materialized_permissions.target_uuid and
-                    user_uuid=materialized_permissions.user_uuid and
-                    val>0)
-}
-
-    ActiveRecord::Base.connection.exec_query %{
-insert into materialized_permissions (user_uuid, target_uuid, perm_level, traverse_owned)
-  select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms} where val>0
-on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_level, traverse_owned=EXCLUDED.traverse_owned;
-}
-
-    # for testing only - make a copy of the table and compare it to the one generated
-    # using a full permission recompute
-
-#     puts "dump--"
-#     User.printdump %{
-# select owner_uuid, uuid from users order by uuid
-#     }
-#     User.printdump %{
-# select * from groups
-#     }
-#     User.printdump %{
-#select * from search_permission_graph('zzzzz-tpzed-000000000000000', 3)
-#     }
-#     puts "--"
-
-    if check
-      q1 = ActiveRecord::Base.connection.exec_query %{
-select user_uuid, target_uuid, perm_level, traverse_owned from materialized_permissions
-order by user_uuid, target_uuid
-}
-
-      q2 = ActiveRecord::Base.connection.exec_query %{
-select users.uuid as user_uuid, g.target_uuid, g.val as perm_level, g.traverse_owned
-from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0
-order by users.uuid, target_uuid
-}
-
-      if q1.count != q2.count
-        puts "Didn't match incremental+: #{q1.count} != full refresh-: #{q2.count}"
-      end
-
-      if q1.count > q2.count
-        q1.each_with_index do |r, i|
-          if r != q2[i]
-            puts "+#{r}\n-#{q2[i]}"
-            raise "Didn't match"
-          end
-        end
-      else
-        q2.each_with_index do |r, i|
-          if r != q1[i]
-            puts "+#{q1[i]}\n-#{r}"
-            raise "Didn't match"
-          end
-        end
-      end
-    end
+                                          "User.recompute_permissions.insert",
+                                          [[nil, uuid]]
   end
 
   # Return a hash of {user_uuid: group_perms}
@@ -535,16 +324,16 @@ SELECT target_uuid, perm_level
       self.uuid = new_uuid
       save!(validate: false)
       change_all_uuid_refs(old_uuid: old_uuid, new_uuid: new_uuid)
-    ActiveRecord::Base.connection.exec_query %{
+    ActiveRecord::Base.connection.exec_update %{
 update #{PERMISSION_VIEW} set user_uuid=$1 where user_uuid = $2
 },
-                                             'Change user uuid',
+                                             'User.update_uuid.update_permissions_user_uuid',
                                              [[nil, new_uuid],
                                               [nil, old_uuid]]
-      ActiveRecord::Base.connection.exec_query %{
+      ActiveRecord::Base.connection.exec_update %{
 update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
 },
-                                             'Change target uuid',
+                                            'User.update_uuid.update_permissions_target_uuid',
                                              [[nil, new_uuid],
                                               [nil, old_uuid]]
     end
@@ -905,7 +694,6 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
 
   # add the user to the 'All users' group
   def create_user_group_link
-    #puts "In create_user_group_link"
     return (Link.where(tail_uuid: self.uuid,
                        head_uuid: all_users_group[:uuid],
                        link_class: 'permission',
index e0d1c495f870c413d562684fe291509b71c26b10..be01e7e509f788e49a9a3ac6cdb7f75ea08d8b20 100644 (file)
@@ -1,5 +1,8 @@
 class PermissionTable < ActiveRecord::Migration[5.0]
   def up
+    ActiveRecord::Base.connection.execute "DROP MATERIALIZED VIEW IF EXISTS materialized_permission_view;"
+    drop_table :permission_refresh_lock
+
     create_table :materialized_permissions, :id => false do |t|
       t.string :user_uuid
       t.string :target_uuid
@@ -31,7 +34,7 @@ $$;
     end
     add_index :trashed_groups, :group_uuid, :unique => true
 
-        ActiveRecord::Base.connection.execute %{
+    ActiveRecord::Base.connection.execute %{
 create or replace function compute_trashed ()
 returns table (uuid varchar(27), trash_at timestamp)
 STABLE
@@ -43,9 +46,9 @@ select ps.target_uuid as group_uuid, ps.trash_at from groups,
 $$;
 }
 
-        ActiveRecord::Base.connection.execute("INSERT INTO trashed_groups select * from compute_trashed()")
+    ActiveRecord::Base.connection.execute("INSERT INTO trashed_groups select * from compute_trashed()")
 
-        ActiveRecord::Base.connection.execute %{
+    ActiveRecord::Base.connection.execute %{
 create or replace function should_traverse_owned (starting_uuid varchar(27),
                                                   starting_perm integer)
   returns bool
@@ -58,7 +61,7 @@ $$;
 }
 
 
-        ActiveRecord::Base.connection.execute %{
+    ActiveRecord::Base.connection.execute %{
 create or replace function permission_graph_edges ()
   returns table (tail_uuid varchar(27), head_uuid varchar(27), val integer)
 STABLE
@@ -91,7 +94,7 @@ $$;
         # Re-runs the query on new rows until there are no more results.
         # This accomplishes a breadth-first search of the permission graph.
         #
-        ActiveRecord::Base.connection.execute %{
+    ActiveRecord::Base.connection.execute %{
 create or replace function search_permission_graph (starting_uuid varchar(27),
                                                     starting_perm integer)
   returns table (target_uuid varchar(27), val integer, traverse_owned bool)
@@ -119,23 +122,7 @@ WITH RECURSIVE edges(tail_uuid, head_uuid, val) as (
 $$;
 }
 
-
-  # owned_by_user_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-  #   select users.owner_uuid as perm_origin_uuid, u.target_uuid, u.val, u.traverse_owned
-  #     from users, lateral search_permission_graph(users.uuid, 3) as u
-  #     where users.owner_uuid not in (select target_uuid from perm_from_start) and
-  #           users.uuid in (select target_uuid from perm_from_start)
-  # ),
-
-  # owned_by_group_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
-  #   select groups.owner_uuid as perm_origin_uuid, groups.uuid, 3, true
-  #     from groups
-  #     where groups.owner_uuid not in (select target_uuid from perm_from_start) and
-  #           groups.uuid in (select target_uuid from perm_from_start)
-  # ),
-
-
-        ActiveRecord::Base.connection.execute %{
+    ActiveRecord::Base.connection.execute %{
 create or replace function compute_permission_subgraph (perm_origin_uuid varchar(27),
                                                         starting_uuid varchar(27),
                                                         starting_perm integer)
@@ -196,12 +183,80 @@ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
 $$;
      }
 
-    ActiveRecord::Base.connection.execute "DROP MATERIALIZED VIEW IF EXISTS materialized_permission_view;"
-
+    ActiveRecord::Base.connection.execute %{
+INSERT INTO materialized_permissions
+select users.uuid, g.target_uuid, g.val, g.traverse_owned
+from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0
+}
   end
+
   def down
+    ActiveRecord::Base.connection.execute(%{
+CREATE MATERIALIZED VIEW materialized_permission_view AS
+ WITH RECURSIVE perm_value(name, val) AS (
+         VALUES ('can_read'::text,(1)::smallint), ('can_login'::text,1), ('can_write'::text,2), ('can_manage'::text,3)
+        ), perm_edges(tail_uuid, head_uuid, val, follow, trashed) AS (
+         SELECT links.tail_uuid,
+            links.head_uuid,
+            pv.val,
+            ((pv.val = 3) OR (groups.uuid IS NOT NULL)) AS follow,
+            (0)::smallint AS trashed,
+            (0)::smallint AS followtrash
+           FROM ((public.links
+             LEFT JOIN perm_value pv ON ((pv.name = (links.name)::text)))
+             LEFT JOIN public.groups ON (((pv.val < 3) AND ((groups.uuid)::text = (links.head_uuid)::text))))
+          WHERE ((links.link_class)::text = 'permission'::text)
+        UNION ALL
+         SELECT groups.owner_uuid,
+            groups.uuid,
+            3,
+            true AS bool,
+                CASE
+                    WHEN ((groups.trash_at IS NOT NULL) AND (groups.trash_at < clock_timestamp())) THEN 1
+                    ELSE 0
+                END AS "case",
+            1
+           FROM public.groups
+        ), perm(val, follow, user_uuid, target_uuid, trashed) AS (
+         SELECT (3)::smallint AS val,
+            true AS follow,
+            (users.uuid)::character varying(32) AS user_uuid,
+            (users.uuid)::character varying(32) AS target_uuid,
+            (0)::smallint AS trashed
+           FROM public.users
+        UNION
+         SELECT (LEAST((perm_1.val)::integer, edges.val))::smallint AS val,
+            edges.follow,
+            perm_1.user_uuid,
+            (edges.head_uuid)::character varying(32) AS target_uuid,
+            ((GREATEST((perm_1.trashed)::integer, edges.trashed) * edges.followtrash))::smallint AS trashed
+           FROM (perm perm_1
+             JOIN perm_edges edges ON ((perm_1.follow AND ((edges.tail_uuid)::text = (perm_1.target_uuid)::text))))
+        )
+ SELECT perm.user_uuid,
+    perm.target_uuid,
+    max(perm.val) AS perm_level,
+        CASE perm.follow
+            WHEN true THEN perm.target_uuid
+            ELSE NULL::character varying
+        END AS target_owner_uuid,
+    max(perm.trashed) AS trashed
+   FROM perm
+  GROUP BY perm.user_uuid, perm.target_uuid,
+        CASE perm.follow
+            WHEN true THEN perm.target_uuid
+            ELSE NULL::character varying
+        END
+  WITH NO DATA;
+}
+    )
+
+    add_index :materialized_permission_view, [:trashed, :target_uuid], name: 'permission_target_trashed'
+    add_index :materialized_permission_view, [:user_uuid, :trashed, :perm_level], name: 'permission_target_user_trashed_level'
+
     drop_table :materialized_permissions
     drop_table :trashed_groups
+    create_table :permission_refresh_lock
 
     ActiveRecord::Base.connection.execute "DROP function project_subtree_with_trash_at (varchar, timestamp);"
     ActiveRecord::Base.connection.execute "DROP function compute_trashed ();"
index 71e482083014e6717e72419af53e66cd741846a2..c34d69e28c01e1b1f01bf801fa59e2d7a143dc94 100644 (file)
@@ -929,34 +929,6 @@ CREATE SEQUENCE public.nodes_id_seq
 ALTER SEQUENCE public.nodes_id_seq OWNED BY public.nodes.id;
 
 
---
--- Name: permission_refresh_lock; Type: TABLE; Schema: public; Owner: -
---
-
-CREATE TABLE public.permission_refresh_lock (
-    id integer NOT NULL
-);
-
-
---
--- Name: permission_refresh_lock_id_seq; Type: SEQUENCE; Schema: public; Owner: -
---
-
-CREATE SEQUENCE public.permission_refresh_lock_id_seq
-    START WITH 1
-    INCREMENT BY 1
-    NO MINVALUE
-    NO MAXVALUE
-    CACHE 1;
-
-
---
--- Name: permission_refresh_lock_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
---
-
-ALTER SEQUENCE public.permission_refresh_lock_id_seq OWNED BY public.permission_refresh_lock.id;
-
-
 --
 -- Name: pipeline_instances; Type: TABLE; Schema: public; Owner: -
 --
@@ -1392,13 +1364,6 @@ ALTER TABLE ONLY public.logs ALTER COLUMN id SET DEFAULT nextval('public.logs_id
 ALTER TABLE ONLY public.nodes ALTER COLUMN id SET DEFAULT nextval('public.nodes_id_seq'::regclass);
 
 
---
--- Name: permission_refresh_lock id; Type: DEFAULT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public.permission_refresh_lock ALTER COLUMN id SET DEFAULT nextval('public.permission_refresh_lock_id_seq'::regclass);
-
-
 --
 -- Name: pipeline_instances id; Type: DEFAULT; Schema: public; Owner: -
 --
@@ -1583,14 +1548,6 @@ ALTER TABLE ONLY public.nodes
     ADD CONSTRAINT nodes_pkey PRIMARY KEY (id);
 
 
---
--- Name: permission_refresh_lock permission_refresh_lock_pkey; Type: CONSTRAINT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public.permission_refresh_lock
-    ADD CONSTRAINT permission_refresh_lock_pkey PRIMARY KEY (id);
-
-
 --
 -- Name: pipeline_instances pipeline_instances_pkey; Type: CONSTRAINT; Schema: public; Owner: -
 --
index 8160502b9a1f2b961488ca1fcbe1bf3f753f2820..e89978d7b3c38dfe9fd53fd3ed0f85ffca8f80eb 100644 (file)
@@ -7,13 +7,14 @@ TRASHED_GROUPS = "trashed_groups"
 
 def do_refresh_permission_view
   ActiveRecord::Base.transaction do
-    ActiveRecord::Base.connection.execute("LOCK TABLE permission_refresh_lock")
+    ActiveRecord::Base.connection.execute("LOCK TABLE #{PERMISSION_VIEW}")
     ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW}")
     ActiveRecord::Base.connection.execute %{
 INSERT INTO #{PERMISSION_VIEW}
 select users.uuid, g.target_uuid, g.val, g.traverse_owned
 from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0
-}
+},
+                                          "refresh_permission_view.do"
   end
 end
 
@@ -44,3 +45,90 @@ def refresh_permission_view(async=false)
     do_refresh_permission_view
   end
 end
+
+
+def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
+  # Update a subset of the permission graph
+  # perm_level is the inherited permission
+  # perm_level is a number from 0-3
+  #   can_read=1
+  #   can_write=2
+  #   can_manage=3
+  #   call with perm_level=0 to revoke permissions
+  #
+  # 1. Compute set (group, permission) implied by traversing
+  #    graph starting at this group
+  # 2. Find links from outside the graph that point inside
+  # 3. For each starting uuid, get the set of permissions from the
+  #    materialized permission table
+  # 3. Delete permissions from table not in our computed subset.
+  # 4. Upsert each permission in our subset (user, group, val)
+
+  ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE"
+
+  temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}"
+  ActiveRecord::Base.connection.exec_query %{
+create temporary table #{temptable_perms} on commit drop
+as select * from compute_permission_subgraph($1, $2, $3)
+},
+                                           'update_permissions.select',
+                                           [[nil, perm_origin_uuid],
+                                            [nil, starting_uuid],
+                                            [nil, perm_level]]
+
+  ActiveRecord::Base.connection.exec_delete %{
+delete from #{PERMISSION_VIEW} where
+  target_uuid in (select target_uuid from #{temptable_perms}) and
+  not exists (select 1 from #{temptable_perms}
+              where target_uuid=#{PERMISSION_VIEW}.target_uuid and
+                    user_uuid=#{PERMISSION_VIEW}.user_uuid and
+                    val>0)
+},
+                                        "update_permissions.delete"
+
+  ActiveRecord::Base.connection.exec_query %{
+insert into #{PERMISSION_VIEW} (user_uuid, target_uuid, perm_level, traverse_owned)
+  select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms} where val>0
+on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_level, traverse_owned=EXCLUDED.traverse_owned;
+},
+                                           "update_permissions.insert"
+
+  if check
+    #
+    # For validation/debugging, this checks contents of the
+    # incrementally-updated 'materialized_permission' against a
+    # from-scratch permission refresh.
+    #
+
+    q1 = ActiveRecord::Base.connection.exec_query %{
+select user_uuid, target_uuid, perm_level, traverse_owned from #{PERMISSION_VIEW}
+order by user_uuid, target_uuid
+}
+
+    q2 = ActiveRecord::Base.connection.exec_query %{
+select users.uuid as user_uuid, g.target_uuid, g.val as perm_level, g.traverse_owned
+from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0
+order by users.uuid, target_uuid
+}
+
+    if q1.count != q2.count
+      puts "Didn't match incremental+: #{q1.count} != full refresh-: #{q2.count}"
+    end
+
+    if q1.count > q2.count
+      q1.each_with_index do |r, i|
+        if r != q2[i]
+          puts "+#{r}\n-#{q2[i]}"
+          raise "Didn't match"
+        end
+      end
+    else
+      q2.each_with_index do |r, i|
+        if r != q1[i]
+          puts "+#{q1[i]}\n-#{r}"
+          raise "Didn't match"
+        end
+      end
+    end
+  end
+end
index eb97fc1f49034165e6ae02a1896b116a3e835890..445670a3d51572827289bb0dc37430168cd4eb9f 100644 (file)
@@ -193,11 +193,15 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
-  test "create request with async=true defers permissions update" do
+  test "create request with async=true does not defer permissions update" do
     Rails.configuration.API.AsyncPermissionsUpdateInterval = 1 # second
     name = "Random group #{rand(1000)}"
     assert_equal nil, Group.find_by_name(name)
 
+    # Following the implementation of incremental permission updates
+    # (#16007) the async flag is now a no-op.  Permission changes are
+    # visible immediately.
+
     # Trigger the asynchronous permission update by using async=true parameter.
     post "/arvados/v1/groups",
       params: {
@@ -209,7 +213,7 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
       headers: auth(:active)
     assert_response 202
 
-    # The group exists on the database, but it's not accessible yet.
+    # The group exists in the database
     assert_not_nil Group.find_by_name(name)
     get "/arvados/v1/groups",
       params: {
@@ -218,7 +222,7 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
       },
       headers: auth(:active)
     assert_response 200
-    assert_equal 0, json_response['items_available']
+    assert_equal 1, json_response['items_available']
 
     # Wait a bit and try again.
     sleep(1)
index 3280b1fc36714767e4f007f9394a3329c023f6b4..2bdc198a35a0a380d04d8b6c5ffe171131f68b53 100644 (file)
@@ -70,7 +70,6 @@ class OwnerTest < ActiveSupport::TestCase
              "new #{o_class} should really be in DB")
       old_uuid = o.uuid
       new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9])
-      puts "//Changing//"
       if o.respond_to? :update_uuid
         o.update_uuid(new_uuid: new_uuid)
       else