Commit 6020f428 authored by Robert Hunt's avatar Robert Hunt Committed by Doug Stull

Create table to store merge request compliance violations

Adds a new table to the database to store merge request compliance
violations and sets up the domain modal to process the violations.
Future MRs will do the actual storing of the violations and passing this
onto the frontend for consumption via GraphQL.

Changelog: added
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74290
parent 312d144a
# 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
# 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
# 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
0ab93a0bfd52d6c13203a0b183b2fcb9d6770334e5b1bd00a28fb623b65c428d
\ No newline at end of file
870100261e3704522d390885b8ff13ebbcb093aa508d79b90f9738f6a0fffd10
\ No newline at end of file
0cc2f19a8e31d9418ffd4fa1307f5210f0f2d781b957d417f06e19aca0b53985
\ No newline at end of file
......@@ -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;
......@@ -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
......
# 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
# 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
# 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
# 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
# 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
# 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
# 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
# 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
......@@ -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
......
# 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
......@@ -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
......
......@@ -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 ""
......
......@@ -198,6 +198,7 @@ merge_requests:
- system_note_metadata
- note_authors
- cleanup_schedule
- compliance_violations
external_pull_requests:
- project
merge_request_diff:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment