From 9abbbd6f2d583a54f5403dd2cd8183c0bdea21e8 Mon Sep 17 00:00:00 2001 From: George Eaton Date: Thu, 5 Sep 2024 16:36:12 +0100 Subject: [PATCH 1/3] Add name to `gds_organisation` factory and update use in tests. Co-authored-by: Ynda Jas --- .../two_step_verification_exemptions_controller_test.rb | 2 +- test/factories/organisation.rb | 1 + test/integration/users/two_step_verification_test.rb | 2 +- test/lib/tasks/permissions_test.rb | 2 +- test/policies/user_policy_test.rb | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/test/controllers/two_step_verification_exemptions_controller_test.rb b/test/controllers/two_step_verification_exemptions_controller_test.rb index 932713f6e..ac9b0b817 100644 --- a/test/controllers/two_step_verification_exemptions_controller_test.rb +++ b/test/controllers/two_step_verification_exemptions_controller_test.rb @@ -2,7 +2,7 @@ class TwoStepVerificationExemptionsControllerTest < ActionController::TestCase setup do - @gds = create(:organisation, content_id: Organisation::GDS_ORG_CONTENT_ID) + @gds = create(:gds_organisation) @organisation = create(:organisation) end diff --git a/test/factories/organisation.rb b/test/factories/organisation.rb index bf751b559..990d501bd 100644 --- a/test/factories/organisation.rb +++ b/test/factories/organisation.rb @@ -8,5 +8,6 @@ factory :gds_organisation, parent: :organisation do content_id { Organisation::GDS_ORG_CONTENT_ID } + name { "Government Digital Service" } end end diff --git a/test/integration/users/two_step_verification_test.rb b/test/integration/users/two_step_verification_test.rb index 011f426f3..b37b93d51 100644 --- a/test/integration/users/two_step_verification_test.rb +++ b/test/integration/users/two_step_verification_test.rb @@ -148,7 +148,7 @@ class Users::TwoStepVerificationTest < ActionDispatch::IntegrationTest context "Exempting a user from 2sv" do context "when logged in as a gds super admin" do setup do - @gds = create(:organisation, content_id: Organisation::GDS_ORG_CONTENT_ID) + @gds = create(:gds_organisation) @super_admin = create(:superadmin_user, organisation: @gds) @reason_for_exemption = "accessibility reasons" @expiry_date = 5.days.from_now.to_date diff --git a/test/lib/tasks/permissions_test.rb b/test/lib/tasks/permissions_test.rb index 85d674d4e..c13c99abf 100644 --- a/test/lib/tasks/permissions_test.rb +++ b/test/lib/tasks/permissions_test.rb @@ -4,7 +4,7 @@ class PermissionsTest < ActiveSupport::TestCase setup do Signon::Application.load_tasks if Rake::Task.tasks.empty? - @gds_org = create(:organisation, name: "Government Digital Service") + @gds_org = create(:gds_organisation) @non_gds_org = create(:organisation, name: "Another Department") $stdout.stubs(:write) end diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index abd3f12d9..f144cbb3a 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -9,7 +9,7 @@ class UserPolicyTest < ActiveSupport::TestCase @child_organisation = create(:organisation, parent: @parent_organisation) @super_org_admin = create(:super_organisation_admin_user, organisation: @parent_organisation) @organisation_admin = create(:organisation_admin_user, organisation: @parent_organisation) - @gds = create(:organisation, name: "Government Digital Services", content_id: Organisation::GDS_ORG_CONTENT_ID) + @gds = create(:gds_organisation) end primary_management_actions = %i[new create assign_organisation] From b2843bebade6e8f7005d2392d50982edb02c52ba Mon Sep 17 00:00:00 2001 From: George Eaton Date: Thu, 5 Sep 2024 17:05:29 +0100 Subject: [PATCH 2/3] Ensure we're creating signin permission correctly in factory These would have had incorrect delegatable/grantable_from_ui values in some tests - there weren't any failures, but this represents reality better. Co-authored-by: Ynda Jas --- test/factories/oauth_applications.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/factories/oauth_applications.rb b/test/factories/oauth_applications.rb index 58e649f0e..e412eeece 100644 --- a/test/factories/oauth_applications.rb +++ b/test/factories/oauth_applications.rb @@ -15,15 +15,13 @@ after(:create) do |app, evaluator| evaluator.with_non_delegatable_supported_permissions.each do |permission_name| - # we create signin in an after_create on application. - # this line takes care of tests creating signin in order to look complete or modify delegatable on it. app.signin_permission.update(delegatable: false) && next if permission_name == SupportedPermission::SIGNIN_NAME create(:supported_permission, application: app, name: permission_name) end evaluator.with_non_delegatable_supported_permissions_not_grantable_from_ui.each do |permission_name| - next if permission_name == SupportedPermission::SIGNIN_NAME + app.signin_permission.update(delegatable: false, grantable_from_ui: false) && next if permission_name == SupportedPermission::SIGNIN_NAME create(:supported_permission, application: app, name: permission_name, grantable_from_ui: false) end @@ -35,7 +33,7 @@ end evaluator.with_delegatable_supported_permissions_not_grantable_from_ui.each do |permission_name| - next if permission_name == SupportedPermission::SIGNIN_NAME + app.signin_permission.update(grantable_from_ui: false) && next if permission_name == SupportedPermission::SIGNIN_NAME create(:delegatable_supported_permission, application: app, name: permission_name, grantable_from_ui: false) end From 4767d16e01a3be0a1f6df5f914f42dff65593160 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Wed, 4 Sep 2024 11:55:55 +0100 Subject: [PATCH 3/3] Add Rake task to remove inappropriately-granted perms These have been identified as inappropriately granted to non-GDS users as part of an exercise to clean up the data after having fixed the way delegatable permissions work. We need to remove these permissions from non-GDS users We're using quite a few methods in the tests, which might feel a little overkill for a single-use Rake task, but we opted for this in order to cut down the number of lines required for all the setup and assertion logic and limit the chance of mistyping any of the important details Co-authored-by: George Eaton User input stubbing taken from: https://medium.com/@bschwartz92/testing-i-o-in-ruby-with-minitest-f775133adf6c --- lib/tasks/permissions.rake | 71 ++++++++++++++++ test/lib/tasks/permissions_test.rb | 128 +++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+) diff --git a/lib/tasks/permissions.rake b/lib/tasks/permissions.rake index 416b7a6ef..6d345204f 100644 --- a/lib/tasks/permissions.rake +++ b/lib/tasks/permissions.rake @@ -121,4 +121,75 @@ namespace :permissions do end end end + + desc "Remove inappropriately granted permissions from non-GDS users" + task remove_inappropriately_granted_permissions_from_non_gds_users: :environment do + supported_permission_ids_to_remove = [ + { application_name: "Collections publisher", name: SupportedPermission::SIGNIN_NAME }, + { application_name: "Collections publisher", name: "2i reviewer" }, + { application_name: "Collections publisher", name: "Coronavirus editor" }, + { application_name: "Collections publisher", name: "Edit Taxonomy" }, + { application_name: "Collections publisher", name: "GDS Editor" }, + { application_name: "Collections publisher", name: "Livestream editor" }, + { application_name: "Collections publisher", name: "Sidekiq Monitoring" }, + { application_name: "Collections publisher", name: "Skip review" }, + { application_name: "Collections publisher", name: "Unreleased feature" }, + { application_name: "Content Data", name: "view_email_subs" }, + { application_name: "Content Data", name: "view_siteimprove" }, + { application_name: "Content Tagger", name: "GDS Editor" }, + { application_name: "Content Tagger", name: "Tagathon participant" }, + { application_name: "Content Tagger", name: "Unreleased feature" }, + { application_name: "Manuals Publisher", name: "gds_editor" }, + { application_name: "Specialist publisher", name: "gds_editor" }, + { application_name: "Support", name: "feedex_exporters" }, + { application_name: "Support", name: "feedex_reviews" }, + ].map do |application_name_and_name| + application_name_and_name => {application_name:, name:} + application_id = Doorkeeper::Application.find_by(name: application_name)&.id + SupportedPermission.find_by(application_id:, name:)&.id + end + + gds_organisation_id = Organisation.find_by(content_id: Organisation::GDS_ORG_CONTENT_ID).id + non_gds_user_ids = User.where.not(organisation_id: gds_organisation_id) + + user_application_permissions_to_destroy = UserApplicationPermission.where( + user_id: non_gds_user_ids, + supported_permission_id: supported_permission_ids_to_remove, + ) + + number_of_permissions_to_destroy = user_application_permissions_to_destroy.count + puts "Number of permissions to remove: #{number_of_permissions_to_destroy}, are you sure? (y)" + + input = $stdin.gets.strip.downcase + + unless input == "y" + abort("Aborting") + end + + ActiveRecord::Base.transaction do + initiator = User.find_by(email: "ynda.jas@digital.cabinet-office.gov.uk") + + puts "Destroying #{number_of_permissions_to_destroy} user application permissions" + + user_application_permissions_to_destroy.each do |user_application_permission| + EventLog.record_event( + user_application_permission.user, + EventLog::PERMISSIONS_REMOVED, + initiator:, + application_id: user_application_permission.application_id, + trailing_message: "(removed inappropriately granted permission from non-GDS user: #{user_application_permission.supported_permission.name})", + ) + + user_application_permission.destroy! + end + + user_application_permissions_remaining = UserApplicationPermission.where( + user_id: non_gds_user_ids, + supported_permission_id: supported_permission_ids_to_remove, + ) + raise "Failed to destroy given permissions" unless user_application_permissions_remaining.count.zero? + + puts "Destroyed #{number_of_permissions_to_destroy} user application permissions" + end + end end diff --git a/test/lib/tasks/permissions_test.rb b/test/lib/tasks/permissions_test.rb index c13c99abf..58a5ea45a 100644 --- a/test/lib/tasks/permissions_test.rb +++ b/test/lib/tasks/permissions_test.rb @@ -110,6 +110,134 @@ class PermissionsTest < ActiveSupport::TestCase end end + context "#remove_inappropriately_granted_permissions_from_non_gds_users" do + setup do + @task = Rake::Task["permissions:remove_inappropriately_granted_permissions_from_non_gds_users"] + + @collections_publisher = application_with_revoke_and_retain_list( + name: "Collections publisher", + non_signin_permissions_to_revoke: [ + "2i reviewer", "Coronavirus editor", "Edit Taxonomy", "GDS Editor", "Livestream editor", "Sidekiq Monitoring", "Skip review", "Unreleased feature" + ], + revoke_signin: true, + retain_non_signin_permission: true, + ) + @content_data = application_with_revoke_and_retain_list( + name: "Content Data", + non_signin_permissions_to_revoke: %w[view_email_subs view_siteimprove], + retain_non_signin_permission: true, + ) + @content_tagger = application_with_revoke_and_retain_list( + name: "Content Tagger", + non_signin_permissions_to_revoke: ["GDS Editor", "Tagathon participant", "Unreleased feature"], + retain_non_signin_permission: true, + ) + @manuals_publisher = application_with_revoke_and_retain_list(name: "Manuals Publisher", non_signin_permissions_to_revoke: %w[gds_editor]) + @specialist_publisher = application_with_revoke_and_retain_list(name: "Specialist Publisher", non_signin_permissions_to_revoke: %w[gds_editor]) + @support = application_with_revoke_and_retain_list(name: "Support", non_signin_permissions_to_revoke: %w[feedex_exporters feedex_reviews]) + @app_to_ignore = application_with_revoke_and_retain_list(name: "Ignore me", non_signin_permissions_to_revoke: [], retain_non_signin_permission: true) + + @non_gds_user_1 = create( + :user, + organisation: @non_gds_org, + with_permissions: all_permissions_by_app([@collections_publisher, @content_data, @content_tagger, @app_to_ignore]), + ) + @non_gds_user_2 = create( + :user, + organisation: @non_gds_org, + with_permissions: all_permissions_by_app([@content_tagger, @manuals_publisher, @specialist_publisher, @support, @app_to_ignore]), + ) + @gds_user = create( + :user, + organisation: @gds_org, + with_permissions: all_permissions_by_app([@collections_publisher, @manuals_publisher, @specialist_publisher, @support, @app_to_ignore]), + ) + + @initiator = create(:user, email: "ynda.jas@digital.cabinet-office.gov.uk") + end + + should "remove inapppropriately-granted permissions from non-GDS users, leaving other permissions in place" do + string_io = StringIO.new + string_io.puts "y" + string_io.rewind + $stdin = string_io + + @task.invoke + + $stdin = STDIN + + [@collections_publisher, @content_data, @content_tagger, @app_to_ignore].each do |app_hash| + assert_equal app_hash[:retained_permissions].map(&:name), @non_gds_user_1.permissions_for(app_hash[:application]) + assert_event_logs_for_removal(@non_gds_user_1.uid, app_hash[:revoked_permissions]) + refute_event_logs_for_removal(@non_gds_user_1.uid, app_hash[:retained_permissions]) + end + + [@content_tagger, @manuals_publisher, @specialist_publisher, @support, @app_to_ignore].each do |app_hash| + assert_equal app_hash[:retained_permissions].map(&:name), @non_gds_user_2.permissions_for(app_hash[:application]) + assert_event_logs_for_removal(@non_gds_user_2.uid, app_hash[:revoked_permissions]) + refute_event_logs_for_removal(@non_gds_user_2.uid, app_hash[:retained_permissions]) + end + + [@collections_publisher, @manuals_publisher, @specialist_publisher, @support, @app_to_ignore].each do |app_hash| + assert_equal app_hash[:application].supported_permissions.order(:name).pluck(:name), @gds_user.permissions_for(app_hash[:application]) + refute_event_logs_for_removal(@gds_user.uid, app_hash[:application].supported_permissions) + end + end + end + + def all_permissions_by_app(app_hashes) + permissions_hash = {} + + app_hashes.each do |app_hash| + application = app_hash[:application] + permissions_hash[application] = application.supported_permissions.map(&:name) + end + + permissions_hash + end + + def application_with_revoke_and_retain_list(name:, non_signin_permissions_to_revoke:, revoke_signin: false, retain_non_signin_permission: false) + application = create(:application, name:) + hash = { application:, retained_permissions: [], revoked_permissions: [] } + + signin_permission = hash[:application].signin_permission + + if revoke_signin + hash[:revoked_permissions].push(signin_permission) + else + hash[:retained_permissions].push(signin_permission) + end + + hash[:retained_permissions].push(create(:supported_permission, application:, name: "Untouchable")) if retain_non_signin_permission + hash[:revoked_permissions] = non_signin_permissions_to_revoke.sort.map { |permission_name| create(:supported_permission, application:, name: permission_name) } + + hash + end + + def assert_event_logs_for_removal(grantee_uid, permissions) + permissions.each do |permission| + assert_predicate EventLog.where( + uid: grantee_uid, + event_id: EventLog::PERMISSIONS_REMOVED.id, + initiator: @initiator, + application_id: permission.application_id, + trailing_message: "(removed inappropriately granted permission from non-GDS user: #{permission.name})", + ), :one? + end + end + + def refute_event_logs_for_removal(grantee_uid, permissions) + permissions.each do |permission| + assert_empty EventLog.where( + uid: grantee_uid, + event_id: EventLog::PERMISSIONS_REMOVED.id, + initiator: @initiator, + application_id: permission.application_id, + trailing_message: "(removed inappropriately granted permission from non-GDS user: #{permission.name})", + ) + end + end + def user_with_permissions(user_type, organisation, permissions_hash) create(user_type, organisation:).tap do |user| permissions_hash.to_a.each do |app, permissions|