Prevent multiple project membership for security policy bot users
Users of type security policy bot have additional permissions as a project member. As a guest they can:
This is okay because security policy bot users can only be created by security policies. However, it creates a hidden risk when a security policy bot user is invited as a guest to a different project. It will not be obvious that the bot user has more than guest permissions.
Implementation plan
We have to re-think the concept of the bot project relationship. Long term, it would be better to create a new access level instead of using guest
access with additional permissions. With the new access level we can add a validation to Member
to ensure uniqueness of user_id
if the access level is security_policy_bot
.
For the short term security fix, we can add a check to the Members::CreatorService:
Suggested diff
diff --git a/ee/app/services/ee/members/creator_service.rb b/ee/app/services/ee/members/creator_service.rb
index 2276964b68c2..75cd4be9a7de 100644
--- a/ee/app/services/ee/members/creator_service.rb
+++ b/ee/app/services/ee/members/creator_service.rb
@@ -19,6 +19,15 @@ def after_commit_tasks
finish_onboarding_user
end
+ override :commit_member
+ def commit_member
+ if security_policy_bot_is_member_of_other_project?
+ member.errors.add(:base, 'security policy bot users cannot be added to other projects')
+ else
+ super
+ end
+ end
+
def finish_onboarding_user
return unless ::Onboarding.user_onboarding_in_progress?(member.user)
return unless finished_welcome_step?
@@ -29,6 +38,12 @@ def finish_onboarding_user
def finished_welcome_step?
member.user.role?
end
+
+ def security_policy_bot_is_member_of_other_project?
+ return false unless member.user.security_policy_bot?
+
+ ::Member.exists?(user_id: member.user.id)
+ end
end
end
end
diff --git a/ee/spec/services/ee/members/creator_service_spec.rb b/ee/spec/services/ee/members/creator_service_spec.rb
index 9fb029629b1e..53d9c2ad8d92 100644
--- a/ee/spec/services/ee/members/creator_service_spec.rb
+++ b/ee/spec/services/ee/members/creator_service_spec.rb
@@ -55,5 +55,29 @@
end
end
end
+
+ context 'when user is a security_policy_bot' do
+ let_it_be(:user) { create(:user, :security_policy_bot) }
+ let_it_be(:project) { create(:project) }
+
+ subject { described_class.add_member(project, user, :guest) }
+
+ it 'adds a member' do
+ expect{ subject }.to change { Member.count }.by(1)
+ end
+
+ context 'when the user is already a member of another project' do
+ let_it_be(:other_project) { create(:project) }
+ let_it_be(:membership) { create(:project_member, :guest, source: other_project, user: user) }
+
+ it 'does not add a member' do
+ expect{ subject }.not_to change { Member.count }
+ end
+
+ it 'adds an error message to the member' do
+ expect(subject.errors.messages).to include(base: ['security policy bot users cannot be added to other projects'])
+ end
+ end
+ end
end
end