Merge branch 'add-more-details-to-protected-branches-audit-events' into 'master'

Add more details to Protected Branches Audit Events

See merge request gitlab-org/gitlab!68869

app/services/protected_branches/api_service.rb

 # 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

app/services/protected_branches/base_service.rb

+# 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

app/services/protected_branches/create_service.rb

 # 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?
 

app/services/protected_branches/destroy_service.rb

 # 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)
 

app/services/protected_branches/update_service.rb

 # 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

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).
 

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

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

ee/lib/ee/audit/protected_branches_changes_auditor.rb

+# 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

ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb

+# 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

ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe AuditEvents::ProtectedBranchAuditEventService, :request_store do
   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 @@ RSpec.describe AuditEvents::ProtectedBranchAuditEventService, :request_store do
         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 @@ RSpec.describe AuditEvents::ProtectedBranchAuditEventService, :request_store do
 
           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 @@ RSpec.describe AuditEvents::ProtectedBranchAuditEventService, :request_store do
             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 @@ RSpec.describe AuditEvents::ProtectedBranchAuditEventService, :request_store do
 
     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) }

ee/spec/services/ee/protected_branches/update_service_spec.rb

@@ -4,35 +4,37 @@ require 'spec_helper'
 
 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