From 2e17182251c74936ea288f491f02c7ebcd423be3 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 10 Mar 2022 16:39:15 +0530 Subject: [PATCH 01/14] Add audit events for merge request settings Changelog: added EE: true --- app/services/base_container_service.rb | 15 ++++ .../merge_requests/external_status_check.rb | 22 ++++- .../external_status_checks/create_service.rb | 8 +- .../external_status_checks/destroy_service.rb | 1 + .../external_status_checks/update_service.rb | 6 +- .../external_status_check_changes_auditor.rb | 40 +++++++++ ee/lib/ee/audit/project_changes_auditor.rb | 1 + .../audit/project_setting_changes_auditor.rb | 2 + ...ernal_status_check_changes_auditor_spec.rb | 41 +++++++++ .../ee/audit/project_changes_auditor_spec.rb | 7 ++ .../project_setting_changes_auditor_spec.rb | 16 +++- ee/spec/models/approval_project_rule_spec.rb | 36 ++------ .../external_status_check_spec.rb | 60 ++++++++++++++ .../create_service_spec.rb | 37 +++++++++ .../destroy_service_spec.rb | 8 ++ .../update_service_spec.rb | 83 +++++++++++++++++++ .../audit_event_queue_shared_context.rb | 27 ++++++ 17 files changed, 370 insertions(+), 40 deletions(-) create mode 100644 ee/lib/ee/audit/external_status_check_changes_auditor.rb create mode 100644 ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb create mode 100644 ee/spec/support/shared_contexts/audit_event_queue_shared_context.rb diff --git a/app/services/base_container_service.rb b/app/services/base_container_service.rb index 86df0236a7f093..866ee7c026cec7 100644 --- a/app/services/base_container_service.rb +++ b/app/services/base_container_service.rb @@ -30,4 +30,19 @@ def group_container? def namespace_container? container.is_a?(::Namespace) end + + def with_audit_logged(rule, &block) + audit_context = { + name: 'status_check', + author: current_user, + scope: rule.project, + target: rule + } + + ::Gitlab::Audit::Auditor.audit(audit_context, &block) + end + + def log_audit_event(rule) + ::EE::Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute + end end diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index 9e6ae021b57c26..6f198f7fbffdf2 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -4,6 +4,7 @@ module MergeRequests class ExternalStatusCheck < ApplicationRecord self.table_name = 'external_status_checks' + include Auditable include IgnorableColumns ignore_column :external_approval_rule_id, remove_with: '14.3', remove_after: '2021-09-22' @@ -15,8 +16,9 @@ class ExternalStatusCheck < ApplicationRecord end belongs_to :project - has_and_belongs_to_many :protected_branches - + has_and_belongs_to_many :protected_branches, + after_add: :audit_protected_branch_add, after_remove: :audit_protected_branch_remove + after_create_commit :audit_creation validates :external_url, presence: true, uniqueness: { scope: :project_id }, addressable_url: true validates :name, uniqueness: { scope: :project_id }, presence: true validate :protected_branches_must_belong_to_project @@ -43,6 +45,22 @@ def to_h } end + def audit_protected_branch_add(model) + message = "Added #{model.class.name} #{model.name} to #{self.name} status check" + message += " and removed All Branches from status check" if protected_branches.count == 1 + push_audit_event(message) + end + + def audit_creation + push_audit_event("Added status check") + end + + def audit_protected_branch_remove(model) + message = "Removed #{model.class.name} #{model.name} from #{self.name} status check" + message += " and added All Branches to status check" if protected_branches.blank? + push_audit_event(message) + end + private def payload_data(merge_request_hook_data) diff --git a/ee/app/services/external_status_checks/create_service.rb b/ee/app/services/external_status_checks/create_service.rb index 4cd0deaa4ec429..72f2252afb04f5 100644 --- a/ee/app/services/external_status_checks/create_service.rb +++ b/ee/app/services/external_status_checks/create_service.rb @@ -6,11 +6,11 @@ def execute return ServiceResponse.error(message: 'Failed to create rule', payload: { errors: ['Not allowed'] }, http_status: :unauthorized) unless current_user.can?(:admin_project, container) rule = container.external_status_checks.new(name: params[:name], - project: container, - external_url: params[:external_url], - protected_branch_ids: params[:protected_branch_ids]) + project: container, + external_url: params[:external_url], + protected_branch_ids: params[:protected_branch_ids]) - if rule.save + if with_audit_logged(rule) { rule.save } ServiceResponse.success(payload: { rule: rule }) else ServiceResponse.error(message: 'Failed to create rule', payload: { errors: rule.errors.full_messages }, http_status: :unprocessable_entity) diff --git a/ee/app/services/external_status_checks/destroy_service.rb b/ee/app/services/external_status_checks/destroy_service.rb index b6bf2ceb69dbc8..6b53da1b0176f4 100644 --- a/ee/app/services/external_status_checks/destroy_service.rb +++ b/ee/app/services/external_status_checks/destroy_service.rb @@ -6,6 +6,7 @@ def execute(rule) return unauthorized_error_response unless current_user.can?(:admin_project, container) if rule.destroy + log_audit_event(rule) ServiceResponse.success else ServiceResponse.error(message: 'Failed to destroy rule', diff --git a/ee/app/services/external_status_checks/update_service.rb b/ee/app/services/external_status_checks/update_service.rb index 31fbed3c31dcf4..eb929384dcb32f 100644 --- a/ee/app/services/external_status_checks/update_service.rb +++ b/ee/app/services/external_status_checks/update_service.rb @@ -5,11 +5,13 @@ class UpdateService < BaseContainerService def execute return unauthorized_error_response unless current_user.can?(:admin_project, container) - if rule.update(resource_params) + current_rule = rule + if with_audit_logged(current_rule) { current_rule.update(resource_params) } + log_audit_event(current_rule) ServiceResponse.success(payload: { rule: rule }) else ServiceResponse.error(message: 'Failed to update rule', - payload: { errors: rule.errors.full_messages }, + payload: { errors: current_rule.errors.full_messages }, http_status: :unprocessable_entity) end end diff --git a/ee/lib/ee/audit/external_status_check_changes_auditor.rb b/ee/lib/ee/audit/external_status_check_changes_auditor.rb new file mode 100644 index 00000000000000..61094ae51e4627 --- /dev/null +++ b/ee/lib/ee/audit/external_status_check_changes_auditor.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true +module EE + module Audit + class ExternalStatusCheckChangesAuditor < BaseChangesAuditor + def initialize(current_user, external_status_check) + @project = external_status_check.project + + super + end + + def execute + if model.destroyed? + audit_deletion + else + audit_changes(:name, as: 'name', entity: @project, model: model) + audit_changes(:external_url, as: 'external url', entity: @project, model: model) + end + end + + def attributes_from_auditable_model(column) + { + from: model.previous_changes[column].first, + to: model.previous_changes[column].last + } + end + + private + + def audit_deletion + audit_context = { + author: @current_user, + scope: @project, + target: model, + message: "Removed status check" + } + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end +end diff --git a/ee/lib/ee/audit/project_changes_auditor.rb b/ee/lib/ee/audit/project_changes_auditor.rb index 1c72fa7e3919f2..e65431089d2711 100644 --- a/ee/lib/ee/audit/project_changes_auditor.rb +++ b/ee/lib/ee/audit/project_changes_auditor.rb @@ -16,6 +16,7 @@ def execute audit_changes(:reset_approvals_on_push, as: 'require new approvals when new commits are added to an MR', model: model) audit_changes(:disable_overriding_approvers_per_merge_request, as: 'prevent users from modifying MR approval rules in merge requests', model: model) audit_changes(:require_password_to_approve, as: 'require user password for approvals', model: model) + audit_changes(:merge_requests_template, as: 'merge_requests_template', model: model) if should_audit? :merge_requests_template audit_changes(:resolve_outdated_diff_discussions, as: 'resolve_outdated_diff_discussions', model: model) audit_changes(:printing_merge_request_link_enabled, as: 'printing_merge_request_link_enabled', model: model) diff --git a/ee/lib/ee/audit/project_setting_changes_auditor.rb b/ee/lib/ee/audit/project_setting_changes_auditor.rb index 65a06595ba22e7..c2dd6f12f77225 100644 --- a/ee/lib/ee/audit/project_setting_changes_auditor.rb +++ b/ee/lib/ee/audit/project_setting_changes_auditor.rb @@ -14,6 +14,8 @@ def execute audit_changes(:squash_option, as: 'squash_option', entity: @project, model: model) audit_changes(:allow_merge_on_skipped_pipeline, as: 'allow_merge_on_skipped_pipeline', entity: @project, model: model) + audit_changes(:merge_commit_template, as: 'merge_commit_template', entity: @project, model: model) + audit_changes(:squash_commit_template, as: 'squash_commit_template', entity: @project, model: model) end def attributes_from_auditable_model(column) diff --git a/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb new file mode 100644 index 00000000000000..f11337dff9ae33 --- /dev/null +++ b/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EE::Audit::ExternalStatusCheckChangesAuditor do + describe 'auditing external status check changes' do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :pages_enabled, visibility_level: 0) } + let_it_be(:rule) { create( :external_status_check, project: project) } + let_it_be(:external_status_check_changes_auditor) { described_class.new(user, rule) } + + before do + stub_licensed_features(extended_audit_events: true) + end + + let(:subject) { described_class.new(user, project.external_status_checks) } + + context 'when audit change happens' do + it 'creates an event when the name changes' do + rule.update!(name: 'name') + + expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'name' + end + + it 'creates an event when the external url changes' do + rule.update!(external_url: 'http://example.com') + + expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'external url' + end + + it 'creates an event when the status check is deleted' do + rule.destroy! + + expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq 'Removed status check' + end + end + end +end diff --git a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb index 32b37188d672f9..6503074fe4b5e1 100644 --- a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb @@ -83,6 +83,13 @@ expect(AuditEvent.last.details[:change]).to eq 'packages_enabled' end + it 'creates an event when the merge requests template changes' do + project.update!(merge_requests_template: 'merge request template') + + expect { foo_instance.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'merge_requests_template' + end + it 'creates an event when the merge requests author approval changes' do project.update!(merge_requests_author_approval: true) diff --git a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb index fe6af59faf22af..4d682a9b538f12 100644 --- a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb @@ -40,7 +40,7 @@ context 'when allow_merge_on_skipped_pipeline is changed' do where(:prev_value, :new_value) do - true | false + true | false false | true end @@ -60,7 +60,21 @@ }) end end + it 'creates an event when the merge_requests_template change' do + project.project_setting.update!(merge_commit_template: 'merge commit template') + + expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'merge_commit_template' + end + + it 'creates an event when the squash_commit_template change' do + project.project_setting.update!(squash_commit_template: 'squash_commit_template') + + expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'squash_commit_template' + end end end end end + diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb index 8c74cfca0850e1..b776b903d859c9 100644 --- a/ee/spec/models/approval_project_rule_spec.rb +++ b/ee/spec/models/approval_project_rule_spec.rb @@ -246,65 +246,39 @@ let_it_be(:rule, reload: true) { create(:approval_project_rule, name: 'Vulnerability', users: [user], groups: [group]) } - shared_examples 'auditable' do - context 'when audit event queue is active' do - before do - allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(true) - end - - it 'adds message to audit event queue' do - action! - - expect(::Gitlab::Audit::EventQueue.current).to contain_exactly(message) - end - end - - context 'when audit event queue is not active' do - before do - allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(false) - end - - it 'does not add message to audit event queue' do - action! - - expect(::Gitlab::Audit::EventQueue.current).to be_empty - end - end - end - describe '#audit_add users after :add' do let(:action!) { rule.update!(users: [user, new_user]) } let(:message) { 'Added User Spiderman to approval group on Vulnerability rule' } - it_behaves_like 'auditable' + it_behaves_like 'audit event queue' end describe '#audit_remove users after :remove' do let(:action!) { rule.update!(users: []) } let(:message) { 'Removed User Batman from approval group on Vulnerability rule' } - it_behaves_like 'auditable' + it_behaves_like 'audit event queue' end describe '#audit_add groups after :add' do let(:action!) { rule.update!(groups: [group, new_group]) } let(:message) { 'Added Group Avengers to approval group on Vulnerability rule' } - it_behaves_like 'auditable' + it_behaves_like 'audit event queue' end describe '#audit_remove groups after :remove' do let(:action!) { rule.update!(groups: []) } let(:message) { 'Removed Group Justice League from approval group on Vulnerability rule' } - it_behaves_like 'auditable' + it_behaves_like 'audit event queue' end describe "#audit_creation after approval rule is created" do let(:action!) { create(:approval_project_rule, approvals_required: 1) } let(:message) {'Added approval rule with number of required approvals of 1'} - it_behaves_like 'auditable' + it_behaves_like 'audit event queue' end describe '#vulnerability_states_for_branch' do diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index 0bd4a4e767c9ae..868aadea0e2b66 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -187,4 +187,64 @@ end end end + + describe 'callbacks', :request_store do + let_it_be(:project) { create(:project) } + let_it_be(:master_branch) { create(:protected_branch, project: project, name: 'master') } + let_it_be(:main_branch) { create(:protected_branch, project: project, name: 'main') } + let_it_be(:check, reload: true) do + create(:external_status_check, project: project, name: 'QA', protected_branches: []) + end + + describe '#audit_add branches after :add' do + context 'when branch is added from zero branches' do + let(:action!) { check.update!(protected_branches: [main_branch]) } + let(:message) { 'Added ProtectedBranch main to QA status check and removed All Branches from status check' } + + it_behaves_like 'audit event queue' + end + + context 'when branch is another branch is added' do + before do + check.update!(protected_branches: [main_branch]) + end + + let(:action!) { check.update!(protected_branches: [main_branch, master_branch]) } + let(:message) { 'Added ProtectedBranch master to QA status check' } + + it_behaves_like 'audit event queue' + end + end + + describe '#audit_remove branches after :remove' do + context 'when all the branches are removed' do + before do + check.update!(protected_branches: [main_branch]) + end + + let(:action!) { check.update!(protected_branches: []) } + let(:message) { 'Removed ProtectedBranch main from QA status check and added All Branches to status check' } + + it_behaves_like 'audit event queue' + end + + context 'when a branch is removed' do + before do + check.update!(protected_branches: [main_branch, master_branch]) + end + + let(:action!) { check.update!(protected_branches: [master_branch]) } + let(:message) { 'Removed ProtectedBranch main from QA status check' } + + it_behaves_like 'audit event queue' + end + end + + describe '#audit_creation external status check after :create' do + let(:action!) { create(:external_status_check) } + let(:message) { 'Added status check' } + + it_behaves_like 'audit event queue' + end + end end diff --git a/ee/spec/services/external_status_checks/create_service_spec.rb b/ee/spec/services/external_status_checks/create_service_spec.rb index b65f9f63e23209..f37d21ff493c3b 100644 --- a/ee/spec/services/external_status_checks/create_service_spec.rb +++ b/ee/spec/services/external_status_checks/create_service_spec.rb @@ -66,4 +66,41 @@ expect(rule.protected_branches).to contain_exactly(protected_branch) end end + + describe 'audit events' do + context 'when licensed' do + before do + stub_licensed_features(audit_events: true) + end + + context 'when rule save operation succeeds', :request_store do + it 'logs an audit event' do + expect { subject }.to change { AuditEvent.count }.by(1) + end + end + + context 'when rule save operation fails' do + before do + allow(::MergeRequests::ExternalStatusCheck).to receive(:save).and_return(false) + end + + it 'does not log any audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end + context 'when not licensed' do + before do + stub_licensed_features( + admin_audit_log: false, + audit_events: false, + extended_audit_events: false + ) + end + + it 'does not log any audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end end diff --git a/ee/spec/services/external_status_checks/destroy_service_spec.rb b/ee/spec/services/external_status_checks/destroy_service_spec.rb index 91dcf02f0f54bd..a03a2c30ad587a 100644 --- a/ee/spec/services/external_status_checks/destroy_service_spec.rb +++ b/ee/spec/services/external_status_checks/destroy_service_spec.rb @@ -40,4 +40,12 @@ expect(subject.payload[:errors]).to contain_exactly('Not allowed') end end + + context 'audit changes' do + it 'execute ExternalStatusCheckChangesAuditor' do + expect(::EE::Audit::ExternalStatusCheckChangesAuditor).to receive(:new).with(current_user, rule).and_call_original + + subject + end + end end diff --git a/ee/spec/services/external_status_checks/update_service_spec.rb b/ee/spec/services/external_status_checks/update_service_spec.rb index e3449571e36824..0a300259538e4c 100644 --- a/ee/spec/services/external_status_checks/update_service_spec.rb +++ b/ee/spec/services/external_status_checks/update_service_spec.rb @@ -48,4 +48,87 @@ expect(subject.payload[:errors]).to contain_exactly('Not allowed') end end + + describe 'audit events' do + context 'when licensed' do + before do + stub_licensed_features(audit_events: true) + end + let_it_be(:master_branch) { create(:protected_branch, project: project, name: 'master') } + let_it_be(:main_branch) {create(:protected_branch, project: project, name: 'main')} + let_it_be(:check, reload: true) { create(:external_status_check, name: 'QA', project: project, protected_branches: []) } + + context 'when a branch is added', :request_store do + context 'when a new branch is added' do + let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [main_branch.id] } } + + it 'logs an audit event' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq "Added ProtectedBranch main to QA status check and removed All Branches from status check" + end + end + + context 'when another branch is added' do + before do + check.update!(protected_branches: [main_branch]) + end + + let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [main_branch.id, master_branch.id] } } + + it 'logs an audit event' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq "Added ProtectedBranch master to QA status check" + end + end + end + + context 'when a branch is removed', :request_store do + context 'when the only branch is removed' do + before do + check.update!(protected_branches: [main_branch]) + end + + let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [] } } + + it 'logs an audit event' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq "Removed ProtectedBranch main from QA status check and added All Branches to status check" + end + end + + context 'when a branch is removed' do + before do + check.update!(protected_branches: [main_branch, master_branch]) + end + + let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [main_branch.id] } } + + it 'logs an audit event' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq "Removed ProtectedBranch master from QA status check" + end + end + end + end + + it 'execute ExternalStatusCheckChangesAuditor' do + expect(::EE::Audit::ExternalStatusCheckChangesAuditor).to receive(:new).with(current_user, check).and_call_original + + subject + end + end + + context 'when not licensed' do + before do + stub_licensed_features( + admin_audit_log: false, + audit_events: false, + extended_audit_events: false + ) + end + + it 'does not log any audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end end diff --git a/ee/spec/support/shared_contexts/audit_event_queue_shared_context.rb b/ee/spec/support/shared_contexts/audit_event_queue_shared_context.rb new file mode 100644 index 00000000000000..4d6ab5205af01f --- /dev/null +++ b/ee/spec/support/shared_contexts/audit_event_queue_shared_context.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +RSpec.shared_context 'audit event queue' do + context 'when audit event queue is active' do + before do + allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(true) + end + + it 'adds message to audit event queue' do + action! + + expect(::Gitlab::Audit::EventQueue.current).to contain_exactly(message) + end + end + + context 'when audit event queue is not active' do + before do + allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(false) + end + + it 'does not add message to audit event queue' do + action! + + expect(::Gitlab::Audit::EventQueue.current).to be_empty + end + end +end -- GitLab From 9cae541cc0b0f271b1a9d315d0ae3d188a8adc04 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 14 Apr 2022 14:07:14 +0530 Subject: [PATCH 02/14] Refactor test cases and callbacks --- app/services/base_container_service.rb | 14 ------- .../merge_requests/external_status_check.rb | 5 +++ .../external_status_checks/create_service.rb | 13 +++++++ .../external_status_checks/destroy_service.rb | 14 ++++++- .../external_status_checks/update_service.rb | 25 ++++++++++--- .../external_status_check_changes_auditor.rb | 20 +--------- .../destroy_service_spec.rb | 37 +++++++++++++++++-- 7 files changed, 85 insertions(+), 43 deletions(-) diff --git a/app/services/base_container_service.rb b/app/services/base_container_service.rb index 866ee7c026cec7..1bdbba04787c76 100644 --- a/app/services/base_container_service.rb +++ b/app/services/base_container_service.rb @@ -31,18 +31,4 @@ def namespace_container? container.is_a?(::Namespace) end - def with_audit_logged(rule, &block) - audit_context = { - name: 'status_check', - author: current_user, - scope: rule.project, - target: rule - } - - ::Gitlab::Audit::Auditor.audit(audit_context, &block) - end - - def log_audit_event(rule) - ::EE::Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute - end end diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index 6f198f7fbffdf2..47892a4b7208da 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -19,6 +19,7 @@ class ExternalStatusCheck < ApplicationRecord has_and_belongs_to_many :protected_branches, after_add: :audit_protected_branch_add, after_remove: :audit_protected_branch_remove after_create_commit :audit_creation + after_destroy_commit :audit_deletion validates :external_url, presence: true, uniqueness: { scope: :project_id }, addressable_url: true validates :name, uniqueness: { scope: :project_id }, presence: true validate :protected_branches_must_belong_to_project @@ -55,6 +56,10 @@ def audit_creation push_audit_event("Added status check") end + def audit_deletion + push_audit_event("Removed status check") + end + def audit_protected_branch_remove(model) message = "Removed #{model.class.name} #{model.name} from #{self.name} status check" message += " and added All Branches to status check" if protected_branches.blank? diff --git a/ee/app/services/external_status_checks/create_service.rb b/ee/app/services/external_status_checks/create_service.rb index 72f2252afb04f5..bc8d897246bf6e 100644 --- a/ee/app/services/external_status_checks/create_service.rb +++ b/ee/app/services/external_status_checks/create_service.rb @@ -16,5 +16,18 @@ def execute ServiceResponse.error(message: 'Failed to create rule', payload: { errors: rule.errors.full_messages }, http_status: :unprocessable_entity) end end + + private + + def with_audit_logged(rule, &block) + audit_context = { + name: 'create_status_check', + author: current_user, + scope: rule.project, + target: rule + } + + ::Gitlab::Audit::Auditor.audit(audit_context, &block) + end end end diff --git a/ee/app/services/external_status_checks/destroy_service.rb b/ee/app/services/external_status_checks/destroy_service.rb index 6b53da1b0176f4..64beeeef6958bf 100644 --- a/ee/app/services/external_status_checks/destroy_service.rb +++ b/ee/app/services/external_status_checks/destroy_service.rb @@ -5,8 +5,7 @@ class DestroyService < BaseContainerService def execute(rule) return unauthorized_error_response unless current_user.can?(:admin_project, container) - if rule.destroy - log_audit_event(rule) + if with_audit_logged(rule) { rule.destroy } ServiceResponse.success else ServiceResponse.error(message: 'Failed to destroy rule', @@ -24,5 +23,16 @@ def unauthorized_error_response http_status: :unauthorized ) end + + def with_audit_logged(rule, &block) + audit_context = { + name: 'delete_status_check', + author: current_user, + scope: rule.project, + target: rule + } + + ::Gitlab::Audit::Auditor.audit(audit_context, &block) + end end end diff --git a/ee/app/services/external_status_checks/update_service.rb b/ee/app/services/external_status_checks/update_service.rb index eb929384dcb32f..0486a34370927c 100644 --- a/ee/app/services/external_status_checks/update_service.rb +++ b/ee/app/services/external_status_checks/update_service.rb @@ -5,13 +5,12 @@ class UpdateService < BaseContainerService def execute return unauthorized_error_response unless current_user.can?(:admin_project, container) - current_rule = rule - if with_audit_logged(current_rule) { current_rule.update(resource_params) } - log_audit_event(current_rule) + if with_audit_logged(rule) { rule.update(resource_params) } + log_audit_event(rule) ServiceResponse.success(payload: { rule: rule }) else ServiceResponse.error(message: 'Failed to update rule', - payload: { errors: current_rule.errors.full_messages }, + payload: { errors: rule.errors.full_messages }, http_status: :unprocessable_entity) end end @@ -23,7 +22,7 @@ def resource_params end def rule - container.external_status_checks.find(params[:check_id]) + @rule ||= container.external_status_checks.find(params[:check_id]) end def unauthorized_error_response @@ -33,5 +32,21 @@ def unauthorized_error_response http_status: :unauthorized ) end + + def with_audit_logged(rule, &block) + audit_context = { + name: 'update_status_check', + author: current_user, + scope: rule.project, + target: rule + } + + ::Gitlab::Audit::Auditor.audit(audit_context, &block) + end + + def log_audit_event(rule) + ::EE::Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute + end + end end diff --git a/ee/lib/ee/audit/external_status_check_changes_auditor.rb b/ee/lib/ee/audit/external_status_check_changes_auditor.rb index 61094ae51e4627..bbb23c8bec9a8b 100644 --- a/ee/lib/ee/audit/external_status_check_changes_auditor.rb +++ b/ee/lib/ee/audit/external_status_check_changes_auditor.rb @@ -9,12 +9,8 @@ def initialize(current_user, external_status_check) end def execute - if model.destroyed? - audit_deletion - else - audit_changes(:name, as: 'name', entity: @project, model: model) - audit_changes(:external_url, as: 'external url', entity: @project, model: model) - end + audit_changes(:name, as: 'name', entity: @project, model: model) + audit_changes(:external_url, as: 'external url', entity: @project, model: model) end def attributes_from_auditable_model(column) @@ -23,18 +19,6 @@ def attributes_from_auditable_model(column) to: model.previous_changes[column].last } end - - private - - def audit_deletion - audit_context = { - author: @current_user, - scope: @project, - target: model, - message: "Removed status check" - } - ::Gitlab::Audit::Auditor.audit(audit_context) - end end end end diff --git a/ee/spec/services/external_status_checks/destroy_service_spec.rb b/ee/spec/services/external_status_checks/destroy_service_spec.rb index a03a2c30ad587a..ab530e679b1f2d 100644 --- a/ee/spec/services/external_status_checks/destroy_service_spec.rb +++ b/ee/spec/services/external_status_checks/destroy_service_spec.rb @@ -41,11 +41,40 @@ end end - context 'audit changes' do - it 'execute ExternalStatusCheckChangesAuditor' do - expect(::EE::Audit::ExternalStatusCheckChangesAuditor).to receive(:new).with(current_user, rule).and_call_original + describe 'audit events' do + context 'when licensed' do + before do + stub_licensed_features(audit_events: true) + end - subject + context 'when rule destroy operation succeeds', :request_store do + it 'logs an audit event' do + expect { subject }.to change { AuditEvent.count }.by(1) + end + end + + context 'when rule destroy operation fails' do + before do + allow(::MergeRequests::ExternalStatusCheck).to receive(:destroy).and_return(false) + end + + it 'does not log any audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end + context 'when not licensed' do + before do + stub_licensed_features( + admin_audit_log: false, + audit_events: false, + extended_audit_events: false + ) + end + + it 'does not log any audit event' do + expect { subject }.not_to change { AuditEvent.count } + end end end end -- GitLab From 013a3a833c89687283f9cb9c4c5809a7f258695d Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 14 Apr 2022 15:50:36 +0530 Subject: [PATCH 03/14] Refactor test cases and make shared context Add should audit check for merge skip --- app/services/base_container_service.rb | 1 - .../external_status_checks/update_service.rb | 1 - .../audit/project_setting_changes_auditor.rb | 8 +++++-- ...ernal_status_check_changes_auditor_spec.rb | 7 ------ .../project_setting_changes_auditor_spec.rb | 22 +++++++++---------- .../external_status_check_spec.rb | 7 ++++++ .../create_service_spec.rb | 13 +---------- .../destroy_service_spec.rb | 13 +---------- .../update_service_spec.rb | 14 +----------- ...audit_event_not_licensed_shared_context.rb | 15 +++++++++++++ 10 files changed, 42 insertions(+), 59 deletions(-) create mode 100644 ee/spec/support/shared_contexts/audit_event_not_licensed_shared_context.rb diff --git a/app/services/base_container_service.rb b/app/services/base_container_service.rb index 1bdbba04787c76..86df0236a7f093 100644 --- a/app/services/base_container_service.rb +++ b/app/services/base_container_service.rb @@ -30,5 +30,4 @@ def group_container? def namespace_container? container.is_a?(::Namespace) end - end diff --git a/ee/app/services/external_status_checks/update_service.rb b/ee/app/services/external_status_checks/update_service.rb index 0486a34370927c..db9ca90efda313 100644 --- a/ee/app/services/external_status_checks/update_service.rb +++ b/ee/app/services/external_status_checks/update_service.rb @@ -47,6 +47,5 @@ def with_audit_logged(rule, &block) def log_audit_event(rule) ::EE::Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute end - end end diff --git a/ee/lib/ee/audit/project_setting_changes_auditor.rb b/ee/lib/ee/audit/project_setting_changes_auditor.rb index c2dd6f12f77225..1e809614a835cf 100644 --- a/ee/lib/ee/audit/project_setting_changes_auditor.rb +++ b/ee/lib/ee/audit/project_setting_changes_auditor.rb @@ -12,8 +12,12 @@ def execute return if model.blank? audit_changes(:squash_option, as: 'squash_option', entity: @project, model: model) - audit_changes(:allow_merge_on_skipped_pipeline, as: 'allow_merge_on_skipped_pipeline', entity: @project, - model: model) + + if should_audit? :allow_merge_on_skipped_pipeline + audit_changes(:allow_merge_on_skipped_pipeline, as: 'allow_merge_on_skipped_pipeline', entity: @project, + model: model) + end + audit_changes(:merge_commit_template, as: 'merge_commit_template', entity: @project, model: model) audit_changes(:squash_commit_template, as: 'squash_commit_template', entity: @project, model: model) end diff --git a/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb index f11337dff9ae33..7acc593adf6fee 100644 --- a/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb @@ -29,13 +29,6 @@ expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:change]).to eq 'external url' end - - it 'creates an event when the status check is deleted' do - rule.destroy! - - expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq 'Removed status check' - end end end end diff --git a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb index 4d682a9b538f12..a3935429c77709 100644 --- a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb @@ -60,21 +60,21 @@ }) end end - it 'creates an event when the merge_requests_template change' do - project.project_setting.update!(merge_commit_template: 'merge commit template') + end - expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:change]).to eq 'merge_commit_template' - end + it 'creates an event when the merge_requests_template change' do + project.project_setting.update!(merge_commit_template: 'merge commit template') + + expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'merge_commit_template' + end - it 'creates an event when the squash_commit_template change' do - project.project_setting.update!(squash_commit_template: 'squash_commit_template') + it 'creates an event when the squash_commit_template change' do + project.project_setting.update!(squash_commit_template: 'squash_commit_template') - expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:change]).to eq 'squash_commit_template' - end + expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'squash_commit_template' end end end end - diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index 868aadea0e2b66..fcfff5a3ba3213 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -246,5 +246,12 @@ it_behaves_like 'audit event queue' end + + describe '#audit_creation external status check after :create' do + let(:action!) { check.destroy! } + let(:message) { 'Removed status check' } + + it_behaves_like 'audit event queue' + end end end diff --git a/ee/spec/services/external_status_checks/create_service_spec.rb b/ee/spec/services/external_status_checks/create_service_spec.rb index f37d21ff493c3b..11065337b871c8 100644 --- a/ee/spec/services/external_status_checks/create_service_spec.rb +++ b/ee/spec/services/external_status_checks/create_service_spec.rb @@ -89,18 +89,7 @@ end end end - context 'when not licensed' do - before do - stub_licensed_features( - admin_audit_log: false, - audit_events: false, - extended_audit_events: false - ) - end - it 'does not log any audit event' do - expect { subject }.not_to change { AuditEvent.count } - end - end + it_behaves_like 'does not create audit event when not licensed' end end diff --git a/ee/spec/services/external_status_checks/destroy_service_spec.rb b/ee/spec/services/external_status_checks/destroy_service_spec.rb index ab530e679b1f2d..87f6473020fe9c 100644 --- a/ee/spec/services/external_status_checks/destroy_service_spec.rb +++ b/ee/spec/services/external_status_checks/destroy_service_spec.rb @@ -63,18 +63,7 @@ end end end - context 'when not licensed' do - before do - stub_licensed_features( - admin_audit_log: false, - audit_events: false, - extended_audit_events: false - ) - end - it 'does not log any audit event' do - expect { subject }.not_to change { AuditEvent.count } - end - end + it_behaves_like 'does not create audit event when not licensed' end end diff --git a/ee/spec/services/external_status_checks/update_service_spec.rb b/ee/spec/services/external_status_checks/update_service_spec.rb index 0a300259538e4c..b8774dfc3e1c14 100644 --- a/ee/spec/services/external_status_checks/update_service_spec.rb +++ b/ee/spec/services/external_status_checks/update_service_spec.rb @@ -118,17 +118,5 @@ end end - context 'when not licensed' do - before do - stub_licensed_features( - admin_audit_log: false, - audit_events: false, - extended_audit_events: false - ) - end - - it 'does not log any audit event' do - expect { subject }.not_to change { AuditEvent.count } - end - end + it_behaves_like 'does not create audit event when not licensed' end diff --git a/ee/spec/support/shared_contexts/audit_event_not_licensed_shared_context.rb b/ee/spec/support/shared_contexts/audit_event_not_licensed_shared_context.rb new file mode 100644 index 00000000000000..7736fdca910acc --- /dev/null +++ b/ee/spec/support/shared_contexts/audit_event_not_licensed_shared_context.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +RSpec.shared_context 'does not create audit event when not licensed' do + before do + stub_licensed_features( + admin_audit_log: false, + audit_events: false, + extended_audit_events: false + ) + end + + it 'does not log any audit event' do + expect { subject }.not_to change { AuditEvent.count } + end +end -- GitLab From e6d4aa88589cec7043840ebcbdf2f76d7fb1a265 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Tue, 19 Apr 2022 14:15:59 +0530 Subject: [PATCH 04/14] Apply review feedback suggestions --- .../merge_requests/external_status_check.rb | 6 +-- .../external_status_checks/base_service.rb | 18 +++++++ .../external_status_checks/create_service.rb | 17 +------ .../external_status_checks/destroy_service.rb | 15 +----- .../external_status_checks/update_service.rb | 15 +----- .../external_status_check_changes_auditor.rb | 23 +++++++++ .../external_status_check_changes_auditor.rb | 24 --------- ...ernal_status_check_changes_auditor_spec.rb | 49 +++++++++++++++++++ ...ernal_status_check_changes_auditor_spec.rb | 34 ------------- .../project_setting_changes_auditor_spec.rb | 18 +++++-- .../external_status_check_spec.rb | 28 +++++------ .../create_service_spec.rb | 8 ++- .../update_service_spec.rb | 2 +- 13 files changed, 133 insertions(+), 124 deletions(-) create mode 100644 ee/app/services/external_status_checks/base_service.rb create mode 100644 ee/lib/audit/external_status_check_changes_auditor.rb delete mode 100644 ee/lib/ee/audit/external_status_check_changes_auditor.rb create mode 100644 ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb delete mode 100644 ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index 47892a4b7208da..e0cb158f88b4aa 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -47,7 +47,7 @@ def to_h end def audit_protected_branch_add(model) - message = "Added #{model.class.name} #{model.name} to #{self.name} status check" + message = "Added #{model.class.name.underscore.humanize.downcase} #{model.name} to #{self.name} status check" message += " and removed All Branches from status check" if protected_branches.count == 1 push_audit_event(message) end @@ -61,8 +61,8 @@ def audit_deletion end def audit_protected_branch_remove(model) - message = "Removed #{model.class.name} #{model.name} from #{self.name} status check" - message += " and added All Branches to status check" if protected_branches.blank? + message = "Removed #{model.class.name.underscore.humanize.downcase} #{model.name} from #{self.name} status check" + message = "Added all branches to #{self.name} status check" if protected_branches.blank? push_audit_event(message) end diff --git a/ee/app/services/external_status_checks/base_service.rb b/ee/app/services/external_status_checks/base_service.rb new file mode 100644 index 00000000000000..b8a0bfd39990ce --- /dev/null +++ b/ee/app/services/external_status_checks/base_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module ExternalStatusChecks + class BaseService < BaseContainerService + private + + def with_audit_logged(rule, name, &block) + audit_context = { + name: name, + author: current_user, + scope: rule.project, + target: rule + } + + ::Gitlab::Audit::Auditor.audit(audit_context, &block) + end + end +end diff --git a/ee/app/services/external_status_checks/create_service.rb b/ee/app/services/external_status_checks/create_service.rb index bc8d897246bf6e..caf04c1a3e3bf4 100644 --- a/ee/app/services/external_status_checks/create_service.rb +++ b/ee/app/services/external_status_checks/create_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ExternalStatusChecks - class CreateService < BaseContainerService + class CreateService < BaseService def execute return ServiceResponse.error(message: 'Failed to create rule', payload: { errors: ['Not allowed'] }, http_status: :unauthorized) unless current_user.can?(:admin_project, container) @@ -10,24 +10,11 @@ def execute external_url: params[:external_url], protected_branch_ids: params[:protected_branch_ids]) - if with_audit_logged(rule) { rule.save } + if with_audit_logged(rule, 'create_status_check') { rule.save } ServiceResponse.success(payload: { rule: rule }) else ServiceResponse.error(message: 'Failed to create rule', payload: { errors: rule.errors.full_messages }, http_status: :unprocessable_entity) end end - - private - - def with_audit_logged(rule, &block) - audit_context = { - name: 'create_status_check', - author: current_user, - scope: rule.project, - target: rule - } - - ::Gitlab::Audit::Auditor.audit(audit_context, &block) - end end end diff --git a/ee/app/services/external_status_checks/destroy_service.rb b/ee/app/services/external_status_checks/destroy_service.rb index 64beeeef6958bf..aae20c047efef9 100644 --- a/ee/app/services/external_status_checks/destroy_service.rb +++ b/ee/app/services/external_status_checks/destroy_service.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true module ExternalStatusChecks - class DestroyService < BaseContainerService + class DestroyService < BaseService def execute(rule) return unauthorized_error_response unless current_user.can?(:admin_project, container) - if with_audit_logged(rule) { rule.destroy } + if with_audit_logged(rule, 'delete_status_check') { rule.destroy } ServiceResponse.success else ServiceResponse.error(message: 'Failed to destroy rule', @@ -23,16 +23,5 @@ def unauthorized_error_response http_status: :unauthorized ) end - - def with_audit_logged(rule, &block) - audit_context = { - name: 'delete_status_check', - author: current_user, - scope: rule.project, - target: rule - } - - ::Gitlab::Audit::Auditor.audit(audit_context, &block) - end end end diff --git a/ee/app/services/external_status_checks/update_service.rb b/ee/app/services/external_status_checks/update_service.rb index db9ca90efda313..926366bdfc884b 100644 --- a/ee/app/services/external_status_checks/update_service.rb +++ b/ee/app/services/external_status_checks/update_service.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true module ExternalStatusChecks - class UpdateService < BaseContainerService + class UpdateService < BaseService def execute return unauthorized_error_response unless current_user.can?(:admin_project, container) - if with_audit_logged(rule) { rule.update(resource_params) } + if with_audit_logged(rule, 'update_status_check') { rule.update(resource_params) } log_audit_event(rule) ServiceResponse.success(payload: { rule: rule }) else @@ -33,17 +33,6 @@ def unauthorized_error_response ) end - def with_audit_logged(rule, &block) - audit_context = { - name: 'update_status_check', - author: current_user, - scope: rule.project, - target: rule - } - - ::Gitlab::Audit::Auditor.audit(audit_context, &block) - end - def log_audit_event(rule) ::EE::Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute end diff --git a/ee/lib/audit/external_status_check_changes_auditor.rb b/ee/lib/audit/external_status_check_changes_auditor.rb new file mode 100644 index 00000000000000..a5d00fd42e9bbd --- /dev/null +++ b/ee/lib/audit/external_status_check_changes_auditor.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Audit + class ExternalStatusCheckChangesAuditor < ::EE::Audit::BaseChangesAuditor + def initialize(current_user, external_status_check) + @project = external_status_check.project + + super + end + + def execute + audit_changes(:name, as: 'name', entity: @project, model: model) + audit_changes(:external_url, as: 'external url', entity: @project, model: model) + end + + def attributes_from_auditable_model(column) + { + from: model.previous_changes[column].first, + to: model.previous_changes[column].last + } + end + end +end diff --git a/ee/lib/ee/audit/external_status_check_changes_auditor.rb b/ee/lib/ee/audit/external_status_check_changes_auditor.rb deleted file mode 100644 index bbb23c8bec9a8b..00000000000000 --- a/ee/lib/ee/audit/external_status_check_changes_auditor.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true -module EE - module Audit - class ExternalStatusCheckChangesAuditor < BaseChangesAuditor - def initialize(current_user, external_status_check) - @project = external_status_check.project - - super - end - - def execute - audit_changes(:name, as: 'name', entity: @project, model: model) - audit_changes(:external_url, as: 'external url', entity: @project, model: model) - end - - def attributes_from_auditable_model(column) - { - from: model.previous_changes[column].first, - to: model.previous_changes[column].last - } - end - end - end -end diff --git a/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb b/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb new file mode 100644 index 00000000000000..d7371c6ab158c4 --- /dev/null +++ b/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::ExternalStatusCheckChangesAuditor do + describe 'auditing external status check changes' do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :pages_enabled, visibility_level: 0) } + let_it_be(:external_status_check) do + create(:external_status_check, + name: 'QA', + external_url: 'http://examplev1.com', + project: project) + end + + let_it_be(:external_status_check_changes_auditor) { described_class.new(user, external_status_check) } + + before do + stub_licensed_features(extended_audit_events: true) + end + + let(:subject) { described_class.new(user, project.external_status_checks) } + + context 'when audit change happens' do + it 'creates an event when the name changes' do + external_status_check.update!(name: 'QAv2') + + expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:change]).to eq 'name' + expect(AuditEvent.last.details).to include({ + change: 'name', + from: 'QA', + to: 'QAv2' + }) + end + + it 'creates an event when the external url changes' do + external_status_check.update!(external_url: 'http://examplev2.com') + + expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details).to include({ + change: 'external url', + from: 'http://examplev1.com', + to: 'http://examplev2.com' + }) + end + end + end +end diff --git a/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb deleted file mode 100644 index 7acc593adf6fee..00000000000000 --- a/ee/spec/lib/ee/audit/external_status_check_changes_auditor_spec.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe EE::Audit::ExternalStatusCheckChangesAuditor do - describe 'auditing external status check changes' do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :pages_enabled, visibility_level: 0) } - let_it_be(:rule) { create( :external_status_check, project: project) } - let_it_be(:external_status_check_changes_auditor) { described_class.new(user, rule) } - - before do - stub_licensed_features(extended_audit_events: true) - end - - let(:subject) { described_class.new(user, project.external_status_checks) } - - context 'when audit change happens' do - it 'creates an event when the name changes' do - rule.update!(name: 'name') - - expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:change]).to eq 'name' - end - - it 'creates an event when the external url changes' do - rule.update!(external_url: 'http://example.com') - - expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:change]).to eq 'external url' - end - end - end -end diff --git a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb index a3935429c77709..03c79b7ed3f070 100644 --- a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb @@ -40,7 +40,7 @@ context 'when allow_merge_on_skipped_pipeline is changed' do where(:prev_value, :new_value) do - true | false + true | false false | true end @@ -63,17 +63,25 @@ end it 'creates an event when the merge_requests_template change' do - project.project_setting.update!(merge_commit_template: 'merge commit template') + project.project_setting.update!(merge_commit_template: 'I am new merge commit template') expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:change]).to eq 'merge_commit_template' + expect(AuditEvent.last.details).to include({ + change: 'merge_commit_template', + from: nil, + to: 'I am new merge commit template' + }) end it 'creates an event when the squash_commit_template change' do - project.project_setting.update!(squash_commit_template: 'squash_commit_template') + project.project_setting.update!(squash_commit_template: 'I am new squash commit template') expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:change]).to eq 'squash_commit_template' + expect(AuditEvent.last.details).to include({ + change: 'squash_commit_template', + from: nil, + to: 'I am new squash commit template' + }) end end end diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index fcfff5a3ba3213..f4484c780b954c 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -192,25 +192,25 @@ let_it_be(:project) { create(:project) } let_it_be(:master_branch) { create(:protected_branch, project: project, name: 'master') } let_it_be(:main_branch) { create(:protected_branch, project: project, name: 'main') } - let_it_be(:check, reload: true) do + let_it_be(:external_status_check, reload: true) do create(:external_status_check, project: project, name: 'QA', protected_branches: []) end describe '#audit_add branches after :add' do context 'when branch is added from zero branches' do - let(:action!) { check.update!(protected_branches: [main_branch]) } - let(:message) { 'Added ProtectedBranch main to QA status check and removed All Branches from status check' } + let(:action!) { external_status_check.update!(protected_branches: [main_branch]) } + let(:message) { 'Added protected branch main to QA status check and removed All Branches from status check' } it_behaves_like 'audit event queue' end - context 'when branch is another branch is added' do + context 'when another branch is added' do before do - check.update!(protected_branches: [main_branch]) + external_status_check.update!(protected_branches: [main_branch]) end - let(:action!) { check.update!(protected_branches: [main_branch, master_branch]) } - let(:message) { 'Added ProtectedBranch master to QA status check' } + let(:action!) { external_status_check.update!(protected_branches: [main_branch, master_branch]) } + let(:message) { 'Added protected branch master to QA status check' } it_behaves_like 'audit event queue' end @@ -219,22 +219,22 @@ describe '#audit_remove branches after :remove' do context 'when all the branches are removed' do before do - check.update!(protected_branches: [main_branch]) + external_status_check.update!(protected_branches: [main_branch]) end - let(:action!) { check.update!(protected_branches: []) } - let(:message) { 'Removed ProtectedBranch main from QA status check and added All Branches to status check' } + let(:action!) { external_status_check.update!(protected_branches: []) } + let(:message) { 'Added all branches to QA status check' } it_behaves_like 'audit event queue' end context 'when a branch is removed' do before do - check.update!(protected_branches: [main_branch, master_branch]) + external_status_check.update!(protected_branches: [main_branch, master_branch]) end - let(:action!) { check.update!(protected_branches: [master_branch]) } - let(:message) { 'Removed ProtectedBranch main from QA status check' } + let(:action!) { external_status_check.update!(protected_branches: [master_branch]) } + let(:message) { 'Removed protected branch main from QA status check' } it_behaves_like 'audit event queue' end @@ -248,7 +248,7 @@ end describe '#audit_creation external status check after :create' do - let(:action!) { check.destroy! } + let(:action!) { external_status_check.destroy! } let(:message) { 'Removed status check' } it_behaves_like 'audit event queue' diff --git a/ee/spec/services/external_status_checks/create_service_spec.rb b/ee/spec/services/external_status_checks/create_service_spec.rb index 11065337b871c8..2bb4c754772af8 100644 --- a/ee/spec/services/external_status_checks/create_service_spec.rb +++ b/ee/spec/services/external_status_checks/create_service_spec.rb @@ -73,13 +73,17 @@ stub_licensed_features(audit_events: true) end - context 'when rule save operation succeeds', :request_store do + context 'when external status check save operation succeeds', :request_store do it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details).to include({ + target_type: 'MergeRequests::ExternalStatusCheck', + custom_message: 'Added status check' + }) end end - context 'when rule save operation fails' do + context 'when external status check save operation fails' do before do allow(::MergeRequests::ExternalStatusCheck).to receive(:save).and_return(false) end diff --git a/ee/spec/services/external_status_checks/update_service_spec.rb b/ee/spec/services/external_status_checks/update_service_spec.rb index b8774dfc3e1c14..d9732965330397 100644 --- a/ee/spec/services/external_status_checks/update_service_spec.rb +++ b/ee/spec/services/external_status_checks/update_service_spec.rb @@ -111,7 +111,7 @@ end end - it 'execute ExternalStatusCheckChangesAuditor' do + it 'executes ExternalStatusCheckChangesAuditor' do expect(::EE::Audit::ExternalStatusCheckChangesAuditor).to receive(:new).with(current_user, check).and_call_original subject -- GitLab From af837b3d9a5b1ac8d58ce79f47a14148349ada94 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Tue, 19 Apr 2022 14:39:31 +0530 Subject: [PATCH 05/14] Refactor update service spec --- .../external_status_checks/update_service.rb | 2 +- .../update_service_spec.rb | 26 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ee/app/services/external_status_checks/update_service.rb b/ee/app/services/external_status_checks/update_service.rb index 926366bdfc884b..68c8ab2f97de5a 100644 --- a/ee/app/services/external_status_checks/update_service.rb +++ b/ee/app/services/external_status_checks/update_service.rb @@ -34,7 +34,7 @@ def unauthorized_error_response end def log_audit_event(rule) - ::EE::Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute + Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute end end end diff --git a/ee/spec/services/external_status_checks/update_service_spec.rb b/ee/spec/services/external_status_checks/update_service_spec.rb index d9732965330397..823a7b16566755 100644 --- a/ee/spec/services/external_status_checks/update_service_spec.rb +++ b/ee/spec/services/external_status_checks/update_service_spec.rb @@ -56,28 +56,28 @@ end let_it_be(:master_branch) { create(:protected_branch, project: project, name: 'master') } let_it_be(:main_branch) {create(:protected_branch, project: project, name: 'main')} - let_it_be(:check, reload: true) { create(:external_status_check, name: 'QA', project: project, protected_branches: []) } + let_it_be(:external_status_check, reload: true) { create(:external_status_check, name: 'QA', project: project, protected_branches: []) } context 'when a branch is added', :request_store do context 'when a new branch is added' do - let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [main_branch.id] } } + let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [main_branch.id] } } it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq "Added ProtectedBranch main to QA status check and removed All Branches from status check" + expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch main to QA status check and removed All Branches from status check" end end context 'when another branch is added' do before do - check.update!(protected_branches: [main_branch]) + external_status_check.update!(protected_branches: [main_branch]) end - let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [main_branch.id, master_branch.id] } } + let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [main_branch.id, master_branch.id] } } it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq "Added ProtectedBranch master to QA status check" + expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch master to QA status check" end end end @@ -85,34 +85,34 @@ context 'when a branch is removed', :request_store do context 'when the only branch is removed' do before do - check.update!(protected_branches: [main_branch]) + external_status_check.update!(protected_branches: [main_branch]) end - let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [] } } + let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [] } } it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq "Removed ProtectedBranch main from QA status check and added All Branches to status check" + expect(AuditEvent.last.details[:custom_message]).to eq "Added all branches to QA status check" end end context 'when a branch is removed' do before do - check.update!(protected_branches: [main_branch, master_branch]) + external_status_check.update!(protected_branches: [main_branch, master_branch]) end - let_it_be(:params) {{ id: project.id, check_id: check.id, protected_branch_ids: [main_branch.id] } } + let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [main_branch.id] } } it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq "Removed ProtectedBranch master from QA status check" + expect(AuditEvent.last.details[:custom_message]).to eq "Removed protected branch master from QA status check" end end end end it 'executes ExternalStatusCheckChangesAuditor' do - expect(::EE::Audit::ExternalStatusCheckChangesAuditor).to receive(:new).with(current_user, check).and_call_original + expect(Audit::ExternalStatusCheckChangesAuditor).to receive(:new).with(current_user, check).and_call_original subject end -- GitLab From 56ac60fc396b2c0b1c3277850a7e5f73b6469bc7 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Tue, 19 Apr 2022 19:34:57 +0530 Subject: [PATCH 06/14] Add details assertions in specs --- .../audit/external_status_check_changes_auditor_spec.rb | 2 +- ee/spec/lib/ee/audit/project_changes_auditor_spec.rb | 7 ++++++- .../external_status_checks/destroy_service_spec.rb | 4 ++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb b/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb index d7371c6ab158c4..9b46c44a23b363 100644 --- a/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Audit::ExternalStatusCheckChangesAuditor do describe 'auditing external status check changes' do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :pages_enabled, visibility_level: 0) } + let_it_be(:project) { create(:project) } let_it_be(:external_status_check) do create(:external_status_check, name: 'QA', diff --git a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb index 6503074fe4b5e1..920e81f47fd491 100644 --- a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb @@ -84,10 +84,15 @@ end it 'creates an event when the merge requests template changes' do - project.update!(merge_requests_template: 'merge request template') + project.update!(merge_requests_template: 'I am a merge request template') expect { foo_instance.execute }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:change]).to eq 'merge_requests_template' + expect(AuditEvent.last.details).to include({ + change: 'merge_requests_template', + from: nil, + to: 'I am a merge request template' + }) end it 'creates an event when the merge requests author approval changes' do diff --git a/ee/spec/services/external_status_checks/destroy_service_spec.rb b/ee/spec/services/external_status_checks/destroy_service_spec.rb index 87f6473020fe9c..2a7e6b2a06183f 100644 --- a/ee/spec/services/external_status_checks/destroy_service_spec.rb +++ b/ee/spec/services/external_status_checks/destroy_service_spec.rb @@ -50,6 +50,10 @@ context 'when rule destroy operation succeeds', :request_store do it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details).to include({ + target_type: 'MergeRequests::ExternalStatusCheck', + custom_message: 'Removed status check' + }) end end -- GitLab From 1ebe5c195c6b0964866663a195aebe52fa181267 Mon Sep 17 00:00:00 2001 From: Huzaifa Iftikhar Date: Wed, 20 Apr 2022 08:46:17 +0000 Subject: [PATCH 07/14] Apply 2 suggestion(s) to 2 file(s) --- ee/app/models/merge_requests/external_status_check.rb | 7 +++++-- .../audit/external_status_check_changes_auditor_spec.rb | 1 - 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index e0cb158f88b4aa..b61fc8f1540f10 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -61,8 +61,11 @@ def audit_deletion end def audit_protected_branch_remove(model) - message = "Removed #{model.class.name.underscore.humanize.downcase} #{model.name} from #{self.name} status check" - message = "Added all branches to #{self.name} status check" if protected_branches.blank? + if protected_branches.blank? + message = "Added all branches to #{self.name} status check" + else + message = "Removed #{model.class.name.underscore.humanize.downcase} #{model.name} from #{self.name} status check" + end push_audit_event(message) end diff --git a/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb b/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb index 9b46c44a23b363..f3521475e7b8c1 100644 --- a/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb @@ -26,7 +26,6 @@ external_status_check.update!(name: 'QAv2') expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:change]).to eq 'name' expect(AuditEvent.last.details).to include({ change: 'name', from: 'QA', -- GitLab From 4dbbe8c6187f6249f81881db567dec1cf764c11f Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 20 Apr 2022 18:28:19 +0530 Subject: [PATCH 08/14] Rubocop lint fix --- .../models/merge_requests/external_status_check.rb | 13 +++++++------ .../merge_requests/external_status_check_spec.rb | 2 +- .../external_status_checks/update_service_spec.rb | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index b61fc8f1540f10..a34fdc5c17f685 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -48,7 +48,7 @@ def to_h def audit_protected_branch_add(model) message = "Added #{model.class.name.underscore.humanize.downcase} #{model.name} to #{self.name} status check" - message += " and removed All Branches from status check" if protected_branches.count == 1 + message += " and removed all branches from status check" if protected_branches.count == 1 push_audit_event(message) end @@ -61,11 +61,12 @@ def audit_deletion end def audit_protected_branch_remove(model) - if protected_branches.blank? - message = "Added all branches to #{self.name} status check" - else - message = "Removed #{model.class.name.underscore.humanize.downcase} #{model.name} from #{self.name} status check" - end + message = if protected_branches.blank? + "Added all branches to #{self.name} status check" + else + "Removed #{model.class.name.underscore.humanize.downcase} #{model.name} from #{self.name} status check" + end + push_audit_event(message) end diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index f4484c780b954c..93d0d5d22cda4a 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -199,7 +199,7 @@ describe '#audit_add branches after :add' do context 'when branch is added from zero branches' do let(:action!) { external_status_check.update!(protected_branches: [main_branch]) } - let(:message) { 'Added protected branch main to QA status check and removed All Branches from status check' } + let(:message) { 'Added protected branch main to QA status check and removed all branches from status check' } it_behaves_like 'audit event queue' end diff --git a/ee/spec/services/external_status_checks/update_service_spec.rb b/ee/spec/services/external_status_checks/update_service_spec.rb index 823a7b16566755..627c57a1133cb1 100644 --- a/ee/spec/services/external_status_checks/update_service_spec.rb +++ b/ee/spec/services/external_status_checks/update_service_spec.rb @@ -64,7 +64,7 @@ it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch main to QA status check and removed All Branches from status check" + expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch main to QA status check and removed all branches from status check" end end -- GitLab From a35012594dbfca6f9d4c898b50757575c04f4c30 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 21 Apr 2022 13:19:01 +0530 Subject: [PATCH 09/14] Add name in status check audit --- ee/app/models/merge_requests/external_status_check.rb | 4 ++-- ee/spec/models/merge_requests/external_status_check_spec.rb | 6 +++--- .../services/external_status_checks/create_service_spec.rb | 2 +- .../services/external_status_checks/destroy_service_spec.rb | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index a34fdc5c17f685..9a817379c63e17 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -53,11 +53,11 @@ def audit_protected_branch_add(model) end def audit_creation - push_audit_event("Added status check") + push_audit_event("Added #{self.name} status check") end def audit_deletion - push_audit_event("Removed status check") + push_audit_event("Removed #{self.name} status check") end def audit_protected_branch_remove(model) diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index 93d0d5d22cda4a..de552e98cc4af8 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -241,15 +241,15 @@ end describe '#audit_creation external status check after :create' do - let(:action!) { create(:external_status_check) } - let(:message) { 'Added status check' } + let(:action!) { create(:external_status_check, name: 'QA') } + let(:message) { 'Added QA status check' } it_behaves_like 'audit event queue' end describe '#audit_creation external status check after :create' do let(:action!) { external_status_check.destroy! } - let(:message) { 'Removed status check' } + let(:message) { 'Removed QA status check' } it_behaves_like 'audit event queue' end diff --git a/ee/spec/services/external_status_checks/create_service_spec.rb b/ee/spec/services/external_status_checks/create_service_spec.rb index 2bb4c754772af8..44c3785cb2bc4b 100644 --- a/ee/spec/services/external_status_checks/create_service_spec.rb +++ b/ee/spec/services/external_status_checks/create_service_spec.rb @@ -78,7 +78,7 @@ expect { subject }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details).to include({ target_type: 'MergeRequests::ExternalStatusCheck', - custom_message: 'Added status check' + custom_message: 'Added Test status check' }) end end diff --git a/ee/spec/services/external_status_checks/destroy_service_spec.rb b/ee/spec/services/external_status_checks/destroy_service_spec.rb index 2a7e6b2a06183f..29ca1d1ccfa1b7 100644 --- a/ee/spec/services/external_status_checks/destroy_service_spec.rb +++ b/ee/spec/services/external_status_checks/destroy_service_spec.rb @@ -4,7 +4,7 @@ RSpec.describe ExternalStatusChecks::DestroyService do let_it_be(:project) { create(:project) } - let_it_be(:rule) { create(:external_status_check, project: project) } + let_it_be(:rule) { create(:external_status_check, name: 'QA', project: project) } let(:current_user) { project.first_owner } @@ -52,7 +52,7 @@ expect { subject }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details).to include({ target_type: 'MergeRequests::ExternalStatusCheck', - custom_message: 'Removed status check' + custom_message: 'Removed QA status check' }) end end -- GitLab From 320d9addd1e2f4568e6676c56eb3fdfcf535dc18 Mon Sep 17 00:00:00 2001 From: Aleksei Lipniagov Date: Thu, 21 Apr 2022 12:07:10 +0000 Subject: [PATCH 10/14] Apply 1 suggestion(s) to 1 file(s) --- ee/app/models/merge_requests/external_status_check.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index 9a817379c63e17..a9cf56b74e5d87 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -61,7 +61,7 @@ def audit_deletion end def audit_protected_branch_remove(model) - message = if protected_branches.blank? + message = if protected_branches.empty? "Added all branches to #{self.name} status check" else "Removed #{model.class.name.underscore.humanize.downcase} #{model.name} from #{self.name} status check" -- GitLab From df0f62075fa780933f4153adca7ace5e2af40ee7 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Mon, 25 Apr 2022 13:34:07 +0530 Subject: [PATCH 11/14] Apply review feedback suggestions --- app/models/protected_branch.rb | 4 ++++ ee/app/models/merge_requests/external_status_check.rb | 4 ++-- ee/app/services/external_status_checks/update_service.rb | 4 ++-- spec/models/protected_branch_spec.rb | 6 ++++++ 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 96002c8668a98f..e236ed69973334 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -68,6 +68,10 @@ def self.by_name(query) def allow_multiple?(type) type == :push end + + def self.underscore_humanize_name + name.underscore.humanize.downcase + end end ProtectedBranch.prepend_mod_with('ProtectedBranch') diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index a9cf56b74e5d87..a73559c49984eb 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -47,7 +47,7 @@ def to_h end def audit_protected_branch_add(model) - message = "Added #{model.class.name.underscore.humanize.downcase} #{model.name} to #{self.name} status check" + message = "Added #{model.class.underscore_humanize_name} #{model.name} to #{self.name} status check" message += " and removed all branches from status check" if protected_branches.count == 1 push_audit_event(message) end @@ -64,7 +64,7 @@ def audit_protected_branch_remove(model) message = if protected_branches.empty? "Added all branches to #{self.name} status check" else - "Removed #{model.class.name.underscore.humanize.downcase} #{model.name} from #{self.name} status check" + "Removed #{model.class.underscore_humanize_name} #{model.name} from #{self.name} status check" end push_audit_event(message) diff --git a/ee/app/services/external_status_checks/update_service.rb b/ee/app/services/external_status_checks/update_service.rb index 68c8ab2f97de5a..b645ef9401f82a 100644 --- a/ee/app/services/external_status_checks/update_service.rb +++ b/ee/app/services/external_status_checks/update_service.rb @@ -6,7 +6,7 @@ def execute return unauthorized_error_response unless current_user.can?(:admin_project, container) if with_audit_logged(rule, 'update_status_check') { rule.update(resource_params) } - log_audit_event(rule) + log_audit_event ServiceResponse.success(payload: { rule: rule }) else ServiceResponse.error(message: 'Failed to update rule', @@ -33,7 +33,7 @@ def unauthorized_error_response ) end - def log_audit_event(rule) + def log_audit_event Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute end end diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index f7c723cd1343b4..94c6779b249556 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -324,4 +324,10 @@ .to match_array([branch_id]) end end + + describe '.underscore_humanize_name' do + it 'returns downcase humanized full name' do + expect(described_class.underscore_humanize_name).to eq 'protected branch' + end + end end -- GitLab From 0528ba57d7ba598991bb593b9b627dd4c0fb94ea Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Mon, 25 Apr 2022 17:33:06 +0530 Subject: [PATCH 12/14] Make audit event message more user friendly --- app/models/protected_branch.rb | 2 +- ee/app/models/merge_requests/external_status_check.rb | 6 +++--- ee/spec/models/merge_requests/external_status_check_spec.rb | 2 +- .../services/external_status_checks/update_service_spec.rb | 2 +- spec/models/protected_branch_spec.rb | 6 +++--- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index e236ed69973334..77038d52efe074 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -69,7 +69,7 @@ def allow_multiple?(type) type == :push end - def self.underscore_humanize_name + def self.downcase_humanized_name name.underscore.humanize.downcase end end diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index a73559c49984eb..cc6b678c08b754 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -47,8 +47,8 @@ def to_h end def audit_protected_branch_add(model) - message = "Added #{model.class.underscore_humanize_name} #{model.name} to #{self.name} status check" - message += " and removed all branches from status check" if protected_branches.count == 1 + message = "Added #{model.class.downcase_humanized_name} #{model.name} to #{self.name} status check" + message += " and removed all other branches from status check" if protected_branches.count == 1 push_audit_event(message) end @@ -64,7 +64,7 @@ def audit_protected_branch_remove(model) message = if protected_branches.empty? "Added all branches to #{self.name} status check" else - "Removed #{model.class.underscore_humanize_name} #{model.name} from #{self.name} status check" + "Removed #{model.class.downcase_humanized_name} #{model.name} from #{self.name} status check" end push_audit_event(message) diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index de552e98cc4af8..c7b3330ffbfa36 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -199,7 +199,7 @@ describe '#audit_add branches after :add' do context 'when branch is added from zero branches' do let(:action!) { external_status_check.update!(protected_branches: [main_branch]) } - let(:message) { 'Added protected branch main to QA status check and removed all branches from status check' } + let(:message) { 'Added protected branch main to QA status check and removed all other branches from status check' } it_behaves_like 'audit event queue' end diff --git a/ee/spec/services/external_status_checks/update_service_spec.rb b/ee/spec/services/external_status_checks/update_service_spec.rb index 627c57a1133cb1..e4a96bc4d675d7 100644 --- a/ee/spec/services/external_status_checks/update_service_spec.rb +++ b/ee/spec/services/external_status_checks/update_service_spec.rb @@ -64,7 +64,7 @@ it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch main to QA status check and removed all branches from status check" + expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch main to QA status check and removed all other branches from status check" end end diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 94c6779b249556..366de809bedb88 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -325,9 +325,9 @@ end end - describe '.underscore_humanize_name' do - it 'returns downcase humanized full name' do - expect(described_class.underscore_humanize_name).to eq 'protected branch' + describe '.downcase_humanized_name' do + it 'returns downcase humanized name' do + expect(described_class.downcase_humanized_name).to eq 'protected branch' end end end -- GitLab From b7ceddaf19834472664d4a0372418f3a344e3f1c Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Tue, 26 Apr 2022 00:36:37 +0530 Subject: [PATCH 13/14] Humanize squash option audit and status check branch creation --- app/models/project_setting.rb | 9 ++++ .../merge_requests/external_status_check.rb | 15 +++++- .../audit/project_setting_changes_auditor.rb | 19 +++++++- .../project_setting_changes_auditor_spec.rb | 46 ++++++------------- .../external_status_check_spec.rb | 29 ++++++++++-- .../create_service_spec.rb | 5 +- locale/gitlab.pot | 3 ++ spec/models/project_setting_spec.rb | 18 ++++++++ 8 files changed, 102 insertions(+), 42 deletions(-) diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index 6cd6eee261652e..64311fde6ac6c1 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -36,6 +36,15 @@ def target_platforms=(val) super(val&.map(&:to_s)&.sort) end + def human_squash_option + case squash_option + when 'never' then 'Do not allow' + when 'always' then'Require' + when 'default_on' then'Encourage' + when 'default_off' then'Allow' + end + end + private def validates_mr_default_target_self diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index cc6b678c08b754..4e37b39f2142ed 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -17,7 +17,7 @@ class ExternalStatusCheck < ApplicationRecord belongs_to :project has_and_belongs_to_many :protected_branches, - after_add: :audit_protected_branch_add, after_remove: :audit_protected_branch_remove + after_add: :audit_protected_branch_add, after_remove: :audit_protected_branch_remove after_create_commit :audit_creation after_destroy_commit :audit_deletion validates :external_url, presence: true, uniqueness: { scope: :project_id }, addressable_url: true @@ -53,7 +53,14 @@ def audit_protected_branch_add(model) end def audit_creation - push_audit_event("Added #{self.name} status check") + message = "Added #{self.name} status check" + message += if protected_branches.empty? + " with all branches" + else + " with protected branch(es) #{self.protected_branches_names}" + end + + push_audit_event(message) end def audit_deletion @@ -72,6 +79,10 @@ def audit_protected_branch_remove(model) private + def protected_branches_names + self.protected_branches.pluck(:name).join(', ') + end + def payload_data(merge_request_hook_data) merge_request_hook_data.merge(external_approval_rule: self.to_h) end diff --git a/ee/lib/ee/audit/project_setting_changes_auditor.rb b/ee/lib/ee/audit/project_setting_changes_auditor.rb index 1e809614a835cf..2e6d34d6620d87 100644 --- a/ee/lib/ee/audit/project_setting_changes_auditor.rb +++ b/ee/lib/ee/audit/project_setting_changes_auditor.rb @@ -11,13 +11,12 @@ def initialize(current_user, project_setting, project) def execute return if model.blank? - audit_changes(:squash_option, as: 'squash_option', entity: @project, model: model) - if should_audit? :allow_merge_on_skipped_pipeline audit_changes(:allow_merge_on_skipped_pipeline, as: 'allow_merge_on_skipped_pipeline', entity: @project, model: model) end + audit_squash_option audit_changes(:merge_commit_template, as: 'merge_commit_template', entity: @project, model: model) audit_changes(:squash_commit_template, as: 'squash_commit_template', entity: @project, model: model) end @@ -29,6 +28,22 @@ def attributes_from_auditable_model(column) target_details: @project.full_path } end + + private + + def audit_squash_option + return unless audit_required? :squash_option + + squash_option_message = _("Changed squash option to %{squash_option}") % + { squash_option: model.human_squash_option } + audit_context = { + author: @current_user, + scope: @project, + target: @project, + message: squash_option_message + } + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end end diff --git a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb index 03c79b7ed3f070..ac3b63f105ed04 100644 --- a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb @@ -22,17 +22,21 @@ project.project_setting.update!(squash_option: prev_value) end - next if new_value == prev_value + if new_value != prev_value + it 'creates an audit event' do + project.project_setting.update!(squash_option: new_value) - it 'creates an audit event' do - project.project_setting.update!(squash_option: new_value) - - expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details).to include({ - change: 'squash_option', - from: prev_value, - to: new_value - }) + expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details).to include( + { + custom_message: "Changed squash option to #{project.project_setting.human_squash_option}" + }) + end + else + it 'does not create audit event' do + project.project_setting.update!(squash_option: new_value) + expect { project_setting_changes_auditor.execute }.to not_change { AuditEvent.count } + end end end end @@ -61,28 +65,6 @@ end end end - - it 'creates an event when the merge_requests_template change' do - project.project_setting.update!(merge_commit_template: 'I am new merge commit template') - - expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details).to include({ - change: 'merge_commit_template', - from: nil, - to: 'I am new merge commit template' - }) - end - - it 'creates an event when the squash_commit_template change' do - project.project_setting.update!(squash_commit_template: 'I am new squash commit template') - - expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details).to include({ - change: 'squash_commit_template', - from: nil, - to: 'I am new squash commit template' - }) - end end end end diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index c7b3330ffbfa36..427baaf8b39247 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -241,10 +241,33 @@ end describe '#audit_creation external status check after :create' do - let(:action!) { create(:external_status_check, name: 'QA') } - let(:message) { 'Added QA status check' } + context 'when protected branches are added' do + let_it_be(:external_status_check) do + described_class.new(name: 'QAv2', + project: project, + external_url: 'https://www.example.com', + protected_branch_ids: [main_branch.id, master_branch.id]) + end - it_behaves_like 'audit event queue' + let(:action!) { external_status_check.save! } + let(:message) { 'Added QAv2 status check with protected branch(es) main, master' } + + it_behaves_like 'audit event queue' + end + + context 'when all branches are added' do + let_it_be(:external_status_check) do + described_class.new(name: 'QAv2', + project: project, + external_url: 'https://www.example.com', + protected_branch_ids: []) + end + + let(:action!) { external_status_check.save! } + let(:message) { 'Added QAv2 status check with all branches' } + + it_behaves_like 'audit event queue' + end end describe '#audit_creation external status check after :create' do diff --git a/ee/spec/services/external_status_checks/create_service_spec.rb b/ee/spec/services/external_status_checks/create_service_spec.rb index 44c3785cb2bc4b..fda0308d890d05 100644 --- a/ee/spec/services/external_status_checks/create_service_spec.rb +++ b/ee/spec/services/external_status_checks/create_service_spec.rb @@ -77,9 +77,8 @@ it 'logs an audit event' do expect { subject }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details).to include({ - target_type: 'MergeRequests::ExternalStatusCheck', - custom_message: 'Added Test status check' - }) + custom_message: "Added Test status check with protected branch(es) #{protected_branch.name}" + }) end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 028e5399373fd3..5160246afb2f46 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7034,6 +7034,9 @@ msgstr "" msgid "Changed reviewer(s)." msgstr "" +msgid "Changed squash option to %{squash_option}" +msgstr "" + msgid "Changed the title to \"%{title_param}\"." msgstr "" diff --git a/spec/models/project_setting_spec.rb b/spec/models/project_setting_spec.rb index d03eb3c8bfe4f3..867ad843406eb8 100644 --- a/spec/models/project_setting_spec.rb +++ b/spec/models/project_setting_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe ProjectSetting, type: :model do + using RSpec::Parameterized::TableSyntax it { is_expected.to belong_to(:project) } describe 'validations' do @@ -27,6 +28,23 @@ end end + describe '#human_squash_option' do + where(:squash_option, :human_squash_option) do + 'never' | 'Do not allow' + 'always' | 'Require' + 'default_on' | 'Encourage' + 'default_off' | 'Allow' + end + + with_them do + let(:project_setting) { create(:project_setting, squash_option: ProjectSetting.squash_options[squash_option]) } + + subject { project_setting.human_squash_option } + + it { is_expected.to eq(human_squash_option) } + end + end + def valid_target_platform_combinations target_platforms = described_class::ALLOWED_TARGET_PLATFORMS -- GitLab From 413aa20f9206a9c1c412e2b79fed142d8c104c2f Mon Sep 17 00:00:00 2001 From: Aleksei Lipniagov Date: Tue, 26 Apr 2022 08:02:54 +0000 Subject: [PATCH 14/14] Apply 3 suggestion(s) to 1 file(s) --- app/models/project_setting.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index 64311fde6ac6c1..e5179b62fbb937 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -39,9 +39,9 @@ def target_platforms=(val) def human_squash_option case squash_option when 'never' then 'Do not allow' - when 'always' then'Require' - when 'default_on' then'Encourage' - when 'default_off' then'Allow' + when 'always' then 'Require' + when 'default_on' then 'Encourage' + when 'default_off' then 'Allow' end end -- GitLab