From 6788fa439143ef41e32f7dcc1373d1ade3d17e01 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Thu, 14 May 2020 12:20:42 -0400 Subject: [PATCH] 16007: Re-enable permission update checking for testing Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- services/api/app/models/link.rb | 6 +-- services/api/app/models/user.rb | 14 ++++-- services/api/lib/refresh_permission_view.rb | 53 +++++++++++---------- 3 files changed, 42 insertions(+), 31 deletions(-) diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index f6210fbb52..33e18b296b 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -17,7 +17,7 @@ class Link < ArvadosModel after_update :call_update_permissions after_create :call_update_permissions before_destroy :clear_permissions - after_destroy :clear_permissions + after_destroy :check_permissions api_accessible :user, extend: :common do |t| t.add :tail_uuid @@ -80,13 +80,13 @@ class Link < ArvadosModel def clear_permissions if self.link_class == 'permission' - update_permissions tail_uuid, head_uuid, 0, false + update_permissions tail_uuid, head_uuid, 0 end end def check_permissions if self.link_class == 'permission' - update_permissions tail_uuid, head_uuid, 0, true + #check_permissions_against_full_refresh end end diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index c27b633acb..d8b18054ba 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -50,7 +50,8 @@ class User < ArvadosModel user.username_changed? and (not user.username_was.nil?) } - after_destroy :clear_permissions + before_destroy :clear_permissions + after_destroy :check_permissions has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid has_many :repositories, foreign_key: :owner_uuid, primary_key: :uuid @@ -143,7 +144,7 @@ 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 - update_permissions self.owner_uuid_was, self.uuid, 0, false + update_permissions self.owner_uuid_was, self.uuid, 0 end end @@ -154,9 +155,14 @@ SELECT 1 FROM #{PERMISSION_VIEW} end def clear_permissions + update_permissions self.owner_uuid, self.uuid, 0 MaterializedPermission.where("user_uuid = ? or target_uuid = ?", uuid, uuid).delete_all end + def check_permissions + check_permissions_against_full_refresh + end + # Return a hash of {user_uuid: group_perms} def self.all_group_permissions all_perms = {} @@ -348,7 +354,7 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2 raise "user does not exist" if !new_user raise "cannot merge to an already merged user" if new_user.redirect_to_user_uuid - update_permissions self.owner_uuid, self.uuid, 0 + self.clear_permissions # If 'self' is a remote user, don't transfer authorizations # (i.e. ability to access the account) to the new user, because @@ -424,7 +430,7 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2 if redirect_to_new_user update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil) end - update_permissions self.owner_uuid, self.uuid, 3 + update_permissions self.owner_uuid, self.uuid, 3, false update_permissions new_user.owner_uuid, new_user.uuid, 3 end end diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/refresh_permission_view.rb index e89978d7b3..77235c0006 100644 --- a/services/api/lib/refresh_permission_view.rb +++ b/services/api/lib/refresh_permission_view.rb @@ -47,7 +47,7 @@ def refresh_permission_view(async=false) end -def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false +def 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 @@ -93,41 +93,46 @@ on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_leve }, "update_permissions.insert" - if check - # - # For validation/debugging, this checks contents of the - # incrementally-updated 'materialized_permission' against a - # from-scratch permission refresh. - # + if check and perm_level>0 + check_permissions_against_full_refresh + end +end + + +def check_permissions_against_full_refresh + # + # For debugging, this checks contents of the + # incrementally-updated 'materialized_permission' against a + # from-scratch permission refresh. + # - q1 = ActiveRecord::Base.connection.exec_query %{ + 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 %{ + 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 + 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 + 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 - else - q2.each_with_index do |r, i| - if r != q1[i] - puts "+#{q1[i]}\n-#{r}" - 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 -- 2.30.2