From a84bd4a098eac53e59602974292982d6260c50dd Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Thu, 24 Jul 2025 16:58:16 +0200 Subject: [PATCH 1/5] Add approval_policy_merge_request_bypass_events to store the bypasses This change adds a new table to track the bypasses through the security policy with bypass_settings. EE: true Changelog: added --- ...val_policy_merge_request_bypass_events.yml | 14 ++++++ ...oval_policy_merge_request_bypass_events.rb | 24 ++++++++++ ..._policy_merge_request_bypass_events_fks.rb | 29 ++++++++++++ db/schema_migrations/20250723205109 | 1 + db/schema_migrations/20250725154259 | 1 + db/structure.sql | 46 +++++++++++++++++++ ee/app/models/ee/merge_request.rb | 2 + ...roval_policy_merge_request_bypass_event.rb | 12 +++++ ee/app/models/security/policy.rb | 6 ++- ...oval_policy_merge_request_bypass_events.rb | 11 +++++ ee/spec/models/merge_request_spec.rb | 1 + ..._policy_merge_request_bypass_event_spec.rb | 12 +++++ ee/spec/models/security/policy_spec.rb | 1 + spec/lib/gitlab/import_export/all_models.yml | 1 + 14 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 db/docs/approval_policy_merge_request_bypass_events.yml create mode 100644 db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb create mode 100644 db/post_migrate/20250725154259_add_approval_policy_merge_request_bypass_events_fks.rb create mode 100644 db/schema_migrations/20250723205109 create mode 100644 db/schema_migrations/20250725154259 create mode 100644 ee/app/models/security/approval_policy_merge_request_bypass_event.rb create mode 100644 ee/spec/factories/security/approval_policy_merge_request_bypass_events.rb create mode 100644 ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb diff --git a/db/docs/approval_policy_merge_request_bypass_events.yml b/db/docs/approval_policy_merge_request_bypass_events.yml new file mode 100644 index 00000000000000..915374adbeee16 --- /dev/null +++ b/db/docs/approval_policy_merge_request_bypass_events.yml @@ -0,0 +1,14 @@ + +--- +table_name: approval_policy_merge_request_bypass_events +classes: +- Security::ApprovalPolicyMergeRequestBypassEvent +feature_categories: +- security_policy_management +description: Stores events of bypasses of approval policies for merge requests. +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/199151 +milestone: '18.3' +gitlab_schema: gitlab_main_cell +sharding_key: + project_id: projects +table_size: small diff --git a/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb new file mode 100644 index 00000000000000..98c68eae494e4e --- /dev/null +++ b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class CreateApprovalPolicyMergeRequestBypassEvents < Gitlab::Database::Migration[2.3] + milestone '18.3' + + def change + create_table :approval_policy_merge_request_bypass_events do |t| + t.references :project, null: false, foreign_key: { on_delete: :cascade }, index: false + t.bigint :merge_request_id, null: false, + index: { name: 'index_approval_policy_merge_request_bypass_events_on_mr_id' } + t.bigint :security_policy_id, null: false, + index: { name: 'index_approval_policy_merge_request_bypass_events_on_policy_id' } + t.bigint :user_id, null: true, index: { name: 'index_approval_policy_merge_request_bypass_events_on_user_id' } + t.text :reason, null: false, limit: 1024 + + t.timestamps_with_timezone null: false + end + + add_index :approval_policy_merge_request_bypass_events, + [:project_id, :merge_request_id, :security_policy_id], + unique: true, + name: 'idx_approval_policy_mr_bypass_events_on_project_mr_policy' + end +end diff --git a/db/post_migrate/20250725154259_add_approval_policy_merge_request_bypass_events_fks.rb b/db/post_migrate/20250725154259_add_approval_policy_merge_request_bypass_events_fks.rb new file mode 100644 index 00000000000000..f04edb659b14a6 --- /dev/null +++ b/db/post_migrate/20250725154259_add_approval_policy_merge_request_bypass_events_fks.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class AddApprovalPolicyMergeRequestBypassEventsFks < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + milestone '18.3' + + def up + add_concurrent_foreign_key :approval_policy_merge_request_bypass_events, :merge_requests, + column: :merge_request_id, + on_delete: :cascade + add_concurrent_foreign_key :approval_policy_merge_request_bypass_events, :security_policies, + column: :security_policy_id, + on_delete: :cascade + add_concurrent_foreign_key :approval_policy_merge_request_bypass_events, :users, column: :user_id, + on_delete: :nullify + end + + def down + with_lock_retries do + remove_foreign_key :approval_policy_merge_request_bypass_events, column: :merge_request_id + end + with_lock_retries do + remove_foreign_key :approval_policy_merge_request_bypass_events, column: :security_policy_id + end + with_lock_retries do + remove_foreign_key :approval_policy_merge_request_bypass_events, column: :user_id + end + end +end diff --git a/db/schema_migrations/20250723205109 b/db/schema_migrations/20250723205109 new file mode 100644 index 00000000000000..6616ef75e68d6f --- /dev/null +++ b/db/schema_migrations/20250723205109 @@ -0,0 +1 @@ +c0f735594416c69457d4d55297c9c25b675118592e1fff534918a0d49d149d33 \ No newline at end of file diff --git a/db/schema_migrations/20250725154259 b/db/schema_migrations/20250725154259 new file mode 100644 index 00000000000000..96484725a44a17 --- /dev/null +++ b/db/schema_migrations/20250725154259 @@ -0,0 +1 @@ +33a85d33bdff307f4eecebafe77c258d2d2c8d3286c075c71bb302508b39496b \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index c0f91e4ef13b22..279d76b36cf9c0 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9950,6 +9950,27 @@ CREATE SEQUENCE approval_merge_request_rules_users_id_seq ALTER SEQUENCE approval_merge_request_rules_users_id_seq OWNED BY approval_merge_request_rules_users.id; +CREATE TABLE approval_policy_merge_request_bypass_events ( + id bigint NOT NULL, + project_id bigint NOT NULL, + merge_request_id bigint NOT NULL, + security_policy_id bigint NOT NULL, + user_id bigint, + reason text NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + CONSTRAINT check_dc373cb020 CHECK ((char_length(reason) <= 1024)) +); + +CREATE SEQUENCE approval_policy_merge_request_bypass_events_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE approval_policy_merge_request_bypass_events_id_seq OWNED BY approval_policy_merge_request_bypass_events.id; + CREATE TABLE approval_policy_rule_project_links ( id bigint NOT NULL, project_id bigint NOT NULL, @@ -27690,6 +27711,8 @@ ALTER TABLE ONLY approval_merge_request_rules_groups ALTER COLUMN id SET DEFAULT ALTER TABLE ONLY approval_merge_request_rules_users ALTER COLUMN id SET DEFAULT nextval('approval_merge_request_rules_users_id_seq'::regclass); +ALTER TABLE ONLY approval_policy_merge_request_bypass_events ALTER COLUMN id SET DEFAULT nextval('approval_policy_merge_request_bypass_events_id_seq'::regclass); + ALTER TABLE ONLY approval_policy_rule_project_links ALTER COLUMN id SET DEFAULT nextval('approval_policy_rule_project_links_id_seq'::regclass); ALTER TABLE ONLY approval_policy_rules ALTER COLUMN id SET DEFAULT nextval('approval_policy_rules_id_seq'::regclass); @@ -29804,6 +29827,9 @@ ALTER TABLE ONLY approval_merge_request_rules ALTER TABLE ONLY approval_merge_request_rules_users ADD CONSTRAINT approval_merge_request_rules_users_pkey PRIMARY KEY (id); +ALTER TABLE ONLY approval_policy_merge_request_bypass_events + ADD CONSTRAINT approval_policy_merge_request_bypass_events_pkey PRIMARY KEY (id); + ALTER TABLE ONLY approval_policy_rule_project_links ADD CONSTRAINT approval_policy_rule_project_links_pkey PRIMARY KEY (id); @@ -34080,6 +34106,8 @@ CREATE INDEX idx_approval_merge_request_rules_on_scan_result_policy_id ON approv CREATE INDEX idx_approval_mr_rules_on_config_id_and_id_and_updated_at ON approval_merge_request_rules USING btree (security_orchestration_policy_configuration_id, id, updated_at); +CREATE UNIQUE INDEX idx_approval_policy_mr_bypass_events_on_project_mr_policy ON approval_policy_merge_request_bypass_events USING btree (project_id, merge_request_id, security_policy_id); + CREATE INDEX idx_approval_policy_rule_project_links_on_project_id_and_id ON approval_policy_rule_project_links USING btree (project_id, id); CREATE INDEX idx_approval_policy_rules_security_policy_id_id ON approval_policy_rules USING btree (security_policy_id, id); @@ -34912,6 +34940,12 @@ CREATE INDEX index_approval_merge_request_rules_users_2 ON approval_merge_reques CREATE INDEX index_approval_mr_rules_on_project_id_policy_rule_id_and_id ON approval_merge_request_rules USING btree (security_orchestration_policy_configuration_id, approval_policy_rule_id, id); +CREATE INDEX index_approval_policy_merge_request_bypass_events_on_mr_id ON approval_policy_merge_request_bypass_events USING btree (merge_request_id); + +CREATE INDEX index_approval_policy_merge_request_bypass_events_on_policy_id ON approval_policy_merge_request_bypass_events USING btree (security_policy_id); + +CREATE INDEX index_approval_policy_merge_request_bypass_events_on_user_id ON approval_policy_merge_request_bypass_events USING btree (user_id); + CREATE UNIQUE INDEX index_approval_policy_rule_on_project_and_rule ON approval_policy_rule_project_links USING btree (approval_policy_rule_id, project_id); CREATE INDEX index_approval_policy_rules_on_policy_management_project_id ON approval_policy_rules USING btree (security_policy_management_project_id); @@ -42894,6 +42928,9 @@ ALTER TABLE ONLY deployment_approvals ALTER TABLE ONLY project_relation_export_uploads ADD CONSTRAINT fk_0f7fad01a3 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY approval_policy_merge_request_bypass_events + ADD CONSTRAINT fk_0fae251483 FOREIGN KEY (security_policy_id) REFERENCES security_policies(id) ON DELETE CASCADE; + ALTER TABLE ONLY board_assignees ADD CONSTRAINT fk_105c1d6d08 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; @@ -44205,6 +44242,9 @@ ALTER TABLE ONLY ml_candidates ALTER TABLE ONLY subscription_add_on_purchases ADD CONSTRAINT fk_a1db288990 FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE; +ALTER TABLE ONLY approval_policy_merge_request_bypass_events + ADD CONSTRAINT fk_a24f768758 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULL; + ALTER TABLE ONLY protected_environment_approval_rules ADD CONSTRAINT fk_a3cc825836 FOREIGN KEY (protected_environment_project_id) REFERENCES projects(id) ON DELETE CASCADE; @@ -44958,6 +44998,9 @@ ALTER TABLE ONLY zoekt_indices ALTER TABLE ONLY status_check_responses ADD CONSTRAINT fk_f3953d86c6 FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE; +ALTER TABLE ONLY approval_policy_merge_request_bypass_events + ADD CONSTRAINT fk_f39e177609 FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE; + ALTER TABLE ONLY user_group_member_roles ADD CONSTRAINT fk_f3b8fc5e4e FOREIGN KEY (shared_with_group_id) REFERENCES namespaces(id) ON DELETE CASCADE; @@ -45327,6 +45370,9 @@ ALTER TABLE ONLY project_ci_feature_usages ALTER TABLE ONLY packages_tags ADD CONSTRAINT fk_rails_1dfc868911 FOREIGN KEY (package_id) REFERENCES packages_packages(id) ON DELETE CASCADE; +ALTER TABLE ONLY approval_policy_merge_request_bypass_events + ADD CONSTRAINT fk_rails_1ebbdcc530 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY boards_epic_board_positions ADD CONSTRAINT fk_rails_1ecfd9f2de FOREIGN KEY (epic_id) REFERENCES epics(id) ON DELETE CASCADE; diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 9312575c780dea..33937d79f55184 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -92,6 +92,8 @@ def set_applicable_when_copying_rules(applicable_ids) 'Security::ScanResultPolicyViolation', inverse_of: :merge_request has_many :merge_request_stage_events, class_name: 'Analytics::CycleAnalytics::MergeRequestStageEvent' + has_many :approval_policy_merge_request_bypass_events, + class_name: 'Security::ApprovalPolicyMergeRequestBypassEvent' # WIP v2 approval rules as part of https://gitlab.com/groups/gitlab-org/-/epics/12955 has_many :v2_approval_rules_merge_requests, class_name: 'MergeRequests::ApprovalRulesMergeRequest', diff --git a/ee/app/models/security/approval_policy_merge_request_bypass_event.rb b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb new file mode 100644 index 00000000000000..d310556bdeb707 --- /dev/null +++ b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Security + class ApprovalPolicyMergeRequestBypassEvent < ApplicationRecord + self.table_name = 'approval_policy_merge_request_bypass_events' + + belongs_to :project + belongs_to :security_policy, class_name: 'Security::Policy' + belongs_to :merge_request + belongs_to :user, optional: true + end +end diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 212bb27f294042..6e80a415a750dd 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -34,7 +34,11 @@ class Policy < ApplicationRecord has_many :projects, through: :security_policy_project_links - has_many :security_pipeline_execution_project_schedules, class_name: 'Security::PipelineExecutionProjectSchedule', + has_many :security_pipeline_execution_project_schedules, + class_name: 'Security::PipelineExecutionProjectSchedule', + foreign_key: :security_policy_id, inverse_of: :security_policy + has_many :approval_policy_merge_request_bypass_events, + class_name: 'Security::ApprovalPolicyMergeRequestBypassEvent', foreign_key: :security_policy_id, inverse_of: :security_policy enum :type, { diff --git a/ee/spec/factories/security/approval_policy_merge_request_bypass_events.rb b/ee/spec/factories/security/approval_policy_merge_request_bypass_events.rb new file mode 100644 index 00000000000000..a9662e7ce17f97 --- /dev/null +++ b/ee/spec/factories/security/approval_policy_merge_request_bypass_events.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :approval_policy_merge_request_bypass_event, class: 'Security::ApprovalPolicyMergeRequestBypassEvent' do + project + security_policy + merge_request + user + reason { 'test' } + end +end diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index 51098914ebcefd..54d2a39147edb5 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -26,6 +26,7 @@ it { is_expected.to have_many(:approved_by_users) } it { is_expected.to have_one(:merge_train_car) } it { is_expected.to have_many(:approval_rules) } + it { is_expected.to have_many(:approval_policy_merge_request_bypass_events) } it { is_expected.to have_many(:approval_merge_request_rule_sources).through(:approval_rules) } it { is_expected.to have_many(:approval_project_rules).through(:approval_merge_request_rule_sources) } it { is_expected.to have_many(:status_check_responses).class_name('MergeRequests::StatusCheckResponse').inverse_of(:merge_request) } diff --git a/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb new file mode 100644 index 00000000000000..c30af56f49a494 --- /dev/null +++ b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::ApprovalPolicyMergeRequestBypassEvent, feature_category: :security_policy_management do + describe 'associations' do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:security_policy) } + it { is_expected.to belong_to(:user).optional } + it { is_expected.to belong_to(:merge_request) } + end +end diff --git a/ee/spec/models/security/policy_spec.rb b/ee/spec/models/security/policy_spec.rb index b6c98f9741d924..5c49c9e8645c26 100644 --- a/ee/spec/models/security/policy_spec.rb +++ b/ee/spec/models/security/policy_spec.rb @@ -12,6 +12,7 @@ it { is_expected.to have_many(:projects).through(:security_policy_project_links) } it { is_expected.to have_one(:security_pipeline_execution_policy_config_link) } it { is_expected.to have_many(:security_pipeline_execution_project_schedules) } + it { is_expected.to have_many(:approval_policy_merge_request_bypass_events) } it do is_expected.to validate_uniqueness_of(:security_orchestration_policy_configuration_id).scoped_to(%i[type diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 99095ac909c317..6a99724c7ff265 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -287,6 +287,7 @@ merge_requests: - running_scan_result_policy_violations - failed_scan_result_policy_violations - approval_metrics +- approval_policy_merge_request_bypass_events external_pull_requests: - project merge_request_diff: -- GitLab From 570dcd390d206b8f29546416df6cd677ad2d10e3 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Mon, 28 Jul 2025 14:39:30 +0200 Subject: [PATCH 2/5] Break the migration to up and down --- ...09_create_approval_policy_merge_request_bypass_events.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb index 98c68eae494e4e..91e031a3866ab9 100644 --- a/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb +++ b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb @@ -3,7 +3,7 @@ class CreateApprovalPolicyMergeRequestBypassEvents < Gitlab::Database::Migration[2.3] milestone '18.3' - def change + def up create_table :approval_policy_merge_request_bypass_events do |t| t.references :project, null: false, foreign_key: { on_delete: :cascade }, index: false t.bigint :merge_request_id, null: false, @@ -21,4 +21,8 @@ def change unique: true, name: 'idx_approval_policy_mr_bypass_events_on_project_mr_policy' end + + def down + drop_table :approval_policy_merge_request_bypass_events + end end -- GitLab From 387b4d8e58eef5ae4382dba8acd1eac5caa3db12 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Tue, 29 Jul 2025 12:15:42 +0200 Subject: [PATCH 3/5] Add validation and refactor spec --- ee/app/models/ee/merge_request.rb | 2 +- .../security/approval_policy_merge_request_bypass_event.rb | 2 ++ .../approval_policy_merge_request_bypass_event_spec.rb | 5 +++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 33937d79f55184..f21f711e8a9996 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -93,7 +93,7 @@ def set_applicable_when_copying_rules(applicable_ids) has_many :merge_request_stage_events, class_name: 'Analytics::CycleAnalytics::MergeRequestStageEvent' has_many :approval_policy_merge_request_bypass_events, - class_name: 'Security::ApprovalPolicyMergeRequestBypassEvent' + class_name: 'Security::ApprovalPolicyMergeRequestBypassEvent', inverse_of: :merge_request # WIP v2 approval rules as part of https://gitlab.com/groups/gitlab-org/-/epics/12955 has_many :v2_approval_rules_merge_requests, class_name: 'MergeRequests::ApprovalRulesMergeRequest', diff --git a/ee/app/models/security/approval_policy_merge_request_bypass_event.rb b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb index d310556bdeb707..e9da41effb6083 100644 --- a/ee/app/models/security/approval_policy_merge_request_bypass_event.rb +++ b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb @@ -8,5 +8,7 @@ class ApprovalPolicyMergeRequestBypassEvent < ApplicationRecord belongs_to :security_policy, class_name: 'Security::Policy' belongs_to :merge_request belongs_to :user, optional: true + + validates :reason, presence: true, length: { minimum: 1, maximum: 1024 } end end diff --git a/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb index c30af56f49a494..1e0491e30fc294 100644 --- a/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb +++ b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb @@ -9,4 +9,9 @@ it { is_expected.to belong_to(:user).optional } it { is_expected.to belong_to(:merge_request) } end + + describe 'validations' do + it { is_expected.to validate_presence_of(:reason) } + it { is_expected.to validate_length_of(:reason).is_at_most(1024).is_at_least(1) } + end end -- GitLab From 4c564f4f3b968f062a6abfb265d4ddec87591707 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Tue, 29 Jul 2025 23:31:30 +0200 Subject: [PATCH 4/5] Update constraint and add validation --- ...eate_approval_policy_merge_request_bypass_events.rb | 10 ++++++++-- db/structure.sql | 4 ++-- .../approval_policy_merge_request_bypass_event.rb | 6 +++++- .../approval_policy_merge_request_bypass_event_spec.rb | 6 +++++- 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb index 91e031a3866ab9..5711206060d5c2 100644 --- a/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb +++ b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb @@ -10,10 +10,16 @@ def up index: { name: 'index_approval_policy_merge_request_bypass_events_on_mr_id' } t.bigint :security_policy_id, null: false, index: { name: 'index_approval_policy_merge_request_bypass_events_on_policy_id' } - t.bigint :user_id, null: true, index: { name: 'index_approval_policy_merge_request_bypass_events_on_user_id' } - t.text :reason, null: false, limit: 1024 + t.bigint :user_id, null: true, + index: { name: 'index_approval_policy_merge_request_bypass_events_on_user_id' } t.timestamps_with_timezone null: false + + # rubocop:disable Migration/AddLimitToTextColumns -- combined with check constraint + t.text :reason, null: false + t.check_constraint "reason != '' AND length(reason) <= 1024", + name: 'check_approval_policy_mr_bypass_events_reason' + # rubocop:enable Migration/AddLimitToTextColumns end add_index :approval_policy_merge_request_bypass_events, diff --git a/db/structure.sql b/db/structure.sql index 279d76b36cf9c0..5beea934136351 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9956,10 +9956,10 @@ CREATE TABLE approval_policy_merge_request_bypass_events ( merge_request_id bigint NOT NULL, security_policy_id bigint NOT NULL, user_id bigint, - reason text NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - CONSTRAINT check_dc373cb020 CHECK ((char_length(reason) <= 1024)) + reason text NOT NULL, + CONSTRAINT check_approval_policy_mr_bypass_events_reason CHECK (((reason <> ''::text) AND (length(reason) <= 1024))) ); CREATE SEQUENCE approval_policy_merge_request_bypass_events_id_seq diff --git a/ee/app/models/security/approval_policy_merge_request_bypass_event.rb b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb index e9da41effb6083..6412f37f81c061 100644 --- a/ee/app/models/security/approval_policy_merge_request_bypass_event.rb +++ b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb @@ -9,6 +9,10 @@ class ApprovalPolicyMergeRequestBypassEvent < ApplicationRecord belongs_to :merge_request belongs_to :user, optional: true - validates :reason, presence: true, length: { minimum: 1, maximum: 1024 } + validates :reason, + presence: true, + length: { maximum: 1024 }, + exclusion: { in: [''], message: "cannot be blank" } + validates_uniqueness_of :project_id, scope: [:merge_request_id, :security_policy_id] end end diff --git a/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb index 1e0491e30fc294..377ba20068b8aa 100644 --- a/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb +++ b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb @@ -11,7 +11,11 @@ end describe 'validations' do + subject { build(:approval_policy_merge_request_bypass_event, security_policy: create(:security_policy)) } + it { is_expected.to validate_presence_of(:reason) } - it { is_expected.to validate_length_of(:reason).is_at_most(1024).is_at_least(1) } + it { is_expected.to validate_length_of(:reason).is_at_most(1024) } + it { is_expected.to validate_exclusion_of(:reason).in_array(['']).with_message("cannot be blank") } + it { is_expected.to validate_uniqueness_of(:project_id).scoped_to([:merge_request_id, :security_policy_id]) } end end -- GitLab From 9861c66ea8bd343b2d330000f23c5c436b03275e Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Wed, 30 Jul 2025 15:03:43 +0200 Subject: [PATCH 5/5] Address review comments --- ...109_create_approval_policy_merge_request_bypass_events.rb | 4 ++-- db/structure.sql | 2 +- .../security/approval_policy_merge_request_bypass_event.rb | 5 +---- .../approval_policy_merge_request_bypass_event_spec.rb | 1 - 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb index 5711206060d5c2..e5da4e148bd530 100644 --- a/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb +++ b/db/migrate/20250723205109_create_approval_policy_merge_request_bypass_events.rb @@ -17,8 +17,8 @@ def up # rubocop:disable Migration/AddLimitToTextColumns -- combined with check constraint t.text :reason, null: false - t.check_constraint "reason != '' AND length(reason) <= 1024", - name: 'check_approval_policy_mr_bypass_events_reason' + t.check_constraint "length(trim(reason)) BETWEEN 1 AND 1024", + name: check_constraint_name(:approval_policy_merge_request_bypass_events, :reason, 'length_between_1_and_1024') # rubocop:enable Migration/AddLimitToTextColumns end diff --git a/db/structure.sql b/db/structure.sql index 5beea934136351..d0986196abd2f1 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9959,7 +9959,7 @@ CREATE TABLE approval_policy_merge_request_bypass_events ( created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, reason text NOT NULL, - CONSTRAINT check_approval_policy_mr_bypass_events_reason CHECK (((reason <> ''::text) AND (length(reason) <= 1024))) + CONSTRAINT check_3169f0d109 CHECK (((length(TRIM(BOTH FROM reason)) >= 1) AND (length(TRIM(BOTH FROM reason)) <= 1024))) ); CREATE SEQUENCE approval_policy_merge_request_bypass_events_id_seq diff --git a/ee/app/models/security/approval_policy_merge_request_bypass_event.rb b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb index 6412f37f81c061..bd13dc0ae835a0 100644 --- a/ee/app/models/security/approval_policy_merge_request_bypass_event.rb +++ b/ee/app/models/security/approval_policy_merge_request_bypass_event.rb @@ -9,10 +9,7 @@ class ApprovalPolicyMergeRequestBypassEvent < ApplicationRecord belongs_to :merge_request belongs_to :user, optional: true - validates :reason, - presence: true, - length: { maximum: 1024 }, - exclusion: { in: [''], message: "cannot be blank" } + validates :reason, presence: true, length: { maximum: 1024 } validates_uniqueness_of :project_id, scope: [:merge_request_id, :security_policy_id] end end diff --git a/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb index 377ba20068b8aa..ee9a8eca713695 100644 --- a/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb +++ b/ee/spec/models/security/approval_policy_merge_request_bypass_event_spec.rb @@ -15,7 +15,6 @@ it { is_expected.to validate_presence_of(:reason) } it { is_expected.to validate_length_of(:reason).is_at_most(1024) } - it { is_expected.to validate_exclusion_of(:reason).in_array(['']).with_message("cannot be blank") } it { is_expected.to validate_uniqueness_of(:project_id).scoped_to([:merge_request_id, :security_policy_id]) } end end -- GitLab