diff --git a/app/helpers/groups/group_members_helper.rb b/app/helpers/groups/group_members_helper.rb index e20b7a984f65fa2c622f9bb05caf6acb3f6bb499..7577a34f2d1f45f12adba5cd387d0feb3ebac228 100644 --- a/app/helpers/groups/group_members_helper.rb +++ b/app/helpers/groups/group_members_helper.rb @@ -31,7 +31,7 @@ def group_members_app_data( placeholder: placeholder_users, available_roles: available_group_roles(group), reassignment_csv_path: group_bulk_reassignment_file_path(group), - allow_inactive_placeholder_reassignment: allow_inactive_placeholder_reassignment?.to_s + allow_inactive_placeholder_reassignment: Import::UserMapping::AdminBypassAuthorizer.new(current_user).allowed?.to_s } end # rubocop:enable Metrics/ParameterLists @@ -93,10 +93,6 @@ def available_group_roles(group) { title: name, value: "static-#{access_level}" } end end - - def allow_inactive_placeholder_reassignment? - Import::UserMapping::BypassConfirmationAuthorizer.new(current_user).allow_mapping_to_inactive_users? - end end Groups::GroupMembersHelper.prepend_mod_with('Groups::GroupMembersHelper') diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb index 2b4849d118a193dde1013b61ca682bbe10c13c3a..54ef73855a82f5d8c42fd7c91dc4051ec618bc6c 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -78,6 +78,10 @@ class SourceUser < ApplicationRecord transition REASSIGNABLE_STATUSES => :awaiting_approval end + event :reassign_without_confirmation do + transition REASSIGNABLE_STATUSES => :reassignment_in_progress, if: :bypass_placeholder_confirmation_allowed? + end + event :cancel_reassignment do transition CANCELABLE_STATUSES => :pending_reassignment end @@ -171,5 +175,11 @@ def validate_source_hostname errors.add(:source_hostname, :invalid, message: 'must contain scheme and host, and not path') end + + private + + def bypass_placeholder_confirmation_allowed? + Import::UserMapping::AdminBypassAuthorizer.new(reassigned_by_user).allowed? + end end end diff --git a/app/services/import/source_users/reassign_service.rb b/app/services/import/source_users/reassign_service.rb index 6c09dfbf07174aafcbccbaaf2b1738d534fad71a..3f3774d813c59e6fc9b2d2205bb1c1eb21d88064 100644 --- a/app/services/import/source_users/reassign_service.rb +++ b/app/services/import/source_users/reassign_service.rb @@ -29,15 +29,20 @@ def execute return error_invalid_status if invalid_status - if reassign_successful - send_user_reassign_email - track_reassignment_event('propose_placeholder_user_reassignment') + unless reassign_successful + track_reassignment_event('fail_placeholder_user_reassignment') + return ServiceResponse.error(payload: import_source_user, message: import_source_user.errors.full_messages) + end - ServiceResponse.success(payload: import_source_user) + if admin_bypass_confirmation? + Import::ReassignPlaceholderUserRecordsWorker.perform_async(import_source_user.id) + track_reassignment_event('reassign_placeholder_user_without_confirmation') else - track_reassignment_event('fail_placeholder_user_reassignment') - ServiceResponse.error(payload: import_source_user, message: import_source_user.errors.full_messages) + send_user_reassign_email + track_reassignment_event('propose_placeholder_user_reassignment') end + + ServiceResponse.success(payload: import_source_user) end private @@ -47,6 +52,9 @@ def execute def reassign_user import_source_user.reassign_to_user = assignee_user import_source_user.reassigned_by_user = current_user + + return import_source_user.reassign_without_confirmation if admin_bypass_confirmation? + import_source_user.reassign end @@ -78,11 +86,11 @@ def error_invalid_assignee end def invalid_assignee_message - if allow_mapping_to_inactive_users? && allow_mapping_to_admins? + if admin_bypass_confirmation? && allow_mapping_to_admins? s_('UserMapping|You can assign users with regular, auditor, or administrator access only.') elsif allow_mapping_to_admins? s_('UserMapping|You can assign active users with regular, auditor, or administrator access only.') - elsif allow_mapping_to_inactive_users? + elsif admin_bypass_confirmation? s_('UserMapping|You can assign users with regular or auditor access only.') else s_('UserMapping|You can assign active users with regular or auditor access only.') @@ -92,25 +100,21 @@ def invalid_assignee_message def valid_assignee? assignee_user.present? && assignee_user.human? && - (allow_mapping_to_inactive_users? || assignee_user.active?) && + (admin_bypass_confirmation? || assignee_user.active?) && # rubocop:disable Cop/UserAdmin -- This should not be affected by admin mode. # We just want to know whether the user CAN have admin privileges or not. (allow_mapping_to_admins? || !assignee_user.admin?) # rubocop:enable Cop/UserAdmin end - def allow_mapping_to_inactive_users? - bypass_confirmation_authorizer.allow_mapping_to_inactive_users? - end - def allow_mapping_to_admins? ::Gitlab::CurrentSettings.allow_contribution_mapping_to_admins? end - def bypass_confirmation_authorizer - Import::UserMapping::BypassConfirmationAuthorizer.new(current_user) + def admin_bypass_confirmation? + Import::UserMapping::AdminBypassAuthorizer.new(current_user).allowed? end - strong_memoize_attr :bypass_confirmation_authorizer + strong_memoize_attr :admin_bypass_confirmation? end end end diff --git a/config/events/reassign_placeholder_user_without_confirmation.yml b/config/events/reassign_placeholder_user_without_confirmation.yml new file mode 100644 index 0000000000000000000000000000000000000000..234789370040a7a112cafb4a42583abe096a969b --- /dev/null +++ b/config/events/reassign_placeholder_user_without_confirmation.yml @@ -0,0 +1,25 @@ +--- +description: Tracks when a placeholder is reassigned without assignee user confirmation +internal_events: true +action: reassign_placeholder_user_without_confirmation +identifiers: +- namespace +- user +additional_properties: + label: + description: id of the placeholder user + property: + description: id of the user that is replacing the placeholder + import_type: + description: the import source e.g. gitlab_migration, github, bitbucket + reassign_to_user_state: + description: the state of the user that is replacing the placeholder e.g. active, blocked +product_group: import_and_integrate +product_categories: +- importers +milestone: '18.0' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/190272 +tiers: +- free +- premium +- ultimate diff --git a/lib/import/user_mapping/bypass_confirmation_authorizer.rb b/lib/import/user_mapping/admin_bypass_authorizer.rb similarity index 74% rename from lib/import/user_mapping/bypass_confirmation_authorizer.rb rename to lib/import/user_mapping/admin_bypass_authorizer.rb index ecfcf83f1d4b279e30259e0ae46cd4243802723c..1d6f910359c0e7212503f97215a5c253b1ae4156 100644 --- a/lib/import/user_mapping/bypass_confirmation_authorizer.rb +++ b/lib/import/user_mapping/admin_bypass_authorizer.rb @@ -2,20 +2,12 @@ module Import module UserMapping - class BypassConfirmationAuthorizer + class AdminBypassAuthorizer def initialize(reassigning_user) @reassigning_user = reassigning_user end - def allow_mapping_to_inactive_users? - allow_admin_bypass_placeholder_confirmation? - end - - private - - attr_reader :reassigning_user - - def allow_admin_bypass_placeholder_confirmation? + def allowed? return false unless reassigning_user return false unless Feature.enabled?(:importer_user_mapping_allow_bypass_of_confirmation, reassigning_user) @@ -23,6 +15,10 @@ def allow_admin_bypass_placeholder_confirmation? reassigning_user.can_admin_all_resources? && Gitlab.config.gitlab.impersonation_enabled end + + private + + attr_reader :reassigning_user end end end diff --git a/spec/helpers/groups/group_members_helper_spec.rb b/spec/helpers/groups/group_members_helper_spec.rb index c95d70806c049d09ef89182b12783ee15c2f5ab3..ff5e188a348dd0f184b01b04ecb656a775ba189d 100644 --- a/spec/helpers/groups/group_members_helper_spec.rb +++ b/spec/helpers/groups/group_members_helper_spec.rb @@ -187,8 +187,8 @@ context 'when allow_bypass_placeholder_confirmation application setting is disabled' do before do - expect_next_instance_of(Import::UserMapping::BypassConfirmationAuthorizer, current_user) do |authorizer| - allow(authorizer).to receive(:allow_mapping_to_inactive_users?).and_return(false) + expect_next_instance_of(Import::UserMapping::AdminBypassAuthorizer, current_user) do |authorizer| + allow(authorizer).to receive(:allowed?).and_return(false) end end @@ -199,8 +199,8 @@ context 'when allow_bypass_placeholder_confirmation application setting is enabled' do before do - expect_next_instance_of(Import::UserMapping::BypassConfirmationAuthorizer, current_user) do |authorizer| - allow(authorizer).to receive(:allow_mapping_to_inactive_users?).and_return(true) + expect_next_instance_of(Import::UserMapping::AdminBypassAuthorizer, current_user) do |authorizer| + allow(authorizer).to receive(:allowed?).and_return(true) end end diff --git a/spec/lib/import/user_mapping/bypass_confirmation_authorizer_spec.rb b/spec/lib/import/user_mapping/admin_bypass_authorizer_spec.rb similarity index 84% rename from spec/lib/import/user_mapping/bypass_confirmation_authorizer_spec.rb rename to spec/lib/import/user_mapping/admin_bypass_authorizer_spec.rb index f5a62ad7a9eb836a6f7612dd2ef76c97263a14f4..3b7d7851f69f01d30589a3aaa2b60607eed7fcd1 100644 --- a/spec/lib/import/user_mapping/bypass_confirmation_authorizer_spec.rb +++ b/spec/lib/import/user_mapping/admin_bypass_authorizer_spec.rb @@ -2,13 +2,13 @@ require 'spec_helper' -RSpec.describe Import::UserMapping::BypassConfirmationAuthorizer, feature_category: :importers do +RSpec.describe Import::UserMapping::AdminBypassAuthorizer, feature_category: :importers do subject(:authorizer) { described_class.new(reassigning_user) } - describe '#allow_mapping_to_inactive_users?' do + describe '#allowed?' do let_it_be(:reassigning_user) { create(:user, :admin) } - subject(:allow_mapping_to_inactive_users) { authorizer.allow_mapping_to_inactive_users? } + subject(:allowed) { authorizer.allowed? } before do stub_application_setting(allow_bypass_placeholder_confirmation: true) @@ -16,7 +16,7 @@ end it 'returns true for admins with bypass application setting and impersonation enabled', :enable_admin_mode do - expect(allow_mapping_to_inactive_users).to be(true) + expect(allowed).to be(true) end context 'when the importer_user_mapping_allow_bypass_of_confirmation flag is disabled', :enable_admin_mode do diff --git a/spec/models/import/source_user_spec.rb b/spec/models/import/source_user_spec.rb index aabcb52ee0c048953e854d7784b0d864beb46e94..555c894b04f898f41203551f2e747ea07ff1f18b 100644 --- a/spec/models/import/source_user_spec.rb +++ b/spec/models/import/source_user_spec.rb @@ -214,6 +214,42 @@ expect { source_user.cancel_reassignment }.to change { source_user.reassignment_token }.to(nil) end end + + context 'when switching to reassignment_in_progress without reassigned to user approval' do + let_it_be(:reassign_to_user) { create(:user) } + let_it_be(:reassigned_by_user) { create(:user, :admin) } + + subject(:source_user) { create(:import_source_user, :pending_reassignment) } + + before do + source_user.reassign_to_user = reassign_to_user + source_user.reassigned_by_user = reassigned_by_user + end + + context 'and admin bypass placeholder user confirmation is allowed' do + before do + expect_next_instance_of(Import::UserMapping::AdminBypassAuthorizer, reassigned_by_user) do |authorizer| + allow(authorizer).to receive(:allowed?).and_return(true) + end + end + + it 'allows the transition' do + expect(source_user.reassign_without_confirmation).to be(true) + end + end + + context 'and admins bypass placeholder user confirmation is not allowed' do + before do + expect_next_instance_of(Import::UserMapping::AdminBypassAuthorizer, reassigned_by_user) do |authorizer| + allow(authorizer).to receive(:allowed?).and_return(false) + end + end + + it 'does not allow the transition' do + expect(source_user.reassign_without_confirmation).to be(false) + end + end + end end describe '.find_source_user' do diff --git a/spec/services/import/source_users/reassign_service_spec.rb b/spec/services/import/source_users/reassign_service_spec.rb index e03452ebc03d94c551e3930a025509d8626859a1..5dc6245dc0e99a4b5262c39b7aec85117542c9c7 100644 --- a/spec/services/import/source_users/reassign_service_spec.rb +++ b/spec/services/import/source_users/reassign_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Import::SourceUsers::ReassignService, feature_category: :importers do - let(:user) { create(:user) } + let_it_be(:user) { create(:user) } let(:import_source_user) { create(:import_source_user) } let(:current_user) { user } let(:assignee_user) { create(:user) } @@ -40,19 +40,46 @@ end end - context 'when the reassigned by and reassigning user are valid' do - it_behaves_like 'a success response' + shared_examples 'a success response that bypasses user confirmation' do + it 'returns success', :aggregate_failures do + expect(Import::ReassignPlaceholderUserRecordsWorker).to receive(:perform_async).with(import_source_user.id) + expect { result } + .to trigger_internal_events('reassign_placeholder_user_without_confirmation') + .with( + namespace: import_source_user.namespace, + user: current_user, + additional_properties: { + label: Gitlab::GlobalAnonymousId.user_id(import_source_user.placeholder_user), + property: Gitlab::GlobalAnonymousId.user_id(assignee_user), + import_type: import_source_user.import_type, + reassign_to_user_state: assignee_user.state + } + ) + + expect(result).to be_success + expect(result.payload.reload).to eq(import_source_user) + expect(result.payload.reassign_to_user).to eq(assignee_user) + expect(result.payload.reassigned_by_user).to eq(current_user) + expect(result.payload.reassignment_in_progress?).to eq(true) + end end shared_examples 'an error response' do |desc, error:| - it "returns #{desc} error" do + it "returns #{desc} error", :aggregate_failures do expect(Notify).not_to receive(:import_source_user_reassign) + expect(Import::ReassignPlaceholderUserRecordsWorker).not_to receive(:perform_async) + + expect { result }.not_to change { import_source_user.status } expect(result).to be_error expect(result.message).to eq(error) end end + context 'when the reassigned by and reassigning user are valid' do + it_behaves_like 'a success response' + end + context 'when current user does not have permission' do let(:current_user) { create(:user) } @@ -84,7 +111,21 @@ error: s_('UserMapping|You can assign active users with regular or auditor access only.') end - context 'when assignee user is not active' do + context 'when assignee user is blocked' do + let(:assignee_user) { create(:user, :blocked) } + + it_behaves_like 'an error response', 'invalid assignee', + error: s_('UserMapping|You can assign active users with regular or auditor access only.') + end + + context 'when assignee user is banned' do + let(:assignee_user) { create(:user, :banned) } + + it_behaves_like 'an error response', 'invalid assignee', + error: s_('UserMapping|You can assign active users with regular or auditor access only.') + end + + context 'when assignee user is deactivated' do let(:assignee_user) { create(:user, :deactivated) } it_behaves_like 'an error response', 'invalid assignee', @@ -116,11 +157,18 @@ it_behaves_like 'a success response' end - context 'and mapping to inactive users is allowed' do + context 'and the current user is an admin with bypass placeholder confirmation enabled', :enable_admin_mode do + let_it_be(:current_user) { create(:user, :admin) } + before do - expect_next_instance_of(Import::UserMapping::BypassConfirmationAuthorizer, current_user) do |authorizer| - allow(authorizer).to receive(:allow_mapping_to_inactive_users?).and_return(true) - end + stub_application_setting(allow_bypass_placeholder_confirmation: true) + stub_config_setting(impersonation_enabled: true) + end + + context 'and the assignee user is an admin' do + let(:assignee_user) { create(:user, :admin) } + + it_behaves_like 'a success response that bypasses user confirmation' end context 'and assignee user is not a human' do @@ -132,37 +180,58 @@ end end - context 'when mapping to inactive users is allowed' do + context 'when the current user is an admin with bypass placeholder confirmation enabled', :enable_admin_mode do + let_it_be(:current_user) { create(:user, :admin) } + before do - expect_next_instance_of(Import::UserMapping::BypassConfirmationAuthorizer, current_user) do |authorizer| - allow(authorizer).to receive(:allow_mapping_to_inactive_users?).and_return(true) - end + stub_application_setting(allow_bypass_placeholder_confirmation: true) + stub_config_setting(impersonation_enabled: true) end - context 'and the assignee user is an admin' do - let(:assignee_user) { create(:user, :admin) } - - it_behaves_like 'an error response', 'invalid assignee', - error: s_('UserMapping|You can assign users with regular or auditor access only.') + context 'when the assignee user is an active human user' do + it_behaves_like 'a success response that bypasses user confirmation' end context 'and the assignee user is blocked' do let(:assignee_user) { create(:user, :blocked) } - it_behaves_like 'a success response' + it_behaves_like 'a success response that bypasses user confirmation' end context 'and the assignee user is banned' do let(:assignee_user) { create(:user, :banned) } - it_behaves_like 'a success response' + it_behaves_like 'a success response that bypasses user confirmation' end context 'and the assignee user is deactivated' do let(:assignee_user) { create(:user, :deactivated) } - it_behaves_like 'a success response' + it_behaves_like 'a success response that bypasses user confirmation' + end + + context 'and the assignee user is an admin' do + let(:assignee_user) { create(:user, :admin) } + + it_behaves_like 'an error response', 'invalid assignee', + error: s_('UserMapping|You can assign users with regular or auditor access only.') end + + context 'and the assignee user is not human' do + let(:assignee_user) { create(:user, :bot) } + + it_behaves_like 'an error response', 'invalid assignee', + error: s_('UserMapping|You can assign users with regular or auditor access only.') + end + end + + context 'when the top level namespace is a personal namespace' do + let(:personal_import_source_user) { create(:import_source_user, :user_type_namespace) } + let(:current_user) { personal_import_source_user.namespace.owner } + let(:service) { described_class.new(personal_import_source_user, assignee_user, current_user: current_user) } + + it_behaves_like 'an error response', 'invalid namespace', + error: s_("UserMapping|You cannot reassign user contributions of imports to a personal namespace.") end context 'when an error occurs' do @@ -190,16 +259,4 @@ end end end - - context 'when the top level namespace is a personal namespace' do - let(:import_source_user) { create(:import_source_user, :user_type_namespace) } - let(:user) { import_source_user.namespace.owner } - let(:error) { 'You cannot reassign user contributions of imports to a personal namespace.' } - - it 'returns an error' do - expect(Notify).not_to receive(:import_source_user_reassign) - expect(result).to be_error - expect(result.message).to eq(error) - end - end end