2025-08-20 16:30:26 +00:00 · 2025-05-12 00:11:19 +00:00
parent e7123234a7
commit a2609cc2ce
9 changed files with 201 additions and 77 deletions
--- a/app/helpers/groups/group_members_helper.rb
+++ b/app/helpers/groups/group_members_helper.rb
@ -31,7 +31,7 @@ module Groups::GroupMembersHelper
      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 @@ module Groups::GroupMembersHelper
      { 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')
--- a/app/models/import/source_user.rb
+++ b/app/models/import/source_user.rb
@ -78,6 +78,10 @@ module Import
        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 @@ module Import

      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
--- a/app/services/import/source_users/reassign_service.rb
+++ b/app/services/import/source_users/reassign_service.rb
@ -29,15 +29,20 @@ module Import

        return error_invalid_status if invalid_status

-        if reassign_successful
+        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
+
+        if admin_bypass_confirmation?
+          Import::ReassignPlaceholderUserRecordsWorker.perform_async(import_source_user.id)
+          track_reassignment_event('reassign_placeholder_user_without_confirmation')
+        else
          send_user_reassign_email
          track_reassignment_event('propose_placeholder_user_reassignment')
-
-          ServiceResponse.success(payload: import_source_user)
-        else
-          track_reassignment_event('fail_placeholder_user_reassignment')
-          ServiceResponse.error(payload: import_source_user, message: import_source_user.errors.full_messages)
        end
+
+        ServiceResponse.success(payload: import_source_user)
      end

      private
@ -47,6 +52,9 @@ module Import
      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 @@ module Import
      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 @@ module Import
      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
--- a/config/events/reassign_placeholder_user_without_confirmation.yml
+++ 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
--- a/lib/import/user_mapping/bypass_confirmation_authorizer.rb
+++ b/lib/import/user_mapping/bypass_confirmation_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 @@ module Import
          reassigning_user.can_admin_all_resources? &&
          Gitlab.config.gitlab.impersonation_enabled
      end
+
+      private
+
+      attr_reader :reassigning_user
    end
  end
 end
--- a/spec/helpers/groups/group_members_helper_spec.rb
+++ b/spec/helpers/groups/group_members_helper_spec.rb
@ -187,8 +187,8 @@ RSpec.describe Groups::GroupMembersHelper, feature_category: :groups_and_project

    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 @@ RSpec.describe Groups::GroupMembersHelper, feature_category: :groups_and_project

    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

--- a/spec/lib/import/user_mapping/bypass_confirmation_authorizer_spec.rb
+++ b/spec/lib/import/user_mapping/bypass_confirmation_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 @@ RSpec.describe Import::UserMapping::BypassConfirmationAuthorizer, feature_catego
    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
--- a/spec/models/import/source_user_spec.rb
+++ b/spec/models/import/source_user_spec.rb
@ -214,6 +214,42 @@ RSpec.describe Import::SourceUser, type: :model, feature_category: :importers do
        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
--- 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 @@ RSpec.describe Import::SourceUsers::ReassignService, feature_category: :importer
      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 @@ RSpec.describe Import::SourceUsers::ReassignService, feature_category: :importer
        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 @@ RSpec.describe Import::SourceUsers::ReassignService, feature_category: :importer
        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,11 +180,34 @@ RSpec.describe Import::SourceUsers::ReassignService, feature_category: :importer
      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 '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 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 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 that bypasses user confirmation'
      end

      context 'and the assignee user is an admin' do
@ -146,23 +217,21 @@ RSpec.describe Import::SourceUsers::ReassignService, feature_category: :importer
          error: s_('UserMapping|You can assign users with regular or auditor access only.')
      end

-      context 'and the assignee user is blocked' do
-        let(:assignee_user) { create(:user, :blocked) }
+      context 'and the assignee user is not human' do
+        let(:assignee_user) { create(:user, :bot) }

-        it_behaves_like 'a success response'
+        it_behaves_like 'an error response', 'invalid assignee',
+          error: s_('UserMapping|You can assign users with regular or auditor access only.')
      end
+    end

-      context 'and the assignee user is banned' do
-        let(:assignee_user) { create(:user, :banned) }
+    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 'a success response'
-      end
-
-      context 'and the assignee user is deactivated' do
-        let(:assignee_user) { create(:user, :deactivated) }
-
-        it_behaves_like 'a success response'
-      end
+      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 @@ RSpec.describe Import::SourceUsers::ReassignService, feature_category: :importer
      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