Fail closed for invalid Security Policy approval checks
Problem
This issue was created from the Discussion for #374602 Situation 4 invalid approver configurations. The current behavior of an invalid MR policy/rule is that it will fail open when it is invalid. When a policy/rule becomes invalid, the policy will automatically pass.
For a company's security team, when they set up a policy requiring approval for an MR if the edge case happens that the policy or rule becomes invalid, the user doesn't want it to pass. Instead, they prefer that the MR fail close and let the team approve or fix the policy. This insight is confirmed by many unrecorded customer calls and some of recorded user study.
From discussion with the code review team, we agreed that fail close should be a consistent behavior for all. The question is how to do it because some users rely on the current behavior. To unblock this work, we plan on iterating toward the end goal by allowing security policy approval checks to fail closed as a first iteration. The behavior of failing to close for other approval rules can then be researched and implemented separately.
Fail closed merge request approval means we will indicate that the given approval rule is invalid and must be fixed before the given MR can be merged.
Release Notes
Security and compliance policies allow organizations to enforce checks and balances across multiple projects to align with their security and governance programs. It's critical for our customers to ensure changes that impact policies do not result in the guardrails coming down. With this update, invalid rules will "fail closed", blocking MRs until invalid rules in any scan result policies are addressed.
Proposal
- In thread, groupcode review agrees to start with
security & compliance policy
approval rules in implementing fail closed for invalid rules. Other approvals and checks should follow suit in future plans outside of the scope of this issue. - Any MR approval checks created through a Security & Compliance Policy will fail closed rather than entering an invalid state. This behavior will not apply to any MR approval rules that were not created through a Security & Compliance Policy (within the scope of this issue).
- If an invalid security or compliance policy is enabled, or a policy becomes invalid after being enabled, we will present users in the MR widget with a status of
(!) Action Required
. This is the fail-closed state. - For any invalid approval rules that are not security or compliance policy checks, we will present users in the MR widget with an
(?) Auto approved
status. This is a fail-open state. - All relevant documentation will be updated to explain the differences between the Invalid fail-open state and the Invalid fail-closed state.
Design proposal
- Please see the design area changes
- We must also update all the related documentation regarding invalid rules! Both in the Merge request and security policy area
Implementation plan
-
backend Add/modify methods in ApprovalWrappedRule
that indicates if the given rule is and can be invalid (behind feature flag):def approved? strong_memoize(:approved) do approvals_left <= 0 || (invalid_rule? && allow_merge_when_invalid?) end end def invalid_rule? unactioned_approvers.size <= 0 end def allow_merge_when_invalid? return true if Feature.disabled?(:invalid_scan_result_policy_prevents_merge, merge_request.project) !approval_rule.from_scan_result_policy? end
-
backend Modify ApprovalRuleLike
to include a method to indicate if the given policy was created from the scan result policy:def from_scan_result_policy? scan_finding? || (license_scanning? && scan_result_policy_read.exists?) end
-
backend Modify Types::ApprovalRuleType
to include new fields:field :invalid, type: GraphQL::Types::Boolean, method: :invalid_rule?, null: true, description: 'Indicates if the rule is invalid and cannot be approved.' field :allow_merge_when_invalid, type: GraphQL::Types::Boolean, method: :allow_merge_when_invalid?, null: true, description: 'Indicates if the rule can be ignored if is invalid.'
-
frontend Modify ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql
andvue_merge_request_widget/components/approvals/queries/approval_rules.query.graphql
to includeinvalid
andallowMergeWhenInvalid
fields inapprovalState
->rules
, -
frontend modify app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
to include modified text based on the rule status (invalid
/auto approved
): -
frontend Modify vue_merge_request_widget/components/approvals/number_of_approvals.vue
to modify values in theApprovals
column to returnⓘ Auto approved
with a tooltip from designs whenrule
isinvalid
andallowMergeWhenInvalid
and to returnⓘ Action required
with tooltip from designs whenrule
isinvalid
and notallowMergeWhenInvalid
:
Incomplete frontend patch
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
index c66c5be68cc4..9c42552e71b6 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
@@ -45,10 +45,6 @@ export default {
type: String,
required: true,
},
- invalidApproversRules: {
- type: Array,
- required: true,
- },
securityApprovalsHelpPagePath: {
type: String,
required: false,
@@ -248,7 +244,7 @@ export default {
/>
</td>
<td class="d-md-table-cell w-0 d-none gl-white-space-nowrap gl-w-full js-pending">
- <number-of-approvals :rule="rule" :invalid-approvers-rules="invalidApproversRules" />
+ <number-of-approvals :rule="rule" />
</td>
<td class="d-md-table-cell d-none gl-w-full js-commented-by">
<user-avatar-list
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/number_of_approvals.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/number_of_approvals.vue
index da14de5fb869..34c53ca78791 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/number_of_approvals.vue
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/number_of_approvals.vue
@@ -15,10 +15,6 @@ export default {
type: Object,
required: true,
},
- invalidApproversRules: {
- type: Array,
- required: true,
- },
},
computed: {
pendingApprovalsText() {
@@ -34,7 +30,7 @@ export default {
});
},
hasInvalidRules() {
- return this.invalidApproversRules.some((invalidRule) => invalidRule.id === this.rule.id);
+ return this.rule.invalid;
},
},
rulesDocsPath: helpPagePath('user/project/merge_requests/approvals/rules.html', {
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approval_rules.query.graphql b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approval_rules.query.graphql
index 31f893f5bdd7..890af6bcb23e 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approval_rules.query.graphql
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approval_rules.query.graphql
@@ -12,8 +12,10 @@ query approvalRules($projectPath: ID!, $iid: String!) {
rules {
id
type
+ allowMergeWhenInvalid
approved
approvalsRequired
+ invalid
name
section
approvedBy {
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql
index 68bd402045a2..0653b2fe41ca 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql
@@ -19,8 +19,10 @@ query approvedByEE($projectPath: ID!, $iid: String!) {
}
rules {
id
+ allowMergeWhenInvalid
approved
approvalsRequired
+ invalid
name
type
}
-
documentation update documentation in https://docs.gitlab.com/ee/user/project/merge_requests/approvals/#invalid-rules to mention that invalid rules created from Scan Result Policies are not automatically approved,
Verification steps
- Create a new security policy with secret detection and require more approvals than number of selected users
- Configure with merge request & Merge
- Open an MR which adds a leaked secret, thus violating the policy
- Add another approval rules to the same MR, which also doesn't have enough approvers See approval rules in this example
- Verify that non-security rule is automatically approved
- Verify that secret detection rule is not approved and shows "Action required"