From fc5742654641a10e765ed81d25ca44cb47976d02 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Thu, 28 May 2020 15:35:29 -0400 Subject: [PATCH] 16007: Enable permission correctness checking (only for tests) * Explicitly set up a transaction in update_permissions * Rename refresh_permission_view.rb -> update_permissions.rb * Add skip_check_permissions_against_full_refresh Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- .../app/controllers/database_controller.rb | 4 +- services/api/app/models/database_seeds.rb | 4 +- services/api/app/models/group.rb | 2 +- services/api/app/models/link.rb | 2 +- services/api/app/models/user.rb | 4 +- services/api/db/structure.sql | 8 +- ...rmission_view.rb => update_permissions.rb} | 136 ++++++++++-------- .../api/test/performance/permission_test.rb | 2 +- services/api/test/test_helper.rb | 4 +- services/api/test/unit/owner_test.rb | 15 +- services/api/test/unit/user_test.rb | 3 - 11 files changed, 102 insertions(+), 82 deletions(-) rename services/api/lib/{refresh_permission_view.rb => update_permissions.rb} (52%) diff --git a/services/api/app/controllers/database_controller.rb b/services/api/app/controllers/database_controller.rb index 24c6cf5a79..5c4cf7bc16 100644 --- a/services/api/app/controllers/database_controller.rb +++ b/services/api/app/controllers/database_controller.rb @@ -75,9 +75,9 @@ class DatabaseController < ApplicationController raise end - require 'refresh_permission_view' + require 'update_permissions' - refresh_permission_view + refresh_permissions refresh_trashed # Done. diff --git a/services/api/app/models/database_seeds.rb b/services/api/app/models/database_seeds.rb index 0fea2cf7b6..39f491503e 100644 --- a/services/api/app/models/database_seeds.rb +++ b/services/api/app/models/database_seeds.rb @@ -2,7 +2,7 @@ # # SPDX-License-Identifier: AGPL-3.0 -require 'refresh_permission_view' +require 'update_permissions' class DatabaseSeeds extend CurrentApiClient @@ -14,7 +14,7 @@ class DatabaseSeeds anonymous_group_read_permission anonymous_user empty_collection - refresh_permission_view + refresh_permissions refresh_trashed end end diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb index 9b8a9877e1..485205f1eb 100644 --- a/services/api/app/models/group.rb +++ b/services/api/app/models/group.rb @@ -80,7 +80,7 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at; def before_ownership_change if owner_uuid_changed? and !self.owner_uuid_was.nil? MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all - update_permissions self.owner_uuid, self.uuid, 0 + update_permissions self.owner_uuid_was, self.uuid, 0 end end diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index 33e18b296b..da4ca6c870 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -86,7 +86,7 @@ class Link < ArvadosModel def check_permissions if self.link_class == 'permission' - #check_permissions_against_full_refresh + check_permissions_against_full_refresh end end diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index d48dde25a4..cb7efe9cca 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -436,7 +436,9 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2 if redirect_to_new_user update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil) end - update_permissions self.owner_uuid, self.uuid, 3, false + skip_check_permissions_against_full_refresh do + update_permissions self.owner_uuid, self.uuid, 3 + end update_permissions new_user.owner_uuid, new_user.uuid, 3 end end diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql index 5f71554ffd..c4bf90dca6 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -46,9 +46,9 @@ CREATE FUNCTION public.compute_permission_subgraph(perm_origin_uuid character va LANGUAGE sql STABLE AS $$ with -perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as ( - select perm_origin_uuid, target_uuid, val, traverse_owned - from search_permission_graph(starting_uuid, starting_perm)), + perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as ( + select perm_origin_uuid, target_uuid, val, traverse_owned + from search_permission_graph(starting_uuid, starting_perm)), additional_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as ( select edges.tail_uuid as perm_origin_uuid, ps.target_uuid, ps.val, @@ -156,7 +156,7 @@ $$; -- CREATE FUNCTION public.should_traverse_owned(starting_uuid character varying, starting_perm integer) RETURNS boolean - LANGUAGE sql STABLE + LANGUAGE sql IMMUTABLE AS $$ select starting_uuid like '_____-j7d0g-_______________' or (starting_uuid like '_____-tpzed-_______________' and starting_perm >= 3); diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/update_permissions.rb similarity index 52% rename from services/api/lib/refresh_permission_view.rb rename to services/api/lib/update_permissions.rb index 826c44c3b4..b2cf6595b8 100644 --- a/services/api/lib/refresh_permission_view.rb +++ b/services/api/lib/update_permissions.rb @@ -5,7 +5,7 @@ PERMISSION_VIEW = "materialized_permissions" TRASHED_GROUPS = "trashed_groups" -def refresh_permission_view +def refresh_permissions ActiveRecord::Base.transaction do ActiveRecord::Base.connection.execute("LOCK TABLE #{PERMISSION_VIEW}") ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW}") @@ -26,7 +26,7 @@ def refresh_trashed end end -def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false +def update_permissions perm_origin_uuid, starting_uuid, perm_level # # Update a subset of the permission table affected by adding or # removing a particular permission relationship (ownership or a @@ -45,7 +45,7 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false # can_manage=3 # or call with perm_level=0 to revoke permissions # - # check: for testing/debugging only, compare the result of the + # check: for testing/debugging, compare the result of the # incremental update against a full table recompute. Throws an # error if the contents are not identical (ie they produce different # permission results) @@ -68,60 +68,62 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false # see db/migrate/20200501150153_permission_table.rb for details on # how the permissions are computed. - # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE - # ROW EXCLUSIVE, EXCLUSIVE, and ACCESS EXCLUSIVE lock modes. This - # mode protects a table against concurrent data changes." - ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE" - - # Workaround for - # BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE - # https://www.postgresql.org/message-id/152395805004.19366.3107109716821067806@wrigleys.postgresql.org - # - # For a crucial join in the compute_permission_subgraph() query, the - # planner mis-estimates the number of rows in a Common Table - # Expression (CTE, this is a subquery in a WITH clause) and as a - # result it chooses the wrong join order. The join starts with the - # permissions table because it mistakenly thinks - # count(materalized_permissions) < count(new computed permissions) - # when actually it is the other way around. - # - # Because of the incorrect join order, it choose the wrong join - # strategy (merge join, which works best when two tables are roughly - # the same size). As a workaround, we can tell it not to use that - # join strategy, this causes it to pick hash join instead, which - # turns out to be a bit better. However, because the join order is - # still wrong, we don't get the full benefit of the index. - # - # This is very unfortunate because it makes the query performance - # dependent on the size of the materalized_permissions table, when - # the goal of this design was to make permission updates scale-free - # and only depend on the number of permissions affected and not the - # total table size. In several hours of researching I wasn't able - # to find a way to force the correct join order, so I'm calling it - # here and I have to move on. - # - # This is apparently addressed in Postgres 12, but I developed & - # tested this on Postgres 9.6, so in the future we should reevaluate - # the performance & query plan on Postgres 12. - # - # https://git.furworks.de/opensourcemirror/postgresql/commit/a314c34079cf06d05265623dd7c056f8fa9d577f - # - # Disable merge join for just this query (also local for this transaction), then reenable it. - ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;" + ActiveRecord::Base.transaction do - temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}" - ActiveRecord::Base.connection.exec_query %{ + # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE + # ROW EXCLUSIVE, EXCLUSIVE, and ACCESS EXCLUSIVE lock modes. This + # mode protects a table against concurrent data changes." + ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE" + + # Workaround for + # BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE + # https://www.postgresql.org/message-id/152395805004.19366.3107109716821067806@wrigleys.postgresql.org + # + # For a crucial join in the compute_permission_subgraph() query, the + # planner mis-estimates the number of rows in a Common Table + # Expression (CTE, this is a subquery in a WITH clause) and as a + # result it chooses the wrong join order. The join starts with the + # permissions table because it mistakenly thinks + # count(materalized_permissions) < count(new computed permissions) + # when actually it is the other way around. + # + # Because of the incorrect join order, it choose the wrong join + # strategy (merge join, which works best when two tables are roughly + # the same size). As a workaround, we can tell it not to use that + # join strategy, this causes it to pick hash join instead, which + # turns out to be a bit better. However, because the join order is + # still wrong, we don't get the full benefit of the index. + # + # This is very unfortunate because it makes the query performance + # dependent on the size of the materalized_permissions table, when + # the goal of this design was to make permission updates scale-free + # and only depend on the number of permissions affected and not the + # total table size. In several hours of researching I wasn't able + # to find a way to force the correct join order, so I'm calling it + # here and I have to move on. + # + # This is apparently addressed in Postgres 12, but I developed & + # tested this on Postgres 9.6, so in the future we should reevaluate + # the performance & query plan on Postgres 12. + # + # https://git.furworks.de/opensourcemirror/postgresql/commit/a314c34079cf06d05265623dd7c056f8fa9d577f + # + # Disable merge join for just this query (also local for this transaction), then reenable it. + ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;" + + temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}" + ActiveRecord::Base.connection.exec_query %{ create temporary table #{temptable_perms} on commit drop as select * from compute_permission_subgraph($1, $2, $3) }, - 'update_permissions.select', - [[nil, perm_origin_uuid], - [nil, starting_uuid], - [nil, perm_level]] + 'update_permissions.select', + [[nil, perm_origin_uuid], + [nil, starting_uuid], + [nil, perm_level]] - ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;" + ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;" - ActiveRecord::Base.connection.exec_delete %{ + ActiveRecord::Base.connection.exec_delete %{ delete from #{PERMISSION_VIEW} where target_uuid in (select target_uuid from #{temptable_perms}) and not exists (select 1 from #{temptable_perms} @@ -129,27 +131,29 @@ delete from #{PERMISSION_VIEW} where user_uuid=#{PERMISSION_VIEW}.user_uuid and val>0) }, - "update_permissions.delete" + "update_permissions.delete" - ActiveRecord::Base.connection.exec_query %{ + ActiveRecord::Base.connection.exec_query %{ insert into #{PERMISSION_VIEW} (user_uuid, target_uuid, perm_level, traverse_owned) select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms} where val>0 on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_level, traverse_owned=EXCLUDED.traverse_owned; }, - "update_permissions.insert" + "update_permissions.insert" - if check and perm_level>0 - check_permissions_against_full_refresh + if perm_level>0 + check_permissions_against_full_refresh + end end end def check_permissions_against_full_refresh - # - # For debugging, this checks contents of the - # incrementally-updated 'materialized_permission' against a - # from-scratch permission refresh. - # + # No-op except when running tests + return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh] + + # For checking correctness of the incremental permission updates. + # Check contents of the current 'materialized_permission' table + # against a from-scratch permission refresh. q1 = ActiveRecord::Base.connection.exec_query %{ select user_uuid, target_uuid, perm_level, traverse_owned from #{PERMISSION_VIEW} @@ -182,3 +186,13 @@ order by users.uuid, target_uuid end end end + +def skip_check_permissions_against_full_refresh + check_perm_was = Thread.current[:no_check_permissions_against_full_refresh] + Thread.current[:no_check_permissions_against_full_refresh] = true + begin + yield + ensure + Thread.current[:no_check_permissions_against_full_refresh] = check_perm_was + end +end diff --git a/services/api/test/performance/permission_test.rb b/services/api/test/performance/permission_test.rb index 0fea3b1350..d0e6413b16 100644 --- a/services/api/test/performance/permission_test.rb +++ b/services/api/test/performance/permission_test.rb @@ -40,7 +40,7 @@ class PermissionPerfTest < ActionDispatch::IntegrationTest end end end - refresh_permission_view + refresh_permissions end end) end diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb index 12f729e338..c99a57aaff 100644 --- a/services/api/test/test_helper.rb +++ b/services/api/test/test_helper.rb @@ -2,7 +2,7 @@ # # SPDX-License-Identifier: AGPL-3.0 -require 'refresh_permission_view' +require 'update_permissions' ENV["RAILS_ENV"] = "test" unless ENV["NO_COVERAGE_TEST"] @@ -209,5 +209,5 @@ class ActionDispatch::IntegrationTest end # Ensure permissions are computed from the test fixtures. -refresh_permission_view +refresh_permissions refresh_trashed diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb index 2bdc198a35..ca02e2db5e 100644 --- a/services/api/test/unit/owner_test.rb +++ b/services/api/test/unit/owner_test.rb @@ -87,9 +87,11 @@ class OwnerTest < ActiveSupport::TestCase assert_equal(true, Specimen.where(owner_uuid: o.uuid).any?, "need something to be owned by #{o.uuid} for this test") - assert_raises(ActiveRecord::DeleteRestrictionError, - "should not delete #{ofixt} that owns objects") do - o.destroy + skip_check_permissions_against_full_refresh do + assert_raises(ActiveRecord::DeleteRestrictionError, + "should not delete #{ofixt} that owns objects") do + o.destroy + end end end @@ -108,9 +110,14 @@ class OwnerTest < ActiveSupport::TestCase assert User.where(uuid: o.uuid).any?, "new User should really be in DB" assert_equal(true, o.update_attributes(owner_uuid: o.uuid), "setting owner to self should work") - assert(o.destroy, "should delete User that owns self") + + skip_check_permissions_against_full_refresh do + assert(o.destroy, "should delete User that owns self") + end + assert_equal(false, User.where(uuid: o.uuid).any?, "#{o.uuid} should not be in DB after deleting") + check_permissions_against_full_refresh end test "change uuid of User that owns self" do diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb index 5d25361eda..596cd415fb 100644 --- a/services/api/test/unit/user_test.rb +++ b/services/api/test/unit/user_test.rb @@ -168,9 +168,6 @@ class UserTest < ActiveSupport::TestCase act_as_system_user do users(:admin).update_attributes!(is_admin: false) end - # need to manually call clear_permissions because we used 'delete' instead of 'destory' - # we use delete here because 'destroy' will throw an error - #users(:admin).clear_permissions @all_users = User.where("uuid not like '%-000000000000000'").where(:is_admin => true) assert_equal 0, @all_users.count, "No admin users should exist (except for the system user)" end -- 2.30.2