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 0b8e6893c0e0afcdb9db689e715e634ccaddd8bd..dea16f3f8a96f0bb51ed5d1ad0411b310faa2b2b 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 d791213a97351d58b15060cf57e16ad95757bacd..ef0e6216a2a7c96d3295c360784db1130fff51a9 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 4dd5b8bbcdd713e70bc55092d084735a3c181402..fb1618927bb8850e5c94f98b7a125f449e15e986 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 28cd87ac89a5cca9d7ed660534901197c6180dcd..0952ce1a97c55277f836d9fcb0afe8bf155b466c 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 d7bf249fd8fe838d0654374ceefb8a88260db2d6..dfdd5397503a1885e378da2d3aebac3346aba844 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,12 @@ 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 + 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 0000000000000000000000000000000000000000..36c78dcff87eba496c0fc5e72a4e00ae6800ee48 --- /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: 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 +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 aa6a92b356953c4227d5e0913887ffe4053f3596..7a2ff18c955e05c14a6d177235803cc22035c76f 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 b58aa5520169e87d3b4e8701cf52d0225a111ae5..9bc7dc338ea8b3e7e1b45cb0a7160ab58b7dbcf3 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 8a6573d1f77eb9269a11f9b48c1fd197548f6c8f..68a68500299c52329a8ef54d51b56c476a7dc8e0 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 408f6c07beecaef1443955b92a9a8405e336507b..7f7275d74207a90f208fa5c89f548aedef97cd86 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 ""