Make security policy bot relationship to the project explicit
Why are we doing this work
Currently, we create a security policy bot for projects with policies in order to run pipelines as a user with the least privilege. In case a group has a policy, a separate bot is created in each project so that each bot can only access the project itself and can't access other projects in the group.
To run the pipelines, we take the first project member of user_type: :security_policy_bot
. This is not explicit and can lead to unexpected behavior, for example if user invites multiple bots in one project.
We should make the bot assignment to the project more explicit and add a safeguard so that only one bot can be added to a project.
Relevant links
Non-functional requirements
-
Documentation: -
Feature flag: -
Performance: -
Testing:
Implementation Plan
Create a join table project_security_policy_bot_users
with project_id
and user_id
foreign keys and unique indexes. When creating the bot user, also write to the join table.
diff --git a/db/docs/project_security_policy_bot_users.yml b/db/docs/project_security_policy_bot_users.yml
new file mode 100644
index 000000000000..f84e7a8e5273
--- /dev/null
+++ b/db/docs/project_security_policy_bot_users.yml
@@ -0,0 +1,9 @@
+---
+table_name: project_security_policy_bot_users
+classes: []
+feature_categories:
+- security_policy_management
+description: Project policy bot users
+introduced_by_url: TODO
+milestone: '16.3'
+gitlab_schema: gitlab_main
diff --git a/db/migrate/20230721124354_create_project_security_policy_bot_users.rb b/db/migrate/20230721124354_create_project_security_policy_bot_users.rb
new file mode 100644
index 000000000000..73a3e0daa72b
--- /dev/null
+++ b/db/migrate/20230721124354_create_project_security_policy_bot_users.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class CreateProjectSecurityPolicyBotUsers < Gitlab::Database::Migration[2.1]
+ def change
+ create_table :project_security_policy_bot_users do |t|
+ t.references :project, foreign_key: true, null: false, index: { unique: true }
+ t.references :user, foreign_key: true, null: false, index: { unique: true }
+
+ t.timestamps null: false
+ end
+ end
+end
diff --git a/db/schema_migrations/20230721124354 b/db/schema_migrations/20230721124354
new file mode 100644
index 000000000000..78e7723e2cc0
--- /dev/null
+++ b/db/schema_migrations/20230721124354
@@ -0,0 +1 @@
+f5c70d56b8aef25b877ea326a1da727fc1462647eda3b8cf3f6d2a9f0f4e900a
\ No newline at end of file
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index 004e86f11df7..c34607372d39 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -150,7 +150,9 @@ def lock_for_confirmation!(id)
class_name: 'Geo::ProjectState'
has_many :compliance_standards_adherence, class_name: 'Projects::ComplianceStandards::Adherence'
- has_many :security_policy_bots, -> { security_policy_bot }, through: :project_members, source: :user
+
+ has_one :security_policy_bot_user, class_name: "Security::PolicyBotUser"
+ has_one :security_policy_bot, through: :security_policy_bot_user, source: :user
elastic_index_dependant_association :issues, on_change: :visibility_level
elastic_index_dependant_association :issues, on_change: :archived, depends_on_finished_migration: :add_archived_to_issues
@@ -1161,16 +1163,6 @@ def merge_train_for(target_branch)
MergeTrains::Train.new(self.id, target_branch)
end
- # Scan execution policies use a security_policy_bot to run pipelines.
- # `security_policy_bots` association is used to preload the bots for scheduled pipelines and to prevent N+1 in
- # `ee/app/workers/security/orchestration_policy_rule_schedule_worker.rb`
- # It is expected to have only one security_policy_bot per project.
- # A follow-up issue exists to make the relationship between the bot and the project more explicit:
- # https://gitlab.com/gitlab-org/gitlab/-/issues/418013
- def security_policy_bot
- security_policy_bots.take
- end
-
private
def lock_memberships_to_ldap_or_saml?
diff --git a/ee/app/models/security/policy_bot_user.rb b/ee/app/models/security/policy_bot_user.rb
new file mode 100644
index 000000000000..d1c835ef1788
--- /dev/null
+++ b/ee/app/models/security/policy_bot_user.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+module Security
+ class PolicyBotUser < ApplicationRecord
+ self.table_name = 'project_security_policy_bot_users'
+
+ belongs_to :project
+ belongs_to :user
+ end
+end
diff --git a/ee/app/services/security/orchestration/create_bot_service.rb b/ee/app/services/security/orchestration/create_bot_service.rb
index faf3382bfbca..abdb77bed959 100644
--- a/ee/app/services/security/orchestration/create_bot_service.rb
+++ b/ee/app/services/security/orchestration/create_bot_service.rb
@@ -22,6 +22,8 @@ def execute
).execute
project.add_guest(bot_user, current_user: current_user)
+
+ project.security_policy_bot_user.create!(user: bot_user)
end
end