Merge branch...

Merge branch '347324-on-mr-merge-save-a-list-of-compliance-violations-that-occurred-at-the-point-of-merging' into 'master'

On MR merge, save a list of compliance violations that occurred at the point of merging

See merge request gitlab-org/gitlab!77954

config/sidekiq_queues.yml

@@ -79,6 +79,8 @@
   - 1
 - - cluster_agent
   - 1
+- - compliance_management_merge_requests_compliance_violations
+  - 1
 - - container_repository
   - 1
 - - create_commit_signature

ee/app/models/merge_requests/compliance_violation.rb

@@ -28,6 +28,7 @@ module MergeRequests
               }
     validates :reason, presence: true
 
+    # The below violations need to either ignore or handle their errors to help prevent the merge process failing
     VIOLATIONS = [
       ::Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestAuthor,
       ::Gitlab::ComplianceManagement::Violations::ApprovedByCommitter,

ee/app/services/compliance_management/merge_requests/base_service.rb

+# frozen_string_literal: true
+
+module ComplianceManagement
+  module MergeRequests
+    class BaseService
+      include BaseServiceUtility
+
+      def initialize(merge_request)
+        @merge_request = merge_request
+      end
+    end
+  end
+end

ee/app/services/compliance_management/merge_requests/create_compliance_violations_service.rb

+# frozen_string_literal: true
+
+module ComplianceManagement
+  module MergeRequests
+    class CreateComplianceViolationsService < ComplianceManagement::MergeRequests::BaseService
+      def execute
+        return ServiceResponse.error(message: _('This group is not permitted to create compliance violations')) unless permitted?(@merge_request.target_project.namespace)
+        return ServiceResponse.error(message: _('Merge request not merged')) unless @merge_request.merged?
+
+        ::MergeRequests::ComplianceViolation.process_merge_request(@merge_request)
+
+        ServiceResponse.success(message: _('Created compliance violations if any were found'))
+      end
+
+      private
+
+      def permitted?(group)
+        ::Feature.enabled?(:compliance_violations_graphql_type, group, default_enabled: :yaml) &&
+          group.licensed_feature_available?(:group_level_compliance_dashboard)
+      end
+    end
+  end
+end

ee/app/services/ee/merge_requests/post_merge_service.rb

@@ -9,6 +9,17 @@ module EE
       def execute(merge_request)
         super
         ApprovalRules::FinalizeService.new(merge_request).execute
+
+        if compliance_violations_enabled?(merge_request.target_project.namespace)
+          ComplianceManagement::MergeRequests::ComplianceViolationsWorker.perform_async(merge_request.id)
+        end
+      end
+
+      private
+
+      def compliance_violations_enabled?(group)
+        ::Feature.enabled?(:compliance_violations_graphql_type, group, default_enabled: :yaml) &&
+          group.licensed_feature_available?(:group_level_compliance_dashboard)
       end
     end
   end

ee/app/workers/all_queues.yml

@@ -921,6 +921,15 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: compliance_management_merge_requests_compliance_violations
+  :worker_name: ComplianceManagement::MergeRequests::ComplianceViolationsWorker
+  :feature_category: :compliance_management
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: create_github_webhook
   :worker_name: CreateGithubWebhookWorker
   :feature_category: :integrations

ee/app/workers/compliance_management/merge_requests/compliance_violations_worker.rb

+# frozen_string_literal: true
+
+module ComplianceManagement
+  module MergeRequests
+    class ComplianceViolationsWorker
+      include ApplicationWorker
+
+      data_consistency :always
+
+      sidekiq_options retry: 3
+      idempotent!
+
+      feature_category :compliance_management
+
+      def perform(merge_request_id)
+        merge_request = MergeRequest.find_by_id(merge_request_id)
+
+        return unless merge_request
+
+        ComplianceManagement::MergeRequests::CreateComplianceViolationsService.new(merge_request).execute
+      end
+    end
+  end
+end

ee/config/feature_flags/development/compliance_violations_graphql_type.yml

+---
+name: compliance_violations_graphql_type
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77954
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350249
+milestone: '14.8'
+type: development
+group: group::compliance
+default_enabled: false

ee/lib/gitlab/compliance_management/violations/approved_by_committer.rb

@@ -35,7 +35,7 @@ module Gitlab
 
         # rubocop: disable CodeReuse/ActiveRecord
         def approving_committer_ids
-          @merge_request.approver_users.pluck(:id) & @merge_request.committers.pluck(:id)
+          @merge_request.approved_by_users.pluck(:id) & @merge_request.committers.pluck(:id)
         end
         # rubocop: enable CodeReuse/ActiveRecord
 

ee/lib/gitlab/compliance_management/violations/approved_by_insufficient_users.rb

@@ -17,7 +17,7 @@ module Gitlab
         def execute
           if violation?
             @merge_request.compliance_violations.create(
-              violating_user: @merge_request.merge_user,
+              violating_user: @merge_request.metrics&.merged_by || @merge_request.merge_user,
               reason: REASON
             )
           end

ee/lib/gitlab/compliance_management/violations/approved_by_merge_request_author.rb

@@ -21,11 +21,9 @@ module Gitlab
 
         private
 
-        # rubocop: disable CodeReuse/ActiveRecord
         def violation?
-          @merge_request.approver_users.exists?(id: @merge_request.author_id)
+          @merge_request.approved_by_users.include?(@merge_request.author)
         end
-        # rubocop: enable CodeReuse/ActiveRecord
       end
     end
   end

ee/spec/lib/gitlab/compliance_management/violations/approved_by_committer_spec.rb

@@ -24,10 +24,12 @@ RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByCommitter do
     end
 
     context 'when merge request is approved by someone who also added a commit' do
+      let_it_be(:merge_request) { create(:merge_request, state: :merged) }
+
       before do
-        merge_request.approver_users << user
-        merge_request.approver_users << user2
-        merge_request.approver_users << user3
+        create(:approval, merge_request: merge_request, user: user)
+        create(:approval, merge_request: merge_request, user: user2)
+        create(:approval, merge_request: merge_request, user: user3)
       end
 
       it 'creates a ComplianceViolation for each violation', :aggregate_failures do

ee/spec/lib/gitlab/compliance_management/violations/approved_by_insufficient_users_spec.rb

@@ -35,6 +35,18 @@ RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByInsufficientU
         expect(violations.map(&:violating_user)).to contain_exactly(user)
       end
 
+      context 'when the merge requests merge user is within metrics' do
+        let_it_be(:merge_request) { create(:merge_request, :with_merged_metrics, author: user) }
+
+        it 'creates a ComplianceViolation', :aggregate_failures do
+          expect { execute }.to change { merge_request.compliance_violations.count }.by(1)
+
+          violations = merge_request.compliance_violations.where(reason: described_class::REASON)
+
+          expect(violations.map(&:violating_user)).to contain_exactly(user)
+        end
+      end
+
       context 'when the merge request does not have a merge user' do
         let_it_be(:merge_request) { create(:merge_request, state: :merged, merge_user: nil) }
 

ee/spec/lib/gitlab/compliance_management/violations/approved_by_merge_request_author_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestA
 
     context 'when merge request is approved by someone other than the author' do
       before do
-        merge_request.approver_users << create(:user)
+        create(:approval, merge_request: merge_request)
       end
 
       it 'does not create a ComplianceViolation' do
@@ -32,7 +32,7 @@ RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestA
 
       context 'when merge request is also approved by the author' do
         before do
-          merge_request.approver_users << author
+          create(:approval, merge_request: merge_request, user: author)
         end
 
         it_behaves_like 'violation'
@@ -41,7 +41,7 @@ RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestA
 
     context 'when merge request is approved by its author' do
       before do
-        merge_request.approver_users << author
+        create(:approval, merge_request: merge_request, user: author)
       end
 
       it_behaves_like 'violation'

ee/spec/services/compliance_management/merge_requests/create_compliance_violations_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ComplianceManagement::MergeRequests::CreateComplianceViolationsService do
+  # Works around https://gitlab.com/gitlab-org/gitlab/-/issues/335054
+  let_it_be_with_refind(:unmerged_merge_request) { create(:merge_request) }
+  let_it_be_with_refind(:merged_merge_request) { create(:merge_request, :merged) }
+
+  shared_examples 'does not call process_merge_request' do
+    subject { described_class.new(merged_merge_request).execute }
+
+    it :aggregate_failures do
+      expect(::MergeRequests::ComplianceViolation).not_to receive(:process_merge_request)
+
+      expect(subject.success?).to be false
+      expect(subject.message).to eq 'This group is not permitted to create compliance violations'
+    end
+  end
+
+  context 'when the compliance report feature is disabled' do
+    before do
+      stub_licensed_features(group_level_compliance_dashboard: false)
+    end
+
+    it_behaves_like 'does not call process_merge_request'
+  end
+
+  context 'when the compliance violations graphql type is disabled' do
+    before do
+      stub_feature_flags(compliance_violations_graphql_type: false)
+    end
+
+    it_behaves_like 'does not call process_merge_request'
+  end
+
+  context 'when the compliance violations graphql type is enabled' do
+    before do
+      stub_feature_flags(compliance_violations_graphql_type: true)
+      stub_licensed_features(group_level_compliance_dashboard: true)
+    end
+
+    context 'and the merge request is not merged', :aggregate_failures do
+      subject { described_class.new(unmerged_merge_request).execute }
+
+      it 'does not call process_merge_request' do
+        expect(::MergeRequests::ComplianceViolation).not_to receive(:process_merge_request)
+
+        expect(subject.success?).to be false
+        expect(subject.message).to eq 'Merge request not merged'
+      end
+    end
+
+    context 'and the merge request is merged' do
+      subject { described_class.new(merged_merge_request).execute }
+
+      it 'calls process_merge_request', :aggregate_failures do
+        expect(::MergeRequests::ComplianceViolation).to receive(:process_merge_request).with(merged_merge_request)
+
+        expect(subject.success?).to be true
+        expect(subject.message).to eq 'Created compliance violations if any were found'
+      end
+    end
+  end
+end

ee/spec/services/ee/merge_requests/post_merge_service_spec.rb

@@ -3,11 +3,15 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequests::PostMergeService do
-  let(:project) { merge_request.target_project }
-  let(:merge_request) { create(:merge_request) }
+  let_it_be(:project) { create(:project, :repository) }
+  # Works around https://gitlab.com/gitlab-org/gitlab/-/issues/335054
+  let_it_be_with_refind(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
   let(:current_user) { merge_request.author }
   let(:service) { described_class.new(project: project, current_user: current_user) }
 
+  subject { service.execute(merge_request) }
+
   describe '#execute' do
     context 'finalize approvals' do
       let(:finalize_service) { double(:finalize_service) }
@@ -16,7 +20,46 @@ RSpec.describe MergeRequests::PostMergeService do
         expect(ApprovalRules::FinalizeService).to receive(:new).and_return(finalize_service)
         expect(finalize_service).to receive(:execute)
 
-        service.execute(merge_request)
+        subject
+      end
+    end
+
+    context 'compliance violations' do
+      shared_examples 'does not call the compliance violations worker' do
+        it do
+          expect(ComplianceManagement::MergeRequests::ComplianceViolationsWorker).not_to receive(:perform_async)
+
+          subject
+        end
+      end
+
+      context 'when the compliance report feature is unlicensed' do
+        before do
+          stub_licensed_features(group_level_compliance_dashboard: false)
+        end
+
+        it_behaves_like 'does not call the compliance violations worker'
+      end
+
+      context 'when the compliance violations graphql type is disabled' do
+        before do
+          stub_feature_flags(compliance_violations_graphql_type: false)
+        end
+
+        it_behaves_like 'does not call the compliance violations worker'
+      end
+
+      context 'when the compliance violations graphql type is enabled' do
+        before do
+          stub_feature_flags(compliance_violations_graphql_type: true)
+          stub_licensed_features(group_level_compliance_dashboard: true)
+        end
+
+        it 'calls the compliance violations worker asynchronously' do
+          expect(ComplianceManagement::MergeRequests::ComplianceViolationsWorker).to receive(:perform_async).with(merge_request.id)
+
+          subject
+        end
       end
     end
   end

ee/spec/workers/compliance_management/merge_requests/compliance_violations_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ComplianceManagement::MergeRequests::ComplianceViolationsWorker do
+  let(:worker) { described_class.new }
+  let_it_be(:merge_request) { create(:merge_request, :merged) }
+
+  let(:compliance_violations_service) { instance_spy(ComplianceManagement::MergeRequests::CreateComplianceViolationsService) }
+  let(:job_args) { [merge_request] }
+
+  include_examples 'an idempotent worker'
+
+  describe "#perform" do
+    before do
+      allow(ComplianceManagement::MergeRequests::CreateComplianceViolationsService).to receive(:new).and_return(compliance_violations_service)
+    end
+
+    context 'if the merge request does not exist' do
+      it 'does not call the service' do
+        worker.perform(non_existing_record_id)
+
+        expect(ComplianceManagement::MergeRequests::CreateComplianceViolationsService).not_to have_received(:new)
+      end
+    end
+
+    context 'if the merge request exists' do
+      it 'calls the service' do
+        worker.perform(merge_request.id)
+
+        expect(ComplianceManagement::MergeRequests::CreateComplianceViolationsService).to have_received(:new).with(merge_request)
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -10369,6 +10369,9 @@ msgstr ""
 msgid "Created by:"
 msgstr ""
 
+msgid "Created compliance violations if any were found"
+msgstr ""
+
 msgid "Created date"
 msgstr ""
 
@@ -22399,6 +22402,9 @@ msgstr ""
 msgid "Merge request events"
 msgstr ""
 
+msgid "Merge request not merged"
+msgstr ""
+
 msgid "Merge request pipelines are configured. A detached pipeline runs in the context of the merge request, and not against the merged result. Learn more in the documentation for merge request pipelines."
 msgstr ""
 
@@ -36593,6 +36599,9 @@ msgstr ""
 msgid "This group is linked to a subscription"
 msgstr ""
 
+msgid "This group is not permitted to create compliance violations"
+msgstr ""
+
 msgid "This group, its subgroups and projects has been scheduled for removal on %{date}."
 msgstr ""
 

spec/workers/every_sidekiq_worker_spec.rb

@@ -457,7 +457,8 @@ RSpec.describe 'Every Sidekiq worker' do
         'WebHooks::DestroyWorker' => 3,
         'WebHooks::LogExecutionWorker' => 3,
         'Wikis::GitGarbageCollectWorker' => false,
-        'X509CertificateRevokeWorker' => 3
+        'X509CertificateRevokeWorker' => 3,
+        'ComplianceManagement::MergeRequests::ComplianceViolationsWorker' => 3
       }
     end