From f793b691ea1711594e208f4e25dc90aaece13781 Mon Sep 17 00:00:00 2001 From: Adrien Gooris Date: Fri, 20 Aug 2021 12:13:57 +0200 Subject: [PATCH 1/4] Add more details to Protected Branches Audit Events Changelog: added MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68869 EE: true --- .../protected_branches/api_service.rb | 2 +- .../protected_branches/base_service.rb | 17 ++++ .../protected_branches/create_service.rb | 2 +- .../protected_branches/destroy_service.rb | 2 +- .../protected_branches/update_service.rb | 10 +- doc/administration/audit_events.md | 2 + .../protected_branch_audit_event_service.rb | 35 +++++-- .../ee/protected_branches/update_service.rb | 10 +- .../protected_branches_changes_auditor.rb | 49 ++++++++++ ...protected_branches_changes_auditor_spec.rb | 98 +++++++++++++++++++ ...otected_branch_audit_event_service_spec.rb | 14 +-- .../protected_branches/update_service_spec.rb | 32 +++--- 12 files changed, 232 insertions(+), 41 deletions(-) create mode 100644 app/services/protected_branches/base_service.rb create mode 100644 ee/lib/ee/audit/protected_branches_changes_auditor.rb create mode 100644 ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb diff --git a/app/services/protected_branches/api_service.rb b/app/services/protected_branches/api_service.rb index 3e5122a1523c8d..d0d0737fd66ca2 100644 --- a/app/services/protected_branches/api_service.rb +++ b/app/services/protected_branches/api_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ProtectedBranches - class ApiService < BaseService + class ApiService < ProtectedBranches::BaseService def create ::ProtectedBranches::CreateService.new(@project, @current_user, protected_branch_params).execute end diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb new file mode 100644 index 00000000000000..3ed4c52a9a7d04 --- /dev/null +++ b/app/services/protected_branches/base_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module ProtectedBranches + class BaseService < ::BaseService + # current_user - The user that performs the action + # params - A hash of parameters + def initialize(project, current_user = nil, params = {}) + @project = project + @current_user = current_user + @params = params + end + + def after_execute(args) + # overridden in EE::ProtectedBranches module + end + end +end diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb index 37083a4a9e432e..dada449989a5c8 100644 --- a/app/services/protected_branches/create_service.rb +++ b/app/services/protected_branches/create_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ProtectedBranches - class CreateService < BaseService + class CreateService < ProtectedBranches::BaseService def execute(skip_authorization: false) raise Gitlab::Access::AccessDeniedError unless skip_authorization || authorized? diff --git a/app/services/protected_branches/destroy_service.rb b/app/services/protected_branches/destroy_service.rb index dc177f0ac098a4..47332ace417e43 100644 --- a/app/services/protected_branches/destroy_service.rb +++ b/app/services/protected_branches/destroy_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ProtectedBranches - class DestroyService < BaseService + class DestroyService < ProtectedBranches::BaseService def execute(protected_branch) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :destroy_protected_branch, protected_branch) diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb index 1815d92421e1d7..1e70f2d9793cf5 100644 --- a/app/services/protected_branches/update_service.rb +++ b/app/services/protected_branches/update_service.rb @@ -1,11 +1,17 @@ # frozen_string_literal: true module ProtectedBranches - class UpdateService < BaseService + class UpdateService < ProtectedBranches::BaseService def execute(protected_branch) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_protected_branch, protected_branch) - protected_branch.update(params) + old_merge_access_levels = protected_branch.merge_access_levels.map(&:clone) + old_push_access_levels = protected_branch.push_access_levels.map(&:clone) + + if protected_branch.update(params) + after_execute(protected_branch: protected_branch, old_merge_access_levels: old_merge_access_levels, old_push_access_levels: old_push_access_levels) + end + protected_branch end end diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 3cee463f41e78a..29078c12bf674a 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -127,6 +127,8 @@ From there, you can see the following actions: - Permission to modify merge requests approval rules in merge requests was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/336211) in GitLab 14.2) - New approvals requirement when new commits are added to an MR was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/336211) in GitLab 14.2) - When [strategies for feature flags](../operations/feature_flags.md#feature-flag-strategies) are changed ([introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68408) in GitLab 14.3) +- Changed allow push force and code owner approval requirement ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/338873) in GitLab 14.3) +- Added or removed users and groups from protected branch allow to merge and allow to push ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/338873) in GitLab 14.3) Project events can also be accessed via the [Project Audit Events API](../api/audit_events.md#project-audit-events). diff --git a/ee/app/services/audit_events/protected_branch_audit_event_service.rb b/ee/app/services/audit_events/protected_branch_audit_event_service.rb index 798caddb35624c..db0ee5ef50917a 100644 --- a/ee/app/services/audit_events/protected_branch_audit_event_service.rb +++ b/ee/app/services/audit_events/protected_branch_audit_event_service.rb @@ -2,19 +2,36 @@ module AuditEvents class ProtectedBranchAuditEventService < ::AuditEventService + attr_accessor :protected_branch + def initialize(author, protected_branch, action) - push_access_levels = protected_branch.push_access_levels.map(&:humanize) - merge_access_levels = protected_branch.merge_access_levels.map(&:humanize) + @action = action + @protected_branch = protected_branch super(author, protected_branch.project, - action => 'protected_branch', - author_name: author.name, - target_id: protected_branch.id, - target_type: protected_branch.class.name, - target_details: protected_branch.name, - push_access_levels: push_access_levels, - merge_access_levels: merge_access_levels + { author_name: author.name, + custom_message: message, + target_id: protected_branch.id, + target_type: protected_branch.class.name, + target_details: protected_branch.name, + push_access_levels: protected_branch.push_access_levels.map(&:humanize), + merge_access_levels: protected_branch.merge_access_levels.map(&:humanize), + allow_force_push: protected_branch.allow_force_push, + code_owner_approval_required: protected_branch.code_owner_approval_required } ) end + + def message + case @action + when :add + "Added protected branch with ["\ + "allowed to push: #{@protected_branch.push_access_levels.map(&:humanize)}, "\ + "allowed to merge: #{@protected_branch.merge_access_levels.map(&:humanize)}, "\ + "allow force push: #{@protected_branch.allow_force_push}, "\ + "code owner approval required: #{@protected_branch.code_owner_approval_required}]" + when :remove + "Unprotected branch" + end + end end end diff --git a/ee/app/services/ee/protected_branches/update_service.rb b/ee/app/services/ee/protected_branches/update_service.rb index eb3e1d8cf46aa6..4bd14693e80503 100644 --- a/ee/app/services/ee/protected_branches/update_service.rb +++ b/ee/app/services/ee/protected_branches/update_service.rb @@ -4,13 +4,11 @@ module EE module ProtectedBranches module UpdateService extend ::Gitlab::Utils::Override - include Loggable - override :execute - def execute(protected_branch) - super(protected_branch).tap do |protected_branch_service| - log_audit_event(protected_branch_service, :update) - end + def after_execute(protected_branch:, old_merge_access_levels:, old_push_access_levels:) + super + + EE::Audit::ProtectedBranchesChangesAuditor.new(current_user, protected_branch, old_merge_access_levels, old_push_access_levels).execute end end end diff --git a/ee/lib/ee/audit/protected_branches_changes_auditor.rb b/ee/lib/ee/audit/protected_branches_changes_auditor.rb new file mode 100644 index 00000000000000..9e090402d984c3 --- /dev/null +++ b/ee/lib/ee/audit/protected_branches_changes_auditor.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module EE + module Audit + class ProtectedBranchesChangesAuditor < BaseChangesAuditor + def initialize(current_user, model, old_merge_access_levels, old_push_access_levels) + @old_merge_access_levels = old_merge_access_levels + @old_push_access_levels = old_push_access_levels + super(current_user, model) + end + + def execute + audit_changes(:allow_force_push, as: 'allow force push', entity: model.project, model: model) + audit_changes(:code_owner_approval_required, as: 'code owner approval required', entity: model.project, model: model) + audit_access_levels :merge, :push + end + + def audit_access_levels(*types) + types.each do |type| + new_access_levels = model.public_send("#{type}_access_levels") # rubocop:disable GitlabSecurity/PublicSend + if self.instance_variable_get("@old_#{type}_access_levels") != new_access_levels + @details = + { + change: "allowed to #{type}", + from: self.instance_variable_get("@old_#{type}_access_levels").map(&:humanize), + to: new_access_levels.map(&:humanize), + target_id: model.id, + target_type: model.class.name, + target_details: model.name + } + + ::AuditEventService.new(@current_user, model.project, @details) # rubocop:disable Gitlab/ModuleWithInstanceVariables + .security_event + end + end + end + + def attributes_from_auditable_model(column) + old = model.previous_changes[column].first + new = model.previous_changes[column].last + + { + from: old, + to: new + } + end + end + end +end diff --git a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb new file mode 100644 index 00000000000000..d01baa656a4f36 --- /dev/null +++ b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EE::Audit::ProtectedBranchesChangesAuditor, :request_store do + let_it_be(:author) { create(:user, :with_sign_ins) } + let_it_be(:user) { create(:user, :with_sign_ins) } + let_it_be(:entity) { create(:project, creator: author) } + + let(:protected_branch) { create(:protected_branch, :no_one_can_push, :no_one_can_merge, allow_force_push: false, code_owner_approval_required: false, project: entity) } + let(:ip_address) { '192.168.15.18' } + + before do + stub_licensed_features(admin_audit_log: true, code_owner_approval_required: true) + allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(ip_address) + end + + describe '#execute' do + let(:old_merge_level) { protected_branch.merge_access_levels.map(&:clone) } + let(:old_push_level) { protected_branch.push_access_levels.map(&:clone) } + + subject(:service) { described_class.new(author, protected_branch, old_merge_level, old_push_level) } + + shared_examples 'settings' do |setting| + context "when #{setting} changed" do + it 'creates an event' do + protected_branch.update_attribute(setting, true) + expect { service.execute }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + expect(event.details).to eq({ change: "#{setting}".humanize(capitalize: false), + author_name: author.name, + target_id: protected_branch.id, + entity_path: entity.full_path, + target_type: 'ProtectedBranch', + target_details: protected_branch.name, + from: false, + to: true, + ip_address: ip_address }) + + expect(event.author_id).to eq(author.id) + expect(event.entity_id).to eq(entity.id) + expect(event.entity_type).to eq('Project') + expect(event.ip_address).to eq(ip_address) + end + end + end + + include_examples 'settings', :allow_force_push + include_examples 'settings', :code_owner_approval_required + + context "when push access levels changed" do + it 'creates an event' do + protected_branch.push_access_levels.new(user: user) + expect { service.execute }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + expect(event.details).to eq({ change: "allowed to push", + from: old_push_level.map(&:humanize), + to: protected_branch.push_access_levels.map(&:humanize), + target_id: protected_branch.id, + target_type: "ProtectedBranch", + target_details: protected_branch.name, + ip_address: ip_address, + entity_path: entity.full_path, + author_name: author.name }) + + expect(event.author_id).to eq(author.id) + expect(event.entity_id).to eq(entity.id) + expect(event.entity_type).to eq('Project') + expect(event.ip_address).to eq(ip_address) + end + end + + context "when merge access levels changed" do + it 'creates an event' do + protected_branch.merge_access_levels.new(user: user) + expect { service.execute }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + expect(event.details).to eq({ change: "allowed to merge", + from: old_merge_level.map(&:humanize), + to: protected_branch.merge_access_levels.map(&:humanize), + target_id: protected_branch.id, + target_type: "ProtectedBranch", + target_details: protected_branch.name, + ip_address: ip_address, + entity_path: entity.full_path, + author_name: author.name }) + + expect(event.author_id).to eq(author.id) + expect(event.entity_id).to eq(entity.id) + expect(event.entity_type).to eq('Project') + expect(event.ip_address).to eq(ip_address) + end + end + end +end diff --git a/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb b/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb index a3e3a190a5f1d6..3360597f0d9197 100644 --- a/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb +++ b/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb @@ -7,7 +7,7 @@ let(:push_level) { 'No one' } let_it_be(:author) { create(:user, :with_sign_ins) } let_it_be(:entity) { create(:project, creator: author) } - let_it_be(:protected_branch) { create(:protected_branch, :no_one_can_push, project: entity) } + let_it_be(:protected_branch) { create(:protected_branch, :no_one_can_push, allow_force_push: true, code_owner_approval_required: true, project: entity) } let(:logger) { instance_spy(Gitlab::AuditJsonLogger) } let(:ip_address) { '192.168.15.18' } @@ -18,7 +18,7 @@ let(:service) { described_class.new(author, protected_branch, action) } before do - stub_licensed_features(admin_audit_log: true) + stub_licensed_features(admin_audit_log: true, code_owner_approval_required: true) allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(ip_address) end @@ -27,13 +27,14 @@ security_event = AuditEvent.last - expect(security_event.details).to eq( - action => 'protected_branch', + expect(security_event.details).to include( author_name: author.name, target_id: protected_branch.id, entity_path: entity.full_path, target_type: 'ProtectedBranch', target_details: protected_branch.name, + allow_force_push: true, + code_owner_approval_required: true, push_access_levels: [push_level], merge_access_levels: [merge_level], ip_address: ip_address @@ -56,12 +57,14 @@ entity_id: entity.id, entity_type: 'Project', entity_path: entity.full_path, + allow_force_push: true, + code_owner_approval_required: true, merge_access_levels: [merge_level], push_access_levels: [push_level], target_details: protected_branch.name, target_id: protected_branch.id, target_type: 'ProtectedBranch', - action => 'protected_branch', + custom_message: action == :add ? /Added/ : /Unprotected/, ip_address: ip_address ) end @@ -70,7 +73,6 @@ include_examples 'loggable', :add include_examples 'loggable', :remove - include_examples 'loggable', :update context 'when not licensed' do let(:service) { described_class.new(author, protected_branch, :add) } diff --git a/ee/spec/services/ee/protected_branches/update_service_spec.rb b/ee/spec/services/ee/protected_branches/update_service_spec.rb index f670ebd1c91991..101442f7f5f9ac 100644 --- a/ee/spec/services/ee/protected_branches/update_service_spec.rb +++ b/ee/spec/services/ee/protected_branches/update_service_spec.rb @@ -4,35 +4,37 @@ RSpec.describe ProtectedBranches::UpdateService do let(:branch_name) { 'feature' } - let(:protected_branch) { create(:protected_branch, :no_one_can_push, name: branch_name) } + let(:protected_branch) { create(:protected_branch, name: branch_name) } let(:project) { protected_branch.project } let(:user) { project.owner } - let(:params) do - { - name: branch_name, - merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }], - push_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }] - } - end + subject(:service) { described_class.new(project, user, params) } describe '#execute' do - subject(:service) { described_class.new(project, user, params) } + context 'with invalid params' do + let(:params) do + { + name: branch_name, + push_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }] + } + end - it 'adds a security audit event entry' do - expect { service.execute(protected_branch) }.to change(::AuditEvent, :count).by(1) + it "does not add a security audit event entry" do + expect { service.execute(protected_branch) }.not_to change(::AuditEvent, :count) + end end - context 'with invalid params' do + context 'with valid params' do let(:params) do { name: branch_name, - merge_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }] + merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }], + push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }] } end - it "doesn't add a security audit event entry" do - expect { service.execute(protected_branch) }.not_to change(::AuditEvent, :count) + it 'adds security audit event entries' do + expect { service.execute(protected_branch) }.to change(::AuditEvent, :count).by(2) end end end -- GitLab From 023a1b665e9b103c0f201aa4db71e2c25359d5b8 Mon Sep 17 00:00:00 2001 From: Adrien Gooris Date: Thu, 2 Sep 2021 14:48:32 +0200 Subject: [PATCH 2/4] Fix after review --- .../protected_branches/base_service.rb | 2 +- .../protected_branch_audit_event_service.rb | 2 + .../protected_branches_changes_auditor.rb | 50 +++++++++------ ...protected_branches_changes_auditor_spec.rb | 62 +++++++------------ 4 files changed, 56 insertions(+), 60 deletions(-) diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb index 3ed4c52a9a7d04..f48e02ab4b5f07 100644 --- a/app/services/protected_branches/base_service.rb +++ b/app/services/protected_branches/base_service.rb @@ -10,7 +10,7 @@ def initialize(project, current_user = nil, params = {}) @params = params end - def after_execute(args) + def after_execute(*) # overridden in EE::ProtectedBranches module end end diff --git a/ee/app/services/audit_events/protected_branch_audit_event_service.rb b/ee/app/services/audit_events/protected_branch_audit_event_service.rb index db0ee5ef50917a..5d2a2fc6850bb9 100644 --- a/ee/app/services/audit_events/protected_branch_audit_event_service.rb +++ b/ee/app/services/audit_events/protected_branch_audit_event_service.rb @@ -31,6 +31,8 @@ def message "code owner approval required: #{@protected_branch.code_owner_approval_required}]" when :remove "Unprotected branch" + else + "no message defined for #{@action}" end end end diff --git a/ee/lib/ee/audit/protected_branches_changes_auditor.rb b/ee/lib/ee/audit/protected_branches_changes_auditor.rb index 9e090402d984c3..03409eb96a4768 100644 --- a/ee/lib/ee/audit/protected_branches_changes_auditor.rb +++ b/ee/lib/ee/audit/protected_branches_changes_auditor.rb @@ -4,34 +4,46 @@ module EE module Audit class ProtectedBranchesChangesAuditor < BaseChangesAuditor def initialize(current_user, model, old_merge_access_levels, old_push_access_levels) + super(current_user, model) @old_merge_access_levels = old_merge_access_levels @old_push_access_levels = old_push_access_levels - super(current_user, model) end def execute audit_changes(:allow_force_push, as: 'allow force push', entity: model.project, model: model) audit_changes(:code_owner_approval_required, as: 'code owner approval required', entity: model.project, model: model) - audit_access_levels :merge, :push + audit_access_levels end - def audit_access_levels(*types) - types.each do |type| - new_access_levels = model.public_send("#{type}_access_levels") # rubocop:disable GitlabSecurity/PublicSend - if self.instance_variable_get("@old_#{type}_access_levels") != new_access_levels - @details = - { - change: "allowed to #{type}", - from: self.instance_variable_get("@old_#{type}_access_levels").map(&:humanize), - to: new_access_levels.map(&:humanize), - target_id: model.id, - target_type: model.class.name, - target_details: model.name - } - - ::AuditEventService.new(@current_user, model.project, @details) # rubocop:disable Gitlab/ModuleWithInstanceVariables - .security_event - end + def audit_access_levels + # push access levels + if @old_push_access_levels != model.push_access_levels + details = + { + change: "allowed to push", + from: @old_push_access_levels.map(&:humanize), + to: model.push_access_levels.map(&:humanize), + target_id: model.id, + target_type: model.class.name, + target_details: model.name + } + + ::AuditEventService.new(@current_user, model.project, details).security_event + end + + # merge access levels + if @old_merge_access_levels != model.merge_access_levels + details = + { + change: "allowed to merge", + from: @old_merge_access_levels.map(&:humanize), + to: model.merge_access_levels.map(&:humanize), + target_id: model.id, + target_type: model.class.name, + target_details: model.name + } + + ::AuditEventService.new(@current_user, model.project, details).security_event end end diff --git a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb index d01baa656a4f36..8973467c5288ee 100644 --- a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb @@ -49,50 +49,32 @@ include_examples 'settings', :allow_force_push include_examples 'settings', :code_owner_approval_required - context "when push access levels changed" do - it 'creates an event' do - protected_branch.push_access_levels.new(user: user) - expect { service.execute }.to change { AuditEvent.count }.by(1) + shared_examples 'access levels' do |access_level_type| + context "when #{access_level_type} access levels changed" do + it 'creates an event' do + protected_branch.send("#{access_level_type}_access_levels").new(user: user) + expect { service.execute }.to change { AuditEvent.count }.by(1) - event = AuditEvent.last - expect(event.details).to eq({ change: "allowed to push", - from: old_push_level.map(&:humanize), - to: protected_branch.push_access_levels.map(&:humanize), - target_id: protected_branch.id, - target_type: "ProtectedBranch", - target_details: protected_branch.name, - ip_address: ip_address, - entity_path: entity.full_path, - author_name: author.name }) + event = AuditEvent.last + expect(event.details).to eq({ change: "allowed to #{access_level_type}", + from: self.send("old_#{access_level_type}_level").map(&:humanize), + to: protected_branch.send("#{access_level_type}_access_levels").map(&:humanize), + target_id: protected_branch.id, + target_type: "ProtectedBranch", + target_details: protected_branch.name, + ip_address: ip_address, + entity_path: entity.full_path, + author_name: author.name }) - expect(event.author_id).to eq(author.id) - expect(event.entity_id).to eq(entity.id) - expect(event.entity_type).to eq('Project') - expect(event.ip_address).to eq(ip_address) + expect(event.author_id).to eq(author.id) + expect(event.entity_id).to eq(entity.id) + expect(event.entity_type).to eq('Project') + expect(event.ip_address).to eq(ip_address) + end end end - context "when merge access levels changed" do - it 'creates an event' do - protected_branch.merge_access_levels.new(user: user) - expect { service.execute }.to change { AuditEvent.count }.by(1) - - event = AuditEvent.last - expect(event.details).to eq({ change: "allowed to merge", - from: old_merge_level.map(&:humanize), - to: protected_branch.merge_access_levels.map(&:humanize), - target_id: protected_branch.id, - target_type: "ProtectedBranch", - target_details: protected_branch.name, - ip_address: ip_address, - entity_path: entity.full_path, - author_name: author.name }) - - expect(event.author_id).to eq(author.id) - expect(event.entity_id).to eq(entity.id) - expect(event.entity_type).to eq('Project') - expect(event.ip_address).to eq(ip_address) - end - end + include_examples 'access levels', :push + include_examples 'access levels', :merge end end -- GitLab From 09380e606fc71164e5b06190088fbcf4ba167775 Mon Sep 17 00:00:00 2001 From: Adrien Gooris Date: Fri, 3 Sep 2021 14:07:13 +0200 Subject: [PATCH 3/4] Refactoring tests following review --- .../protected_branches_changes_auditor.rb | 37 +++++++------------ ...protected_branches_changes_auditor_spec.rb | 31 ++++++++++------ 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/ee/lib/ee/audit/protected_branches_changes_auditor.rb b/ee/lib/ee/audit/protected_branches_changes_auditor.rb index 03409eb96a4768..52fd69f97da881 100644 --- a/ee/lib/ee/audit/protected_branches_changes_auditor.rb +++ b/ee/lib/ee/audit/protected_branches_changes_auditor.rb @@ -16,34 +16,23 @@ def execute end def audit_access_levels - # push access levels - if @old_push_access_levels != model.push_access_levels - details = - { - change: "allowed to push", - from: @old_push_access_levels.map(&:humanize), - to: model.push_access_levels.map(&:humanize), + access_inputs = [ + ["allowed to push", @old_push_access_levels, model.push_access_levels], + ["allowed to merge", @old_merge_access_levels, model.merge_access_levels] + ] + + access_inputs.each do |change, old_access_levels, new_access_levels| + unless old_access_levels == new_access_levels + details = { + change: change, + from: old_access_levels.map(&:humanize), + to: new_access_levels.map(&:humanize), target_id: model.id, target_type: model.class.name, target_details: model.name } - - ::AuditEventService.new(@current_user, model.project, details).security_event - end - - # merge access levels - if @old_merge_access_levels != model.merge_access_levels - details = - { - change: "allowed to merge", - from: @old_merge_access_levels.map(&:humanize), - to: model.merge_access_levels.map(&:humanize), - target_id: model.id, - target_type: model.class.name, - target_details: model.name - } - - ::AuditEventService.new(@current_user, model.project, details).security_event + ::AuditEventService.new(@current_user, model.project, details).security_event + end end end diff --git a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb index 8973467c5288ee..48ef94e24631b7 100644 --- a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe EE::Audit::ProtectedBranchesChangesAuditor, :request_store do + using RSpec::Parameterized::TableSyntax + let_it_be(:author) { create(:user, :with_sign_ins) } let_it_be(:user) { create(:user, :with_sign_ins) } let_it_be(:entity) { create(:project, creator: author) } @@ -16,10 +18,10 @@ end describe '#execute' do - let(:old_merge_level) { protected_branch.merge_access_levels.map(&:clone) } - let(:old_push_level) { protected_branch.push_access_levels.map(&:clone) } + let(:old_merge_access_levels) { protected_branch.merge_access_levels.map(&:clone) } + let(:old_push_access_levels) { protected_branch.push_access_levels.map(&:clone) } - subject(:service) { described_class.new(author, protected_branch, old_merge_level, old_push_level) } + subject(:service) { described_class.new(author, protected_branch, old_merge_access_levels, old_push_access_levels) } shared_examples 'settings' do |setting| context "when #{setting} changed" do @@ -49,16 +51,24 @@ include_examples 'settings', :allow_force_push include_examples 'settings', :code_owner_approval_required - shared_examples 'access levels' do |access_level_type| - context "when #{access_level_type} access levels changed" do + let(:new_merge_access_levels) { protected_branch.merge_access_levels } + let(:new_push_access_levels) { protected_branch.push_access_levels } + + where(:type, :old_access_levels, :new_access_levels, :change_text) do + 'push' | ref(:old_push_access_levels) | ref(:new_push_access_levels) | 'allowed to push' + 'merge' | ref(:old_merge_access_levels) | ref(:new_merge_access_levels) | 'allowed to merge' + end + + with_them do + context "when access levels changed" do it 'creates an event' do - protected_branch.send("#{access_level_type}_access_levels").new(user: user) + new_access_levels.new(user: user) expect { service.execute }.to change { AuditEvent.count }.by(1) event = AuditEvent.last - expect(event.details).to eq({ change: "allowed to #{access_level_type}", - from: self.send("old_#{access_level_type}_level").map(&:humanize), - to: protected_branch.send("#{access_level_type}_access_levels").map(&:humanize), + expect(event.details).to eq({ change: change_text, + from: old_access_levels.map(&:humanize), + to: new_access_levels.map(&:humanize), target_id: protected_branch.id, target_type: "ProtectedBranch", target_details: protected_branch.name, @@ -73,8 +83,5 @@ end end end - - include_examples 'access levels', :push - include_examples 'access levels', :merge end end -- GitLab From d92b770a715f0001837886c1418430ee7f945ae7 Mon Sep 17 00:00:00 2001 From: Adrien Gooris Date: Fri, 3 Sep 2021 15:28:51 +0200 Subject: [PATCH 4/4] Fix lint failures --- .../ee/audit/protected_branches_changes_auditor.rb | 2 +- .../protected_branches_changes_auditor_spec.rb | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/ee/lib/ee/audit/protected_branches_changes_auditor.rb b/ee/lib/ee/audit/protected_branches_changes_auditor.rb index 52fd69f97da881..64513ae25e2467 100644 --- a/ee/lib/ee/audit/protected_branches_changes_auditor.rb +++ b/ee/lib/ee/audit/protected_branches_changes_auditor.rb @@ -22,7 +22,7 @@ def audit_access_levels ] access_inputs.each do |change, old_access_levels, new_access_levels| - unless old_access_levels == new_access_levels + unless old_access_levels == new_access_levels details = { change: change, from: old_access_levels.map(&:humanize), diff --git a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb index 48ef94e24631b7..1e9fba51684dcb 100644 --- a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe EE::Audit::ProtectedBranchesChangesAuditor, :request_store do - using RSpec::Parameterized::TableSyntax - let_it_be(:author) { create(:user, :with_sign_ins) } let_it_be(:user) { create(:user, :with_sign_ins) } let_it_be(:entity) { create(:project, creator: author) } @@ -18,8 +16,12 @@ end describe '#execute' do + using RSpec::Parameterized::TableSyntax + let(:old_merge_access_levels) { protected_branch.merge_access_levels.map(&:clone) } let(:old_push_access_levels) { protected_branch.push_access_levels.map(&:clone) } + let(:new_merge_access_levels) { protected_branch.merge_access_levels } + let(:new_push_access_levels) { protected_branch.push_access_levels } subject(:service) { described_class.new(author, protected_branch, old_merge_access_levels, old_push_access_levels) } @@ -51,12 +53,9 @@ include_examples 'settings', :allow_force_push include_examples 'settings', :code_owner_approval_required - let(:new_merge_access_levels) { protected_branch.merge_access_levels } - let(:new_push_access_levels) { protected_branch.push_access_levels } - where(:type, :old_access_levels, :new_access_levels, :change_text) do - 'push' | ref(:old_push_access_levels) | ref(:new_push_access_levels) | 'allowed to push' - 'merge' | ref(:old_merge_access_levels) | ref(:new_merge_access_levels) | 'allowed to merge' + :push | ref(:old_push_access_levels) | ref(:new_push_access_levels) | 'allowed to push' + :merge | ref(:old_merge_access_levels) | ref(:new_merge_access_levels) | 'allowed to merge' end with_them do -- GitLab