From 6511b4179add28becae27d5d9089c5fed6acd08b Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Fri, 25 Jul 2025 16:21:40 +0200 Subject: [PATCH 1/3] Disable `Pipeline Must Succeed` setting for security projects Changelog: fixed EE: true --- .../components/merge_checks_app.vue | 8 +- .../javascripts/merge_checks/constants.js | 3 + .../assets/javascripts/merge_checks/index.js | 2 +- ee/app/helpers/merge_checks_helper.rb | 27 ++++++- ee/app/models/ee/project.rb | 15 ++-- ...y_policy_project_pipeline_must_succeed.yml | 10 +++ .../components/merge_checks_app_spec.js | 29 +++++++ ee/spec/helpers/merge_checks_helper_spec.rb | 48 ++++++++++++ ee/spec/models/ee/project_spec.rb | 78 ++++++++++++++++--- locale/gitlab.pot | 3 + 10 files changed, 202 insertions(+), 21 deletions(-) create mode 100644 ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml diff --git a/ee/app/assets/javascripts/merge_checks/components/merge_checks_app.vue b/ee/app/assets/javascripts/merge_checks/components/merge_checks_app.vue index 0b8e6893c0e0af..dea16f3f8a96f0 100644 --- a/ee/app/assets/javascripts/merge_checks/components/merge_checks_app.vue +++ b/ee/app/assets/javascripts/merge_checks/components/merge_checks_app.vue @@ -28,6 +28,12 @@ export default { lockedText() { return sprintf(I18N.lockedText, { groupName: this.parentGroupName }); }, + pipelineMustSucceedLockedText() { + if (this.pipelineMustSucceed.securityPolicyProject) { + return sprintf(I18N.lockedSecurityPolicyText); + } + return this.lockedText; + }, skippedPipelineLocked() { return this.allowMergeOnSkippedPipeline.locked || !this.hasPipelineMustSucceed; }, @@ -60,7 +66,7 @@ export default { :label="$options.i18n.pipelineMustSucceed.label" :checked="hasPipelineMustSucceed" :locked="pipelineMustSucceed.locked" - :locked-text="lockedText" + :locked-text="pipelineMustSucceedLockedText" data-testid="allow-merge-if-pipeline-succeeds-checkbox" @input="toggleChecked('hasPipelineMustSucceed')" > diff --git a/ee/app/assets/javascripts/merge_checks/constants.js b/ee/app/assets/javascripts/merge_checks/constants.js index d791213a97351d..ef0e6216a2a7c9 100644 --- a/ee/app/assets/javascripts/merge_checks/constants.js +++ b/ee/app/assets/javascripts/merge_checks/constants.js @@ -16,4 +16,7 @@ export const I18N = { 'MergeChecks|This setting is configured in group %{groupName} and can only be changed in the group settings by an administrator or group owner.', ), lockedUponPipelineMustSucceed: s__('MergeChecks|Enable "Pipelines must succeed" first.'), + lockedSecurityPolicyText: s__( + 'MergeChecks|This setting is disabled because the project is linked as a security policy project.', + ), }; diff --git a/ee/app/assets/javascripts/merge_checks/index.js b/ee/app/assets/javascripts/merge_checks/index.js index 4dd5b8bbcdd713..fb1618927bb885 100644 --- a/ee/app/assets/javascripts/merge_checks/index.js +++ b/ee/app/assets/javascripts/merge_checks/index.js @@ -24,7 +24,7 @@ export const initMergeRequestMergeChecksApp = async () => { sourceType, allowMergeOnSkippedPipeline, onlyAllowMergeIfAllResolved, - pipelineMustSucceed, + pipelineMustSucceed: convertObjectPropsToCamelCase(pipelineMustSucceed), }, render(createElement) { return createElement(MergeChecksApp); diff --git a/ee/app/helpers/merge_checks_helper.rb b/ee/app/helpers/merge_checks_helper.rb index 28cd87ac89a5cc..0952ce1a97c552 100644 --- a/ee/app/helpers/merge_checks_helper.rb +++ b/ee/app/helpers/merge_checks_helper.rb @@ -17,10 +17,7 @@ def merge_checks(source) { source_type: source_type, settings: { - pipeline_must_succeed: { - locked: target.only_allow_merge_if_pipeline_succeeds_locked?, - value: target.only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: true) - }, + pipeline_must_succeed: pipeline_must_succeed_settings(target), allow_merge_on_skipped_pipeline: { locked: target.allow_merge_on_skipped_pipeline_locked?, value: target.allow_merge_on_skipped_pipeline?(inherit_group_setting: true) @@ -37,4 +34,26 @@ def merge_checks(source) parent_group_name: parent_group_name } end + + private + + def pipeline_must_succeed_settings(target) + if security_policy_project?(target) + { + locked: true, + value: false, + security_policy_project: true + } + else + { + locked: target.only_allow_merge_if_pipeline_succeeds_locked?, + value: target.only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: true) + } + end + end + + def security_policy_project?(target) + target.is_a?(Project) && Feature.enabled?(:security_policy_project_pipeline_must_succeed, + target) && target.has_linked_configurations? + end end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index d7bf249fd8fe83..376a9e15f2a099 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -612,6 +612,8 @@ def project_epics_enabled? end def has_linked_configurations? + return false unless licensed_feature_available?(:security_orchestration_policies) + assoc = association(:security_policy_management_project_linked_configurations) assoc.loaded? ? assoc.target.any? : assoc.scope.exists? @@ -624,11 +626,7 @@ def self.cascading_with_parent_namespace(attribute) end define_method("#{attribute}?") do |inherit_group_setting: false| - if attribute == :only_allow_merge_if_pipeline_succeeds && - licensed_feature_available?(:security_orchestration_policies) && - has_linked_configurations? - return false - end + return false if attribute == :only_allow_merge_if_pipeline_succeeds && has_linked_configurations? return super() unless licensed_feature_available?(:group_level_merge_checks_setting) @@ -653,6 +651,13 @@ def self.cascading_with_parent_namespace(attribute) cascading_with_parent_namespace :only_allow_merge_if_all_discussions_are_resolved cascading_with_parent_namespace :allow_merge_without_pipeline + override :only_allow_merge_if_pipeline_succeeds + def only_allow_merge_if_pipeline_succeeds + return false if ::Feature.enabled?(:security_policy_project_pipeline_must_succeed, self) && has_linked_configurations? + + super + end + def mirror_last_update_succeeded? !!import_state&.last_update_succeeded? end diff --git a/ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml b/ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml new file mode 100644 index 00000000000000..b057312b7d284c --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml @@ -0,0 +1,10 @@ +--- +name: security_policy_project_pipeline_must_succeed +description: Correctly disable `Pipeline must succeed` setting for security policy projects +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/534276 +introduced_by_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/557921 +milestone: '18.3' +group: group::security policies +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/spec/frontend/merge_checks/components/merge_checks_app_spec.js b/ee/spec/frontend/merge_checks/components/merge_checks_app_spec.js index aa6a92b356953c..7a2ff18c955e05 100644 --- a/ee/spec/frontend/merge_checks/components/merge_checks_app_spec.js +++ b/ee/spec/frontend/merge_checks/components/merge_checks_app_spec.js @@ -131,4 +131,33 @@ describe('MergeChecksApp', () => { expect(findSkippedPipelineCheckbox().props('checked')).toBe(false); }); }); + + describe('security policy project', () => { + it('should show security policy locked text with a security policy project', () => { + createWrapper({ + pipelineMustSucceed: { + value: true, + locked: true, + securityPolicyProject: true, + }, + }); + + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( + sprintf(I18N.lockedSecurityPolicyText), + ); + }); + + it('should show default locked text without a security policy project', () => { + createWrapper({ + pipelineMustSucceed: { + value: true, + locked: true, + }, + }); + + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( + sprintf(I18N.lockedText, { groupName: defaultGroupName }), + ); + }); + }); }); diff --git a/ee/spec/helpers/merge_checks_helper_spec.rb b/ee/spec/helpers/merge_checks_helper_spec.rb index b58aa5520169e8..9bc7dc338ea8b3 100644 --- a/ee/spec/helpers/merge_checks_helper_spec.rb +++ b/ee/spec/helpers/merge_checks_helper_spec.rb @@ -98,6 +98,54 @@ parent_group_name: group.name }) end + + # rubocop:disable RSpec/FactoryBot/AvoidCreate -- requires foreign key + context 'when project is linked as security policy project' do + let_it_be(:project_setting) { true } + let_it_be(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: project_setting) } + + let(:settings) do + helper + .merge_checks(source) + .fetch(:settings) + .then { |json| Gitlab::Json.parse(json) } + .transform_values(&:symbolize_keys) + end + + subject { settings["pipeline_must_succeed"] } + + before_all do + create(:security_orchestration_policy_configuration, security_policy_management_project: project) + end + + context 'with licensed feature' do + before do + stub_licensed_features(security_orchestration_policies: true) + end + + it { is_expected.to eq(locked: true, value: false, security_policy_project: true) } + + context 'with feature disabled' do + before do + stub_feature_flags(security_policy_project_pipeline_must_succeed: false) + end + + # `Project::cascading_with_parent_namespace` already contains a security policy project check which + # correctly affects the cascading attribute `Project#only_allow_merge_if_pipeline_succeeds?`. + # Therefore assert `false` instead of `project_setting`. + it { is_expected.to eq(locked: false, value: false) } + end + end + + context 'without licensed feature' do + before do + stub_licensed_features(security_orchestration_policies: false) + end + + it { is_expected.to eq(locked: false, value: project_setting) } + end + end + # rubocop:enable RSpec/FactoryBot/AvoidCreate end end end diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb index 8a6573d1f77eb9..68a68500299c52 100644 --- a/ee/spec/models/ee/project_spec.rb +++ b/ee/spec/models/ee/project_spec.rb @@ -5453,23 +5453,81 @@ def stub_default_url_options(host) allow(target).to receive(:any?).and_return(true) end - context 'when the association is already loaded' do - it 'calls #any? on the target' do - expect(target).to receive(:any?) - expect(scope).not_to receive(:exists?) + context 'with licensed feature' do + before do + stub_licensed_features(security_orchestration_policies: true) + end + + context 'when the association is already loaded' do + it 'calls #any? on the target' do + expect(target).to receive(:any?) + expect(scope).not_to receive(:exists?) + + project.has_linked_configurations? + end + end + + context 'when the association is not loaded' do + let(:loaded) { false } - project.has_linked_configurations? + it 'calls #exists? on the scope' do + expect(scope).to receive(:exists?) + expect(target).not_to receive(:any?) + + project.has_linked_configurations? + end end end - context 'when the association is not loaded' do - let(:loaded) { false } + context 'without licensed feature' do + before do + stub_licensed_features(security_orchestration_policies: false) + end - it 'calls #exists? on the scope' do - expect(scope).to receive(:exists?) + it 'returns `false` early' do expect(target).not_to receive(:any?) + expect(scope).not_to receive(:exists?) + + expect(project.has_linked_configurations?).to be(false) + end + end + end + + describe '#only_allow_merge_if_pipeline_succeeds' do + let_it_be(:project_setting) { true } + let_it_be(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: project_setting) } + + subject(:only_allow_merge_if_pipeline_succeeds) { project.only_allow_merge_if_pipeline_succeeds } + + context 'when linked as security policy project' do + before_all do + create(:security_orchestration_policy_configuration, security_policy_management_project: project) + end + + it { is_expected.to be(project_setting) } + + context 'with licensed feature' do + before do + stub_licensed_features(security_orchestration_policies: true) + end + + it { is_expected.to be(false) } + + context 'with feature disabled' do + before do + stub_feature_flags(security_policy_project_pipeline_must_succeed: false) + end + + it { is_expected.to be(project_setting) } + end + end + + context 'without licensed feature' do + before do + stub_licensed_features(security_orchestration_policies: false) + end - project.has_linked_configurations? + it { is_expected.to be(project_setting) } end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 408f6c07beecae..7f7275d74207a9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -39342,6 +39342,9 @@ msgstr "" msgid "MergeChecks|This setting is configured in group %{groupName} and can only be changed in the group settings by an administrator or group owner." msgstr "" +msgid "MergeChecks|This setting is disabled because the project is linked as a security policy project." +msgstr "" + msgid "MergeChecks|View locked files" msgstr "" -- GitLab From ddc5864295bdfbf3c6b6ac1ecc64bbaef975f13c Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Wed, 30 Jul 2025 11:13:55 +0200 Subject: [PATCH 2/3] Update feature flag manifest URL --- .../security_policy_project_pipeline_must_succeed.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml b/ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml index b057312b7d284c..36c78dcff87eba 100644 --- a/ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml +++ b/ee/config/feature_flags/gitlab_com_derisk/security_policy_project_pipeline_must_succeed.yml @@ -2,7 +2,7 @@ name: security_policy_project_pipeline_must_succeed description: Correctly disable `Pipeline must succeed` setting for security policy projects feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/534276 -introduced_by_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/199721 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/557921 milestone: '18.3' group: group::security policies -- GitLab From 49f5aa507abce46315f88a0b672750b9654a6cb9 Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Wed, 30 Jul 2025 11:17:43 +0200 Subject: [PATCH 3/3] Remove `override` directive --- ee/app/models/ee/project.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 376a9e15f2a099..dfdd5397503a18 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -651,7 +651,6 @@ def self.cascading_with_parent_namespace(attribute) cascading_with_parent_namespace :only_allow_merge_if_all_discussions_are_resolved cascading_with_parent_namespace :allow_merge_without_pipeline - override :only_allow_merge_if_pipeline_succeeds def only_allow_merge_if_pipeline_succeeds return false if ::Feature.enabled?(:security_policy_project_pipeline_must_succeed, self) && has_linked_configurations? -- GitLab