## Why are we doing this work We are extending policies to allow users to override project settings for MR approvals, namely: - Prevent approval by merge request's author - Prevent approval by anyone who added a commit - Remove all approvals when a commit is added - Require the user's password to approve ## Relevant links - See epic ## Non-functional requirements <!-- Add details for required items and delete others. --> - [x] Documentation: Document the new `approval_settings` option in YAML - [x] Feature flag: Put the changes behind a feature flag - [ ] Performance: Lookup of the approval settings for MR should perform well - [ ] Testing: ## Implementation plan - [x] MR 1: Extend `ee/app/validators/json_schemas/security_orchestration_policy.json` with new attributes Example YAML: ```yaml type: scan_result_policy name: MR approvals description: This policy overrides project approval settings enabled: true rules: - type: any_merge_request branch_type: protected # or branches: ['main'] commits: unsigned # or any actions: - type: require_approval approvals_required: 1 user_approvers: - the.one approval_settings: prevent_approval_by_author: true prevent_approval_by_commit_author: true remove_approvals_with_new_commit: true require_password_to_approve: false ``` ```diff diff --git a/ee/app/validators/json_schemas/security_orchestration_policy.json b/ee/app/validators/json_schemas/security_orchestration_policy.json index ad0ccacd30d7d95d23b0a35cfa23104a081efe76..ac5bdffbb114590bde6b9bea3802c7a7f785a22a 100644 --- a/ee/app/validators/json_schemas/security_orchestration_policy.json +++ b/ee/app/validators/json_schemas/security_orchestration_policy.json @@ -340,14 +340,15 @@ "type": { "enum": [ "scan_finding", - "license_finding" + "license_finding", + "any_merge_request" ], "type": "string", - "description": "Specified a type of the policy rule. `scan_finding`/`license_finding` rule enforces the defined actions based on the provided information." + "description": "Specified a type of the policy rule. `scan_finding`/`license_finding`/`any_merge_request` rule enforces the defined actions based on the provided information." }, "branches": { "type": "array", - "description": "Specifies a list of protected branches that should be concidered to enforce this policy.", + "description": "Specifies a list of protected branches that should be considered to enforce this policy.", "additionalItems": false, "items": { "minLength": 1, @@ -512,6 +513,14 @@ "detected" ] } + }, + "commits": { + "type": "string", + "description": "Specifies the commits to match.", + "enum": [ + "any", + "unsigned" + ] } }, "oneOf": [ @@ -561,6 +570,21 @@ "license_states" ] } + }, + { + "if": { + "properties": { + "type": { + "const": "any_merge_request" + } + } + }, + "then": { + "required": [ + "type", + "commits" + ] + } } ], "additionalProperties": false @@ -682,6 +706,24 @@ } } } + }, + "approval_settings": { + "type": "object", + "additionalProperties": false, + "properties": { + "prevent_approval_by_author": { + "type": "boolean" + }, + "prevent_approval_by_commit_author": { + "type": "boolean" + }, + "remove_approvals_with_new_commit": { + "type": "boolean" + }, + "require_password_to_approve": { + "type": "boolean" + } + } } }, "additionalProperties": false ``` - MR 2: Add new columns in `scan_result_policies` table and persist the values from YAML: - [x] New columns ```ruby add_column :scan_result_policies, :approval_settings, :jsonb, default: {}, null: false add_column :scan_result_policies, :commits, :smallint ``` - [x] Persist the values: ```diff diff --git a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb index 88bc0a2c302a64ca35dcdd398a4535e0123fd732..e4cad04318e2e192465e13f43b1c50cda0e7bd0c 100644 --- a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb +++ b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb @@ -26,7 +26,15 @@ def create_new_approval_rules policy[:rules].first(Security::ScanResultPolicy::LIMIT).each_with_index do |rule, rule_index| next unless rule_type_allowed?(rule[:type]) - scan_result_policy_read = create_scan_result_policy(rule, action_info, project, rule_index) + scan_result_policy_read = create_scan_result_policy( + rule, + action_info, + policy[:approval_settings], + project, + rule_index + ) create_software_license_policies(rule, rule_index, scan_result_policy_read) if license_finding?(rule) @@ -40,6 +48,10 @@ def license_finding?(rule) rule[:type] == Security::ScanResultPolicy::LICENSE_FINDING end def create_software_license_policies(rule, _rule_index, scan_result_policy_read) rule[:license_types].each do |license_type| create_params = { @@ -54,7 +66,7 @@ def create_software_license_policies(rule, _rule_index, scan_result_policy_read) end end - def create_scan_result_policy(rule, action_info, project, rule_index) + def create_scan_result_policy(rule, action_info, approval_settings, project, rule_index) policy_configuration.scan_result_policy_reads.create!( orchestration_policy_idx: policy_index, rule_idx: rule_index, @@ -65,7 +77,9 @@ def create_scan_result_policy(rule, action_info, project, rule_index) project_id: project.id, age_operator: rule.dig(:vulnerability_age, :operator), age_interval: rule.dig(:vulnerability_age, :interval), - age_value: rule.dig(:vulnerability_age, :value) + age_value: rule.dig(:vulnerability_age, :value), + commits: rule[:commits], + approval_settings: approval_settings || {} ) end @@ -116,7 +130,8 @@ def applies_to_all_protected_branches?(rule) def rule_type_allowed?(rule_type) [ Security::ScanResultPolicy::SCAN_FINDING, - Security::ScanResultPolicy::LICENSE_FINDING + Security::ScanResultPolicy::LICENSE_FINDING, + Security::ScanResultPolicy::ANY_MERGE_REQUEST ].include?(rule_type) end def report_type(rule_type) - rule_type == Security::ScanResultPolicy::LICENSE_FINDING ? :license_scanning : :scan_finding + case rule_type + when Security::ScanResultPolicy::LICENSE_FINDING + :license_scanning + when Security::ScanResultPolicy::ANY_MERGE_REQUEST + :any_merge_request + else + :scan_finding + end end def rule_name(policy_name, rule_index) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 49dab697e785..a9287bf72942 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -35,8 +35,7 @@ module ApprovalRuleLike vulnerability: 1, # To be removed after all MRs (related to https://gitlab.com/gitlab-org/gitlab/-/issues/356996) get merged license_scanning: 2, code_coverage: 3, - scan_finding: 4 + scan_finding: 4, + any_merge_request: 5 } validates :name, presence: true diff --git a/ee/app/models/concerns/security/scan_result_policy.rb b/ee/app/models/concerns/security/scan_result_policy.rb index c9056231496397b94a5ad3361b2722cde4a61190..1c43a2ecb5d90e71ca81207481ef679daba59c1b 100644 --- a/ee/app/models/concerns/security/scan_result_policy.rb +++ b/ee/app/models/concerns/security/scan_result_policy.rb @@ -11,6 +11,7 @@ module ScanResultPolicy SCAN_FINDING = 'scan_finding' LICENSE_FINDING = 'license_finding' + ANY_MERGE_REQUEST = 'any_merge_request' REQUIRE_APPROVAL = 'require_approval' ``` - MR 3: Enforce the policy and override the project settings - [ ] Extend `update_approvals_service` to remove required approvals ```diff diff --git a/ee/app/services/security/scan_result_policies/update_approvals_service.rb b/ee/app/services/security/scan_result_policies/update_approvals_service.rb index 3df6bb9771553d1ce7222100726ac3b61016a5d6..6f1d29e21685e7af8f82b814a09ae8372e8e4cac 100644 --- a/ee/app/services/security/scan_result_policies/update_approvals_service.rb +++ b/ee/app/services/security/scan_result_policies/update_approvals_service.rb @@ -13,6 +13,13 @@ def initialize(merge_request:, pipeline:) end def execute + update_scan_finding_rules + update_any_merge_request_rules + end + + private + + def update_scan_finding_rules approval_rules = merge_request.approval_rules.scan_finding return if approval_rules.empty? @@ -26,10 +33,24 @@ def execute violated_rules, unviolated_rules = partition_rules(approval_rules) ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any? generate_policy_bot_comment(violated_rules.any?) end - private + def update_any_merge_request_rules + approval_rules = merge_request.approval_rules.any_merge_request + return if approval_rules.empty? + + merge_request_has_unsigned_commits = !merge_request.commits(load_from_gitaly: true).all?(&:has_signature?) + violated_rules, unviolated_rules = approval_rules.including_scan_result_policy_read + .partition do |approval_rule| + scan_result_policy_read = approval_rule.scan_result_policy_read + scan_result_policy_read.commits_any? || + (scan_result_policy_read.commits_unsigned? && merge_request_has_unsigned_commits) + end + + ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any? + end delegate :project, to: :pipeline ``` - [ ] Add method to lookup approval settings for a `merge_request` ```diff diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 49dab697e785..c0cfac9eb7d1 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -28,7 +28,7 @@ module ApprovalRuleLike belongs_to :scan_result_policy_read, class_name: 'Security::ScanResultPolicyRead', foreign_key: 'scan_result_policy_id', - inverse_of: :security_orchestration_policy_configuration, + inverse_of: :approval_merge_request_rules, optional: true enum report_type: { @@ -49,7 +49,6 @@ module ApprovalRuleLike scope :with_users, -> { preload(:users, :group_users) } scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) } + scope :scan_finding_or_license_scanning, -> { where(report_type: %i[scan_finding license_scanning]) } scope :for_groups, -> (groups) { joins(:groups).where(approval_project_rules_groups: { group_id: groups }) } scope :including_scan_result_policy_read, -> { includes(:scan_result_policy_read) } scope :with_scan_result_policy_read, -> { where.not(scan_result_policy_id: nil) } diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb index 7be63a51646e84f57c637504ea5790572801de69..d08aea59b9432c1e9c3eb87ab05836d51abffcab 100644 --- a/ee/app/models/security/scan_result_policy_read.rb +++ b/ee/app/models/security/scan_result_policy_read.rb @@ -8,10 +8,12 @@ class ScanResultPolicyRead < ApplicationRecord enum age_operator: { greater_than: 0, less_than: 1 } enum age_interval: { day: 0, week: 1, month: 2, year: 3 } + enum commits: { any: 0, unsigned: 1 }, _prefix: true belongs_to :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration' belongs_to :project, optional: true has_many :software_license_policies + has_many :approval_merge_request_rules, foreign_key: 'scan_result_policy_id', inverse_of: :scan_result_policy_read validates :match_on_inclusion, inclusion: { in: [true, false], message: 'must be a boolean value' } validates :role_approvers, inclusion: { in: Gitlab::Access.all_values } @@ -31,5 +33,26 @@ def vulnerability_age { operator: age_operator.to_sym, interval: age_interval.to_sym, value: age_value } end + + def self.approval_settings_for_merge_request(merge_request) + commits_lookup = %i[any] + commits_lookup << :unsigned unless merge_request.commits(load_from_gitaly: true).all?(&:has_signature?) + # scan_finding and license_scanning should enforce approval_settings only when there is a violation, + # which we can only find out by approvals_required > 0. + # If policy doesn't require approvals, we may still have violations and should enforce approval_settings, + # but we wouldn't find them at this point. + # Should we store `violations: true/false` for approval_merge_request_rules? + enforced_merge_request_rules = merge_request.approval_rules.scan_finding_or_license_scanning + .where(ApprovalMergeRequestRule.arel_table[:approvals_required].gt(0)) + # any_merge_request should enforce approval_settings based on commits type + all_approval_settings = joins(:approval_merge_request_rules) + .where(project: merge_request.target_project, commits: commits_lookup) + .or(enforced_merge_request_rules) + .pluck(:approval_settings) + + all_approval_settings.each_with_object({}) do |setting, hash| + setting.each { |key, value| hash[key] = value if value } + end.symbolize_keys + end end end ``` - [ ] `prevent_approval_by_author ` and `prevent_approval_by_commit_author` ```diff diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index f5dde9bd606d69c3305ca427a216ecc37093e193..24bd77f0d47cd4969db154801d24983be4d46b29 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -21,7 +21,7 @@ def initialize(merge_request, target_branch: nil) # Excludes the author if 'author-approval' is explicitly disabled on project settings. def self.filter_author(users, merge_request) - return users if merge_request.target_project.merge_requests_author_approval? + return users if merge_request.merge_requests_author_approval? if users.is_a?(ActiveRecord::Relation) && !users.loaded? users.where.not(id: merge_request.author_id).allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/417459") @@ -32,7 +32,7 @@ def self.filter_author(users, merge_request) # Excludes the author if 'committers-approval' is explicitly disabled on project settings. def self.filter_committers(users, merge_request) - return users unless merge_request.target_project.merge_requests_disable_committers_approval? + return users unless merge_request.merge_requests_disable_committers_approval? with_merge_commits = Feature.enabled?(:keep_merge_commits_for_approvals, merge_request.target_project) @@ -181,11 +181,11 @@ def eligible_for_approval_by?(user) end def authors_can_approve? - project.merge_requests_author_approval? + merge_request.merge_requests_author_approval? end def committers_can_approve? - !project.merge_requests_disable_committers_approval? + !merge_request.merge_requests_disable_committers_approval? end # TODO: remove after #1979 is closed ``` - [ ] `remove_approvals_with_new_commit` ```diff diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb index e14892443e2bd734344f59cf78bb7f2fa3f9aa25..7924c67f9e0e13f3f880b179d9c1f07ff9e3ebcd 100644 --- a/ee/app/services/ee/merge_requests/base_service.rb +++ b/ee/app/services/ee/merge_requests/base_service.rb @@ -39,14 +39,22 @@ def filter_suggested_reviewers end def reset_approvals?(merge_request, _newrev) - (merge_request.target_project.reset_approvals_on_push || - merge_request.target_project.project_setting.selective_code_owner_removals) + delete_approvals?(merge_request) || merge_request.target_project.project_setting.selective_code_owner_removals + end + + def delete_approvals?(merge_request) + merge_request.target_project.reset_approvals_on_push || + policy_approval_settings(merge_request)[:remove_approvals_with_new_commit] + end + + def policy_approval_settings(merge_request) + Security::ScanResultPolicyRead.approval_settings_for_merge_request(merge_request) end def reset_approvals(merge_request, newrev = nil) return unless reset_approvals?(merge_request, newrev) - if merge_request.target_project.reset_approvals_on_push + if delete_approvals?(merge_request) delete_approvals(merge_request) elsif merge_request.target_project.project_setting.selective_code_owner_removals delete_code_owner_approvals(merge_request) ``` - [ ] `require_password_to_approve` ```diff diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index fce8f72bb1ea63db0d0921619b343f422e1ab024..290ca7a7306ab47c5fae4fad70448447e9779511 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -100,11 +100,22 @@ def sync_merge_request_compliance_violation end def merge_requests_author_approval? - !!target_project&.merge_requests_author_approval? + !!target_project&.merge_requests_author_approval? && !policy_approval_settings[:prevent_approval_by_author] end def merge_requests_disable_committers_approval? - !!target_project&.merge_requests_disable_committers_approval? + !!target_project&.merge_requests_disable_committers_approval? || + policy_approval_settings[:prevent_approval_by_commit_author] + end + + def require_password_to_approve? + target_project&.require_password_to_approve? || policy_approval_settings[:require_password_to_approve] + end + + def policy_approval_settings + strong_memoize(:policy_approval_settings) do + Security::ScanResultPolicyRead.approval_settings_for_merge_request(self) + end end end diff --git a/ee/app/serializers/ee/merge_request_widget_entity.rb b/ee/app/serializers/ee/merge_request_widget_entity.rb index 6d18ac96b7a33dd782d4545d07e016d3ebc9e35c..b29c9b0bc589c67c88f2671541fd8346409afe8f 100644 --- a/ee/app/serializers/ee/merge_request_widget_entity.rb +++ b/ee/app/serializers/ee/merge_request_widget_entity.rb @@ -7,7 +7,7 @@ module MergeRequestWidgetEntity prepended do expose :require_password_to_approve do |merge_request| - merge_request.target_project.require_password_to_approve? + merge_request.require_password_to_approve? end expose :merge_request_approvers_available do |merge_request| diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb index 019a013168587c5b4fcab8d9739f8334b3a0120a..b28fe283820fdb658bdc962e143e1a9236d34474 100644 --- a/ee/app/services/ee/merge_requests/approval_service.rb +++ b/ee/app/services/ee/merge_requests/approval_service.rb @@ -17,7 +17,7 @@ def execute(merge_request) private def incorrect_approval_password?(merge_request) - merge_request.project.require_password_to_approve? && + merge_request.require_password_to_approve? && !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password]) end ``` ## Verification steps ### Requiring approvals based on commits signature 1. Go to Secure -> Policies and create a scan result policies with the example YAML: ```yaml type: scan_result_policy name: MR approvals description: This policy requires approvals for unsigned commits and overrides project approval settings enabled: true rules: - type: any_merge_request branch_type: protected commits: unsigned actions: - type: require_approval approvals_required: 1 user_approvers: - the.one approval_settings: prevent_approval_by_author: true prevent_approval_by_commit_author: true remove_approvals_with_new_commit: true require_password_to_approve: true ``` 1. Go to Settings -> Merge requests and set all the approval settings to `false` 1. Create MR using unsigned commits 1. Check that each setting is enforced 1. Check that a rule corresponding to the policy is present and an approval is required 2. Create MR using signed commits and check that setting is not enforced 3. Change the policy to use `commits: any` and check that the settings are enforced also for MR with signed commits 4. Create MR for unprotected branch and check that the policy is not enforced 5. Change the target branch to a protected one and check that policy is enforced ### Only overriding project settings 1. Go to Secure -> Policies and create a scan result policies with the example YAML: ```yaml type: scan_result_policy name: Approval settings override description: This policy overrides project approval settings enabled: true rules: - type: any_merge_request branch_type: protected commits: any approval_settings: prevent_approval_by_author: true prevent_approval_by_commit_author: true remove_approvals_with_new_commit: true require_password_to_approve: true ``` 1. Go to Settings -> Merge requests and set all the approval settings to `false` 1. Create MR 1. Check that no rule has been created 1. Check that each setting is enforced 2. Create MR for unprotected branch and check that the policy is not enforced 5. Change the target branch to a protected one and check that policy is enforced