Skip to content
Snippets Groups Projects
Verified Commit 7ee947eb authored by Martin Čavoj's avatar Martin Čavoj :palm_tree: Committed by GitLab
Browse files

Merge branch '432127-feature-flag-cleanup-scan_result_any_merge_request' into 'master'

Clean up FF scan_result_any_merge_request

See merge request gitlab-org/gitlab!141506



Merged-by: default avatarMartin Čavoj <mcavoj@gitlab.com>
parents 46f3eb96 5c7bffb4
No related branches found
No related tags found
No related merge requests found
Showing
with 69 additions and 131 deletions
......@@ -187,6 +187,7 @@ the defined policy.
> - The `prevent_approval_by_author`, `prevent_approval_by_commit_author`, `remove_approvals_with_new_commit`, and `require_password_to_approve` fields were [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/418752) in GitLab 16.4 [with flag](../../../administration/feature_flags.md) named `scan_result_any_merge_request`. Disabled by default.
> - The above fields were [enabled on GitLab.com](https://gitlab.com/gitlab-org/gitlab/-/issues/423988) in GitLab 16.6.
> - The above fields were [enabled on self-managed](https://gitlab.com/gitlab-org/gitlab/-/issues/423988) in GitLab 16.7.
> - Feature flag `scan_result_any_merge_request` [was removed](https://gitlab.com/gitlab-org/gitlab/-/issues/432127) in GitLab 16.8.
> - The `prevent_pushing_and_force_pushing` field was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/420629) in GitLab 16.4 [with flag](../../../administration/feature_flags.md) named `scan_result_policies_block_force_push`. Disabled by default.
> - The above field was [enabled on GitLab.com](https://gitlab.com/gitlab-org/gitlab/-/issues/427260) in GitLab 16.6.
> - The above field was [enabled on self-managed](https://gitlab.com/gitlab-org/gitlab/-/issues/427260) in GitLab 16.7.
......
......@@ -96,7 +96,15 @@ export const RULE_MODE_SCANNERS = {
export const MAX_ALLOWED_RULES_LENGTH = 5;
export const PRIMARY_POLICY_KEYS = ['type', 'name', 'description', 'enabled', 'rules', 'actions'];
export const PRIMARY_POLICY_KEYS = [
'type',
'name',
'description',
'enabled',
'rules',
'actions',
'approval_settings',
];
export const SPECIFIC_BRANCHES = {
id: 'SPECIFIC_BRANCHES',
......
......@@ -206,13 +206,6 @@ export default {
rulesHaveBranches() {
return this.policy.rules.some(this.ruleHasBranchesProperty);
},
shouldShowSettings() {
return (
this.glFeatures.scanResultPoliciesBlockUnprotectingBranches ||
this.glFeatures.scanResultAnyMergeRequest ||
this.glFeatures.scanResultPoliciesBlockForcePush
);
},
settingAlert() {
if (this.hasEmptySettings) {
return {
......@@ -275,13 +268,7 @@ export default {
},
updateRule(ruleIndex, rule) {
this.policy.rules.splice(ruleIndex, 1, rule);
if (
this.glFeatures.scanResultPoliciesBlockUnprotectingBranches ||
this.glFeatures.scanResultAnyMergeRequest ||
this.glFeatures.scanResultPoliciesBlockForcePush
) {
this.updateSettings(this.settings);
}
this.updateSettings(this.settings);
this.updateYamlEditorValue(this.policy);
},
handleError(error) {
......@@ -484,7 +471,7 @@ export default {
</dim-disable-container>
</template>
<template #settings>
<dim-disable-container v-if="shouldShowSettings" :disabled="hasParsingError">
<dim-disable-container :disabled="hasParsingError">
<template #title>
<h4>{{ $options.i18n.settingsTitle }}</h4>
</template>
......
......@@ -22,16 +22,9 @@ export const fromYaml = ({ manifest, validateRuleMode = false }) => {
* schema. These values should not be retrieved from the backend schema because
* the UI for new attributes may not be available.
*/
const hasApprovalSettings =
gon.features?.scanResultPoliciesBlockUnprotectingBranches ||
gon.features?.scanResultAnyMergeRequest ||
gon.features?.scanResultPoliciesBlockForcePush;
const primaryKeys = [
...PRIMARY_POLICY_KEYS,
...(hasApprovalSettings || isEqual(policy.approval_settings, PERMITTED_INVALID_SETTINGS) // Temporary workaround to allow the rule builder to load with wrongly persisted settings
? [`approval_settings`]
: []),
...(gon?.features?.securityPoliciesPolicyScope ? ['policy_scope'] : []),
];
const rulesKeys = [
......@@ -62,14 +55,14 @@ export const fromYaml = ({ manifest, validateRuleMode = false }) => {
const { approval_settings: settings = {} } = policy;
const hasInvalidApprovalSettings = hasApprovalSettings
? hasInvalidKey(settings, [...VALID_APPROVAL_SETTINGS, PERMITTED_INVALID_SETTINGS_KEY])
: false;
const hasInvalidApprovalSettings = hasInvalidKey(settings, [
...VALID_APPROVAL_SETTINGS,
PERMITTED_INVALID_SETTINGS_KEY,
]);
const hasInvalidSettingStructure =
hasApprovalSettings && !isEqual(settings, PERMITTED_INVALID_SETTINGS)
? !Object.values(settings).every((setting) => isBoolean(setting))
: false;
const hasInvalidSettingStructure = !isEqual(settings, PERMITTED_INVALID_SETTINGS)
? !Object.values(settings).every((setting) => isBoolean(setting))
: false;
return isValidPolicy({ policy, primaryKeys, rulesKeys, actionsKeys }) &&
!hasInvalidApprovalSettings &&
......
<script>
import { GlAlert, GlSprintf } from '@gitlab/ui';
import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import { ANY_MERGE_REQUEST, SCAN_FINDING, LICENSE_FINDING } from '../lib';
import AnyMergeRequestRuleBuilder from './any_merge_request_rule_builder.vue';
import SecurityScanRuleBuilder from './security_scan_rule_builder.vue';
......@@ -16,7 +15,6 @@ export default {
SecurityScanRuleBuilder,
LicenseScanRuleBuilder,
},
mixins: [glFeatureFlagsMixin()],
props: {
initRule: {
type: Object,
......@@ -101,7 +99,7 @@ export default {
/>
<any-merge-request-rule-builder
v-else-if="isAnyMergeRequestRule && glFeatures.scanResultAnyMergeRequest"
v-else-if="isAnyMergeRequestRule"
:init-rule="initRule"
@changed="updateRule"
@remove="removeRule"
......
<script>
import { GlCollapsibleListbox } from '@gitlab/ui';
import { s__ } from '~/locale';
import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import { ANY_MERGE_REQUEST, SCAN_FINDING, LICENSE_FINDING } from '../lib';
export default {
......@@ -22,7 +21,6 @@ export default {
components: {
GlCollapsibleListbox,
},
mixins: [glFeatureFlagsMixin()],
props: {
scanType: {
type: String,
......@@ -40,14 +38,12 @@ export default {
return this.scanType ? '' : this.$options.i18n.scanRuleTypeToggleText;
},
anyMergeRequestItem() {
return this.glFeatures.scanResultAnyMergeRequest
? [
{
value: ANY_MERGE_REQUEST,
text: s__('SecurityOrchestration|Any merge request'),
},
]
: [];
return [
{
value: ANY_MERGE_REQUEST,
text: s__('SecurityOrchestration|Any merge request'),
},
];
},
listBoxItems() {
if (this.items?.length > 0) {
......
......@@ -11,7 +11,6 @@ class PoliciesController < Groups::ApplicationController
before_action do
push_frontend_feature_flag(:scan_result_policies_block_unprotecting_branches, group)
push_frontend_feature_flag(:scan_result_any_merge_request, group)
push_frontend_feature_flag(:scan_result_policies_block_force_push, group)
push_frontend_feature_flag(:security_policies_policy_scope, group)
push_frontend_feature_flag(:compliance_pipeline_in_policies, group)
......
......@@ -12,7 +12,6 @@ class PoliciesController < Projects::ApplicationController
before_action do
push_frontend_feature_flag(:scan_result_policies_block_unprotecting_branches, project)
push_frontend_feature_flag(:scan_result_any_merge_request, project)
push_frontend_feature_flag(:scan_result_policies_block_force_push, project)
push_frontend_feature_flag(:compliance_pipeline_in_policies, project)
end
......
......@@ -96,7 +96,6 @@ def apply_report_approver_rules_to(merge_request)
rule = merge_request_report_approver_rule(merge_request)
rule.update!(report_approver_attributes)
next rule unless Feature.enabled?(:scan_result_any_merge_request, merge_request.project)
next rule unless rule.scan_result_policy_id
Security::ScanResultPolicyViolation.upsert_all(
......
......@@ -124,7 +124,7 @@ def overridden?
def from_scan_result_policy?
return true if scan_finding?
return true if license_scanning? && scan_result_policy_id.present?
return true if any_merge_request? && ::Feature.enabled?(:scan_result_any_merge_request, project)
return true if any_merge_request?
false
end
......
......@@ -29,8 +29,7 @@ def schedule_approval_notifications(merge_request)
end
def schedule_sync_for(merge_request)
if ::Feature.enabled?(:scan_result_any_merge_request, merge_request.project) &&
merge_request.approval_rules.any_merge_request.any?
if merge_request.approval_rules.any_merge_request.any?
# We need to make sure to run the merge request worker after hooks were called to
# get correct commit signatures
::Security::ScanResultPolicies::SyncAnyMergeRequestApprovalRulesWorker.perform_async(merge_request.id)
......
......@@ -65,8 +65,6 @@ def update_approvers_for_target_branch_merge_requests
end
def sync_any_merge_request_approval_rules
return unless ::Feature.enabled?(:scan_result_any_merge_request, project)
merge_requests_for_source_branch.each do |merge_request|
if merge_request.approval_rules.any_merge_request.any?
::Security::ScanResultPolicies::SyncAnyMergeRequestApprovalRulesWorker.perform_async(merge_request.id)
......
......@@ -52,7 +52,6 @@ def reset_approval_rules(merge_request)
end
def sync_any_merge_request_approval_rules(merge_request)
return if ::Feature.disabled?(:scan_result_any_merge_request, merge_request.project)
return unless merge_request.approval_rules.any_merge_request.any?
::Security::ScanResultPolicies::SyncAnyMergeRequestApprovalRulesWorker.perform_async(merge_request.id)
......
......@@ -50,7 +50,7 @@ def create_approval_rule?(rule)
# For `any_merge_request` rules, the approval rules can be created without approvers and can override
# project approval settings in general.
# The violations in this case are handled via SyncAnyMergeRequestRulesService
Feature.enabled?(:scan_result_any_merge_request, project) && policy[:actions].present?
policy[:actions].present?
end
def license_finding?(rule)
......
......@@ -29,7 +29,6 @@ def execute
private
def sync_any_merge_request_approval_rules(merge_request)
return if ::Feature.disabled?(:scan_result_any_merge_request, merge_request.project)
return unless merge_request.approval_rules.any_merge_request.any?
::Security::ScanResultPolicies::SyncAnyMergeRequestApprovalRulesWorker.perform_async(merge_request.id)
......
......@@ -19,9 +19,6 @@ def add(violated_ids, unviolated_ids)
end
def execute
return [violated_policy_ids.clear, unviolated_policy_ids.clear] unless Feature.enabled?(
:scan_result_any_merge_request, merge_request.project)
Security::ScanResultPolicyViolation.transaction do
delete_violations if unviolated_policy_ids.any?
create_violations if violated_policy_ids.any?
......
......@@ -16,7 +16,6 @@ def perform(merge_request_id)
merge_request = MergeRequest.find_by_id(merge_request_id)
return unless merge_request
return unless Feature.enabled?(:scan_result_any_merge_request, merge_request.project)
Security::ScanResultPolicies::SyncAnyMergeRequestRulesService.new(merge_request).execute
end
......
---
name: scan_result_any_merge_request
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130630
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/423988
milestone: '16.4'
type: development
group: group::security policies
default_enabled: true
......@@ -304,10 +304,9 @@ describe('EditorComponent', () => {
[PREVENT_PUSHING_AND_FORCE_PUSHING]: true,
};
it('does update the settings with the "scanResultPoliciesBlockUnprotectingBranches" ff enabled and the "scanResultAnyMergeRequest" ff enabled and the "scanResultPoliciesBlockForcePush" ff enabled', () => {
it('does update the settings with the "scanResultPoliciesBlockUnprotectingBranches" ff enabled and the "scanResultPoliciesBlockForcePush" ff enabled', () => {
const features = {
scanResultPoliciesBlockUnprotectingBranches: true,
scanResultAnyMergeRequest: true,
scanResultPoliciesBlockForcePush: true,
};
window.gon = { features };
......@@ -329,7 +328,7 @@ describe('EditorComponent', () => {
);
});
it('does update the settings with the "scanResultPoliciesBlockUnprotectingBranches" ff enabled and the "scanResultAnyMergeRequest" ff disabled', () => {
it('does update the settings with the "scanResultPoliciesBlockUnprotectingBranches" ff enabled', () => {
const features = {
scanResultPoliciesBlockUnprotectingBranches: true,
};
......@@ -351,9 +350,9 @@ describe('EditorComponent', () => {
);
});
it('does update the settings with the "scanResultAnyMergeRequest" ff enabled', () => {
it('does update the settings with ANY_MERGE_REQUEST type', () => {
const newValue = { type: ANY_MERGE_REQUEST };
factory({ glFeatures: { scanResultAnyMergeRequest: true } });
factory();
expect(findPolicyEditorLayout().props('policy')).not.toHaveProperty('approval_settings');
findAllRuleSections().at(0).vm.$emit('changed', newValue);
expect(findPolicyEditorLayout().props('policy')).toEqual(
......@@ -378,14 +377,6 @@ describe('EditorComponent', () => {
}),
);
});
it('does not update the settings with no feature flags enabled', () => {
const newValue = { type: ANY_MERGE_REQUEST };
factory();
expect(findPolicyEditorLayout().props('policy')).not.toHaveProperty('approval_settings');
findAllRuleSections().at(0).vm.$emit('changed', newValue);
expect(findPolicyEditorLayout().props('policy')).not.toHaveProperty('approval_settings');
});
});
});
......@@ -735,12 +726,41 @@ describe('EditorComponent', () => {
describe('settings section', () => {
describe('settings', () => {
it('does not display the settings', () => {
factory();
expect(findSettingsSection().exists()).toBe(false);
describe('without default flags', () => {
beforeEach(() => {
factory();
});
it('displays setting section', () => {
expect(findSettingsSection().exists()).toBe(true);
});
it('does not show settings for non-merge request rules', async () => {
await findAllRuleSections().at(0).vm.$emit('changed', { type: 'scan_finding' });
expect(findSettingsSection().exists()).toBe(true);
expect(findSettingsSection().props('settings')).toEqual({});
});
it('does show the policy for merge request rule', async () => {
await findAllRuleSections().at(0).vm.$emit('changed', { type: 'any_merge_request' });
expect(findSettingsSection().props('settings')).toEqual({
...mergeRequestConfiguration,
});
});
it('updates the policy for merge request rule', async () => {
findAllRuleSections().at(0).vm.$emit('changed', { type: 'any_merge_request' });
await findSettingsSection().vm.$emit('changed', {
[PREVENT_APPROVAL_BY_AUTHOR]: false,
});
expect(findSettingsSection().props('settings')).toEqual({
...mergeRequestConfiguration,
[PREVENT_APPROVAL_BY_AUTHOR]: false,
});
});
});
describe('feature flags', () => {
describe('with feature flags', () => {
describe('with "scanResultPoliciesBlockUnprotectingBranches" feature flag enabled', () => {
beforeEach(() => {
const features = { scanResultPoliciesBlockUnprotectingBranches: true };
......@@ -765,42 +785,6 @@ describe('EditorComponent', () => {
});
});
describe('with "scanResultAnyMergeRequest" feature flag enabled', () => {
beforeEach(() => {
const features = { scanResultAnyMergeRequest: true };
window.gon = { features };
factory({ glFeatures: features });
});
it('displays setting section', () => {
expect(findSettingsSection().exists()).toBe(true);
});
it('does not show settings for non-merge request rules', async () => {
await findAllRuleSections().at(0).vm.$emit('changed', { type: 'scan_finding' });
expect(findSettingsSection().exists()).toBe(true);
expect(findSettingsSection().props('settings')).toEqual({});
});
it('does show the policy for merge request rule', async () => {
await findAllRuleSections().at(0).vm.$emit('changed', { type: 'any_merge_request' });
expect(findSettingsSection().props('settings')).toEqual({
...mergeRequestConfiguration,
});
});
it('updates the policy for merge request rule', async () => {
findAllRuleSections().at(0).vm.$emit('changed', { type: 'any_merge_request' });
await findSettingsSection().vm.$emit('changed', {
[PREVENT_APPROVAL_BY_AUTHOR]: false,
});
expect(findSettingsSection().props('settings')).toEqual({
...mergeRequestConfiguration,
[PREVENT_APPROVAL_BY_AUTHOR]: false,
});
});
});
describe('with "scanResultPoliciesBlockForcePush" feature flag enabled', () => {
beforeEach(() => {
const features = { scanResultPoliciesBlockForcePush: true };
......
......@@ -61,13 +61,6 @@ describe('fromYaml', () => {
expect(fromYaml(input)).toStrictEqual(output);
});
it('returns the policy object for a manifest with "approval_settings" with the "scanResultAnyMergeRequest" feature flag on', () => {
const input = { manifest: mockApprovalSettingsScanResultManifest, validateRuleMode: true };
const output = mockApprovalSettingsScanResultObject;
window.gon = { features: { scanResultAnyMergeRequest: true } };
expect(fromYaml(input)).toStrictEqual(output);
});
it('returns the policy object for a manifest with "approval_settings" with the "scanResultPoliciesBlockForcePush" feature flag on', () => {
const input = { manifest: mockApprovalSettingsScanResultManifest, validateRuleMode: true };
const output = mockApprovalSettingsScanResultObject;
......@@ -75,9 +68,9 @@ describe('fromYaml', () => {
expect(fromYaml(input)).toStrictEqual(output);
});
it('returns the error object for a manifest with "approval_settings" with all feature flags off', () => {
it('returns the policy object for a manifest with "approval_settings" with all feature flags off', () => {
const input = { manifest: mockApprovalSettingsScanResultManifest, validateRuleMode: true };
const output = { error: true };
const output = mockApprovalSettingsScanResultObject;
window.gon = { features: {} };
expect(fromYaml(input)).toStrictEqual(output);
});
......@@ -101,9 +94,7 @@ describe('createPolicyObject', () => {
${'returns the policy object for a manifest with `approval_settings` containing permitted invalid settings and the `scanResultPoliciesBlockUnprotectingBranches` feature flag on'} | ${{ scanResultPoliciesBlockUnprotectingBranches: true }} | ${mockApprovalSettingsPermittedInvalidScanResultManifest} | ${{ policy: mockApprovalSettingsPermittedInvalidScanResultObject, hasParsingError: false }}
${'returns the policy object for a manifest with `policy_scope` feature flag on'} | ${{ securityPoliciesPolicyScope: true }} | ${mockPolicyScopeScanResultManifest} | ${{ policy: mockPolicyScopeScanResultObject, hasParsingError: false }}
${'returns the error object for a manifest with `approval_settings` containing permitted invalid settings and the `scanResultPoliciesBlockUnprotectingBranches` feature flag off'} | ${{}} | ${mockApprovalSettingsPermittedInvalidScanResultManifest} | ${{ policy: mockApprovalSettingsPermittedInvalidScanResultObject, hasParsingError: false }}
${'returns the policy object for a manifest with `approval_settings` with the `scanResultAnyMergeRequest` feature flag on'} | ${{ scanResultAnyMergeRequest: true }} | ${mockApprovalSettingsScanResultManifest} | ${{ policy: mockApprovalSettingsScanResultObject, hasParsingError: false }}
${'returns the error object for a manifest with `approval_settings` with all feature flags off'} | ${{}} | ${mockApprovalSettingsScanResultManifest} | ${{ policy: { error: true }, hasParsingError: true }}
${'returns the error object for a manifest with `approval_settings` with all feature flags off'} | ${{}} | ${mockApprovalSettingsScanResultManifest} | ${{ policy: { error: true }, hasParsingError: true }}
${'returns the policy object for a manifest with `approval_settings` with all feature flags off'} | ${{}} | ${mockApprovalSettingsScanResultManifest} | ${{ policy: mockApprovalSettingsScanResultObject, hasParsingError: false }}
${'returns the error object for a manifest with `policy_scope` feature flag off'} | ${{}} | ${mockPolicyScopeScanResultManifest} | ${{ policy: { error: true }, hasParsingError: true }}
`('$title', ({ features, input, output }) => {
window.gon = { features };
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment