diff --git a/app/services/protected_branches/api_service.rb b/app/services/protected_branches/api_service.rb index 3e5122a1523c8dd6a5d595be785eba05316dceee..d0d0737fd66ca2dd467a85b4536ff2cedbae9c0a 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 0000000000000000000000000000000000000000..f48e02ab4b5f07e06caa3ca27d34515998ae0f5b --- /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(*) + # 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 37083a4a9e432e70e3430401d5c2f153a7ba1b9f..dada449989a5c8da9af34fc1cfb4b20388da493d 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 dc177f0ac098a4497536774b38d30027d56dc097..47332ace417e43811ef5c2bec14687fe79abc81d 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 1815d92421e1d76447f0f03f5219d02a9a43928b..1e70f2d9793cf589429e93b0f8dd3b6a65084f16 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 3cee463f41e78a9c60e393b805a543916ff1f453..29078c12bf674ab9542f090dfca898c35e08a102 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 798caddb35624cf1070fe28edcd85130c2d19dfb..5d2a2fc6850bb9bd84eef0e3d13aa27d4fe5fa89 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,38 @@ 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" + else + "no message defined for #{@action}" + 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 eb3e1d8cf46aa604ee7aecad06d481223df3acd1..4bd14693e8050393de529af0cd6f85a5665e1744 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 0000000000000000000000000000000000000000..64513ae25e24672677daf39236f20388002fe305 --- /dev/null +++ b/ee/lib/ee/audit/protected_branches_changes_auditor.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +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 + 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 + end + + def audit_access_levels + 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 + 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 0000000000000000000000000000000000000000..1e9fba51684dcb27b01d2dda641b04d5799ada15 --- /dev/null +++ b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb @@ -0,0 +1,86 @@ +# 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 + 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) } + + 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 + + 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 + new_access_levels.new(user: user) + expect { service.execute }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + 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, + 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 +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 a3e3a190a5f1d6e9607dee8df4ecc23485b52496..3360597f0d9197fd034785e55546477edd783fd3 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 f670ebd1c91991c59d110ea06ebfda27e18e246d..101442f7f5f9ac250e307644878d1cc35fe3b990 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