From d88b330cc7ecd367c7a0ac504fd93045c1a1bd54 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Mon, 4 Sep 2023 17:04:23 +0100 Subject: [PATCH] Extract SupportedPermission::SIGNIN_NAME To remove duplication of the "signin" string. --- app/models/doorkeeper/application.rb | 8 +-- app/models/supported_permission.rb | 6 +- app/models/user.rb | 2 +- app/views/shared/_user_permissions.html.erb | 8 +-- app/views/users/_app_permissions.html.erb | 4 +- lib/numbers/metrics.rb | 2 +- .../authorisations_controller_test.rb | 2 +- test/controllers/root_controller_test.rb | 2 +- test/controllers/users_controller_test.rb | 70 +++++++++---------- test/factories/oauth_applications.rb | 6 +- test/integration/api_authentication_test.rb | 2 +- test/integration/batch_inviting_users_test.rb | 2 +- .../bulk_granting_permisions_test.rb | 4 +- test/integration/granting_permissions_test.rb | 8 +-- test/integration/manage_api_users_test.rb | 2 +- test/lib/sso_push_credential_test.rb | 4 +- test/lib/user_creator_test.rb | 14 ++-- test/lib/user_permission_migrator_test.rb | 20 +++--- test/models/batch_invitation_test.rb | 4 +- test/models/bulk_grant_permission_set_test.rb | 20 +++--- test/models/doorkeeper_application_test.rb | 6 +- test/models/user_o_auth_presenter_test.rb | 2 +- test/models/user_test.rb | 4 +- test/services/user_update_test.rb | 2 +- 24 files changed, 103 insertions(+), 101 deletions(-) diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index 2b69fc913..fcebfcebb 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -11,13 +11,13 @@ class ::Doorkeeper::Application < ActiveRecord::Base lambda { |user| joins(supported_permissions: :user_application_permissions) .where("user_application_permissions.user_id" => user.id) - .where("supported_permissions.name" => "signin") + .where("supported_permissions.name" => SupportedPermission::SIGNIN_NAME) .where(retired: false) } scope :with_signin_delegatable, lambda { joins(:supported_permissions) - .where(supported_permissions: { name: "signin", delegatable: true }) + .where(supported_permissions: { name: SupportedPermission::SIGNIN_NAME, delegatable: true }) } after_create :create_signin_supported_permission @@ -36,7 +36,7 @@ def supported_permission_strings(user = nil) end def signin_permission - supported_permissions.find_by(name: "signin") + supported_permissions.find_by(name: SupportedPermission::SIGNIN_NAME) end def sorted_supported_permissions_grantable_from_ui @@ -78,7 +78,7 @@ def substituted_uri(uri) end def create_signin_supported_permission - supported_permissions.create!(name: "signin", delegatable: true) + supported_permissions.create!(name: SupportedPermission::SIGNIN_NAME, delegatable: true) end def create_user_update_supported_permission diff --git a/app/models/supported_permission.rb b/app/models/supported_permission.rb index eea6c2e7d..188bbc3d4 100644 --- a/app/models/supported_permission.rb +++ b/app/models/supported_permission.rb @@ -1,4 +1,6 @@ class SupportedPermission < ApplicationRecord + SIGNIN_NAME = "signin".freeze + belongs_to :application, class_name: "Doorkeeper::Application" has_many :user_application_permissions, dependent: :destroy, inverse_of: :supported_permission @@ -11,7 +13,7 @@ class SupportedPermission < ApplicationRecord scope :default, -> { where(default: true) } def signin? - name.try(:downcase) == "signin" + name.try(:downcase) == SIGNIN_NAME end private @@ -19,7 +21,7 @@ def signin? def signin_permission_name_not_changed return if new_record? || !name_changed? - if name_change.first.casecmp("signin").zero? + if name_change.first.casecmp(SIGNIN_NAME).zero? errors.add(:name, "of permission #{name_change.first} can't be changed") end end diff --git a/app/models/user.rb b/app/models/user.rb index b7686e1c8..063bbcec9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -168,7 +168,7 @@ def revoke_all_authorisations end def grant_application_signin_permission(application) - grant_application_permission(application, "signin") + grant_application_permission(application, SupportedPermission::SIGNIN_NAME) end def grant_application_permission(application, supported_permission_name) diff --git a/app/views/shared/_user_permissions.html.erb b/app/views/shared/_user_permissions.html.erb index f63e2eb03..873e44cf5 100644 --- a/app/views/shared/_user_permissions.html.erb +++ b/app/views/shared/_user_permissions.html.erb @@ -31,19 +31,19 @@ # API Users will always have a "signin" permission for apps for which they have access token. # The hidden field ensures it is not lost. %> - <%= hidden_field_tag supported_permission_field_name, application.signin_permission.id, id: "#{supported_permission_field_prefix}_signin" %> + <%= hidden_field_tag supported_permission_field_name, application.signin_permission.id, id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> <% else %> - <%= label_tag "#{supported_permission_field_prefix}_signin", "Has access to #{application.name}?", class: "rm" %> + <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> <%= check_box_tag supported_permission_field_name, application.signin_permission.id, user_object.has_access_to?(application), - id: "#{supported_permission_field_prefix}_signin" %> + id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> <% end %> <%= label_tag "#{supported_permission_field_prefix}_ids", "Permissions for #{application.name}", class: "rm" %> <% supported_permissions_options = application.supported_permissions.grantable_from_ui .inject({}) {|h, per| h.merge(per.name => per.id) } - supported_permissions_options.delete('signin') %> + supported_permissions_options.delete(SupportedPermission::SIGNIN_NAME) %> <%= select_tag supported_permission_field_name, options_for_select(supported_permissions_options, user_object.permission_ids_for(application) - [application.signin_permission.id]), diff --git a/app/views/users/_app_permissions.html.erb b/app/views/users/_app_permissions.html.erb index 9de20ac1f..ecc98413a 100644 --- a/app/views/users/_app_permissions.html.erb +++ b/app/views/users/_app_permissions.html.erb @@ -22,11 +22,11 @@ <% unless user_object.api_user? %> - <%= permission_names.include?('signin') ? 'Yes' : 'No' %> + <%= permission_names.include?(SupportedPermission::SIGNIN_NAME) ? 'Yes' : 'No' %> <% end %> - <%= permission_names.reject{|p| p == 'signin'}.to_sentence %> + <%= permission_names.reject{|p| p == SupportedPermission::SIGNIN_NAME}.to_sentence %> <% end %> diff --git a/lib/numbers/metrics.rb b/lib/numbers/metrics.rb index b5574b0a7..b352d15c8 100644 --- a/lib/numbers/metrics.rb +++ b/lib/numbers/metrics.rb @@ -65,7 +65,7 @@ def to_a private def has_signin_permissions?(permission) - permission.permissions.include?("signin") + permission.permissions.include?(SupportedPermission::SIGNIN_NAME) end def count_values(map) diff --git a/test/controllers/authorisations_controller_test.rb b/test/controllers/authorisations_controller_test.rb index 5ae066da1..d62d6b56a 100644 --- a/test/controllers/authorisations_controller_test.rb +++ b/test/controllers/authorisations_controller_test.rb @@ -62,7 +62,7 @@ class AuthorisationsControllerTest < ActionController::TestCase post :create, params: { api_user_id: @api_user.id, doorkeeper_access_token: { application_id: @application.id } } - assert_equal %w[signin], @api_user.permissions_for(@application) + assert_equal [SupportedPermission::SIGNIN_NAME], @api_user.permissions_for(@application) end end end diff --git a/test/controllers/root_controller_test.rb b/test/controllers/root_controller_test.rb index 72f31aa0c..b8229e380 100644 --- a/test/controllers/root_controller_test.rb +++ b/test/controllers/root_controller_test.rb @@ -26,7 +26,7 @@ def setup test "Your applications should include apps you have permission to signin to" do exclusive_app = create(:application, name: "Exclusive app") everybody_app = create(:application, name: "Everybody app") - user = create(:user, with_permissions: { exclusive_app => [], everybody_app => %w[signin] }) + user = create(:user, with_permissions: { exclusive_app => [], everybody_app => [SupportedPermission::SIGNIN_NAME] }) sign_in user diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index facbe7cdc..82f45e744 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -200,7 +200,7 @@ def change_user_password(user_factory, new_password) @request.env["HTTP_AUTHORIZATION"] = "Bearer #{token.token}" get :show, params: { client_id: @application.uid, format: :json } json = JSON.parse(response.body) - assert_equal(%w[signin], json["user"]["permissions"]) + assert_equal([SupportedPermission::SIGNIN_NAME], json["user"]["permissions"]) end should "fetching json profile should include only permissions for the relevant app" do @@ -212,7 +212,7 @@ def change_user_password(user_factory, new_password) @request.env["HTTP_AUTHORIZATION"] = "Bearer #{token.token}" get :show, params: { client_id: @application.uid, format: :json } json = JSON.parse(response.body) - assert_equal(%w[signin], json["user"]["permissions"]) + assert_equal([SupportedPermission::SIGNIN_NAME], json["user"]["permissions"]) end should "fetching json profile should update last_synced_at for the relevant app" do @@ -469,10 +469,10 @@ def change_user_password(user_factory, new_password) end should "can give permissions to all applications" do - delegatable_app = create(:application, with_delegatable_supported_permissions: %w[signin]) - non_delegatable_app = create(:application, with_supported_permissions: %w[signin]) - delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: %w[signin]) - non_delegatable_no_access_to_app = create(:application, with_supported_permissions: %w[signin]) + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_no_access_to_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) @user.grant_application_signin_permission(delegatable_app) @user.grant_application_signin_permission(non_delegatable_app) @@ -507,10 +507,10 @@ def change_user_password(user_factory, new_password) end should "be able to give permissions only to applications they themselves have access to and that also have delegatable signin permissions" do - delegatable_app = create(:application, with_delegatable_supported_permissions: %w[signin]) - non_delegatable_app = create(:application, with_supported_permissions: %w[signin]) - delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: %w[signin]) - non_delegatable_no_access_to_app = create(:application, with_supported_permissions: %w[signin]) + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_no_access_to_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) organisation_admin = create(:organisation_admin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) @@ -533,10 +533,10 @@ def change_user_password(user_factory, new_password) end should "be able to see all permissions to applications for a user" do - delegatable_app = create(:application, with_delegatable_supported_permissions: %w[signin Editor]) - non_delegatable_app = create(:application, with_supported_permissions: ["signin", "GDS Admin"]) - delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: ["signin", "GDS Editor"]) - non_delegatable_no_access_to_app = create(:application, with_supported_permissions: ["signin", "Import CSVs"]) + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Editor"]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Admin"]) + delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Editor"]) + non_delegatable_no_access_to_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Import CSVs"]) organisation_admin = create(:organisation_admin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) @@ -546,9 +546,9 @@ def change_user_password(user_factory, new_password) :user_in_organisation, organisation: organisation_admin.organisation, with_permissions: { delegatable_app => %w[Editor], - non_delegatable_app => ["signin", "GDS Admin"], - delegatable_no_access_to_app => ["signin", "GDS Editor"], - non_delegatable_no_access_to_app => ["signin", "Import CSVs"] }, + non_delegatable_app => [SupportedPermission::SIGNIN_NAME, "GDS Admin"], + delegatable_no_access_to_app => [SupportedPermission::SIGNIN_NAME, "GDS Editor"], + non_delegatable_no_access_to_app => [SupportedPermission::SIGNIN_NAME, "Import CSVs"] }, ) get :edit, params: { id: user.id } @@ -590,10 +590,10 @@ def change_user_password(user_factory, new_password) end should "be able to give permissions only to applications they themselves have access to and that also have delegatable signin permissions" do - delegatable_app = create(:application, with_delegatable_supported_permissions: %w[signin]) - non_delegatable_app = create(:application, with_supported_permissions: %w[signin]) - delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: %w[signin]) - non_delegatable_no_access_to_app = create(:application, with_supported_permissions: %w[signin]) + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_no_access_to_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) super_org_admin = create(:super_organisation_admin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) @@ -616,10 +616,10 @@ def change_user_password(user_factory, new_password) end should "be able to see all permissions to applications for a user" do - delegatable_app = create(:application, with_delegatable_supported_permissions: %w[signin Editor]) - non_delegatable_app = create(:application, with_supported_permissions: ["signin", "GDS Admin"]) - delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: ["signin", "GDS Editor"]) - non_delegatable_no_access_to_app = create(:application, with_supported_permissions: ["signin", "Import CSVs"]) + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Editor"]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Admin"]) + delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Editor"]) + non_delegatable_no_access_to_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Import CSVs"]) super_org_admin = create(:super_organisation_admin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) @@ -629,9 +629,9 @@ def change_user_password(user_factory, new_password) :user_in_organisation, organisation: super_org_admin.organisation, with_permissions: { delegatable_app => %w[Editor], - non_delegatable_app => ["signin", "GDS Admin"], - delegatable_no_access_to_app => ["signin", "GDS Editor"], - non_delegatable_no_access_to_app => ["signin", "Import CSVs"] }, + non_delegatable_app => [SupportedPermission::SIGNIN_NAME, "GDS Admin"], + delegatable_no_access_to_app => [SupportedPermission::SIGNIN_NAME, "GDS Editor"], + non_delegatable_no_access_to_app => [SupportedPermission::SIGNIN_NAME, "Import CSVs"] }, ) get :edit, params: { id: user.id } @@ -660,10 +660,10 @@ def change_user_password(user_factory, new_password) context "superadmin" do should "not be able to see all permissions to applications for a user" do - delegatable_app = create(:application, with_delegatable_supported_permissions: %w[signin Editor]) - non_delegatable_app = create(:application, with_supported_permissions: ["signin", "GDS Admin"]) - delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: ["signin", "GDS Editor"]) - non_delegatable_no_access_to_app = create(:application, with_supported_permissions: ["signin", "Import CSVs"]) + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Editor"]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Admin"]) + delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Editor"]) + non_delegatable_no_access_to_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Import CSVs"]) superadmin = create(:superadmin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) @@ -673,9 +673,9 @@ def change_user_password(user_factory, new_password) :user_in_organisation, organisation: superadmin.organisation, with_permissions: { delegatable_app => %w[Editor], - non_delegatable_app => ["signin", "GDS Admin"], - delegatable_no_access_to_app => ["signin", "GDS Editor"], - non_delegatable_no_access_to_app => ["signin", "Import CSVs"] }, + non_delegatable_app => [SupportedPermission::SIGNIN_NAME, "GDS Admin"], + delegatable_no_access_to_app => [SupportedPermission::SIGNIN_NAME, "GDS Editor"], + non_delegatable_no_access_to_app => [SupportedPermission::SIGNIN_NAME, "Import CSVs"] }, ) get :edit, params: { id: user.id } diff --git a/test/factories/oauth_applications.rb b/test/factories/oauth_applications.rb index 1ff8dfa6e..2e1bbb89b 100644 --- a/test/factories/oauth_applications.rb +++ b/test/factories/oauth_applications.rb @@ -16,19 +16,19 @@ evaluator.with_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 == "signin" + app.signin_permission.update(delegatable: false) && next if permission_name == SupportedPermission::SIGNIN_NAME create(:supported_permission, application_id: app.id, name: permission_name) end evaluator.with_supported_permissions_not_grantable_from_ui.each do |permission_name| - next if permission_name == "signin" + next if permission_name == SupportedPermission::SIGNIN_NAME create(:supported_permission, application_id: app.id, name: permission_name, grantable_from_ui: false) end evaluator.with_delegatable_supported_permissions.each do |permission_name| - next if permission_name == "signin" + next if permission_name == SupportedPermission::SIGNIN_NAME create(:delegatable_supported_permission, application_id: app.id, name: permission_name) end diff --git a/test/integration/api_authentication_test.rb b/test/integration/api_authentication_test.rb index 7521d2927..8526da4eb 100644 --- a/test/integration/api_authentication_test.rb +++ b/test/integration/api_authentication_test.rb @@ -11,7 +11,7 @@ def access_user_endpoint(token = nil, params = {}) setup do @app1 = create(:application, name: "MyApp", with_supported_permissions: %w[write]) - @user = create(:user, with_permissions: { @app1 => %w[signin write] }) + @user = create(:user, with_permissions: { @app1 => [SupportedPermission::SIGNIN_NAME, "write"] }) @user.authorisations.create!(application_id: @app1.id) end diff --git a/test/integration/batch_inviting_users_test.rb b/test/integration/batch_inviting_users_test.rb index be7a8f380..fea5ca3ea 100644 --- a/test/integration/batch_inviting_users_test.rb +++ b/test/integration/batch_inviting_users_test.rb @@ -77,7 +77,7 @@ class BatchInvitingUsersTest < ActionDispatch::IntegrationTest should "ensure that batch invited users get default permissions even when not checked in UI" do create(:supported_permission, application: @application, name: "reader", default: true) - support_app = create(:application, name: "support", with_supported_permissions: %w[signin]) + support_app = create(:application, name: "support", with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) support_app.signin_permission.update!(default: true) user = create(:admin_user) diff --git a/test/integration/bulk_granting_permisions_test.rb b/test/integration/bulk_granting_permisions_test.rb index a764e5c5b..bb6ed85e0 100644 --- a/test/integration/bulk_granting_permisions_test.rb +++ b/test/integration/bulk_granting_permisions_test.rb @@ -9,7 +9,7 @@ class BulkGrantingPermissionsTest < ActionDispatch::IntegrationTest @admins = create_list(:admin_user, 2) @superadmins = create_list(:superadmin_user, 2) - @application = create(:application, with_supported_permissions: %w[signin]) + @application = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) end should "superadmin user can grant multiple permissions to all users in one go" do @@ -74,7 +74,7 @@ def perform_bulk_grant_as_user(acting_user, application) [acting_user, @users, @org_admins, @admins, @superadmins].flatten.each do |user| user.reload - assert_equal %w[signin], user.permissions_for(application) + assert_equal [SupportedPermission::SIGNIN_NAME], user.permissions_for(application) end end end diff --git a/test/integration/granting_permissions_test.rb b/test/integration/granting_permissions_test.rb index 0f22ba500..465064e44 100644 --- a/test/integration/granting_permissions_test.rb +++ b/test/integration/granting_permissions_test.rb @@ -99,7 +99,7 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest end should "support granting signin permissions to delegatable apps that the super organisation admin has access to" do - app = create(:application, name: "MyApp", with_delegatable_supported_permissions: %w[signin]) + app = create(:application, name: "MyApp", with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) @super_org_admin.grant_application_signin_permission(app) visit edit_user_path(@user) @@ -120,7 +120,7 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest end should "not support granting signin permissions to apps that the super organisation admin doesn't have access to" do - create(:application, name: "MyApp", with_delegatable_supported_permissions: %w[signin]) + create(:application, name: "MyApp", with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) visit edit_user_path(@user) assert page.has_no_field? "Has access to MyApp?" @@ -185,7 +185,7 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest end should "support granting signin permissions to delegatable apps that the organisation admin has access to" do - app = create(:application, name: "MyApp", with_delegatable_supported_permissions: %w[signin]) + app = create(:application, name: "MyApp", with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) @organisation_admin.grant_application_signin_permission(app) visit edit_user_path(@user) @@ -206,7 +206,7 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest end should "not support granting signin permissions to apps that the organisation admin doesn't have access to" do - create(:application, name: "MyApp", with_delegatable_supported_permissions: %w[signin]) + create(:application, name: "MyApp", with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) visit edit_user_path(@user) assert page.has_no_field? "Has access to MyApp?" diff --git a/test/integration/manage_api_users_test.rb b/test/integration/manage_api_users_test.rb index bcaa65890..3f7524553 100644 --- a/test/integration/manage_api_users_test.rb +++ b/test/integration/manage_api_users_test.rb @@ -34,7 +34,7 @@ class ManageApiUsersTest < ActionDispatch::IntegrationTest end should "be able to authorise application access and manage permissions for an API user which should get recorded in event log" do - create(:application, name: "Whitehall", with_supported_permissions: ["Managing Editor", "signin"]) + create(:application, name: "Whitehall", with_supported_permissions: ["Managing Editor", SupportedPermission::SIGNIN_NAME]) click_link @api_user.name click_link "Add application token" diff --git a/test/lib/sso_push_credential_test.rb b/test/lib/sso_push_credential_test.rb index e6d83fa4a..869ccdec4 100644 --- a/test/lib/sso_push_credential_test.rb +++ b/test/lib/sso_push_credential_test.rb @@ -23,7 +23,7 @@ class SSOPushCredentialTest < ActiveSupport::TestCase SSOPushCredential.credentials(@application) assert_equal 2, @user.application_permissions.count - assert_same_elements %w[signin user_update_permission], @user.permissions_for(@application) + assert_same_elements [SupportedPermission::SIGNIN_NAME, "user_update_permission"], @user.permissions_for(@application) end should "not create new application permissions if both already exist" do @@ -34,7 +34,7 @@ class SSOPushCredentialTest < ActiveSupport::TestCase SSOPushCredential.credentials(@application) assert_equal 2, @user.application_permissions.count - assert_same_elements %w[user_update_permission signin], @user.permissions_for(@application) + assert_same_elements ["user_update_permission", SupportedPermission::SIGNIN_NAME], @user.permissions_for(@application) end end diff --git a/test/lib/user_creator_test.rb b/test/lib/user_creator_test.rb index 7287da38e..f710b41a5 100644 --- a/test/lib/user_creator_test.rb +++ b/test/lib/user_creator_test.rb @@ -2,7 +2,7 @@ class UserCreatorTest < ActiveSupport::TestCase test "it creates a new user with the supplied name and email" do - FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: %w[signin]) + FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) user_creator = UserCreator.new("Alicia", "alicia@example.com", "app-o-tron") user_creator.create_user! @@ -13,7 +13,7 @@ class UserCreatorTest < ActiveSupport::TestCase end test "invites the new user, so they must validate their email before they can signin" do - FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: %w[signin]) + FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) user_creator = UserCreator.new("Alicia", "alicia@example.com", "app-o-tron") user_creator.create_user! @@ -22,8 +22,8 @@ class UserCreatorTest < ActiveSupport::TestCase end test 'it grants "signin" permission to each application supplied' do - app_o_tron = FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: %w[signin]) - app_erator = FactoryBot.create(:application, name: "app-erator", with_supported_permissions: %w[signin]) + app_o_tron = FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + app_erator = FactoryBot.create(:application, name: "app-erator", with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) user_creator = UserCreator.new("Alicia", "alicia@example.com", "app-o-tron,app-erator") user_creator.create_user! @@ -33,8 +33,8 @@ class UserCreatorTest < ActiveSupport::TestCase end test "it grants all default permissions, even if not signin" do - app_o_tron = FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: %w[signin]) - app_erator = FactoryBot.create(:application, name: "app-erator", with_supported_permissions: %w[signin fall]) + app_o_tron = FactoryBot.create(:application, name: "app-o-tron", with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + app_erator = FactoryBot.create(:application, name: "app-erator", with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "fall"]) create(:supported_permission, application: app_o_tron, name: "bounce", default: true) app_erator.signin_permission.update!(default: true) user_creator = UserCreator.new("Alicia", "alicia@example.com", "") @@ -43,6 +43,6 @@ class UserCreatorTest < ActiveSupport::TestCase created_user = user_creator.user assert_equal %w[bounce], created_user.permissions_for(app_o_tron) - assert_equal %w[signin], created_user.permissions_for(app_erator) + assert_equal [SupportedPermission::SIGNIN_NAME], created_user.permissions_for(app_erator) end end diff --git a/test/lib/user_permission_migrator_test.rb b/test/lib/user_permission_migrator_test.rb index f75b27350..f976e4b76 100644 --- a/test/lib/user_permission_migrator_test.rb +++ b/test/lib/user_permission_migrator_test.rb @@ -15,11 +15,11 @@ class UserPermissionMigrationTest < ActiveSupport::TestCase create(:supported_permission, application: @unrelated_application, name: "gds_editor") create(:supported_permission, application: @unrelated_application, name: "editor") - @gds_editor = create(:user, with_permissions: { "Specialist Publisher" => %w[editor gds_editor signin] }) - @editor = create(:user, with_permissions: { "Specialist Publisher" => %w[editor signin] }) - @writer = create(:user, with_permissions: { "Specialist Publisher" => %w[signin] }) + @gds_editor = create(:user, with_permissions: { "Specialist Publisher" => ["editor", "gds_editor", SupportedPermission::SIGNIN_NAME] }) + @editor = create(:user, with_permissions: { "Specialist Publisher" => ["editor", SupportedPermission::SIGNIN_NAME] }) + @writer = create(:user, with_permissions: { "Specialist Publisher" => [SupportedPermission::SIGNIN_NAME] }) @user_without_access = create(:user) - @user_with_unrelated_access = create(:user, with_permissions: { "unrelated application" => %w[editor gds_editor signin] }) + @user_with_unrelated_access = create(:user, with_permissions: { "unrelated application" => ["editor", "gds_editor", SupportedPermission::SIGNIN_NAME] }) end should "copy permissions over for all users of an application to another application" do @@ -28,15 +28,15 @@ class UserPermissionMigrationTest < ActiveSupport::TestCase target: "Manuals Publisher", ) - assert_equal %w[editor gds_editor signin], @gds_editor.permissions_for(@manuals_publisher) - assert_equal %w[editor signin], @editor.permissions_for(@manuals_publisher) - assert_equal %w[signin], @writer.permissions_for(@manuals_publisher) + assert_equal ["editor", "gds_editor", SupportedPermission::SIGNIN_NAME], @gds_editor.permissions_for(@manuals_publisher) + assert_equal ["editor", SupportedPermission::SIGNIN_NAME], @editor.permissions_for(@manuals_publisher) + assert_equal [SupportedPermission::SIGNIN_NAME], @writer.permissions_for(@manuals_publisher) assert_equal %w[], @user_without_access.permissions_for(@manuals_publisher) assert_equal %w[], @user_with_unrelated_access.permissions_for(@manuals_publisher) - assert_equal %w[editor gds_editor signin], @gds_editor.permissions_for(@specialist_publisher) - assert_equal %w[editor signin], @editor.permissions_for(@specialist_publisher) - assert_equal %w[signin], @writer.permissions_for(@specialist_publisher) + assert_equal ["editor", "gds_editor", SupportedPermission::SIGNIN_NAME], @gds_editor.permissions_for(@specialist_publisher) + assert_equal ["editor", SupportedPermission::SIGNIN_NAME], @editor.permissions_for(@specialist_publisher) + assert_equal [SupportedPermission::SIGNIN_NAME], @writer.permissions_for(@specialist_publisher) assert_equal %w[], @user_without_access.permissions_for(@specialist_publisher) assert_equal %w[], @user_with_unrelated_access.permissions_for(@specialist_publisher) end diff --git a/test/models/batch_invitation_test.rb b/test/models/batch_invitation_test.rb index f378dc8bc..08a4dfa2f 100644 --- a/test/models/batch_invitation_test.rb +++ b/test/models/batch_invitation_test.rb @@ -33,7 +33,7 @@ class BatchInvitationTest < ActiveSupport::TestCase user = User.find_by(email: "a@m.com") assert_not_nil user assert_equal "A", user.name - assert_equal %w[signin], user.permissions_for(@app) + assert_equal [SupportedPermission::SIGNIN_NAME], user.permissions_for(@app) end should "trigger an invitation email" do @@ -78,7 +78,7 @@ class BatchInvitationTest < ActiveSupport::TestCase @bi.perform assert_empty @user.permissions_for(app) - assert_same_elements %w[signin foo], @user.permissions_for(another_app) + assert_same_elements [SupportedPermission::SIGNIN_NAME, "foo"], @user.permissions_for(another_app) end end diff --git a/test/models/bulk_grant_permission_set_test.rb b/test/models/bulk_grant_permission_set_test.rb index 2b3141b6c..4496f8ba3 100644 --- a/test/models/bulk_grant_permission_set_test.rb +++ b/test/models/bulk_grant_permission_set_test.rb @@ -56,10 +56,10 @@ class BulkGrantPermissionSetTest < ActiveSupport::TestCase @permission_set.perform - assert_equal %w[signin], user.permissions_for(@app) - assert_equal %w[signin], admin_user.permissions_for(@app) - assert_equal %w[signin], superadmin_user.permissions_for(@app) - assert_equal %w[signin], orgadmin_user.permissions_for(@app) + assert_equal [SupportedPermission::SIGNIN_NAME], user.permissions_for(@app) + assert_equal [SupportedPermission::SIGNIN_NAME], admin_user.permissions_for(@app) + assert_equal [SupportedPermission::SIGNIN_NAME], superadmin_user.permissions_for(@app) + assert_equal [SupportedPermission::SIGNIN_NAME], orgadmin_user.permissions_for(@app) end should "record the total number of users to be processed" do @@ -86,11 +86,11 @@ class BulkGrantPermissionSetTest < ActiveSupport::TestCase end should "not fail if a user already has one of the supplied permissions" do - user = create(:user, with_permissions: { @app => %w[signin] }) + user = create(:user, with_permissions: { @app => [SupportedPermission::SIGNIN_NAME] }) @permission_set.perform - assert_equal %w[signin], user.permissions_for(@app) + assert_equal [SupportedPermission::SIGNIN_NAME], user.permissions_for(@app) end should "not remove permissions a user has that are not part of the supplied ones for the bulk grant set" do @@ -101,7 +101,7 @@ class BulkGrantPermissionSetTest < ActiveSupport::TestCase @permission_set.perform assert_equal %w[editor], user.permissions_for(other_app) - assert_equal %w[signin admin].sort, user.permissions_for(@app).sort + assert_equal [SupportedPermission::SIGNIN_NAME, "admin"].sort, user.permissions_for(@app).sort end should "mark it as failed if there is an error during processing and pass the error on for the worker to record the error details" do @@ -125,7 +125,7 @@ class BulkGrantPermissionSetTest < ActiveSupport::TestCase context "recording events against users" do setup do - @app2 = create(:application, with_supported_permissions: %w[signin editor]) + @app2 = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "editor"]) @permission_set.supported_permissions += @app2.supported_permissions @permission_set.save! end @@ -161,7 +161,7 @@ class BulkGrantPermissionSetTest < ActiveSupport::TestCase end should "not create an event log for the app if the user already has all the permissions we are trying to grant for that app" do - user = create(:user, with_permissions: { @app => %w[signin] }) + user = create(:user, with_permissions: { @app => [SupportedPermission::SIGNIN_NAME] }) user.event_logs.destroy_all @permission_set.perform @@ -173,7 +173,7 @@ class BulkGrantPermissionSetTest < ActiveSupport::TestCase end should "not create any event logs if the user already has all the permissions we are trying to grant" do - user = create(:user, with_permissions: { @app => %w[signin], @app2 => %w[signin editor] }) + user = create(:user, with_permissions: { @app => [SupportedPermission::SIGNIN_NAME], @app2 => [SupportedPermission::SIGNIN_NAME, "editor"] }) user.event_logs.destroy_all @permission_set.perform diff --git a/test/models/doorkeeper_application_test.rb b/test/models/doorkeeper_application_test.rb index 960f6242e..01d54f87b 100644 --- a/test/models/doorkeeper_application_test.rb +++ b/test/models/doorkeeper_application_test.rb @@ -29,7 +29,7 @@ class ::Doorkeeper::ApplicationTest < ActiveSupport::TestCase user = create(:user) app = create(:application, with_supported_permissions: %w[write]) - assert_equal %w[signin write], app.supported_permission_strings(user) + assert_equal [SupportedPermission::SIGNIN_NAME, "write"], app.supported_permission_strings(user) end should "only show permissions that super organisation admins themselves have" do @@ -140,13 +140,13 @@ class ::Doorkeeper::ApplicationTest < ActiveSupport::TestCase end should "return applications that support delegation of signin permission" do - application = create(:application, with_delegatable_supported_permissions: %w[signin]) + application = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) assert_includes Doorkeeper::Application.with_signin_delegatable, application end should "not return applications that don't support delegation of signin permission" do - create(:application, with_supported_permissions: %w[signin]) + create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) assert_empty Doorkeeper::Application.with_signin_delegatable end diff --git a/test/models/user_o_auth_presenter_test.rb b/test/models/user_o_auth_presenter_test.rb index 7ecb52fc2..db722ac40 100644 --- a/test/models/user_o_auth_presenter_test.rb +++ b/test/models/user_o_auth_presenter_test.rb @@ -22,7 +22,7 @@ class UserOAuthPresenterTest < ActiveSupport::TestCase } user_representation = UserOAuthPresenter.new(user, @application).as_hash[:user] - assert_same_elements %w[signin managing_editor], user_representation.delete(:permissions) + assert_same_elements [SupportedPermission::SIGNIN_NAME, "managing_editor"], user_representation.delete(:permissions) assert_equal expected_user_attributes, user_representation end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index ddc04f1ff..8670684f3 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -577,7 +577,7 @@ def setup user.grant_application_signin_permission(app) user.grant_application_signin_permission(app) - assert_user_has_permissions %w[signin], app, user + assert_user_has_permissions [SupportedPermission::SIGNIN_NAME], app, user end test "returns multiple permissions in name order" do @@ -587,7 +587,7 @@ def setup user.grant_application_signin_permission(app) user.grant_application_permission(app, "edit") - assert_user_has_permissions %w[edit signin], app, user + assert_user_has_permissions ["edit", SupportedPermission::SIGNIN_NAME], app, user end test "inviting a user sets confirmed_at" do diff --git a/test/services/user_update_test.rb b/test/services/user_update_test.rb index d50143cbb..99485c8b6 100644 --- a/test/services/user_update_test.rb +++ b/test/services/user_update_test.rb @@ -18,7 +18,7 @@ class UserUpdateTest < ActionView::TestCase parsed_ip_address = IPAddr.new(ip_address, Socket::AF_INET).to_s affected_user = create(:user) - app = create(:application, name: "App", with_supported_permissions: ["Editor", "signin", "Something Else"]) + app = create(:application, name: "App", with_supported_permissions: ["Editor", SupportedPermission::SIGNIN_NAME, "Something Else"]) affected_user.grant_application_permission(app, "Something Else") perms = app.supported_permissions.first(2).map(&:id)