From 5d54e7ca2c17d3b424047cf589974d0c5004ef2b Mon Sep 17 00:00:00 2001 From: GitLab Duo Date: Thu, 4 Sep 2025 18:30:50 +0000 Subject: [PATCH] Duo Workflow: Resolve issue #561887 --- .../merge_request_update_mutation.md | 94 ++++++ .../ee/mutations/merge_requests/update.rb | 36 +++ .../security/scan_result_policy_violation.rb | 22 +- .../dismiss_policy_violations_service.rb | 95 ++++++ .../mutations/merge_requests/update_spec.rb | 241 +++++++++++++++ .../update_security_policy_override_spec.rb | 292 ++++++++++++++++++ .../dismiss_policy_violations_service_spec.rb | 252 +++++++++++++++ 7 files changed, 1031 insertions(+), 1 deletion(-) create mode 100644 doc/api/graphql/reference/merge_request_update_mutation.md create mode 100644 ee/app/services/security/findings/dismiss_policy_violations_service.rb create mode 100644 ee/spec/graphql/ee/mutations/merge_requests/update_spec.rb create mode 100644 ee/spec/requests/api/graphql/mutations/merge_requests/update_security_policy_override_spec.rb create mode 100644 ee/spec/services/security/findings/dismiss_policy_violations_service_spec.rb diff --git a/doc/api/graphql/reference/merge_request_update_mutation.md b/doc/api/graphql/reference/merge_request_update_mutation.md new file mode 100644 index 00000000000000..66a7b4ef93bfb2 --- /dev/null +++ b/doc/api/graphql/reference/merge_request_update_mutation.md @@ -0,0 +1,94 @@ +# MergeRequestUpdate Mutation + +The `mergeRequestUpdate` mutation allows you to update various attributes of a merge request, including the ability to override security policy violations in warn mode. + +## Arguments + +| Name | Type | Description | +|------|------|-------------| +| `projectPath` | `ID!` | The full path of the project containing the merge request | +| `iid` | `String!` | The internal ID of the merge request | +| `title` | `String` | New title for the merge request | +| `description` | `String` | New description for the merge request | +| `targetBranch` | `String` | New target branch for the merge request | +| `state` | `MergeRequestStateEvent` | Action to perform to change the state | +| `timeEstimate` | `String` | Estimated time to complete the merge request | +| `mergeAfter` | `Time` | Date after which the merge request can be merged | +| `overrideRequestedChanges` | `Boolean` | Override all requested changes. Can only be set by users who have permission to merge this merge request | +| `overrideSecurityPolicies` | `Boolean` | Override security policy violations in warn mode. Can only be set by users who have Developer+ permissions in the project | + +## Security Policy Override + +The `overrideSecurityPolicies` parameter allows developers to dismiss security policy violations that are in "warn" mode. This feature: + +- Only affects policy violations with `enforcement_type: "warn"` +- Requires Developer+ role permissions in the project +- Creates audit events for all dismissal actions +- Updates scan result policy violations with dismissal metadata +- Works with bulk operations for efficiency + +### Usage Example + +```graphql +mutation { + mergeRequestUpdate(input: { + projectPath: "group/project" + iid: "123" + overrideSecurityPolicies: true + }) { + mergeRequest { + id + title + state + } + errors + } +} +``` + +### Authorization + +- **Developer+**: Can override security policy violations in warn mode +- **Maintainer+**: Can override both security policies and requested changes +- **Reporter and below**: Cannot override security policies + +### Audit Trail + +When `overrideSecurityPolicies` is used, the system: + +1. Creates audit events for each dismissed violation +2. Records the user who performed the dismissal +3. Timestamps the dismissal action +4. Stores the dismissal reason as "policy_override" +5. Adds a comment indicating the dismissal was performed via GraphQL + +### Error Handling + +The mutation will return errors in the following cases: + +- User lacks Developer+ permissions: `"Access denied"` +- Merge request not found: `"Merge request not found"` +- Database transaction fails: `"Failed to dismiss policy violations: [details]"` +- Invalid policy violations: Specific error messages for each violation + +### Integration with Warn Mode + +This feature is designed to work with the Security Policy Warn Mode functionality: + +- Only policies with `enforcement_type: "warn"` can be overridden +- Policies in "enforce" mode continue to block merge requests +- Dismissed violations are tracked separately from regular approvals +- New violations after dismissal will require new dismissals + +## Response + +The mutation returns a `MergeRequestUpdatePayload` containing: + +- `mergeRequest`: The updated merge request object +- `errors`: Array of error messages if the operation failed + +## Related Documentation + +- [Security Policies](../../../user/application_security/policies/_index.md) +- [Merge Request Approval Policies](../../../user/application_security/policies/merge_request_approval_policies.md) +- [GraphQL API](../../../api/graphql/_index.md) \ No newline at end of file diff --git a/ee/app/graphql/ee/mutations/merge_requests/update.rb b/ee/app/graphql/ee/mutations/merge_requests/update.rb index 334cfb3b95a86a..61f0da3a1c592c 100644 --- a/ee/app/graphql/ee/mutations/merge_requests/update.rb +++ b/ee/app/graphql/ee/mutations/merge_requests/update.rb @@ -11,6 +11,42 @@ module Update required: false, description: 'Override all requested changes. Can only be set by users who have permission' \ 'to merge this merge request.' + + argument :override_security_policies, GraphQL::Types::Boolean, + required: false, + description: 'Override security policy violations in warn mode. Can only be set by users who have ' \ + 'Developer+ permissions in the project.' + end + + def resolve(project_path:, iid:, **args) + merge_request = authorized_find!(project_path: project_path, iid: iid) + + # Handle security policy override if requested + if args[:override_security_policies] + handle_security_policy_override(merge_request, args) + end + + # Remove override_security_policies from args before passing to parent + args = args.except(:override_security_policies) + + super(project_path: project_path, iid: iid, **args) + end + + private + + def handle_security_policy_override(merge_request, args) + return unless args[:override_security_policies] + + result = ::Security::Findings::DismissPolicyViolationsService.new( + user: current_user, + merge_request: merge_request, + comment: "Dismissed via GraphQL mutation", + dismissal_reason: "policy_override" + ).execute + + unless result.success? + raise GraphQL::ExecutionError, result.message + end end end end diff --git a/ee/app/models/security/scan_result_policy_violation.rb b/ee/app/models/security/scan_result_policy_violation.rb index 56b75ef30706b5..2a1a4116daae9c 100644 --- a/ee/app/models/security/scan_result_policy_violation.rb +++ b/ee/app/models/security/scan_result_policy_violation.rb @@ -13,6 +13,7 @@ class ScanResultPolicyViolation < ApplicationRecord belongs_to :merge_request, inverse_of: :scan_result_policy_violations belongs_to :approval_policy_rule, class_name: 'Security::ApprovalPolicyRule', inverse_of: :violations has_one :security_policy, class_name: 'Security::Policy', through: :approval_policy_rule + belongs_to :dismissed_by, class_name: 'User', optional: true validates :scan_result_policy_id, uniqueness: { scope: %i[merge_request_id] } validates :violation_data, json_schema: { filename: 'scan_result_policy_violation_data' }, allow_blank: true @@ -23,7 +24,8 @@ class ScanResultPolicyViolation < ApplicationRecord running: 0, failed: 1, warn: 2, - skipped: 3 + skipped: 3, + dismissed: 4 } scope :including_scan_result_policy_reads, -> { includes(:scan_result_policy_read) } @@ -50,5 +52,23 @@ class ScanResultPolicyViolation < ApplicationRecord def self.trim_violations(violations) Array.wrap(violations)[..MAX_VIOLATIONS] end + + def dismissed? + status == 'dismissed' + end + + def can_be_dismissed? + failed? && approval_policy_rule&.security_policy&.warn_mode? + end + + def dismiss!(user:, reason: nil, comment: nil) + update!( + status: :dismissed, + dismissed_at: Time.current, + dismissed_by: user, + dismissal_reason: reason, + dismissal_comment: comment + ) + end end end diff --git a/ee/app/services/security/findings/dismiss_policy_violations_service.rb b/ee/app/services/security/findings/dismiss_policy_violations_service.rb new file mode 100644 index 00000000000000..665fca20dcb659 --- /dev/null +++ b/ee/app/services/security/findings/dismiss_policy_violations_service.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +module Security + module Findings + class DismissPolicyViolationsService < BaseProjectService + include Gitlab::Allowable + + def initialize(user:, merge_request:, comment: nil, dismissal_reason: nil) + super(project: merge_request.project, current_user: user) + @merge_request = merge_request + @comment = comment + @dismissal_reason = dismissal_reason + end + + def execute + return ServiceResponse.error(message: "Access denied", http_status: :forbidden) unless authorized? + return ServiceResponse.error(message: "Merge request not found", http_status: :not_found) unless @merge_request + + dismiss_policy_violations + end + + private + + def authorized? + # Check if user has Developer+ role permissions in project + can?(@current_user, :update_merge_request, @project) + end + + def dismiss_policy_violations + @error_messages = [] + dismissed_count = 0 + + ApplicationRecord.transaction do + policy_violations = @merge_request.scan_result_policy_violations.failed + + policy_violations.each do |violation| + next unless violation.approval_policy_rule&.security_policy&.warn_mode? + + if dismiss_violation(violation) + dismissed_count += 1 + create_audit_event(violation) + end + end + + if @error_messages.any? + raise ActiveRecord::Rollback + end + end + + if @error_messages.any? + error_string = format(_("Failed to dismiss policy violations: %{messages}"), + messages: @error_messages.join(", ")) + ServiceResponse.error(message: error_string, http_status: :unprocessable_entity) + else + ServiceResponse.success( + payload: { + merge_request: @merge_request, + dismissed_count: dismissed_count + } + ) + end + end + + def dismiss_violation(violation) + violation.update( + status: :dismissed, + dismissed_at: Time.current, + dismissed_by: @current_user, + dismissal_reason: @dismissal_reason, + dismissal_comment: @comment + ) + rescue StandardError => e + @error_messages << "Violation #{violation.id}: #{e.message}" + false + end + + def create_audit_event(violation) + # Create audit event for dismissal action + AuditEventService.new( + @current_user, + @project, + { + action: :custom, + custom_message: "Dismissed security policy violation for merge request #{@merge_request.iid}", + target_id: violation.id, + target_type: 'Security::ScanResultPolicyViolation', + target_details: violation.approval_policy_rule&.security_policy&.name + } + ).for_project.security_event + rescue StandardError => e + Rails.logger.warn("Failed to create audit event for policy violation dismissal: #{e.message}") + end + end + end +end \ No newline at end of file diff --git a/ee/spec/graphql/ee/mutations/merge_requests/update_spec.rb b/ee/spec/graphql/ee/mutations/merge_requests/update_spec.rb new file mode 100644 index 00000000000000..3ac97bbce63047 --- /dev/null +++ b/ee/spec/graphql/ee/mutations/merge_requests/update_spec.rb @@ -0,0 +1,241 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EE::Mutations::MergeRequests::Update, feature_category: :security_policy_management do + include GraphqlHelpers + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let_it_be(:security_policy) { create(:security_policy, project: project, policy_type: :scan_result_policy) } + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, project: project, security_policy: security_policy) + end + + let(:mutation) { graphql_mutation(:merge_request_update, input) } + let(:mutation_response) { graphql_mutation_response(:merge_request_update) } + + let(:input) do + { + project_path: project.full_path, + iid: merge_request.iid.to_s, + override_security_policies: true + } + end + + describe '#resolve' do + context 'when user has developer permissions' do + before do + project.add_developer(user) + end + + context 'when there are dismissible policy violations' do + let!(:policy_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + allow(security_policy).to receive(:warn_mode?).and_return(true) + end + + it 'successfully dismisses policy violations' do + expect_next_instance_of(Security::Findings::DismissPolicyViolationsService) do |service| + expect(service).to receive(:execute).and_return( + ServiceResponse.success(payload: { merge_request: merge_request, dismissed_count: 1 }) + ) + end + + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['errors']).to be_empty + end + + it 'calls the dismissal service with correct parameters' do + expect(Security::Findings::DismissPolicyViolationsService).to receive(:new).with( + user: user, + merge_request: merge_request, + comment: "Dismissed via GraphQL mutation", + dismissal_reason: "policy_override" + ).and_call_original + + post_graphql_mutation(mutation, current_user: user) + end + end + + context 'when dismissal service returns an error' do + before do + allow_next_instance_of(Security::Findings::DismissPolicyViolationsService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'Access denied', http_status: :forbidden) + ) + end + end + + it 'raises a GraphQL execution error' do + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include( + hash_including('message' => 'Access denied') + ) + end + end + + context 'when override_security_policies is false' do + let(:input) do + { + project_path: project.full_path, + iid: merge_request.iid.to_s, + override_security_policies: false + } + end + + it 'does not call the dismissal service' do + expect(Security::Findings::DismissPolicyViolationsService).not_to receive(:new) + + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + end + end + + context 'when override_security_policies is not provided' do + let(:input) do + { + project_path: project.full_path, + iid: merge_request.iid.to_s, + title: 'Updated title' + } + end + + it 'does not call the dismissal service' do + expect(Security::Findings::DismissPolicyViolationsService).not_to receive(:new) + + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + end + end + end + + context 'when user lacks developer permissions' do + before do + project.add_guest(user) + end + + it 'returns an authorization error' do + expect_next_instance_of(Security::Findings::DismissPolicyViolationsService) do |service| + expect(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'Access denied', http_status: :forbidden) + ) + end + + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include( + hash_including('message' => 'Access denied') + ) + end + end + + context 'when merge request does not exist' do + let(:input) do + { + project_path: project.full_path, + iid: '999999', + override_security_policies: true + } + end + + it 'returns a not found error' do + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include( + hash_including('message' => include('not found')) + ) + end + end + + context 'when combining with other mutation parameters' do + let(:input) do + { + project_path: project.full_path, + iid: merge_request.iid.to_s, + title: 'Updated title', + description: 'Updated description', + override_security_policies: true + } + end + + before do + project.add_developer(user) + end + + it 'processes both the dismissal and the regular update' do + expect_next_instance_of(Security::Findings::DismissPolicyViolationsService) do |service| + expect(service).to receive(:execute).and_return( + ServiceResponse.success(payload: { merge_request: merge_request, dismissed_count: 0 }) + ) + end + + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['errors']).to be_empty + expect(mutation_response['mergeRequest']['title']).to eq('Updated title') + expect(mutation_response['mergeRequest']['description']).to eq('Updated description') + end + end + + context 'when override_requested_changes is also provided' do + let(:input) do + { + project_path: project.full_path, + iid: merge_request.iid.to_s, + override_requested_changes: true, + override_security_policies: true + } + end + + before do + project.add_developer(user) + end + + it 'processes both overrides' do + expect_next_instance_of(Security::Findings::DismissPolicyViolationsService) do |service| + expect(service).to receive(:execute).and_return( + ServiceResponse.success(payload: { merge_request: merge_request, dismissed_count: 0 }) + ) + end + + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['errors']).to be_empty + end + end + end + + describe 'argument validation' do + it 'accepts override_security_policies as a boolean argument' do + expect(described_class.arguments['overrideSecurityPolicies'].type.to_type_signature).to eq('Boolean') + end + + it 'has the correct description for override_security_policies' do + expect(described_class.arguments['overrideSecurityPolicies'].description).to include( + 'Override security policy violations in warn mode' + ) + end + + it 'marks override_security_policies as optional' do + expect(described_class.arguments['overrideSecurityPolicies'].type.non_null?).to be_falsey + end + end +end \ No newline at end of file diff --git a/ee/spec/requests/api/graphql/mutations/merge_requests/update_security_policy_override_spec.rb b/ee/spec/requests/api/graphql/mutations/merge_requests/update_security_policy_override_spec.rb new file mode 100644 index 00000000000000..c9255d016ae5ca --- /dev/null +++ b/ee/spec/requests/api/graphql/mutations/merge_requests/update_security_policy_override_spec.rb @@ -0,0 +1,292 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Updating a merge request with security policy override', :aggregate_failures, feature_category: :security_policy_management do + include GraphqlHelpers + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let_it_be(:security_policy) { create(:security_policy, project: project, policy_type: :scan_result_policy) } + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, project: project, security_policy: security_policy) + end + + let(:mutation) do + graphql_mutation( + :merge_request_update, + project_path: project.full_path, + iid: merge_request.iid.to_s, + override_security_policies: true + ) + end + + describe 'complete workflow integration' do + context 'when user has developer permissions' do + before do + project.add_developer(user) + end + + context 'with dismissible policy violations in warn mode' do + let!(:policy_violation_1) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + let!(:policy_violation_2) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + allow(security_policy).to receive(:warn_mode?).and_return(true) + end + + it 'successfully dismisses all policy violations and creates audit events' do + expect { post_graphql_mutation(mutation, current_user: user) } + .to change { policy_violation_1.reload.status }.from('failed').to('dismissed') + .and change { policy_violation_2.reload.status }.from('failed').to('dismissed') + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to be_blank + + # Verify dismissal metadata + [policy_violation_1, policy_violation_2].each do |violation| + violation.reload + expect(violation.dismissed_by).to eq(user) + expect(violation.dismissal_comment).to eq('Dismissed via GraphQL mutation') + expect(violation.dismissal_reason).to eq('policy_override') + expect(violation.dismissed_at).to be_within(1.minute).of(Time.current) + end + + # Verify audit events were created + audit_events = AuditEvent.where( + author: user, + entity_type: 'Project', + entity_id: project.id + ) + expect(audit_events.count).to eq(2) + + audit_events.each do |event| + expect(event.details[:custom_message]).to include( + "Dismissed security policy violation for merge request #{merge_request.iid}" + ) + expect(event.details[:target_type]).to eq('Security::ScanResultPolicyViolation') + end + end + + it 'does not affect non-failed violations' do + running_violation = create(:scan_result_policy_violation, :running, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + + expect { post_graphql_mutation(mutation, current_user: user) } + .not_to change { running_violation.reload.status } + + expect(running_violation.dismissed_by).to be_nil + end + + it 'does not affect violations from enforce mode policies' do + enforce_policy = create(:security_policy, project: project, policy_type: :scan_result_policy) + enforce_rule = create(:approval_policy_rule, project: project, security_policy: enforce_policy) + enforce_violation = create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: enforce_rule) + + allow(enforce_policy).to receive(:warn_mode?).and_return(false) + + expect { post_graphql_mutation(mutation, current_user: user) } + .not_to change { enforce_violation.reload.status } + + expect(enforce_violation.dismissed_by).to be_nil + end + end + + context 'when there are no dismissible violations' do + let!(:running_violation) do + create(:scan_result_policy_violation, :running, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + it 'completes successfully without dismissing anything' do + expect { post_graphql_mutation(mutation, current_user: user) } + .not_to change { running_violation.reload.status } + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to be_blank + end + + it 'does not create any audit events' do + expect { post_graphql_mutation(mutation, current_user: user) } + .not_to change { AuditEvent.count } + end + end + + context 'when policy violations belong to different merge requests' do + let!(:other_merge_request) { create(:merge_request, source_project: project, target_project: project) } + let!(:own_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + let!(:other_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: other_merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + allow(security_policy).to receive(:warn_mode?).and_return(true) + end + + it 'only dismisses violations for the specified merge request' do + expect { post_graphql_mutation(mutation, current_user: user) } + .to change { own_violation.reload.status }.from('failed').to('dismissed') + .and not_change { other_violation.reload.status } + + expect(other_violation.dismissed_by).to be_nil + end + end + end + + context 'when user lacks developer permissions' do + before do + project.add_reporter(user) + end + + let!(:policy_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + it 'returns authorization error and does not dismiss violations' do + expect { post_graphql_mutation(mutation, current_user: user) } + .not_to change { policy_violation.reload.status } + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include( + hash_including('message' => 'Access denied') + ) + + expect(policy_violation.dismissed_by).to be_nil + end + + it 'does not create audit events' do + expect { post_graphql_mutation(mutation, current_user: user) } + .not_to change { AuditEvent.count } + end + end + + context 'when merge request does not exist' do + let(:mutation) do + graphql_mutation( + :merge_request_update, + project_path: project.full_path, + iid: '999999', + override_security_policies: true + ) + end + + before do + project.add_developer(user) + end + + it 'returns not found error' do + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include( + hash_including('message' => include('not found')) + ) + end + end + + context 'when project does not exist' do + let(:mutation) do + graphql_mutation( + :merge_request_update, + project_path: 'non-existent/project', + iid: merge_request.iid.to_s, + override_security_policies: true + ) + end + + it 'returns not found error' do + post_graphql_mutation(mutation, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include( + hash_including('message' => include('not found')) + ) + end + end + + context 'when database transaction fails' do + before do + project.add_developer(user) + allow_any_instance_of(Security::ScanResultPolicyViolation).to receive(:update).and_raise(ActiveRecord::RecordInvalid) + end + + let!(:policy_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + it 'rolls back changes and returns error' do + expect { post_graphql_mutation(mutation, current_user: user) } + .not_to change { policy_violation.reload.status } + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to include( + hash_including('message' => include('Failed to dismiss policy violations')) + ) + end + end + end + + describe 'performance considerations' do + before do + project.add_developer(user) + allow(security_policy).to receive(:warn_mode?).and_return(true) + end + + context 'with many policy violations' do + let!(:policy_violations) do + create_list(:scan_result_policy_violation, 10, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + it 'processes all violations efficiently' do + expect { post_graphql_mutation(mutation, current_user: user) } + .to change { merge_request.scan_result_policy_violations.dismissed.count }.from(0).to(10) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to be_blank + + # Verify all violations were dismissed + policy_violations.each do |violation| + violation.reload + expect(violation).to be_dismissed + expect(violation.dismissed_by).to eq(user) + end + end + end + end +end \ No newline at end of file diff --git a/ee/spec/services/security/findings/dismiss_policy_violations_service_spec.rb b/ee/spec/services/security/findings/dismiss_policy_violations_service_spec.rb new file mode 100644 index 00000000000000..9670d9f4359679 --- /dev/null +++ b/ee/spec/services/security/findings/dismiss_policy_violations_service_spec.rb @@ -0,0 +1,252 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::Findings::DismissPolicyViolationsService, feature_category: :security_policy_management do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let_it_be(:security_policy) { create(:security_policy, project: project, policy_type: :scan_result_policy) } + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, project: project, security_policy: security_policy) + end + + let(:service) do + described_class.new( + user: user, + merge_request: merge_request, + comment: 'Test dismissal comment', + dismissal_reason: 'policy_override' + ) + end + + describe '#execute' do + context 'when user has proper permissions' do + before do + project.add_developer(user) + end + + context 'when there are failed policy violations in warn mode' do + let!(:policy_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + allow(security_policy).to receive(:warn_mode?).and_return(true) + end + + it 'successfully dismisses the violations' do + result = service.execute + + expect(result).to be_success + expect(result.payload[:dismissed_count]).to eq(1) + expect(policy_violation.reload).to be_dismissed + expect(policy_violation.dismissed_by).to eq(user) + expect(policy_violation.dismissal_comment).to eq('Test dismissal comment') + expect(policy_violation.dismissal_reason).to eq('policy_override') + end + + it 'creates audit events for dismissed violations' do + expect(AuditEventService).to receive(:new).with( + user, + project, + hash_including( + action: :custom, + custom_message: "Dismissed security policy violation for merge request #{merge_request.iid}", + target_id: policy_violation.id, + target_type: 'Security::ScanResultPolicyViolation' + ) + ).and_call_original + + service.execute + end + end + + context 'when there are multiple policy violations' do + let!(:policy_violation_1) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + let!(:policy_violation_2) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + allow(security_policy).to receive(:warn_mode?).and_return(true) + end + + it 'dismisses all eligible violations' do + result = service.execute + + expect(result).to be_success + expect(result.payload[:dismissed_count]).to eq(2) + expect(policy_violation_1.reload).to be_dismissed + expect(policy_violation_2.reload).to be_dismissed + end + end + + context 'when policy is not in warn mode' do + let!(:policy_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + allow(security_policy).to receive(:warn_mode?).and_return(false) + end + + it 'does not dismiss violations' do + result = service.execute + + expect(result).to be_success + expect(result.payload[:dismissed_count]).to eq(0) + expect(policy_violation.reload).to be_failed + end + end + + context 'when there are no failed violations' do + let!(:policy_violation) do + create(:scan_result_policy_violation, :running, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + it 'returns success with zero dismissed count' do + result = service.execute + + expect(result).to be_success + expect(result.payload[:dismissed_count]).to eq(0) + end + end + + context 'when dismissal fails' do + let!(:policy_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + allow(security_policy).to receive(:warn_mode?).and_return(true) + allow(policy_violation).to receive(:update).and_raise(StandardError, 'Database error') + end + + it 'returns error with details' do + result = service.execute + + expect(result).to be_error + expect(result.message).to include('Failed to dismiss policy violations') + expect(result.message).to include('Database error') + end + end + end + + context 'when user lacks proper permissions' do + before do + project.add_guest(user) + end + + it 'returns access denied error' do + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Access denied') + expect(result.http_status).to eq(:forbidden) + end + end + + context 'when merge request is nil' do + let(:service) do + described_class.new( + user: user, + merge_request: nil, + comment: 'Test comment', + dismissal_reason: 'test' + ) + end + + it 'returns not found error' do + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('Merge request not found') + expect(result.http_status).to eq(:not_found) + end + end + + context 'when audit event creation fails' do + let!(:policy_violation) do + create(:scan_result_policy_violation, :failed, + project: project, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule) + end + + before do + project.add_developer(user) + allow(security_policy).to receive(:warn_mode?).and_return(true) + allow(AuditEventService).to receive(:new).and_raise(StandardError, 'Audit error') + end + + it 'still dismisses the violation but logs the audit error' do + expect(Rails.logger).to receive(:warn).with(/Failed to create audit event/) + + result = service.execute + + expect(result).to be_success + expect(policy_violation.reload).to be_dismissed + end + end + end + + describe 'authorization' do + context 'when user is a developer' do + before do + project.add_developer(user) + end + + it 'allows dismissal' do + expect(service.send(:authorized?)).to be_truthy + end + end + + context 'when user is a maintainer' do + before do + project.add_maintainer(user) + end + + it 'allows dismissal' do + expect(service.send(:authorized?)).to be_truthy + end + end + + context 'when user is a reporter' do + before do + project.add_reporter(user) + end + + it 'denies dismissal' do + expect(service.send(:authorized?)).to be_falsey + end + end + + context 'when user is not a project member' do + it 'denies dismissal' do + expect(service.send(:authorized?)).to be_falsey + end + end + end +end \ No newline at end of file -- GitLab