From bdcaf420d31622af8c46b8049d8e614d086eb19c Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Mon, 11 Aug 2025 17:24:37 +0200 Subject: [PATCH] Add MR bypass security policy graphql API This change adds a new graphql mutation to bypass a security policy at MR level. This also adds new fields to approval rule graphql type to check if the MR can be bypassed by the current user. EE: true Changelog: added --- doc/api/graphql/reference/_index.md | 26 ++ doc/user/compliance/audit_event_types.md | 1 + ee/app/graphql/ee/types/mutation_type.rb | 1 + .../merge_requests/bypass_security_policy.rb | 53 +++ ee/app/graphql/types/approval_rule_type.rb | 26 ++ ee/app/models/approval_wrapped_rule.rb | 29 +- ee/app/models/security/policy.rb | 9 + .../bypass_security_policy_service.rb | 61 ++++ .../security_policy_merge_request_bypass.yml | 10 + ...rity_policies_bypass_options_mr_widget.yml | 9 + .../policy_bypass_auditor.rb | 57 +++- ee/spec/factories/security/policies.rb | 44 ++- .../graphql/types/approval_rule_type_spec.rb | 2 +- .../policy_bypass_auditor_spec.rb | 31 ++ ee/spec/models/approval_wrapped_rule_spec.rb | 118 +++++++ ee/spec/models/security/policy_spec.rb | 76 ++++- .../bypass_security_policy_spec.rb | 200 ++++++++++++ .../bypass_security_policy_service_spec.rb | 308 ++++++++++++++++++ 18 files changed, 1042 insertions(+), 19 deletions(-) create mode 100644 ee/app/graphql/mutations/merge_requests/bypass_security_policy.rb create mode 100644 ee/app/services/merge_requests/bypass_security_policy_service.rb create mode 100644 ee/config/audit_events/types/security_policy_merge_request_bypass.yml create mode 100644 ee/config/feature_flags/wip/security_policies_bypass_options_mr_widget.yml create mode 100644 ee/spec/requests/api/graphql/mutations/merge_requests/bypass_security_policy_spec.rb create mode 100644 ee/spec/services/merge_requests/bypass_security_policy_service_spec.rb diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index f59e2d0ef0f357..caeea6e2c85603 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -8918,6 +8918,30 @@ Input type: `MergeRequestAcceptInput` | `errors` | [`[String!]!`](#string) | Errors encountered during the mutation. | | `mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | +### `Mutation.mergeRequestBypassSecurityPolicy` + +Bypasses a security policy approval rule for a merge request.Does not mutate the object if `security_policies_bypass_options_mr_widget` feature flag is disabled. + +Input type: `MergeRequestBypassSecurityPolicyInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `approvalRuleId` | [`Int!`](#int) | ID of the approval rule to bypass. | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `iid` | [`String!`](#string) | IID of the merge request to mutate. | +| `projectPath` | [`ID!`](#id) | Project the merge request to mutate is in. | +| `reason` | [`String!`](#string) | Reason for bypassing the security policy. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during the mutation. | +| `mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | + ### `Mutation.mergeRequestCreate` Input type: `MergeRequestCreateInput` @@ -23635,10 +23659,12 @@ Describes a rule for who can approve merge requests. | Name | Type | Description | | ---- | ---- | ----------- | +| `allowBypass` {{< icon name="warning-solid" >}} | [`Boolean`](#boolean) | **Introduced** in GitLab 18.4. **Status**: Experiment. Indicates if the rule can be bypassed.Returns `false` if the feature flag `security_policies_bypass_options_mr_widget` is disabled. | | `allowMergeWhenInvalid` | [`Boolean`](#boolean) | Indicates if the rule can be ignored if it is invalid. | | `approvalsRequired` | [`Int`](#int) | Number of required approvals. | | `approved` | [`Boolean`](#boolean) | Indicates if the rule is satisfied. | | `approvedBy` | [`UserCoreConnection`](#usercoreconnection) | List of users defined in the rule that approved the merge request. (see [Connections](#connections)) | +| `bypassed` {{< icon name="warning-solid" >}} | [`Boolean`](#boolean) | **Introduced** in GitLab 18.4. **Status**: Experiment. Indicates if the rule was bypassed for the merge request.Returns `false` if the feature flag `security_policies_bypass_options_mr_widget` is disabled. | | `commentedBy` | [`UserCoreConnection`](#usercoreconnection) | List of users, defined in the rule, who commented on the merge request. (see [Connections](#connections)) | | `containsHiddenGroups` | [`Boolean`](#boolean) | Indicates if the rule contains approvers from a hidden group. | | `eligibleApprovers` | [`[UserCore!]`](#usercore) | List of all users eligible to approve the merge request (defined explicitly and from associated groups). | diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 538e1a676a37c2..fb911d78b03f0d 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -543,6 +543,7 @@ Audit event types belong to the following product categories. | [`security_policy_create`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/192797) | A security policy is created | {{< icon name="check-circle" >}} Yes | GitLab [18.1](https://gitlab.com/gitlab-org/gitlab/-/issues/539230) | Project | | [`security_policy_delete`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/192797) | A security policy is deleted | {{< icon name="check-circle" >}} Yes | GitLab [18.1](https://gitlab.com/gitlab-org/gitlab/-/issues/539230) | Project | | [`security_policy_limit_exceeded`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/196005) | Enabled policies count exceeded the maximum allowed limit for policy type | {{< icon name="check-circle" >}} Yes | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/work_items/550891) | Project | +| [`security_policy_merge_request_bypass`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/201001) | A security policy is bypassed for a merge request | {{< icon name="check-circle" >}} Yes | GitLab [18.4](https://gitlab.com/gitlab-org/gitlab/-/issues/549797) | Project | | [`security_policy_merge_request_merged_with_policy_violations`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195775) | A merge request merged with security policy violations | {{< icon name="check-circle" >}} Yes | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/work_items/549813) | Project | | [`security_policy_pipeline_failed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/196628) | A pipeline with security policy jobs failed | {{< icon name="dotted-circle" >}} No | GitLab [18.3](https://gitlab.com/gitlab-org/gitlab/-/issues/539232) | Project | | [`security_policy_pipeline_skipped`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195325) | A security policy pipeline is skipped | {{< icon name="dotted-circle" >}} No | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/issues/539232) | Project | diff --git a/ee/app/graphql/ee/types/mutation_type.rb b/ee/app/graphql/ee/types/mutation_type.rb index cba60e283791d3..bd9f1ad4e69b17 100644 --- a/ee/app/graphql/ee/types/mutation_type.rb +++ b/ee/app/graphql/ee/types/mutation_type.rb @@ -221,6 +221,7 @@ def self.authorization_scopes mount_mutation ::Mutations::Deployments::DeploymentApprove mount_mutation ::Mutations::MergeRequests::UpdateApprovalRule mount_mutation ::Mutations::MergeRequests::DestroyRequestedChanges + mount_mutation ::Mutations::MergeRequests::BypassSecurityPolicy mount_mutation ::Mutations::Ai::Action, experiment: { milestone: '15.11' }, scopes: [:api, :ai_features] mount_mutation ::Mutations::Ai::DuoUserFeedback, experiment: { milestone: '16.10' diff --git a/ee/app/graphql/mutations/merge_requests/bypass_security_policy.rb b/ee/app/graphql/mutations/merge_requests/bypass_security_policy.rb new file mode 100644 index 00000000000000..bfe1a8fd85ec6c --- /dev/null +++ b/ee/app/graphql/mutations/merge_requests/bypass_security_policy.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module Mutations + module MergeRequests + class BypassSecurityPolicy < Base + graphql_name 'MergeRequestBypassSecurityPolicy' + description 'Bypasses a security policy approval rule for a merge request.' \ + 'Does not mutate the object if `security_policies_bypass_options_mr_widget` feature flag is disabled.' + + argument :approval_rule_id, + GraphQL::Types::Int, + required: true, + description: 'ID of the approval rule to bypass.' + + argument :reason, + GraphQL::Types::String, + required: true, + description: 'Reason for bypassing the security policy.' + + def resolve(project_path:, iid:, approval_rule_id:, reason:) + merge_request = authorized_find!(project_path: project_path, iid: iid) + + unless Feature.enabled?(:security_policies_bypass_options_mr_widget, merge_request.project) + raise raise_resource_not_available_error!( + '`security_policies_bypass_options_mr_widget` feature flag is disabled.' + ) + end + + approval_rule = find_merge_request_approval_rule(merge_request, approval_rule_id) + raise raise_resource_not_available_error!('Approval rule not found') unless approval_rule + + service = ::MergeRequests::BypassSecurityPolicyService.new( + merge_request: merge_request, + approval_rule: approval_rule, + current_user: current_user + ) + + result = service.execute(reason) + + { + merge_request: merge_request, + errors: result.success? ? [] : [result.message] + } + end + + private + + def find_merge_request_approval_rule(merge_request, id) + merge_request.approval_rules.find_by_id(id) + end + end + end +end diff --git a/ee/app/graphql/types/approval_rule_type.rb b/ee/app/graphql/types/approval_rule_type.rb index cd6c204101044c..e7f8be4d3e9f25 100644 --- a/ee/app/graphql/types/approval_rule_type.rb +++ b/ee/app/graphql/types/approval_rule_type.rb @@ -98,9 +98,35 @@ class ApprovalRuleType < BaseObject null: true, description: 'Indicates if the rule can be ignored if it is invalid.' + field :allow_bypass, # rubocop:disable GraphQL/ExtractType -- will keep it separate for backward compatibility + type: GraphQL::Types::Boolean, + null: true, + experiment: { milestone: '18.4' }, + description: 'Indicates if the rule can be bypassed.' \ + 'Returns `false` if the feature flag `security_policies_bypass_options_mr_widget` is disabled.' + + field :bypassed, + type: GraphQL::Types::Boolean, + null: true, + experiment: { milestone: '18.4' }, + description: 'Indicates if the rule was bypassed for the merge request.' \ + 'Returns `false` if the feature flag `security_policies_bypass_options_mr_widget` is disabled.' + field :scan_result_policies, type: [Types::SecurityOrchestration::ApprovalScanResultPolicyType], null: true, description: 'List of scan result policies associated with the rule.' + + def allow_bypass + return false if Feature.disabled?(:security_policies_bypass_options_mr_widget, object.project) + + object.allow_bypass?(context[:current_user]) + end + + def bypassed + return false if Feature.disabled?(:security_policies_bypass_options_mr_widget, object.project) + + object.bypassed? + end end end diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index 94f21603d39e90..3f1858973b5ed4 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -88,10 +88,33 @@ def commented_approvers def approved? strong_memoize(:approved) do - approvals_left <= 0 || (invalid_rule? && allow_merge_when_invalid?) + approvals_left <= 0 || (invalid_rule? && allow_merge_when_invalid?) || bypassed? end end + def bypassed? + return false if ::Feature.disabled?(:security_policies_bypass_options_group_roles, project) + return false unless from_scan_result_policy? + + merge_request.approval_policy_merge_request_bypass_events.any? + end + + def allow_bypass?(user) + return false if Feature.disabled?(:security_policies_bypass_options_group_roles, project) + return false unless from_scan_result_policy? + + security_policy = approval_policy_rule.security_policy + bypass_settings = security_policy.bypass_settings + return false if bypass_settings.user_ids.empty? && bypass_settings.group_ids.empty? && + bypass_settings.default_roles.empty? && bypass_settings.custom_role_ids.empty? + + user_bypass_checker = Security::ScanResultPolicies::UserBypassChecker.new( + security_policy: security_policy, project: project, current_user: user + ) + + user_bypass_checker.bypass_scope.present? + end + def invalid_rule? !any_approver? && approvals_required > approvers.size end @@ -156,6 +179,10 @@ def fail_open? approval_rule.scan_result_policy_read&.fail_open? || false end + def approval_policy_rule + approval_rule.approval_policy_rule + end + private def filter_approvers(approvers) diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 67406a33b63370..49843397131d54 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -344,6 +344,15 @@ def supports_policy_rules? Security::PolicyRule::SUPPORTED_POLICY_TYPES.include?(type.to_sym) end + def create_merge_request_bypass_event!(project:, user:, reason:, merge_request:) + approval_policy_merge_request_bypass_events.create!( + project: project, + user: user, + reason: reason, + merge_request: merge_request + ) + end + private def content_by_type diff --git a/ee/app/services/merge_requests/bypass_security_policy_service.rb b/ee/app/services/merge_requests/bypass_security_policy_service.rb new file mode 100644 index 00000000000000..078fdb89dac68a --- /dev/null +++ b/ee/app/services/merge_requests/bypass_security_policy_service.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module MergeRequests + class BypassSecurityPolicyService + include ::Gitlab::Utils::StrongMemoize + + def initialize(merge_request:, approval_rule:, current_user:) + @merge_request = merge_request + @approval_rule = approval_rule + @current_user = current_user + end + + def execute(reason) + return error('You have already bypassed this security policy.') if approval_wrapped_rule.bypassed? + + unless approval_wrapped_rule.allow_bypass?(current_user) + return error('You are not allowed to bypass this security policy.') + end + + bypass_approval_rule(reason) + success(merge_request) + end + + private + + attr_reader :merge_request, :approval_rule, :current_user + + def approval_wrapped_rule + ApprovalWrappedRule.wrap(merge_request, approval_rule) + end + strong_memoize_attr :approval_wrapped_rule + + def bypass_approval_rule(reason) + security_policy = approval_rule.approval_policy_rule.security_policy + + security_policy.create_merge_request_bypass_event!( + project: merge_request.project, + user: current_user, + reason: reason, + merge_request: merge_request + ) + + auditor = Security::ScanResultPolicies::PolicyBypassAuditor.new( + security_policy: security_policy, + project: merge_request.project, + user: current_user, + branch_name: merge_request.target_branch + ) + + auditor.log_merge_request_bypass(merge_request, approval_rule, reason) + end + + def error(message) + ServiceResponse.error(message: message) + end + + def success(merge_request) + ServiceResponse.success(payload: { merge_request: merge_request }) + end + end +end diff --git a/ee/config/audit_events/types/security_policy_merge_request_bypass.yml b/ee/config/audit_events/types/security_policy_merge_request_bypass.yml new file mode 100644 index 00000000000000..8d8eabd74d739b --- /dev/null +++ b/ee/config/audit_events/types/security_policy_merge_request_bypass.yml @@ -0,0 +1,10 @@ +--- +name: security_policy_merge_request_bypass +description: A security policy is bypassed for a merge request +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/549797 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/201001 +feature_category: security_policy_management +milestone: '18.4' +saved_to_database: true +streamed: true +scope: [Project] diff --git a/ee/config/feature_flags/wip/security_policies_bypass_options_mr_widget.yml b/ee/config/feature_flags/wip/security_policies_bypass_options_mr_widget.yml new file mode 100644 index 00000000000000..5faefe10618428 --- /dev/null +++ b/ee/config/feature_flags/wip/security_policies_bypass_options_mr_widget.yml @@ -0,0 +1,9 @@ +--- +name: security_policies_bypass_options_mr_widget +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/541468 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/202259 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/563875 +milestone: '18.4' +group: group::security policies +type: wip +default_enabled: false diff --git a/ee/lib/security/scan_result_policies/policy_bypass_auditor.rb b/ee/lib/security/scan_result_policies/policy_bypass_auditor.rb index a858bb6c5a8977..2bbee89299f689 100644 --- a/ee/lib/security/scan_result_policies/policy_bypass_auditor.rb +++ b/ee/lib/security/scan_result_policies/policy_bypass_auditor.rb @@ -25,15 +25,21 @@ def log_user_bypass(user_bypass_scope, reason) log_bypass_event(:user, user.id, audit_details, reason) end + def log_merge_request_bypass(merge_request, approval_rule, reason) + audit_details = build_merge_request_audit_details(merge_request, approval_rule, reason) + log_bypass_event(:merge_request, approval_rule, audit_details, reason, merge_request) + end + private attr_reader :security_policy, :project, :user, :branch_name - def log_bypass_event(bypass_type, identifier, additional_details, reason = nil) - message = bypass_audit_message(bypass_type, identifier, reason) + def log_bypass_event(bypass_type, identifier, additional_details, reason = nil, merge_request = nil) + message = build_audit_message(bypass_type, identifier, reason, merge_request) + audit_name = audit_name_for_bypass_type(bypass_type) Gitlab::Audit::Auditor.audit( - name: "security_policy_#{bypass_type}_push_bypass", + name: audit_name, author: user, scope: security_policy.security_policy_management_project, target: security_policy, @@ -69,6 +75,17 @@ def build_user_audit_details(user_bypass_scope, reason) }.merge(bypass_scope_details(user_bypass_scope)) end + def build_merge_request_audit_details(merge_request, approval_rule, reason) + { + bypass_type: :merge_request, + approval_rule_id: approval_rule.id, + merge_request_id: merge_request.id, + merge_request_iid: merge_request.iid, + approval_rule_name: approval_rule.name, + reason: reason + } + end + def bypass_scope_details(user_bypass_scope) case user_bypass_scope when :group @@ -83,15 +100,39 @@ def bypass_scope_details(user_bypass_scope) end end - def bypass_audit_message(type, identifier, reason = nil) - message = <<~MSG.squish - Branch push restriction on '#{branch_name}' for project '#{project.full_path}' - has been bypassed by #{type} with ID: #{identifier} - MSG + def audit_name_for_bypass_type(bypass_type) + case bypass_type + when :merge_request + 'security_policy_merge_request_bypass' + else + "security_policy_#{bypass_type}_push_bypass" + end + end + def build_audit_message(bypass_type, identifier, reason, merge_request) + message = case bypass_type + when :merge_request + merge_request_audit_message(identifier, merge_request) + else + bypass_audit_message(bypass_type, identifier) + end message += " with reason: #{reason}" if reason.present? message end + + def merge_request_audit_message(approval_rule, merge_request) + <<~MSG.squish + Approval rule #{approval_rule.name} in merge request + (#{project.full_path}!#{merge_request.iid}) has been bypassed by #{user.name} + MSG + end + + def bypass_audit_message(type, identifier) + <<~MSG.squish + Branch push restriction on '#{branch_name}' for project '#{project.full_path}' + has been bypassed by #{type} with ID: #{identifier} + MSG + end end end end diff --git a/ee/spec/factories/security/policies.rb b/ee/spec/factories/security/policies.rb index e42f01f9b9b1c5..9ea65405488c96 100644 --- a/ee/spec/factories/security/policies.rb +++ b/ee/spec/factories/security/policies.rb @@ -50,22 +50,50 @@ transient do bypass_access_token_ids { [] } bypass_service_account_ids { [] } + bypass_user_ids { [] } + bypass_group_ids { [] } + bypass_default_roles { [] } + bypass_custom_role_ids { [] } end - after(:build) do |policy, evaluator| - next if evaluator.bypass_access_token_ids.blank? + after(:create) do |policy, evaluator| + next if evaluator.bypass_access_token_ids.blank? && evaluator.bypass_service_account_ids.blank? && + evaluator.bypass_user_ids.blank? && evaluator.bypass_group_ids.blank? && + evaluator.bypass_default_roles.blank? && evaluator.bypass_custom_role_ids.blank? policy.content ||= {} policy.content[:bypass_settings] ||= {} - policy.content[:bypass_settings][:access_tokens] = evaluator.bypass_access_token_ids.map do |token_id| - { id: token_id } + + unless evaluator.bypass_access_token_ids.blank? + policy.content[:bypass_settings][:access_tokens] = evaluator.bypass_access_token_ids.map do |token_id| + { id: token_id } + end end - next if evaluator.bypass_service_account_ids.blank? + unless evaluator.bypass_service_account_ids.blank? + policy.content[:bypass_settings][:service_accounts] = evaluator + .bypass_service_account_ids.map { |service_account_id| { id: service_account_id } } + end - policy.content[:bypass_settings] ||= {} - policy.content[:bypass_settings][:service_accounts] = evaluator - .bypass_service_account_ids.map { |service_account_id| { id: service_account_id } } + unless evaluator.bypass_user_ids.blank? + policy.content[:bypass_settings][:users] = evaluator.bypass_user_ids.map { |user_id| { id: user_id } } + end + + unless evaluator.bypass_group_ids.blank? + policy.content[:bypass_settings][:groups] = evaluator.bypass_group_ids.map { |group_id| { id: group_id } } + end + + unless evaluator.bypass_default_roles.blank? + policy.content[:bypass_settings][:roles] = evaluator.bypass_default_roles + end + + unless evaluator.bypass_custom_role_ids.blank? + policy.content[:bypass_settings][:custom_roles] = evaluator.bypass_custom_role_ids.map do |custom_role_id| + { id: custom_role_id } + end + end + + policy.save! end trait :deleted do diff --git a/ee/spec/graphql/types/approval_rule_type_spec.rb b/ee/spec/graphql/types/approval_rule_type_spec.rb index c9d7a62bfd2235..1c55e8349eda28 100644 --- a/ee/spec/graphql/types/approval_rule_type_spec.rb +++ b/ee/spec/graphql/types/approval_rule_type_spec.rb @@ -7,7 +7,7 @@ %i[ id name type approvals_required approved overridden section contains_hidden_groups source_rule eligible_approvers users approved_by groups section commented_by invalid allow_merge_when_invalid - scan_result_policies + scan_result_policies bypassed allow_bypass ] end diff --git a/ee/spec/lib/security/scan_result_policies/policy_bypass_auditor_spec.rb b/ee/spec/lib/security/scan_result_policies/policy_bypass_auditor_spec.rb index 4d9a8ccb4e8b1f..cd9c666de64f69 100644 --- a/ee/spec/lib/security/scan_result_policies/policy_bypass_auditor_spec.rb +++ b/ee/spec/lib/security/scan_result_policies/policy_bypass_auditor_spec.rb @@ -239,4 +239,35 @@ end end end + + describe '#log_merge_request_bypass' do + let(:merge_request) { create(:merge_request, source_project: project) } + let(:approval_rule) { create(:approval_merge_request_rule, name: 'Test Security Policy Rule') } + let(:reason) { 'Emergency security fix' } + + it 'logs the bypass event with correct details' do + auditor.log_merge_request_bypass(merge_request, approval_rule, reason) + + expect(Gitlab::Audit::Auditor).to have_received(:audit).with( + name: 'security_policy_merge_request_bypass', + author: user, + scope: security_policy.security_policy_management_project, + target: security_policy, + message: "Approval rule #{approval_rule.name} in merge request (#{project.full_path}!#{merge_request.iid}) " \ + "has been bypassed by #{user.name} with reason: #{reason}", + additional_details: { + project_id: project.id, + security_policy_name: security_policy.name, + security_policy_id: security_policy.id, + branch_name: branch_name, + bypass_type: :merge_request, + approval_rule_id: approval_rule.id, + merge_request_id: merge_request.id, + merge_request_iid: merge_request.iid, + approval_rule_name: approval_rule.name, + reason: reason + } + ) + end + end end diff --git a/ee/spec/models/approval_wrapped_rule_spec.rb b/ee/spec/models/approval_wrapped_rule_spec.rb index 909b8a19b9c9fd..979ab2a0b08759 100644 --- a/ee/spec/models/approval_wrapped_rule_spec.rb +++ b/ee/spec/models/approval_wrapped_rule_spec.rb @@ -559,4 +559,122 @@ def approved_approvers_for_rule_id(rule_id) it { is_expected.to eq(false) } end end + + describe '#bypassed?' do + subject { approval_wrapped_rule.bypassed? } + + context 'when security_policies_bypass_options_group_roles feature is disabled' do + before do + stub_feature_flags(security_policies_bypass_options_group_roles: false) + end + + it { is_expected.to eq(false) } + end + + context 'when rule is not from scan result policy' do + before do + rule.update!(report_type: :code_coverage) + end + + it { is_expected.to eq(false) } + end + + context 'when rule is from scan result policy' do + before do + rule.update!(report_type: :scan_finding) + end + + context 'when no bypass events exist' do + it { is_expected.to eq(false) } + end + + context 'when bypass events exist' do + let!(:bypass_event) do + create(:approval_policy_merge_request_bypass_event, + merge_request: merge_request, + project: merge_request.project) + end + + it { is_expected.to eq(true) } + end + end + end + + describe '#allow_bypass?' do + let(:user) { create(:user) } + let(:security_policy) { create(:security_policy) } + let(:approval_policy_rule) { create(:approval_policy_rule, security_policy: security_policy) } + + subject { approval_wrapped_rule.allow_bypass?(user) } + + before do + rule.update!(approval_policy_rule: approval_policy_rule) + end + + context 'when security_policies_bypass_options_group_roles feature is disabled' do + before do + stub_feature_flags(security_policies_bypass_options_group_roles: false) + end + + it { is_expected.to eq(false) } + end + + context 'when rule is not from scan result policy' do + before do + rule.update!(report_type: :code_coverage) + end + + it { is_expected.to eq(false) } + end + + context 'when rule is from scan result policy' do + before do + rule.update!(report_type: :scan_finding) + end + + context 'when security policy has no bypass settings' do + before do + security_policy.update!(content: { + actions: [{ type: 'require_approval', approvals_required: 1, user_approvers: %w[owner] }] + }) + end + + it { is_expected.to eq(false) } + end + + context 'when security policy has bypass settings' do + before do + security_policy.update!(content: { + actions: [{ type: 'require_approval', approvals_required: 1, user_approvers: %w[owner] }], + bypass_settings: { + users: [{ id: user.id }], + groups: [], + roles: [], + custom_roles: [] + } + }) + end + + context 'when user bypass checker allows bypass' do + it { is_expected.to eq(true) } + end + + context 'when user bypass checker does not allow bypass' do + before do + security_policy.update!(content: { + actions: [{ type: 'require_approval', approvals_required: 1, user_approvers: %w[owner] }], + bypass_settings: { + users: [], + groups: [], + roles: [], + custom_roles: [] + } + }) + end + + it { is_expected.to eq(false) } + end + end + end + end end diff --git a/ee/spec/models/security/policy_spec.rb b/ee/spec/models/security/policy_spec.rb index d592b727c45fb3..3696001dd8e43a 100644 --- a/ee/spec/models/security/policy_spec.rb +++ b/ee/spec/models/security/policy_spec.rb @@ -1356,7 +1356,7 @@ context 'when bypass_settings has access_tokens and service_accounts' do let(:policy) do - build(:security_policy, + create(:security_policy, bypass_access_token_ids: [access_token_id], bypass_service_account_ids: [service_account_id] ) @@ -1368,4 +1368,78 @@ end end end + + describe '#create_merge_request_bypass_event!' do + let_it_be(:policy) { create(:security_policy) } + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let(:reason) { 'Security policy bypassed due to emergency' } + + subject(:create_bypass_event!) do + policy.create_merge_request_bypass_event!( + project: project, + user: user, + reason: reason, + merge_request: merge_request + ) + end + + it 'creates a new bypass event' do + expect { create_bypass_event! }.to change { Security::ApprovalPolicyMergeRequestBypassEvent.count }.by(1) + end + + it 'sets the correct attributes' do + bypass_event = create_bypass_event! + + expect(bypass_event.project).to eq(project) + expect(bypass_event.user).to eq(user) + expect(bypass_event.reason).to eq(reason) + expect(bypass_event.merge_request).to eq(merge_request) + expect(bypass_event.security_policy).to eq(policy) + end + + context 'when bypass event already exists for the same project, merge request, and policy' do + before do + create(:approval_policy_merge_request_bypass_event, + project: project, + merge_request: merge_request, + security_policy: policy, + user: user, + reason: 'Previous bypass' + ) + end + + it 'raises a validation error due to uniqueness constraint' do + expect { create_bypass_event! }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + context 'when creating multiple bypass events for different merge requests' do + let_it_be(:other_merge_request) do + create(:merge_request, source_project: project, source_branch: 'feature-branch-2') + end + + it 'allows creating bypass events for different merge requests' do + first_event = policy.create_merge_request_bypass_event!( + project: project, + user: user, + reason: 'First bypass', + merge_request: merge_request + ) + + second_event = policy.create_merge_request_bypass_event!( + project: project, + user: user, + reason: 'Second bypass', + merge_request: other_merge_request + ) + + expect(first_event).to be_persisted + expect(second_event).to be_persisted + expect(first_event.merge_request).to eq(merge_request) + expect(second_event.merge_request).to eq(other_merge_request) + end + end + end end diff --git a/ee/spec/requests/api/graphql/mutations/merge_requests/bypass_security_policy_spec.rb b/ee/spec/requests/api/graphql/mutations/merge_requests/bypass_security_policy_spec.rb new file mode 100644 index 00000000000000..099d8a83d0b444 --- /dev/null +++ b/ee/spec/requests/api/graphql/mutations/merge_requests/bypass_security_policy_spec.rb @@ -0,0 +1,200 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Bypassing a security policy approval rule', feature_category: :security_policy_management do + include GraphqlHelpers + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, maintainer_of: project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:security_policy) { create(:security_policy, :approval_policy) } + let_it_be(:approval_policy_rule) { create(:approval_policy_rule, security_policy: security_policy) } + let_it_be(:approval_rule) do + create(:approval_merge_request_rule, + name: "security-policy-rule", + merge_request: merge_request, + approvals_required: 1, + approval_policy_rule: approval_policy_rule) + end + + def mutation(vars = {}, mr = merge_request) + variables = vars.reverse_merge( + project_path: mr.project.full_path, + iid: mr.iid.to_s, + approval_rule_id: approval_rule.id, + reason: "Security policy bypass reason" + ) + + graphql_mutation(:merge_request_bypass_security_policy, variables, <<-QL.strip_heredoc) + mergeRequest { + id + } + errors + QL + end + + def mutation_response + graphql_mutation_response(:merge_request_bypass_security_policy) + end + + before_all do + project.add_maintainer(current_user) + end + + before do + stub_licensed_features(security_orchestration_policies: true) + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(security_policies_bypass_options_mr_widget: false) + end + + it 'returns an error' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include(a_hash_including( + 'message' => /security_policies_bypass_options_mr_widget.*disabled/) + ) + end + end + + context 'when feature flag is enabled' do + before do + stub_feature_flags(security_policies_bypass_options_mr_widget: true) + end + + context 'when user can bypass the security policy' do + before do + allow_next_instance_of(ApprovalWrappedRule) do |wrapped_rule| + allow(wrapped_rule).to receive(:allow_bypass?).with(current_user).and_return(true) + allow(wrapped_rule).to receive(:bypassed?).and_return(false) + end + end + + it 'successfully bypasses the security policy' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.to change { security_policy.approval_policy_merge_request_bypass_events.count }.by(1) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response&.dig('errors')).to be_empty + expect(mutation_response&.dig('mergeRequest')).to be_present + end + + context 'with custom reason' do + let(:input) { { reason: "Custom bypass reason for testing" } } + + it 'bypasses with the custom reason' do + expect do + post_graphql_mutation(mutation(input), current_user: current_user) + end.to change { security_policy.approval_policy_merge_request_bypass_events.count }.by(1) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response&.dig('errors')).to be_empty + end + end + end + + context 'when user cannot bypass the security policy' do + before do + allow_next_instance_of(ApprovalWrappedRule) do |wrapped_rule| + allow(wrapped_rule).to receive(:allow_bypass?).with(current_user).and_return(false) + end + end + + it 'returns an authorization error' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response&.dig('errors')).to include('You are not allowed to bypass this security policy.') + end + end + + context 'when security policy is already bypassed' do + before do + allow_next_instance_of(ApprovalWrappedRule) do |wrapped_rule| + allow(wrapped_rule).to receive(:allow_bypass?).with(current_user).and_return(true) + allow(wrapped_rule).to receive(:bypassed?).and_return(true) + end + end + + it 'returns an error for already bypassed policy' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response&.dig('errors')).to include('You have already bypassed this security policy.') + end + end + + context 'when approval rule does not exist' do + let(:input) { { approval_rule_id: non_existing_record_id } } + + it 'returns a not found error' do + post_graphql_mutation(mutation(input), current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include(a_hash_including('message' => /Approval rule not found/)) + end + end + + context 'when merge request does not exist' do + let(:input) { { iid: non_existing_record_id.to_s } } + + it 'returns a not found error' do + post_graphql_mutation(mutation(input), current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include(a_hash_including( + 'message' => /The resource that you are attempting to access/) + ) + end + end + + context 'when user is not authorized to access the merge request' do + let(:unauthorized_user) { create(:user) } + + it 'returns an authorization error' do + post_graphql_mutation(mutation, current_user: unauthorized_user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include(a_hash_including( + 'message' => /The resource that you are attempting to access/) + ) + end + end + + context 'with invalid input' do + context 'when reason is empty' do + let(:input) { { reason: '' } } + + it 'returns a validation error' do + post_graphql_mutation(mutation(input), current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + # Empty string might not trigger GraphQL validation, so check if it's handled by the service + if graphql_errors.present? + expect(graphql_errors.first['message']).to include('Expected value to not be null') + else + # If no GraphQL errors, the service should handle it + expect(mutation_response&.dig('errors')).to be_present + end + end + end + + context 'when approval_rule_id is missing' do + let(:input) { { approval_rule_id: nil } } + + it 'returns a validation error' do + post_graphql_mutation(mutation(input), current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to be_present + expect(graphql_errors.first['message']).to include('Expected value to not be null') + end + end + end + end +end diff --git a/ee/spec/services/merge_requests/bypass_security_policy_service_spec.rb b/ee/spec/services/merge_requests/bypass_security_policy_service_spec.rb new file mode 100644 index 00000000000000..7d0d39887bbc73 --- /dev/null +++ b/ee/spec/services/merge_requests/bypass_security_policy_service_spec.rb @@ -0,0 +1,308 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::BypassSecurityPolicyService, feature_category: :security_policy_management do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:security_orchestration_policy_configuration) do + create(:security_orchestration_policy_configuration) + end + + let_it_be(:security_policy) do + create(:security_policy, :approval_policy, bypass_user_ids: [user.id]) + end + + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, security_policy: security_policy) + end + + let_it_be(:approval_rule) do + create(:approval_project_rule, + project: project, + approval_policy_rule: approval_policy_rule, + name: 'Test Approval Rule', + report_type: 'scan_finding') + end + + let(:reason) { 'Emergency security fix' } + let(:service) { described_class.new(merge_request: merge_request, approval_rule: approval_rule, current_user: user) } + + before_all do + project.add_maintainer(user) + end + + describe '#execute' do + context 'when bypass is successful' do + it 'creates a bypass event and returns success' do + response = nil + expect { response = service.execute(reason) } + .to change { Security::ApprovalPolicyMergeRequestBypassEvent.count }.by(1) + + expect(response).to be_success + expect(response.payload[:merge_request]).to eq(merge_request) + + bypass_event = Security::ApprovalPolicyMergeRequestBypassEvent.last + expect(bypass_event.project).to eq(project) + expect(bypass_event.security_policy).to eq(security_policy) + expect(bypass_event.merge_request).to eq(merge_request) + expect(bypass_event.user).to eq(user) + expect(bypass_event.reason).to eq(reason) + end + + it 'creates an audit event' do + expect_next_instance_of(Security::ScanResultPolicies::PolicyBypassAuditor) do |instance| + expect(instance).to receive(:log_merge_request_bypass).with( + merge_request, approval_rule, reason) + end + + service.execute(reason) + end + + it 'allows bypass when user is in bypass settings' do + response = service.execute(reason) + expect(response).to be_success + end + + it 'allows bypass when user has maintainer role' do + security_policy.update!( + content: security_policy.content.merge( + bypass_settings: { + users: [], + groups: [], + roles: ['maintainer'], + custom_roles: [] + } + ) + ) + + response = service.execute(reason) + expect(response).to be_success + end + end + + context 'when policy is already bypassed' do + before do + create(:approval_policy_merge_request_bypass_event, + project: project, + security_policy: security_policy, + merge_request: merge_request, + user: user, + reason: 'Previous bypass') + end + + it 'returns error without creating new bypass event' do + expect { service.execute(reason) } + .not_to change { Security::ApprovalPolicyMergeRequestBypassEvent.count } + + response = service.execute(reason) + expect(response).to be_error + expect(response.message).to eq('You have already bypassed this security policy.') + end + end + + context 'when user cannot bypass policy' do + let_it_be(:regular_approval_rule) do + create(:approval_project_rule, + project: project, + name: 'Regular Approval Rule') + end + + let(:service) do + described_class.new(merge_request: merge_request, approval_rule: regular_approval_rule, current_user: user) + end + + before_all do + project.team.find_member(user.id).destroy! + project.add_developer(user) + end + + it 'returns error without creating bypass event' do + expect { service.execute(reason) } + .not_to change { Security::ApprovalPolicyMergeRequestBypassEvent.count } + + response = service.execute(reason) + expect(response).to be_error + expect(response.message).to eq('You are not allowed to bypass this security policy.') + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(security_policies_bypass_options_group_roles: false) + end + + it 'returns error when bypass is not allowed' do + response = service.execute(reason) + expect(response).to be_error + expect(response.message).to eq('You are not allowed to bypass this security policy.') + end + end + + context 'when approval rule is not from scan result policy' do + let_it_be(:regular_approval_rule) do + create(:approval_project_rule, + project: project, + name: 'Regular Approval Rule') + end + + it 'returns error when bypass is not allowed' do + service_with_regular_rule = described_class.new( + merge_request: merge_request, + approval_rule: regular_approval_rule, + current_user: user + ) + response = service_with_regular_rule.execute(reason) + expect(response).to be_error + expect(response.message).to eq('You are not allowed to bypass this security policy.') + end + end + + context 'when user is in bypass group' do + let_it_be(:group) { create(:group) } + let_it_be(:group_member) { create(:group_member, group: group, user: user) } + let_it_be(:group_security_policy) do + create(:security_policy, :approval_policy, bypass_group_ids: [group.id]) + end + + let_it_be(:group_approval_policy_rule) do + create(:approval_policy_rule, security_policy: group_security_policy) + end + + let_it_be(:group_approval_rule) do + create(:approval_project_rule, + project: project, + approval_policy_rule: group_approval_policy_rule, + name: 'Group Approval Rule', + report_type: 'scan_finding') + end + + let(:group_service) do + described_class.new(merge_request: merge_request, approval_rule: group_approval_rule, current_user: user) + end + + it 'allows bypass when user is in bypass group' do + response = group_service.execute(reason) + expect(response).to be_success + end + end + + context 'when user has custom role that can bypass' do + let_it_be(:custom_role) { create(:member_role, namespace: project.group) } + let_it_be(:custom_role_security_policy) do + create(:security_policy, :approval_policy, bypass_custom_role_ids: [custom_role.id]) + end + + let_it_be(:custom_role_approval_policy_rule) do + create(:approval_policy_rule, security_policy: custom_role_security_policy) + end + + let_it_be(:custom_role_approval_rule) do + create(:approval_project_rule, + project: project, + approval_policy_rule: custom_role_approval_policy_rule, + name: 'Custom Role Approval Rule', + report_type: 'scan_finding') + end + + let(:custom_role_service) do + described_class.new(merge_request: merge_request, approval_rule: custom_role_approval_rule, current_user: user) + end + + before do + project.team.find_member(user.id).destroy! + project.add_member(user, :maintainer) + member = project.members.find_by(user: user) + member.update!(member_role: custom_role) + end + + it 'allows bypass when user has custom role' do + response = custom_role_service.execute(reason) + expect(response).to be_success + end + end + + context 'when user is a project bot' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:bot_security_policy) do + create(:security_policy, :approval_policy, bypass_user_ids: [project_bot.id]) + end + + let_it_be(:bot_approval_policy_rule) do + create(:approval_policy_rule, security_policy: bot_security_policy) + end + + let_it_be(:bot_approval_rule) do + create(:approval_project_rule, + project: project, + approval_policy_rule: bot_approval_policy_rule, + name: 'Bot Approval Rule') + end + + let(:bot_service) do + described_class.new(merge_request: merge_request, approval_rule: bot_approval_rule, current_user: project_bot) + end + + before_all do + project.add_maintainer(project_bot) + end + + it 'does not allow bypass for project bot even if in bypass settings' do + response = bot_service.execute(reason) + expect(response).to be_error + expect(response.message).to eq('You are not allowed to bypass this security policy.') + end + end + + context 'when user is a service account' do + let_it_be(:service_account) { create(:user, :service_account) } + let_it_be(:service_account_security_policy) do + create(:security_policy, :approval_policy, bypass_user_ids: [service_account.id]) + end + + let_it_be(:service_account_approval_policy_rule) do + create(:approval_policy_rule, security_policy: service_account_security_policy) + end + + let_it_be(:service_account_approval_rule) do + create(:approval_project_rule, + project: project, + approval_policy_rule: service_account_approval_policy_rule, + name: 'Service Account Approval Rule') + end + + let(:service_account_service) do + described_class.new(merge_request: merge_request, approval_rule: service_account_approval_rule, + current_user: service_account) + end + + before_all do + project.add_maintainer(service_account) + end + + it 'does not allow bypass for service account even if in bypass settings' do + response = service_account_service.execute(reason) + expect(response).to be_error + expect(response.message).to eq('You are not allowed to bypass this security policy.') + end + end + + context 'when merge request is from different project' do + let_it_be(:other_project) { create(:project, :public) } + let_it_be(:other_merge_request) { create(:merge_request, source_project: other_project) } + + it 'creates bypass event for the correct project' do + service = described_class.new(merge_request: other_merge_request, approval_rule: approval_rule, + current_user: user) + + expect { service.execute(reason) } + .to change { Security::ApprovalPolicyMergeRequestBypassEvent.count }.by(1) + + bypass_event = Security::ApprovalPolicyMergeRequestBypassEvent.last + expect(bypass_event.project).to eq(other_project) + expect(bypass_event.merge_request).to eq(other_merge_request) + end + end + end +end -- GitLab