Merge branch '327119-1-create_merge_request_compliance_violation_model' into 'master'

Create table to store merge request compliance violations

See merge request gitlab-org/gitlab!74290

db/migrate/20211111112425_create_merge_requests_compliance_violations.rb

+# frozen_string_literal: true
+
+class CreateMergeRequestsComplianceViolations < Gitlab::Database::Migration[1.0]
+  def change
+    create_table :merge_requests_compliance_violations do |t|
+      t.bigint :violating_user_id, null: false
+      t.bigint :merge_request_id, null: false
+      t.integer :reason, limit: 2, null: false
+      t.index :violating_user_id
+      t.index [:merge_request_id, :violating_user_id, :reason], unique: true, name: 'index_merge_requests_compliance_violations_unique_columns'
+    end
+  end
+end

db/migrate/20211111112639_add_fk_compliance_violations_merge_request.rb

+# frozen_string_literal: true
+
+class AddFkComplianceViolationsMergeRequest < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :merge_requests_compliance_violations,
+                               :merge_requests,
+                               column: :merge_request_id,
+                               on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_requests_compliance_violations, column: :merge_request_id
+    end
+  end
+end

db/migrate/20211111112713_add_fk_compliance_violations_violating_user.rb

+# frozen_string_literal: true
+
+class AddFkComplianceViolationsViolatingUser < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :merge_requests_compliance_violations,
+                               :users,
+                               column: :violating_user_id,
+                               on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_requests_compliance_violations, column: :violating_user_id
+    end
+  end
+end

db/schema_migrations/20211111112425

+0ab93a0bfd52d6c13203a0b183b2fcb9d6770334e5b1bd00a28fb623b65c428d
\ No newline at end of file

db/schema_migrations/20211111112639

+870100261e3704522d390885b8ff13ebbcb093aa508d79b90f9738f6a0fffd10
\ No newline at end of file

db/schema_migrations/20211111112713

+0cc2f19a8e31d9418ffd4fa1307f5210f0f2d781b957d417f06e19aca0b53985
\ No newline at end of file

db/structure.sql

@@ -16272,6 +16272,22 @@ CREATE SEQUENCE merge_requests_closing_issues_id_seq
 
 ALTER SEQUENCE merge_requests_closing_issues_id_seq OWNED BY merge_requests_closing_issues.id;
 
+CREATE TABLE merge_requests_compliance_violations (
+    id bigint NOT NULL,
+    violating_user_id bigint NOT NULL,
+    merge_request_id bigint NOT NULL,
+    reason smallint NOT NULL
+);
+
+CREATE SEQUENCE merge_requests_compliance_violations_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE merge_requests_compliance_violations_id_seq OWNED BY merge_requests_compliance_violations.id;
+
 CREATE SEQUENCE merge_requests_id_seq
     START WITH 1
     INCREMENT BY 1
@@ -21883,6 +21899,8 @@ ALTER TABLE ONLY merge_requests ALTER COLUMN id SET DEFAULT nextval('merge_reque
 
 ALTER TABLE ONLY merge_requests_closing_issues ALTER COLUMN id SET DEFAULT nextval('merge_requests_closing_issues_id_seq'::regclass);
 
+ALTER TABLE ONLY merge_requests_compliance_violations ALTER COLUMN id SET DEFAULT nextval('merge_requests_compliance_violations_id_seq'::regclass);
+
 ALTER TABLE ONLY merge_trains ALTER COLUMN id SET DEFAULT nextval('merge_trains_id_seq'::regclass);
 
 ALTER TABLE ONLY metrics_dashboard_annotations ALTER COLUMN id SET DEFAULT nextval('metrics_dashboard_annotations_id_seq'::regclass);
@@ -23632,6 +23650,9 @@ ALTER TABLE ONLY merge_request_user_mentions
 ALTER TABLE ONLY merge_requests_closing_issues
     ADD CONSTRAINT merge_requests_closing_issues_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY merge_requests_compliance_violations
+    ADD CONSTRAINT merge_requests_compliance_violations_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY merge_requests
     ADD CONSTRAINT merge_requests_pkey PRIMARY KEY (id);
 
@@ -26708,6 +26729,10 @@ CREATE INDEX index_merge_requests_closing_issues_on_issue_id ON merge_requests_c
 
 CREATE INDEX index_merge_requests_closing_issues_on_merge_request_id ON merge_requests_closing_issues USING btree (merge_request_id);
 
+CREATE INDEX index_merge_requests_compliance_violations_on_violating_user_id ON merge_requests_compliance_violations USING btree (violating_user_id);
+
+CREATE UNIQUE INDEX index_merge_requests_compliance_violations_unique_columns ON merge_requests_compliance_violations USING btree (merge_request_id, violating_user_id, reason);
+
 CREATE INDEX index_merge_requests_on_assignee_id ON merge_requests USING btree (assignee_id);
 
 CREATE INDEX index_merge_requests_on_author_id ON merge_requests USING btree (author_id);
@@ -29250,6 +29275,9 @@ ALTER TABLE ONLY geo_event_log
 ALTER TABLE ONLY deployments
     ADD CONSTRAINT fk_289bba3222 FOREIGN KEY (cluster_id) REFERENCES clusters(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY merge_requests_compliance_violations
+    ADD CONSTRAINT fk_290ec1ab02 FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY coverage_fuzzing_corpuses
     ADD CONSTRAINT fk_29f6f15f82 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
 
@@ -29925,6 +29953,9 @@ ALTER TABLE ONLY pages_domains
 ALTER TABLE ONLY application_settings
     ADD CONSTRAINT fk_ec757bd087 FOREIGN KEY (file_template_project_id) REFERENCES projects(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY merge_requests_compliance_violations
+    ADD CONSTRAINT fk_ec881c1c6f FOREIGN KEY (violating_user_id) REFERENCES users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY events
     ADD CONSTRAINT fk_edfd187b6f FOREIGN KEY (author_id) REFERENCES users(id) ON DELETE CASCADE;
 

ee/app/models/ee/merge_request.rb

@@ -51,6 +51,8 @@ module EE
 
       has_many :blocked_merge_requests, through: :blocks_as_blocker
 
+      has_many :compliance_violations, class_name: 'MergeRequests::ComplianceViolation'
+
       delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
       delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
 

ee/app/models/merge_requests/compliance_violation.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class ComplianceViolation < ApplicationRecord
+    include BulkInsertSafe
+
+    self.table_name = 'merge_requests_compliance_violations'
+
+    # Reasons are defined by GitLab in our public documentation.
+    # https://docs.gitlab.com/ee/user/compliance/compliance_dashboard/#approval-status-and-separation-of-duties
+    enum reason: {
+      ::Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestAuthor::REASON => 0,
+      ::Gitlab::ComplianceManagement::Violations::ApprovedByCommitter::REASON => 1,
+      ::Gitlab::ComplianceManagement::Violations::ApprovedByInsufficientUsers::REASON => 2
+    }
+
+    scope :approved_by_committer, -> { where(reason: ::Gitlab::ComplianceManagement::Violations::ApprovedByCommitter::REASON) }
+
+    belongs_to :violating_user, class_name: 'User'
+    belongs_to :merge_request
+
+    validates :violating_user, presence: true
+    validates :merge_request,
+              presence: true,
+              uniqueness: {
+                scope: [:violating_user, :reason],
+                message: -> (_object, _data) { _('compliance violation has already been recorded') }
+              }
+    validates :reason, presence: true
+
+    VIOLATIONS = [
+      ::Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestAuthor,
+      ::Gitlab::ComplianceManagement::Violations::ApprovedByCommitter,
+      ::Gitlab::ComplianceManagement::Violations::ApprovedByInsufficientUsers
+    ].freeze
+
+    def self.process_merge_request(merge_request)
+      VIOLATIONS.each do |violation_check|
+        violation_check.new(merge_request).execute
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module ComplianceManagement
+    module Violations
+      class ApprovedByCommitter
+        REASON = :approved_by_committer
+
+        def initialize(merge_request)
+          @merge_request = merge_request
+        end
+
+        def execute
+          ::MergeRequests::ComplianceViolation.bulk_upsert!(
+            violations,
+            unique_by: [:merge_request_id, :violating_user_id, :reason],
+            batch_size: 100
+          )
+        end
+
+        private
+
+        def violations
+          violating_user_ids.map do |user_id|
+            @merge_request.compliance_violations.new(
+              violating_user_id: user_id,
+              reason: REASON
+            )
+          end
+        end
+
+        def violating_user_ids
+          approving_committer_ids.reject { |user_id| existing_violating_user_ids.include?(user_id) }
+        end
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def approving_committer_ids
+          @merge_request.approver_users.pluck(:id) & @merge_request.committers.pluck(:id)
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def existing_violating_user_ids
+          @merge_request.compliance_violations.approved_by_committer.pluck(:violating_user_id)
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module ComplianceManagement
+    module Violations
+      class ApprovedByInsufficientUsers
+        REASON = :approved_by_insufficient_users
+
+        # The minimum number of approvers is defined by GitLab in our public documentation.
+        # https://docs.gitlab.com/ee/user/compliance/compliance_dashboard/#approval-status-and-separation-of-duties
+        MINIMUM_NUMBER_OF_APPROVERS = 2
+
+        def initialize(merge_request)
+          @merge_request = merge_request
+        end
+
+        def execute
+          if violation?
+            @merge_request.compliance_violations.create(
+              violating_user: @merge_request.merge_user,
+              reason: REASON
+            )
+          end
+        end
+
+        private
+
+        def violation?
+          @merge_request.approvers.count < MINIMUM_NUMBER_OF_APPROVERS
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module ComplianceManagement
+    module Violations
+      class ApprovedByMergeRequestAuthor
+        REASON = :approved_by_merge_request_author
+
+        def initialize(merge_request)
+          @merge_request = merge_request
+        end
+
+        def execute
+          if violation?
+            @merge_request.compliance_violations.create(
+              violating_user: @merge_request.author,
+              reason: REASON
+            )
+          end
+        end
+
+        private
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def violation?
+          @merge_request.approver_users.exists?(id: @merge_request.author_id)
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+      end
+    end
+  end
+end

ee/spec/factories/compliance_violations.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :compliance_violation, class: 'MergeRequests::ComplianceViolation' do
+    violating_user factory: :user
+    merge_request
+
+    trait :approved_by_merge_request_author do
+      reason { :approved_by_merge_request_author }
+    end
+
+    trait :approved_by_committer do
+      reason { :approved_by_committer }
+    end
+
+    trait :approved_by_insufficient_users do
+      reason { :approved_by_insufficient_users }
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByCommitter do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:user2) { create(:user) }
+  let_it_be(:user3) { create(:user) }
+  let_it_be(:merge_request) { create(:merge_request, state: :merged) }
+
+  subject(:violation) { described_class.new(merge_request) }
+
+  describe '#execute' do
+    before do
+      allow(merge_request).to receive(:committers).and_return([user, user2])
+    end
+
+    subject(:execute) { violation.execute }
+
+    context 'when merge request is approved by someone who did not add a commit' do
+      it 'does not create a ComplianceViolation' do
+        expect { execute }.not_to change(MergeRequests::ComplianceViolation, :count)
+      end
+    end
+
+    context 'when merge request is approved by someone who also added a commit' do
+      before do
+        merge_request.approver_users << user
+        merge_request.approver_users << user2
+        merge_request.approver_users << user3
+      end
+
+      it 'creates a ComplianceViolation for each violation', :aggregate_failures do
+        expect { execute }.to change { merge_request.compliance_violations.count }.by(2)
+
+        violations = merge_request.compliance_violations.where(reason: described_class::REASON)
+
+        expect(violations.map(&:violating_user)).to contain_exactly(user, user2)
+      end
+
+      context 'when called more than once with the same violations' do
+        before do
+          create(:compliance_violation, :approved_by_committer, merge_request: merge_request, violating_user: user)
+          create(:compliance_violation, :approved_by_committer, merge_request: merge_request, violating_user: user2)
+
+          allow(merge_request).to receive(:committers).and_return([user, user3])
+        end
+
+        it 'does not insert duplicates', :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, user2, user3)
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByInsufficientUsers do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:merge_request) { create(:merge_request, state: :merged, merge_user: user) }
+
+  subject(:violation) { described_class.new(merge_request) }
+
+  describe '#execute' do
+    subject(:execute) { violation.execute }
+
+    context 'when merge request is approved by sufficient number of users' do
+      before do
+        merge_request.approver_users << create(:user)
+        merge_request.approver_users << create(:user)
+      end
+
+      it 'does not create a ComplianceViolation' do
+        expect { execute }.not_to change(MergeRequests::ComplianceViolation, :count)
+      end
+    end
+
+    context 'when merge request is approved by insufficient number of users' do
+      before do
+        merge_request.approver_users << create(:user)
+      end
+
+      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
+
+      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) }
+
+        it 'does not create a ComplianceViolation', :aggregate_failures do
+          expect(execute).not_to be_valid
+          expect(execute.errors.full_messages.join).to eq('Violating user can\'t be blank')
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestAuthor do
+  let_it_be(:author) { create(:user) }
+  let_it_be(:merge_request) { create(:merge_request, state: :merged, author: author) }
+
+  subject(:violation) { described_class.new(merge_request) }
+
+  describe '#execute' do
+    shared_examples 'violation' do
+      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(author)
+      end
+    end
+
+    subject(:execute) { violation.execute }
+
+    context 'when merge request is approved by someone other than the author' do
+      before do
+        merge_request.approver_users << create(:user)
+      end
+
+      it 'does not create a ComplianceViolation' do
+        expect { execute }.not_to change(MergeRequests::ComplianceViolation, :count)
+      end
+
+      context 'when merge request is also approved by the author' do
+        before do
+          merge_request.approver_users << author
+        end
+
+        it_behaves_like 'violation'
+      end
+    end
+
+    context 'when merge request is approved by its author' do
+      before do
+        merge_request.approver_users << author
+      end
+
+      it_behaves_like 'violation'
+    end
+  end
+end

ee/spec/models/merge_request_spec.rb

@@ -28,6 +28,7 @@ RSpec.describe MergeRequest do
     it { is_expected.to have_many(:approval_merge_request_rule_sources).through(:approval_rules) }
     it { is_expected.to have_many(:approval_project_rules).through(:approval_merge_request_rule_sources) }
     it { is_expected.to have_many(:status_check_responses).class_name('MergeRequests::StatusCheckResponse').inverse_of(:merge_request) }
+    it { is_expected.to have_many(:compliance_violations).class_name('MergeRequests::ComplianceViolation') }
 
     describe 'approval_rules association' do
       describe '#applicable_to_branch' do

ee/spec/models/merge_requests/compliance_violation_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::ComplianceViolation, type: :model do
+  let_it_be(:merge_request) { create(:merge_request, state: :merged) }
+
+  describe "Associations" do
+    it { is_expected.to belong_to(:violating_user) }
+    it { is_expected.to belong_to(:merge_request) }
+  end
+
+  describe "Validations" do
+    it { is_expected.to validate_presence_of(:violating_user) }
+    it { is_expected.to validate_presence_of(:merge_request) }
+    it { is_expected.to validate_presence_of(:reason) }
+
+    context "when a violation exists with the same reason and user for a merge request" do
+      let_it_be(:user) { create(:user) }
+
+      before do
+        allow(merge_request).to receive(:committers).and_return([user])
+        merge_request.approver_users << user
+      end
+
+      it "does not create a duplicate merge request violation", :aggregate_failures do
+        violation = create(:compliance_violation, :approved_by_committer, merge_request: merge_request, violating_user: user)
+        duplicate_violation = build(:compliance_violation, :approved_by_committer, merge_request: merge_request, violating_user: user)
+
+        expect(violation).to be_valid
+        expect(duplicate_violation).not_to be_valid
+        expect(duplicate_violation.errors.full_messages.join).to eq('Merge request compliance violation has already been recorded')
+      end
+    end
+  end
+
+  describe "Enums" do
+    it {
+      is_expected.to define_enum_for(:reason).with_values(
+        [
+          ::Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestAuthor::REASON,
+          ::Gitlab::ComplianceManagement::Violations::ApprovedByCommitter::REASON,
+          ::Gitlab::ComplianceManagement::Violations::ApprovedByInsufficientUsers::REASON
+        ]
+      )
+    }
+  end
+
+  describe '#approved_by_committer' do
+    let_it_be(:violations) do
+      [
+        create(:compliance_violation, :approved_by_committer, merge_request: merge_request, violating_user: create(:user)),
+        create(:compliance_violation, :approved_by_committer, merge_request: merge_request, violating_user: create(:user))
+      ]
+    end
+
+    before do
+      # Add a violation using a different reason to make sure it doesn't pick it up
+      create(:compliance_violation, :approved_by_insufficient_users, merge_request: merge_request, violating_user: create(:user))
+    end
+
+    it 'returns the correct collection of violations' do
+      expect(merge_request.compliance_violations.approved_by_committer).to eq violations
+    end
+  end
+
+  describe '.process_merge_request' do
+    it 'loops through each violation class', :aggregate_failures do
+      expect_next_instance_of(::Gitlab::ComplianceManagement::Violations::ApprovedByMergeRequestAuthor, merge_request) do |violation|
+        expect(violation).to receive(:execute)
+      end
+
+      expect_next_instance_of(::Gitlab::ComplianceManagement::Violations::ApprovedByCommitter, merge_request) do |violation|
+        expect(violation).to receive(:execute)
+      end
+
+      expect_next_instance_of(::Gitlab::ComplianceManagement::Violations::ApprovedByInsufficientUsers, merge_request) do |violation|
+        expect(violation).to receive(:execute)
+      end
+
+      described_class.process_merge_request(merge_request)
+    end
+  end
+end

lib/gitlab/database/gitlab_schemas.yml

@@ -297,6 +297,7 @@ members: :gitlab_main
 merge_request_assignees: :gitlab_main
 merge_request_blocks: :gitlab_main
 merge_request_cleanup_schedules: :gitlab_main
+merge_requests_compliance_violations: :gitlab_main
 merge_request_context_commit_diff_files: :gitlab_main
 merge_request_context_commits: :gitlab_main
 merge_request_diff_commits: :gitlab_main

locale/gitlab.pot

@@ -41531,6 +41531,9 @@ msgstr ""
 msgid "committed"
 msgstr ""
 
+msgid "compliance violation has already been recorded"
+msgstr ""
+
 msgid "container_name can contain only lowercase letters, digits, '-', and '.' and must start and end with an alphanumeric character"
 msgstr ""
 

spec/lib/gitlab/import_export/all_models.yml

@@ -198,6 +198,7 @@ merge_requests:
 - system_note_metadata
 - note_authors
 - cleanup_schedule
+- compliance_violations
 external_pull_requests:
 - project
 merge_request_diff: