Merge branch '1487-audit-events-for-protected-events' into 'master'

Protected branches log audits events

Closes #1487

See merge request gitlab-org/gitlab!16399

app/services/protected_branches/destroy_service.rb

@@ -9,3 +9,5 @@ module ProtectedBranches
     end
   end
 end
+
+ProtectedBranches::DestroyService.prepend_if_ee('EE::ProtectedBranches::DestroyService')

app/services/protected_branches/update_service.rb

@@ -10,3 +10,5 @@ module ProtectedBranches
     end
   end
 end
+
+ProtectedBranches::UpdateService.prepend_if_ee('EE::ProtectedBranches::UpdateService')

doc/administration/audit_events.md

 ---
-last_updated: 2019-02-04
+last_updated: 2019-09-16
 ---
 
 # Audit Events **(STARTER)**
@@ -77,6 +77,7 @@ From there, you can see the following actions:
 - Project repository was downloaded
 - Project was archived
 - Project was unarchived
+- Added/removed/updated protected branches
 
 ### Instance events **(PREMIUM ONLY)**
 

ee/app/services/ee/audit_events/protected_branch_audit_event_service.rb

+# frozen_string_literal: true
+
+module EE
+  module AuditEvents
+    class ProtectedBranchAuditEventService < ::AuditEventService
+      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)
+
+        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
+        })
+      end
+    end
+  end
+end

ee/app/services/ee/protected_branches/create_service.rb

@@ -5,6 +5,14 @@ module EE
     module CreateService
       extend ::Gitlab::Utils::Override
       include ::Gitlab::Utils::StrongMemoize
+      include Loggable
+
+      override :execute
+      def execute(skip_authorization: false)
+        super(skip_authorization: skip_authorization).tap do |protected_branch_service|
+          log_audit_event(protected_branch_service, :add)
+        end
+      end
 
       private
 

ee/app/services/ee/protected_branches/destroy_service.rb

+# frozen_string_literal: true
+
+module EE
+  module ProtectedBranches
+    module DestroyService
+      extend ::Gitlab::Utils::Override
+      include Loggable
+
+      override :execute
+      def execute(protected_branch)
+        super(protected_branch).tap do |protected_branch_service|
+          # DestroyService returns the value of #.destroy instead of the
+          # instance, in comparison with the other services
+          # (CreateService and UpdateService) so if the destroy service
+          # doesn't succeed the value will be false instead of an instance
+          log_audit_event(protected_branch_service, :remove) if protected_branch_service
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/protected_branches/loggable.rb

+# frozen_string_literal: true
+
+module EE
+  module ProtectedBranches
+    module Loggable
+      def log_audit_event(protected_branch_service, action)
+        if protected_branch_service.errors.blank?
+          ::EE::AuditEvents::ProtectedBranchAuditEventService
+            .new(current_user, protected_branch_service, action)
+            .security_event
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/protected_branches/update_service.rb

+# frozen_string_literal: true
+
+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
+      end
+    end
+  end
+end

ee/changelogs/unreleased/1487-audit-events-for-protected-events.yml

+---
+title: Add audit events for protected branches
+merge_request: 16399
+author:
+type: added

ee/spec/services/ee/audit_events/protected_branch_audit_event_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::AuditEvents::ProtectedBranchAuditEventService do
+  let(:protected_branch) { create(:protected_branch, :no_one_can_push) }
+  let(:merge_level) { 'Maintainers' }
+  let(:push_level) { 'No one' }
+  let(:entity) { protected_branch.project }
+  let(:author) { entity.owner }
+  let(:logger) { instance_double(Gitlab::AuditJsonLogger) }
+
+  describe '#security_event' do
+    shared_examples 'loggable' do |action|
+      context "when a protected_branch is #{action}" do
+        let(:service) { described_class.new(author, protected_branch, action) }
+
+        it 'creates an event and logs to a file with the provided details' do
+          expect(service).to receive(:file_logger).and_return(logger)
+
+          expect(logger).to receive(:info).with(author_id: author.id,
+                                                author_name: author.name,
+                                                entity_id: entity.id,
+                                                entity_type: 'Project',
+                                                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')
+
+          expect { service.security_event }.to change(SecurityEvent, :count).by(1)
+
+          security_event = SecurityEvent.last
+
+          expect(security_event.details).to eq(action => 'protected_branch',
+                                               author_name: author.name,
+                                               target_id:  protected_branch.id,
+                                               target_type: 'ProtectedBranch',
+                                               target_details: protected_branch.name,
+                                               push_access_levels: [push_level],
+                                               merge_access_levels: [merge_level])
+
+          expect(security_event.author_id).to eq(author.id)
+          expect(security_event.entity_id).to eq(entity.id)
+          expect(security_event.entity_type).to eq('Project')
+        end
+      end
+    end
+
+    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) }
+
+      before do
+        stub_licensed_features(audit_events: false,
+                               extended_audit_events: false,
+                               admin_audit_log: false)
+      end
+
+      it "doesn't create an event or log to a file" do
+        expect(service).not_to receive(:file_logger)
+
+        expect { service.security_event }.not_to change(SecurityEvent, :count)
+      end
+    end
+  end
+
+  describe '#enabled?' do
+    let(:service) { described_class.new(author, protected_branch, :any) }
+
+    subject { service.enabled? }
+
+    context 'when not licensed' do
+      before do
+        stub_licensed_features(audit_events: false,
+                               extended_audit_events: false,
+                               admin_audit_log: false)
+      end
+
+      it { is_expected.to be(false) }
+    end
+
+    context 'when licensed' do
+      before do
+        stub_licensed_features(audit_events: true,
+                               extended_audit_events: false,
+                               admin_audit_log: false)
+      end
+
+      it { is_expected.to be(true) }
+    end
+  end
+end

ee/spec/services/ee/protected_branches/create_service_spec.rb

 # frozen_string_literal: true
 
-require "spec_helper"
+require 'spec_helper'
 
 describe ProtectedBranches::CreateService do
   include ProjectForksHelper
@@ -52,5 +52,17 @@ describe ProtectedBranches::CreateService do
         end
       end
     end
+
+    it 'adds a security audit event entry' do
+      expect { service.execute }.to change(::SecurityEvent, :count).by(1)
+    end
+
+    context 'with invalid params' do
+      let(:params) { nil }
+
+      it "doesn't add a security audit event entry" do
+        expect { service.execute }.not_to change(::SecurityEvent, :count)
+      end
+    end
   end
 end

ee/spec/services/ee/protected_branches/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ProtectedBranches::DestroyService do
+  let(:protected_branch) { create(:protected_branch) }
+  let(:branch_name) { protected_branch.name }
+  let(:project) { protected_branch.project }
+  let(:user) { project.owner }
+
+  describe '#execute' do
+    subject(:service) { described_class.new(project, user) }
+
+    it 'adds a security audit event entry' do
+      expect { service.execute(protected_branch) }.to change(::SecurityEvent, :count).by(1)
+    end
+
+    context 'when destroy fails' do
+      before do
+        expect(protected_branch).to receive(:destroy).and_return(false)
+      end
+
+      it "doesn't add a security audit event entry" do
+        expect { service.execute(protected_branch) }.not_to change(::SecurityEvent, :count)
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ProtectedBranches::UpdateService do
+  let(:branch_name) { 'feature' }
+  let(:protected_branch) { create(:protected_branch, :no_one_can_push, 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
+
+  describe '#execute' do
+    subject(:service) { described_class.new(project, user, params) }
+
+    it 'adds a security audit event entry' do
+      expect { service.execute(protected_branch) }.to change(::SecurityEvent, :count).by(1)
+    end
+
+    context 'with invalid params' do
+      let(:params) do
+        {
+          name: branch_name,
+          merge_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }]
+        }
+      end
+
+      it "doesn't add a security audit event entry" do
+        expect { service.execute(protected_branch) }.not_to change(::SecurityEvent, :count)
+      end
+    end
+  end
+end

spec/services/git/branch_push_service_spec.rb

@@ -197,7 +197,7 @@ describe Git::BranchPushService, services: true do
         create(:protected_branch, :no_one_can_push, :developers_can_merge, project: project, name: 'master')
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect_any_instance_of(ProtectedBranches::CreateService).not_to receive(:execute)
+        expect(ProtectedBranches::CreateService).not_to receive(:new)
 
         execute_service(project, user, blankrev, 'newrev', ref)