Merge branch '1371-audit-project-events' into 'master'

Add project actions in Audit events

Closes #1371

See merge request gitlab-org/gitlab-ee!3160

app/models/audit_event.rb

 class AuditEvent < ActiveRecord::Base
+  prepend EE::AuditEvent
+
   serialize :details, Hash # rubocop:disable Cop/ActiveRecordSerialize
 
   belongs_to :user, foreign_key: :author_id
@@ -9,15 +11,11 @@ class AuditEvent < ActiveRecord::Base
 
   after_initialize :initialize_details
 
-  def author_name
-    details[:author_name].blank? ? user&.name : details[:author_name]
-  end
-
   def initialize_details
     self.details = {} if details.nil?
   end
 
-  def present
-    AuditEventPresenter.new(self)
+  def author_name
+    self.user.name
   end
 end

app/presenters/audit_event_presenter.rb

@@ -3,19 +3,31 @@ AuditEventPresenter < Gitlab::View::Presenter::Simple
   presents :audit_event
 
   def author_name
-    audit_event.author_name || '(removed)'
+    user = audit_event.user
+
+    return nil unless user
+
+    link_to(user.name, user_path(user))
   end
 
   def target
-    audit_event.details[:target_details]
+    details[:target_details]
   end
 
   def ip_address
-    audit_event.details[:ip_address]
+    details[:ip_address]
+  end
+
+  def details
+    audit_event.details
   end
 
   def object
-    audit_event.details[:entity_path]
+    entity = audit_event.entity
+
+    return nil unless entity
+
+    link_to(details[:entity_path], entity).html_safe
   end
 
   def date
@@ -23,6 +35,16 @@ AuditEventPresenter < Gitlab::View::Presenter::Simple
   end
 
   def action
-    Audit::Details.humanize(audit_event.details)
+    Audit::Details.humanize(details)
+  end
+
+  private
+
+  # The class can't include ActionView::Helpers::UrlHelper because it overwrites
+  # the method url_for. In this helper, that implementation of that method
+  # doesn't accept objects to resolve their route. That's why here we call the
+  # native url_for to get the route of the object and then call the link_to with it
+  def link_to(name, object)
+    ActionController::Base.helpers.link_to(name, url_for(object))
   end
 end

app/views/admin/audit_logs/index.html.haml

@@ -20,7 +20,7 @@
         - elsif params[:event_type] == 'Project'
           .filter-item.inline
             = project_select_tag(:project_id, { class: 'project-item-select hidden-filter-value', toggle_class: 'js-project-search js-project-filter js-filter-submit', dropdown_class: 'dropdown-menu-selectable dropdown-menu-project js-filter-submit',
-              placeholder: admin_project_dropdown_label('Search projects'), idAttribute: 'id', data: { order_by: 'last_activity_at', idattribute: 'id', allprojects: 'true'} })
+              placeholder: admin_project_dropdown_label('Search projects'), idAttribute: 'id', data: { order_by: 'last_activity_at', idattribute: 'id', all_projects: 'true', simple_filter: true} })
         - elsif params[:event_type] == 'Group'
           .filter-item.inline
             = groups_select_tag(:group_id, { required: true, class: 'group-item-select project-item-select hidden-filter-value', toggle_class: 'js-group-search js-group-filter js-filter-submit', dropdown_class: 'dropdown-menu-selectable dropdown-menu-group js-filter-submit',
@@ -39,8 +39,16 @@
       %tbody
         - @events.map(&:present).each do |event|
           %tr
-            %td= event.author_name
-            %td= event.object
+            %td
+              - if (author_link = event.author_name)
+                = author_link
+              - else
+                #{event.details[:author_name]} <small>(removed)</small>
+            %td
+              - if (object_link = event.object)
+                = object_link
+              - else
+                #{event.details[:entity_path]} <small>(removed)</small>
             %td= event.action
             %td= event.target
             %td= event.ip_address

changelogs/unreleased-ee/1371-audit-project-events.yml

+---
+title: Add project actions in Audit events
+merge_request: 3160
+author:
+type: changed

ee/app/models/ee/audit_event.rb

+module EE
+  module AuditEvent
+    extend ActiveSupport::Concern
+
+    def author_name
+      details[:author_name].blank? ? user&.name : details[:author_name]
+    end
+
+    def entity
+      return unless entity_type && entity_id
+
+      # Avoiding exception if the record doesn't exist
+      @entity ||= entity_type.constantize.find_by_id(entity_id)
+    end
+
+    def present
+      AuditEventPresenter.new(self)
+    end
+  end
+end

ee/app/models/ee/project.rb

@@ -29,7 +29,7 @@ module EE
 
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-      has_many :audit_events, as: :entity, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+      has_many :audit_events, as: :entity
       has_many :remote_mirrors, inverse_of: :project
       has_many :path_locks
 

ee/app/services/ee/audit_event_service.rb

@@ -63,8 +63,8 @@ module EE
             to: @details[:to],
             author_name: @author.name,
             target_id: @entity.id,
-            target_type: @entity.class,
-            target_details: @entity.name
+            target_type: @entity.class.name,
+            target_details: @details[:target_details] || @entity.name
         }
       self
     end
@@ -93,6 +93,10 @@ module EE
       )
     end
 
+    def for_project
+      for_custom_model('project', @entity.full_path)
+    end
+
     def entity_audit_events_enabled?
       @entity.respond_to?(:feature_available?) && @entity.feature_available?(:audit_events)
     end

ee/app/services/ee/projects/create_service.rb

@@ -20,7 +20,10 @@ module EE
           end
         end
 
-        log_geo_event(project) if project&.persisted?
+        if project&.persisted?
+          log_geo_event(project)
+          log_audit_event(project)
+        end
 
         project
       end
@@ -51,6 +54,14 @@ module EE
           project.push_rule = push_rule
         end
       end
+
+      def log_audit_event(project)
+        ::AuditEventService.new(
+          current_user,
+          project,
+          action: :create
+        ).for_project.security_event
+      end
     end
   end
 end

ee/app/services/ee/projects/destroy_service.rb

@@ -9,6 +9,7 @@ module EE
         if succeeded
           mirror_cleanup(project)
           log_geo_event(project)
+          log_audit_event(project)
         end
 
         succeeded
@@ -45,6 +46,16 @@ module EE
 
         ::Geo::ProjectRegistry.where(project_id: project.id).delete_all
       end
+
+      private
+
+      def log_audit_event(project)
+        ::AuditEventService.new(
+          current_user,
+          project,
+          action: :destroy
+        ).for_project.security_event
+      end
     end
   end
 end

ee/app/services/ee/projects/transfer_service.rb

@@ -8,6 +8,8 @@ module EE
 
         super
 
+        EE::Audit::ProjectChangesAuditor.new(@current_user, project).execute
+
         ::Geo::RepositoryRenamedEventStore.new(
           project,
           old_path: project.path,

ee/app/services/ee/projects/update_service.rb

@@ -10,7 +10,17 @@ module EE
           params.delete(:mirror_trigger_builds)
         end
 
-        super
+        result = super
+
+        log_audit_events if result[:status] == :success
+
+        result
+      end
+
+      private
+
+      def log_audit_events
+        EE::Audit::ProjectChangesAuditor.new(current_user, project).execute
       end
     end
   end

lib/ee/audit/base_changes_auditor.rb

+module EE
+  module Audit
+    class BaseChangesAuditor
+      include Changes
+
+      def initialize(current_user, model)
+        @model = model
+        @current_user = current_user
+      end
+
+      def parse_options(column, options)
+        super.merge(attributes_from_auditable_model(column))
+      end
+
+      def attributes_from_auditable_model(column)
+        raise NotImplementedError
+      end
+    end
+  end
+end

lib/ee/audit/project_changes_auditor.rb

+module EE
+  module Audit
+    class ProjectChangesAuditor < BaseChangesAuditor
+      def execute
+        audit_changes(:visibility_level, as: 'visibility', model: model)
+        audit_changes(:path, as: 'path', model: model)
+        audit_changes(:name, as: 'name', model: model)
+        audit_changes(:namespace_id, as: 'namespace', model: model)
+      end
+
+      def attributes_from_auditable_model(column)
+        case column
+        when :name
+          {
+            from: model.namespace.human_name + ' / ' + model.previous_changes[column].first.to_s,
+            to: model.full_name
+          }
+        when :path
+          {
+            from: model.old_path_with_namespace.to_s,
+            to: model.full_path
+          }
+        when :visibility_level
+          {
+            from:  ::Gitlab::VisibilityLevel.level_name(model.previous_changes[column].first),
+            to: ::Gitlab::VisibilityLevel.level_name(model.previous_changes[column].last)
+          }
+        when :namespace_id
+          {
+            from: model.old_path_with_namespace,
+            to: model.full_path
+          }
+        end.merge(target_details: model.full_path)
+      end
+    end
+  end
+end

spec/ee/spec/models/ee/project_spec.rb

@@ -16,6 +16,7 @@ describe Project do
     it { is_expected.to have_many(:path_locks) }
     it { is_expected.to have_many(:sourced_pipelines) }
     it { is_expected.to have_many(:source_pipelines) }
+    it { is_expected.to have_many(:audit_events) }
   end
 
   describe '#push_rule' do

spec/ee/spec/services/ee/audit_event_service_spec.rb

@@ -32,4 +32,71 @@ describe AuditEventService do
       expect(event.details[:failed_login]).to eq('LDAP')
     end
   end
+
+  describe 'license' do
+    let(:project) { create(:project) }
+    let(:user) { create(:user) }
+    let!(:service) { described_class.new(user, project, action: :create) }
+    let(:event) { service.for_project.security_event }
+
+    before do
+      disable_license_audit_features(service)
+    end
+
+    describe 'has the audit_admin feature' do
+      before do
+        allow(service).to receive(:admin_audit_log_enabled?).and_return(true)
+      end
+
+      it 'logs an audit event' do
+        expect { event }.to change(AuditEvent, :count).by(1)
+      end
+
+      it 'has the entity_path' do
+        expect(event.details[:entity_path]).to eq(project.full_path)
+      end
+    end
+
+    describe 'has the extended_audit_events feature' do
+      before do
+        allow(service).to receive(:entity_audit_events_enabled?).and_return(true)
+      end
+
+      it 'logs an audit event' do
+        expect { event }.to change(AuditEvent, :count).by(1)
+      end
+
+      it 'has not the entity_path' do
+        expect(event.details[:entity_path]).not_to eq(project.full_path)
+      end
+    end
+
+    describe 'entity has the audit_events feature' do
+      before do
+        allow(service).to receive(:audit_events_enabled?).and_return(true)
+      end
+
+      it 'logs an audit event' do
+        expect { event }.to change(AuditEvent, :count).by(1)
+      end
+
+      it 'has not the entity_path' do
+        expect(event.details[:entity_path]).not_to eq(project.full_path)
+      end
+    end
+
+    describe 'has not any audit event feature' do
+      it 'does not log the audit event' do
+        expect { event }.not_to change(AuditEvent, :count)
+      end
+    end
+
+    def disable_license_audit_features(service)
+      [:entity_audit_events_enabled?,
+       :admin_audit_log_enabled?,
+       :audit_events_enabled?].each do |f|
+        allow(service).to receive(f).and_return(false)
+      end
+    end
+  end
 end

spec/ee/spec/services/ee/projects/create_service_spec.rb

@@ -202,6 +202,29 @@ describe Projects::CreateService, '#execute' do
     end
   end
 
+  context 'audit events' do
+    include_examples 'audit event logging' do
+      let(:operation) { create_project(user, opts) }
+      let(:fail_condition!) do
+        allow(Gitlab::VisibilityLevel).to receive(:allowed_for?).and_return(false)
+      end
+      let(:attributes) do
+        {
+           author_id: user.id,
+           entity_id: @resource.id,
+           entity_type: 'Project',
+           details: {
+             add: 'project',
+             author_name: user.name,
+             target_id: @resource.full_path,
+             target_type: 'Project',
+             target_details: @resource.full_path
+           }
+         }
+      end
+    end
+  end
+
   def create_project(user, opts)
     described_class.new(user, opts).execute
   end

spec/ee/spec/services/ee/projects/destroy_service_spec.rb

@@ -49,4 +49,28 @@ describe Projects::DestroyService do
       end
     end
   end
+
+  context 'audit events' do
+    include_examples 'audit event logging' do
+      let(:operation) { subject.execute }
+      let(:fail_condition!) do
+        expect_any_instance_of(Project)
+          .to receive(:destroy!).and_raise(StandardError.new('Other error message'))
+      end
+      let(:attributes) do
+        {
+           author_id: user.id,
+           entity_id: project.id,
+           entity_type: 'Project',
+           details: {
+             remove: 'project',
+             author_name: user.name,
+             target_id: project.full_path,
+             target_type: 'Project',
+             target_details: project.full_path
+           }
+         }
+      end
+    end
+  end
 end

spec/ee/spec/services/ee/projects/transfer_service_spec.rb

@@ -18,4 +18,30 @@ describe Projects::TransferService do
       expect { subject.execute(group) }.to change(Geo::RepositoryRenamedEvent, :count).by(1)
     end
   end
+
+  context 'audit events' do
+    include_examples 'audit event logging' do
+      let(:operation) { subject.execute(group) }
+      let(:fail_condition!) do
+        expect_any_instance_of(Project)
+          .to receive(:has_container_registry_tags?).and_return(true)
+      end
+      let(:attributes) do
+        {
+           author_id: user.id,
+           entity_id: project.id,
+           entity_type: 'Project',
+           details: {
+             change: 'namespace',
+             from: project.old_path_with_namespace,
+             to: project.full_path,
+             author_name: user.name,
+             target_id: project.id,
+             target_type: 'Project',
+             target_details: project.full_path
+           }
+         }
+      end
+    end
+  end
 end

spec/ee/spec/services/ee/projects/update_service_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Projects::UpdateService, '#execute' do
   let(:user) { create(:user) }
-  let(:project) { create(:project, creator: user, namespace: user.namespace) }
+  let(:project) { create(:project, :repository, creator: user, namespace: user.namespace) }
 
   context 'repository mirror' do
     let!(:opts) do
@@ -47,6 +47,82 @@ describe Projects::UpdateService, '#execute' do
     end
   end
 
+  context 'audit events' do
+    let(:audit_event_params) do
+      {
+        author_id: user.id,
+        entity_id: project.id,
+        entity_type: 'Project',
+        details: {
+          author_name: user.name,
+          target_id: project.id,
+          target_type: 'Project',
+          target_details: project.full_path
+        }
+      }
+    end
+
+    context '#name' do
+      include_examples 'audit event logging' do
+        let!(:old_name) { project.full_name }
+        let(:operation) { update_project(project, user, name: 'foobar') }
+        let(:fail_condition!) do
+          allow_any_instance_of(Project).to receive(:update_attributes).and_return(false)
+        end
+
+        let(:attributes) do
+          audit_event_params.tap do |param|
+            param[:details].merge!(
+              change: 'name',
+              from: old_name,
+              to: project.full_name
+            )
+          end
+        end
+      end
+    end
+
+    context '#path' do
+      include_examples 'audit event logging' do
+        let(:operation) { update_project(project, user, path: 'foobar1') }
+        let(:fail_condition!) do
+          allow_any_instance_of(Project).to receive(:update_attributes).and_return(false)
+        end
+
+        let(:attributes) do
+          audit_event_params.tap do |param|
+            param[:details].merge!(
+              change: 'path',
+              from: project.old_path_with_namespace,
+              to: project.full_path
+            )
+          end
+        end
+      end
+    end
+
+    context '#visibility' do
+      include_examples 'audit event logging' do
+        let(:operation) do
+          update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        end
+        let(:fail_condition!) do
+          allow_any_instance_of(Project).to receive(:update_attributes).and_return(false)
+        end
+
+        let(:attributes) do
+          audit_event_params.tap do |param|
+            param[:details].merge!(
+              change: 'visibility',
+              from: 'Private',
+              to: 'Internal'
+            )
+          end
+        end
+      end
+    end
+  end
+
   def update_project(project, user, opts)
     Projects::UpdateService.new(project, user, opts).execute
   end

spec/lib/audit/details_spec.rb

@@ -15,7 +15,9 @@ describe Audit::Details do
       end
 
       it 'humanizes user login action' do
-        expect(described_class.humanize(login_action)).to eq('Signed in with LDAP authentication')
+        string = described_class.humanize(login_action)
+
+        expect(string).to eq('Signed in with LDAP authentication')
       end
     end
 
@@ -35,7 +37,9 @@ describe Audit::Details do
       end
 
       it 'humanizes add project member access action' do
-        expect(described_class.humanize(member_access_action)).to eq('Added user access as Developer')
+        string = described_class.humanize(member_access_action)
+
+        expect(string).to eq('Added user access as Developer')
       end
     end
 
@@ -56,7 +60,9 @@ describe Audit::Details do
       end
 
       it 'humanizes add group member access action' do
-        expect(described_class.humanize(member_access_action)).to eq('Changed access level from Guest to Owner')
+        string = described_class.humanize(member_access_action)
+
+        expect(string).to eq('Changed access level from Guest to Owner')
       end
     end
 
@@ -72,7 +78,9 @@ describe Audit::Details do
       end
 
       it 'humanizes the removal action' do
-        expect(described_class.humanize(removal_action)).to eq('Removed deploy key')
+        string = described_class.humanize(removal_action)
+
+        expect(string).to eq('Removed deploy key')
       end
     end
 
@@ -90,7 +98,9 @@ describe Audit::Details do
       end
 
       it 'humanizes the removal action' do
-        expect(described_class.humanize(action)).to eq('Changed email from a@b.com to c@b.com')
+        string = described_class.humanize(action)
+
+        expect(string).to eq('Changed email from a@b.com to c@b.com')
       end
     end
   end

spec/lib/ee/audit/project_changes_auditor_spec.rb

+require 'spec_helper'
+
+describe EE::Audit::ProjectChangesAuditor do
+  describe '.audit_changes' do
+    let!(:user) { create(:user) }
+    let!(:project) { create(:project, visibility_level: 0) }
+    let(:foo_instance) { described_class.new(user, project) }
+
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
+    describe 'non audit changes' do
+      it 'does not call the audit event service' do
+        project.update!(description: 'new description')
+
+        expect { foo_instance.execute }.not_to change { SecurityEvent.count }
+      end
+    end
+
+    describe 'audit changes' do
+      it 'creates an event when the visibility change' do
+        project.update!(visibility_level: 20)
+
+        expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1)
+        expect(SecurityEvent.last.details[:change]).to eq 'visibility'
+      end
+
+      it 'creates an event when the name change' do
+        project.update!(name: 'new name')
+
+        expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1)
+        expect(SecurityEvent.last.details[:change]).to eq 'name'
+      end
+
+      it 'creates an event when the path change' do
+        project.update!(path: 'newpath')
+
+        expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1)
+        expect(SecurityEvent.last.details[:change]).to eq 'path'
+      end
+
+      it 'creates an event when the namespace change' do
+        new_namespace = create(:namespace)
+
+        project.update!(namespace: new_namespace)
+
+        expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1)
+        expect(SecurityEvent.last.details[:change]).to eq 'namespace'
+      end
+    end
+  end
+end

spec/models/audit_event_spec.rb

@@ -43,6 +43,26 @@ RSpec.describe AuditEvent, type: :model do
     end
   end
 
+  describe '#entity' do
+    context 'when entity exists' do
+      let(:user) { create(:user, name: 'John Doe') }
+
+      subject(:event) { described_class.new(entity_id: user.id, entity_type: user.class.name) }
+
+      it 'returns the entity object' do
+        expect(event.entity).to eq user
+      end
+    end
+
+    context 'when entity does not exist' do
+      subject(:event) { described_class.new(entity_id: 99999, entity_type: 'User') }
+
+      it 'returns nil' do
+        expect(event.entity).to be_blank
+      end
+    end
+  end
+
   describe '#present' do
     it 'returns a presenter' do
       expect(subject.present).to be_an_instance_of(AuditEventPresenter)

spec/presenters/audit_event_presenter_spec.rb

 require 'spec_helper'
 
 describe AuditEventPresenter do
+  include Gitlab::Routing.url_helpers
+
   let(:details) do
     {
       author_name: 'author',
@@ -18,8 +20,16 @@ describe AuditEventPresenter do
     described_class.new(audit_event)
   end
 
-  it 'exposes the author name' do
-    expect(presenter.author_name).to eq(details[:author_name])
+  context 'exposes the author' do
+    it 'shows a link if it exists' do
+      expect(presenter.author_name).to eq("<a href=\"#{user_path(audit_event.user)}\">#{audit_event.user.name}</a>")
+    end
+
+    it 'stores the name if it has been deleted' do
+      audit_event.user = nil
+
+      expect(presenter.author_name).to be_blank
+    end
   end
 
   it 'exposes the target' do
@@ -30,8 +40,16 @@ describe AuditEventPresenter do
     expect(presenter.ip_address).to eq(details[:ip_address])
   end
 
-  it 'exposes the object' do
-    expect(presenter.object).to eq(details[:entity_path])
+  context 'exposes the object' do
+    it 'link if it exists' do
+      expect(presenter.object).to eq("<a href=\"#{url_for(audit_event.entity)}\">#{details[:entity_path]}</a>")
+    end
+
+    it 'stored name if it has been deleted' do
+      audit_event.entity_id = nil
+
+      expect(presenter.object).to be_blank
+    end
   end
 
   it 'exposes the date' do

spec/support/shared_examples/audit_event_logging.rb

+shared_examples_for 'audit event logging' do
+  before do
+    stub_licensed_features(extended_audit_events: true)
+  end
+
+  context 'if operation succeed' do
+    it 'logs an audit event if operation succeed' do
+      expect { operation }.to change(AuditEvent, :count).by(1)
+    end
+
+    it 'logs the project info' do
+      @resource = operation
+
+      expect(AuditEvent.last).to have_attributes(attributes)
+    end
+  end
+
+  it 'does not log audit event if project operation fails' do
+    fail_condition!
+
+    expect { operation }.not_to change(AuditEvent, :count)
+  end
+end