From e3970a7de431f76f223a486194fd203928150adf Mon Sep 17 00:00:00 2001
From: Keeyan Nejad <knejad@gitlab.com>
Date: Wed, 9 Oct 2024 09:24:31 +0000
Subject: [PATCH] Wrap source user reassignments in a lock
This prevents placeholder contributions being remapped to the wrong user
after a reassignment has already been accepted.
Changelog: fixed
---
.../import/source_users_controller.rb | 14 ++++++----
app/models/import/source_user.rb | 10 +++++++
.../accept_reassignment_service.rb | 24 ++++++++++++++---
.../cancel_reassignment_service.rb | 16 +++++++++--
.../import/source_users/reassign_service.rb | 16 +++++++++--
.../reject_reassignment_service.rb | 25 ++++++++++++-----
app/views/import/source_users/show.html.haml | 4 +--
.../import_source_user_reassign.html.haml | 2 +-
.../import_source_user_reassign.text.erb | 2 +-
config/routes/import.rb | 2 +-
...assignment_token_to_import_source_users.rb | 16 +++++++++++
db/schema_migrations/20240930133006 | 1 +
db/structure.sql | 4 +++
spec/factories/import_source_users.rb | 1 +
spec/mailers/emails/imports_spec.rb | 6 +++--
spec/models/import/source_user_spec.rb | 25 +++++++++++++++--
.../import/source_users_controller_spec.rb | 22 ++++++++-------
...n_placeholder_user_records_service_spec.rb | 2 +-
.../accept_reassignment_service_spec.rb | 17 +++++++++++-
.../reject_reassignment_service_spec.rb | 27 ++++++++++++++++---
20 files changed, 193 insertions(+), 43 deletions(-)
create mode 100644 db/migrate/20240930133006_add_reassignment_token_to_import_source_users.rb
create mode 100644 db/schema_migrations/20240930133006
diff --git a/app/controllers/import/source_users_controller.rb b/app/controllers/import/source_users_controller.rb
index 5972511dd5f81..75989c38a2571 100644
--- a/app/controllers/import/source_users_controller.rb
+++ b/app/controllers/import/source_users_controller.rb
@@ -10,7 +10,9 @@ class SourceUsersController < ApplicationController
feature_category :importers
def accept
- result = ::Import::SourceUsers::AcceptReassignmentService.new(source_user, current_user: current_user).execute
+ result = ::Import::SourceUsers::AcceptReassignmentService.new(
+ source_user, current_user: current_user, reassignment_token: params[:reassignment_token]
+ ).execute
if result.success?
flash[:raw] = banner('accept_invite')
@@ -21,7 +23,9 @@ def accept
end
def decline
- result = ::Import::SourceUsers::RejectReassignmentService.new(source_user, current_user: current_user).execute
+ result = ::Import::SourceUsers::RejectReassignmentService.new(
+ source_user, current_user: current_user, reassignment_token: params[:reassignment_token]
+ ).execute
if result.success?
flash[:raw] = banner('reject_invite')
@@ -36,7 +40,7 @@ def show; end
private
def check_source_user_valid!
- return if source_user.awaiting_approval? && current_user_matches_invite?
+ return if source_user&.awaiting_approval? && current_user_matches_invite?
flash[:raw] = banner('invalid_invite')
redirect_to(root_path)
@@ -47,12 +51,12 @@ def current_user_matches_invite?
end
def source_user
- Import::SourceUser.find(params[:id])
+ Import::SourceUser.find_by_reassignment_token(params[:reassignment_token])
end
strong_memoize_attr :source_user
def check_feature_flag!
- not_found unless Feature.enabled?(:importer_user_mapping, source_user.reassigned_by_user)
+ not_found unless source_user.nil? || Feature.enabled?(:importer_user_mapping, source_user.reassigned_by_user)
end
def banner(partial)
diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb
index 5c34366882707..0fdbf216aa288 100644
--- a/app/models/import/source_user.rb
+++ b/app/models/import/source_user.rb
@@ -22,6 +22,8 @@ class SourceUser < ApplicationRecord
validates :namespace_id, :import_type, :source_hostname, :source_user_identifier, :status, presence: true
validates :source_user_identifier, uniqueness: { scope: [:namespace_id, :source_hostname, :import_type] }
validates :placeholder_user_id, presence: true, unless: :completed?
+ validates :reassignment_token, absence: true, unless: :awaiting_approval?
+ validates :reassignment_token, length: { is: 32 }, if: :awaiting_approval?
validates :reassign_to_user_id, presence: true, if: -> {
awaiting_approval? || reassignment_in_progress? || completed?
}
@@ -61,6 +63,14 @@ class SourceUser < ApplicationRecord
state status_name, value: value
end
+ before_transition awaiting_approval: any do |source_user|
+ source_user.reassignment_token = nil
+ end
+
+ before_transition any => :awaiting_approval do |source_user|
+ source_user.reassignment_token = SecureRandom.hex
+ end
+
event :reassign do
transition REASSIGNABLE_STATUSES => :awaiting_approval
end
diff --git a/app/services/import/source_users/accept_reassignment_service.rb b/app/services/import/source_users/accept_reassignment_service.rb
index f0bbab94c3540..b6c8810f9a101 100644
--- a/app/services/import/source_users/accept_reassignment_service.rb
+++ b/app/services/import/source_users/accept_reassignment_service.rb
@@ -3,15 +3,25 @@
module Import
module SourceUsers
class AcceptReassignmentService < BaseService
- def initialize(import_source_user, current_user:)
+ def initialize(import_source_user, current_user:, reassignment_token:)
@import_source_user = import_source_user
@current_user = current_user
+ @reassignment_token = reassignment_token
end
def execute
- return error_invalid_permissions unless current_user_matches_reassign_to_user
+ invalid_permissions = false
+ accept_successful = false
- if import_source_user.accept
+ import_source_user.with_lock do
+ next invalid_permissions = true unless current_user_matches_reassign_to_user? && reassignment_token_is_valid?
+
+ accept_successful = import_source_user.accept
+ end
+
+ return error_invalid_permissions if invalid_permissions
+
+ if accept_successful
Import::ReassignPlaceholderUserRecordsWorker.perform_async(import_source_user.id)
ServiceResponse.success(payload: import_source_user)
else
@@ -21,11 +31,17 @@ def execute
private
- def current_user_matches_reassign_to_user
+ attr_reader :reassignment_token
+
+ def current_user_matches_reassign_to_user?
return false if current_user.nil?
current_user.id == import_source_user.reassign_to_user_id
end
+
+ def reassignment_token_is_valid?
+ reassignment_token == import_source_user.reassignment_token
+ end
end
end
end
diff --git a/app/services/import/source_users/cancel_reassignment_service.rb b/app/services/import/source_users/cancel_reassignment_service.rb
index 65069d297428a..b3ee14fee7db7 100644
--- a/app/services/import/source_users/cancel_reassignment_service.rb
+++ b/app/services/import/source_users/cancel_reassignment_service.rb
@@ -10,9 +10,21 @@ def initialize(import_source_user, current_user:)
def execute
return error_invalid_permissions unless current_user.can?(:admin_import_source_user, import_source_user)
- return error_invalid_status unless import_source_user.cancelable_status?
- if cancel_reassignment
+ invalid_status = false
+ cancel_successful = false
+
+ import_source_user.with_lock do
+ if import_source_user.cancelable_status?
+ cancel_successful = cancel_reassignment
+ else
+ invalid_status = true
+ end
+ end
+
+ return error_invalid_status if invalid_status
+
+ if cancel_successful
ServiceResponse.success(payload: import_source_user)
else
ServiceResponse.error(payload: import_source_user, message: import_source_user.errors.full_messages)
diff --git a/app/services/import/source_users/reassign_service.rb b/app/services/import/source_users/reassign_service.rb
index 9a6020119c5d2..2172f3e0fec12 100644
--- a/app/services/import/source_users/reassign_service.rb
+++ b/app/services/import/source_users/reassign_service.rb
@@ -11,10 +11,22 @@ def initialize(import_source_user, assignee_user, current_user:)
def execute
return error_invalid_permissions unless current_user.can?(:admin_import_source_user, import_source_user)
- return error_invalid_status unless import_source_user.reassignable_status?
return error_invalid_assignee unless valid_assignee?(assignee_user)
- if reassign_user
+ invalid_status = false
+ reassign_successful = false
+
+ import_source_user.with_lock do
+ if import_source_user.reassignable_status?
+ reassign_successful = reassign_user
+ else
+ invalid_status = true
+ end
+ end
+
+ return error_invalid_status if invalid_status
+
+ if reassign_successful
send_user_reassign_email
ServiceResponse.success(payload: import_source_user)
diff --git a/app/services/import/source_users/reject_reassignment_service.rb b/app/services/import/source_users/reject_reassignment_service.rb
index ad99163ad6d9f..212354bb7feaf 100644
--- a/app/services/import/source_users/reject_reassignment_service.rb
+++ b/app/services/import/source_users/reject_reassignment_service.rb
@@ -3,16 +3,27 @@
module Import
module SourceUsers
class RejectReassignmentService < BaseService
- def initialize(import_source_user, current_user:)
+ def initialize(import_source_user, current_user:, reassignment_token:)
@import_source_user = import_source_user
@current_user = current_user
+ @reassignment_token = reassignment_token
end
def execute
- return error_invalid_permissions unless current_user_matches_reassign_to_user
return error_invalid_status unless import_source_user.awaiting_approval?
- if reject
+ invalid_permissions = false
+ reject_successful = false
+
+ import_source_user.with_lock do
+ next invalid_permissions = true unless current_user_matches_reassign_to_user? && reassignment_token_is_valid?
+
+ reject_successful = import_source_user.reject
+ end
+
+ return error_invalid_permissions if invalid_permissions
+
+ if reject_successful
send_user_reassign_rejected_email
ServiceResponse.success(payload: import_source_user)
@@ -27,14 +38,16 @@ def send_user_reassign_rejected_email
private
- def current_user_matches_reassign_to_user
+ attr_reader :reassignment_token
+
+ def current_user_matches_reassign_to_user?
return false if current_user.nil?
current_user.id == import_source_user.reassign_to_user_id
end
- def reject
- import_source_user.reject
+ def reassignment_token_is_valid?
+ reassignment_token == import_source_user.reassignment_token
end
end
end
diff --git a/app/views/import/source_users/show.html.haml b/app/views/import/source_users/show.html.haml
index 56e165e35540b..1fec771ec7514 100644
--- a/app/views/import/source_users/show.html.haml
+++ b/app/views/import/source_users/show.html.haml
@@ -48,7 +48,7 @@
- c.with_footer do
.gl-flex.gl-gap-3
- = render Pajamas::ButtonComponent.new(variant: :danger, method: :post, href: accept_import_source_user_path(@source_user)) do
+ = render Pajamas::ButtonComponent.new(variant: :danger, method: :post, href: accept_import_source_user_path(@source_user.reassignment_token)) do
= s_('UserMapping|Approve reassignment')
- = render Pajamas::ButtonComponent.new(method: :post, href: decline_import_source_user_path(@source_user)) do
+ = render Pajamas::ButtonComponent.new(method: :post, href: decline_import_source_user_path(@source_user.reassignment_token)) do
= s_('UserMapping|Reject')
diff --git a/app/views/notify/import_source_user_reassign.html.haml b/app/views/notify/import_source_user_reassign.html.haml
index 362b64e9dc301..72946cabf408d 100644
--- a/app/views/notify/import_source_user_reassign.html.haml
+++ b/app/views/notify/import_source_user_reassign.html.haml
@@ -27,7 +27,7 @@
destination_group: destination_group)
%p{ style: text_style }
- = link_to import_source_user_url(@source_user), target: '_blank', rel: 'noopener noreferrer' do
+ = link_to import_source_user_url(@source_user.reassignment_token), target: '_blank', rel: 'noopener noreferrer' do
%button{ type: 'button', style: button_style }
= s_('UserMapping|Review reassignment details')
diff --git a/app/views/notify/import_source_user_reassign.text.erb b/app/views/notify/import_source_user_reassign.text.erb
index 9ba3af278ac46..2d53d5399f214 100644
--- a/app/views/notify/import_source_user_reassign.text.erb
+++ b/app/views/notify/import_source_user_reassign.text.erb
@@ -16,7 +16,7 @@
source_hostname: source_hostname,
destination_group: destination_group } %>
-<%= s_('UserMapping|Review reassignment details') %>: <%= import_source_user_url(@source_user) %>
+<%= s_('UserMapping|Review reassignment details') %>: <%= import_source_user_url(@source_user.reassignment_token) %>
<%= s_('UserMapping|Import details:') %>
<%= safe_format(s_('UserMapping|Imported from: %{source_hostname}'), source_hostname: source_hostname) %>
diff --git a/config/routes/import.rb b/config/routes/import.rb
index 5962a68c64a8b..f0e4c376ba2e7 100644
--- a/config/routes/import.rb
+++ b/config/routes/import.rb
@@ -89,7 +89,7 @@
post :upload
end
- resources :source_users, only: [] do
+ resources :source_users, param: :reassignment_token, only: [] do
member do
get :show
post :accept
diff --git a/db/migrate/20240930133006_add_reassignment_token_to_import_source_users.rb b/db/migrate/20240930133006_add_reassignment_token_to_import_source_users.rb
new file mode 100644
index 0000000000000..7d0fe8fd37648
--- /dev/null
+++ b/db/migrate/20240930133006_add_reassignment_token_to_import_source_users.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class AddReassignmentTokenToImportSourceUsers < Gitlab::Database::Migration[2.2]
+ disable_ddl_transaction!
+ milestone '17.5'
+
+ def up
+ add_column :import_source_users, :reassignment_token, :text
+ add_concurrent_index :import_source_users, :reassignment_token, unique: true
+ add_text_limit :import_source_users, :reassignment_token, 32
+ end
+
+ def down
+ remove_column :import_source_users, :reassignment_token, :text
+ end
+end
diff --git a/db/schema_migrations/20240930133006 b/db/schema_migrations/20240930133006
new file mode 100644
index 0000000000000..216067307a987
--- /dev/null
+++ b/db/schema_migrations/20240930133006
@@ -0,0 +1 @@
+1a74228cbfeb2795e3ea6c544cfedee3b3f809e86bd379cd89bfec748be17ded
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index b5f71e89968c0..a637245151086 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -12247,11 +12247,13 @@ CREATE TABLE import_source_users (
import_type text NOT NULL,
reassigned_by_user_id bigint,
reassignment_error text,
+ reassignment_token text,
CONSTRAINT check_05708218cd CHECK ((char_length(reassignment_error) <= 255)),
CONSTRAINT check_0d7295a307 CHECK ((char_length(import_type) <= 255)),
CONSTRAINT check_199c28ec54 CHECK ((char_length(source_username) <= 255)),
CONSTRAINT check_562655155f CHECK ((char_length(source_name) <= 255)),
CONSTRAINT check_cc9d4093b5 CHECK ((char_length(source_user_identifier) <= 255)),
+ CONSTRAINT check_cd2edb9334 CHECK ((char_length(reassignment_token) <= 32)),
CONSTRAINT check_e2039840c5 CHECK ((char_length(source_hostname) <= 255))
);
@@ -29248,6 +29250,8 @@ CREATE INDEX index_import_source_users_on_placeholder_user_id ON import_source_u
CREATE INDEX index_import_source_users_on_reassigned_by_user_id ON import_source_users USING btree (reassigned_by_user_id);
+CREATE UNIQUE INDEX index_import_source_users_on_reassignment_token ON import_source_users USING btree (reassignment_token);
+
CREATE INDEX index_imported_projects_on_import_type_creator_id_created_at ON projects USING btree (import_type, creator_id, created_at) WHERE (import_type IS NOT NULL);
CREATE INDEX index_imported_projects_on_import_type_id ON projects USING btree (import_type, id) WHERE (import_type IS NOT NULL);
diff --git a/spec/factories/import_source_users.rb b/spec/factories/import_source_users.rb
index 1575ebd7f341e..6fbe9df4ddbf9 100644
--- a/spec/factories/import_source_users.rb
+++ b/spec/factories/import_source_users.rb
@@ -24,6 +24,7 @@
trait :awaiting_approval do
with_reassign_to_user
+ reassignment_token { SecureRandom.hex }
status { 1 }
end
diff --git a/spec/mailers/emails/imports_spec.rb b/spec/mailers/emails/imports_spec.rb
index b5b1600691230..2ad19d5f2260d 100644
--- a/spec/mailers/emails/imports_spec.rb
+++ b/spec/mailers/emails/imports_spec.rb
@@ -91,7 +91,9 @@
let(:user) { build_stubbed(:user) }
let(:group) { build_stubbed(:group) }
let(:source_user) do
- build_stubbed(:import_source_user, :with_reassigned_by_user, namespace: group, reassign_to_user: user)
+ build_stubbed(
+ :import_source_user, :awaiting_approval, :with_reassigned_by_user, namespace: group, reassign_to_user: user
+ )
end
subject { Notify.import_source_user_reassign('user_id') }
@@ -109,7 +111,7 @@
is_expected.to have_content(
"Reassigned by: #{source_user.reassigned_by_user.name} (@#{source_user.reassigned_by_user.username})"
)
- is_expected.to have_body_text(import_source_user_url(source_user))
+ is_expected.to have_body_text(import_source_user_url(source_user.reassignment_token))
end
it_behaves_like 'appearance header and footer enabled'
diff --git a/spec/models/import/source_user_spec.rb b/spec/models/import/source_user_spec.rb
index 069939428db2a..f0cdbf1951f0a 100644
--- a/spec/models/import/source_user_spec.rb
+++ b/spec/models/import/source_user_spec.rb
@@ -16,7 +16,7 @@
it { is_expected.to validate_presence_of(:source_hostname) }
it { is_expected.to validate_presence_of(:source_user_identifier) }
it { is_expected.to validate_presence_of(:status) }
-
+ it { is_expected.to validate_absence_of(:reassignment_token) }
it { is_expected.not_to validate_presence_of(:reassign_to_user_id) }
it 'validates source_hostname has port and scheme' do
@@ -52,6 +52,8 @@
subject { build(:import_source_user, :awaiting_approval) }
it { is_expected.to validate_presence_of(:reassign_to_user_id) }
+ it { is_expected.to validate_length_of(:reassignment_token).is_equal_to(32) }
+ it { is_expected.to validate_presence_of(:reassignment_token).with_message(/is the wrong length/) }
end
context 'when reassignment_in_progress' do
@@ -120,6 +122,22 @@
it 'begins in pending state' do
expect(described_class.new.pending_reassignment?).to eq(true)
end
+
+ context 'when switching to awaiting_approval' do
+ subject(:source_user) { create(:import_source_user, :pending_reassignment) }
+
+ it 'assigns a reassignment_token' do
+ expect { source_user.reassign }.to change { source_user.reassignment_token }.from(nil)
+ end
+ end
+
+ context 'when switching from awaiting_approval' do
+ subject(:source_user) { create(:import_source_user, :awaiting_approval) }
+
+ it 'removes the reassignment_token' do
+ expect { source_user.cancel_reassignment }.to change { source_user.reassignment_token }.to(nil)
+ end
+ end
end
describe '.find_source_user' do
@@ -194,7 +212,10 @@
let_it_be(:source_user_1) { create(:import_source_user, namespace: namespace, status: 4, source_name: 'd') }
let_it_be(:source_user_2) { create(:import_source_user, namespace: namespace, status: 3, source_name: 'c') }
let_it_be(:source_user_3) do
- create(:import_source_user, :with_reassign_to_user, namespace: namespace, status: 1, source_name: 'a')
+ create(
+ :import_source_user, :with_reassign_to_user,
+ namespace: namespace, status: 1, source_name: 'a', reassignment_token: SecureRandom.hex
+ )
end
let_it_be(:source_user_4) do
diff --git a/spec/requests/import/source_users_controller_spec.rb b/spec/requests/import/source_users_controller_spec.rb
index ea3915f14583a..cfdee1b2b0768 100644
--- a/spec/requests/import/source_users_controller_spec.rb
+++ b/spec/requests/import/source_users_controller_spec.rb
@@ -25,7 +25,7 @@
end
end
- shared_examples 'it requires awaiting approval status' do
+ shared_examples 'it notifies about unavailable reassignments' do
it 'shows error message' do
source_user.accept!
@@ -51,8 +51,10 @@
create(:import_source_user, :with_reassigned_by_user, :awaiting_approval)
end
+ let!(:reassignment_token) { source_user.reassignment_token }
+
describe 'POST /accept' do
- let(:path) { accept_import_source_user_path(source_user) }
+ let(:path) { accept_import_source_user_path(reassignment_token: reassignment_token) }
subject(:accept_invite) { post path }
@@ -77,7 +79,7 @@
end
it 'cannot be accepted twice' do
- allow(Import::SourceUser).to receive(:find).and_return(source_user)
+ allow(Import::SourceUser).to receive(:find_by_reassignment_token).and_return(source_user)
allow(source_user).to receive(:accept).and_return(false)
accept_invite
@@ -86,7 +88,7 @@
expect(flash[:alert]).to match(/The invitation could not be accepted/)
end
- it_behaves_like 'it requires awaiting approval status'
+ it_behaves_like 'it notifies about unavailable reassignments'
it_behaves_like 'it requires the user is the reassign to user'
end
@@ -95,7 +97,8 @@
end
describe 'POST /decline' do
- let(:path) { decline_import_source_user_path(source_user) }
+ let(:path) { decline_import_source_user_path(reassignment_token: reassignment_token) }
+
let(:message_delivery) { instance_double(ActionMailer::MessageDelivery) }
subject(:reject_invite) { post path }
@@ -117,7 +120,7 @@
end
it 'cannot be declined twice' do
- allow(Import::SourceUser).to receive(:find).and_return(source_user)
+ allow(Import::SourceUser).to receive(:find_by_reassignment_token).and_return(source_user)
allow(source_user).to receive(:reject).and_return(false)
reject_invite
@@ -126,7 +129,7 @@
expect(flash[:alert]).to match(/The invitation could not be declined/)
end
- it_behaves_like 'it requires awaiting approval status'
+ it_behaves_like 'it notifies about unavailable reassignments'
it_behaves_like 'it requires the user is the reassign to user'
end
@@ -135,7 +138,8 @@
end
describe 'GET /show' do
- let(:path) { import_source_user_path(source_user) }
+ let(:path) { import_source_user_path(reassignment_token: reassignment_token) }
+ let(:reassignment_token) { source_user.reassignment_token }
subject(:show_invite) { get path }
@@ -150,7 +154,7 @@
expect(response).to have_gitlab_http_status(:success)
end
- it_behaves_like 'it requires awaiting approval status'
+ it_behaves_like 'it notifies about unavailable reassignments'
it_behaves_like 'it requires the user is the reassign to user'
end
diff --git a/spec/services/import/reassign_placeholder_user_records_service_spec.rb b/spec/services/import/reassign_placeholder_user_records_service_spec.rb
index cace45aab4f95..be99e1c38aa80 100644
--- a/spec/services/import/reassign_placeholder_user_records_service_spec.rb
+++ b/spec/services/import/reassign_placeholder_user_records_service_spec.rb
@@ -467,7 +467,7 @@ def expect_skipped_membership_log(message, placeholder_membership, existing_memb
context 'when the source user is not in reassignment_in_progress status' do
before do
- source_user.update!(status: 1)
+ source_user.complete!
end
it 'does not reassign any contributions or create memberships' do
diff --git a/spec/services/import/source_users/accept_reassignment_service_spec.rb b/spec/services/import/source_users/accept_reassignment_service_spec.rb
index 74ee78c05706a..bb9a2b9b1c79f 100644
--- a/spec/services/import/source_users/accept_reassignment_service_spec.rb
+++ b/spec/services/import/source_users/accept_reassignment_service_spec.rb
@@ -4,8 +4,11 @@
RSpec.describe Import::SourceUsers::AcceptReassignmentService, feature_category: :importers do
let(:import_source_user) { create(:import_source_user, :awaiting_approval) }
+ let(:reassignment_token) { import_source_user.reassignment_token }
let(:current_user) { import_source_user.reassign_to_user }
- let(:service) { described_class.new(import_source_user, current_user: current_user) }
+ let(:service) do
+ described_class.new(import_source_user, current_user: current_user, reassignment_token: reassignment_token)
+ end
describe '#execute' do
it 'returns success' do
@@ -51,6 +54,18 @@
it_behaves_like 'current user does not have permission to accept reassignment'
end
+ context 'when passing the wrong reassignment_token' do
+ let(:reassignment_token) { '1234567890abcdef' }
+
+ it_behaves_like 'current user does not have permission to accept reassignment'
+ end
+
+ context 'when not passing a reassignment_token' do
+ let(:reassignment_token) { nil }
+
+ it_behaves_like 'current user does not have permission to accept reassignment'
+ end
+
context 'when the source user is not awaiting approval' do
let(:import_source_user) { create(:import_source_user, :reassignment_in_progress) }
diff --git a/spec/services/import/source_users/reject_reassignment_service_spec.rb b/spec/services/import/source_users/reject_reassignment_service_spec.rb
index 04d21f2359da5..1b8aba193980e 100644
--- a/spec/services/import/source_users/reject_reassignment_service_spec.rb
+++ b/spec/services/import/source_users/reject_reassignment_service_spec.rb
@@ -4,8 +4,11 @@
RSpec.describe Import::SourceUsers::RejectReassignmentService, feature_category: :importers do
let(:import_source_user) { create(:import_source_user, :awaiting_approval) }
+ let(:reassignment_token) { import_source_user.reassignment_token }
let(:current_user) { import_source_user.reassign_to_user }
- let(:service) { described_class.new(import_source_user, current_user: current_user) }
+ let(:service) do
+ described_class.new(import_source_user, current_user: current_user, reassignment_token: reassignment_token)
+ end
describe '#execute' do
let(:message_delivery) { instance_double(ActionMailer::MessageDelivery) }
@@ -25,9 +28,7 @@
expect(import_source_user.reload).to be_rejected
end
- context 'when current user does not have permission to reject' do
- let(:current_user) { create(:user) }
-
+ shared_examples 'current user does not have permission to reject reassignment' do
it 'returns error no permissions' do
result = service.execute
@@ -38,6 +39,24 @@
end
end
+ context 'when passing the wrong reassignment_token' do
+ let(:reassignment_token) { '1234567890abcdef' }
+
+ it_behaves_like 'current user does not have permission to reject reassignment'
+ end
+
+ context 'when not passing a reassignment_token' do
+ let(:reassignment_token) { nil }
+
+ it_behaves_like 'current user does not have permission to reject reassignment'
+ end
+
+ context 'when current user is not the assigned user' do
+ let(:current_user) { create(:user) }
+
+ it_behaves_like 'current user does not have permission to reject reassignment'
+ end
+
context 'when import source user does not have a rejectable status' do
let(:import_source_user) { create(:import_source_user, :reassignment_in_progress) }
--
GitLab