Merge branch '1372-audit-group-events' into 'master'

Resolve "Add group actions to Audit events"

Closes #1372

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

app/controllers/concerns/membership_actions.rb

@@ -54,8 +54,6 @@ module MembershipActions
         "You left the \"#{membershipable.human_name}\" #{source_type}."
       end
 
-    log_audit_event(member, action: :destroy) unless member.request?
-
     respond_to do |format|
       format.html do
         redirect_path = member.request? ? member.source : [:dashboard, membershipable.class.to_s.tableize]

app/presenters/audit_event_presenter.rb

@@ -27,7 +27,7 @@ AuditEventPresenter < Gitlab::View::Presenter::Simple
 
     return nil unless entity
 
-    link_to(details[:entity_path], entity).html_safe
+    link_to(details[:entity_path] || entity.name, entity).html_safe
   end
 
   def date

app/services/groups/create_service.rb

 module Groups
   class CreateService < Groups::BaseService
+    prepend ::EE::Groups::CreateService
+
     def initialize(user, params = {})
       @current_user, @params = user, params.dup
       @chat_team = @params.delete(:create_chat_team)

app/services/groups/destroy_service.rb

 module Groups
   class DestroyService < Groups::BaseService
+    prepend ::EE::Groups::DestroyService
+
     def async_execute
       group.soft_delete_without_removing_associations
       job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)

app/services/groups/update_service.rb

 module Groups
   class UpdateService < Groups::BaseService
     include UpdateVisibilityLevel
+    prepend ::EE::Groups::UpdateService
 
     def execute
       reject_parent_id!

app/services/projects/group_links/create_service.rb

 module Projects
   module GroupLinks
     class CreateService < BaseService
+      prepend ::EE::Projects::GroupLinks::CreateService
+
       def execute(group)
         return false unless group
 

app/services/projects/group_links/destroy_service.rb

 module Projects
   module GroupLinks
     class DestroyService < BaseService
+      prepend ::EE::Projects::GroupLinks::DestroyService
+
       def execute(group_link)
         return false unless group_link
+
         group_link.destroy
       end
     end

changelogs/unreleased-ee/1372-audit-group-events.yml

+---
+title: Add group actions in Audit events
+merge_request: 3176
+author:
+type: changed

ee/app/services/ee/audit_event_service.rb

@@ -41,6 +41,16 @@ module EE
       self
     end
 
+    def for_project_group_link(group_link)
+      @details = custom_project_link_group_attributes(group_link)
+                 .merge(author_name: @author.name,
+                        target_id: group_link.project.id,
+                        target_type: 'Project',
+                        target_details: group_link.project.full_path)
+
+      self
+    end
+
     def for_failed_login
       ip = @details[:ip_address]
       auth = @details[:with] || 'STANDARD'
@@ -97,6 +107,10 @@ module EE
       for_custom_model('project', @entity.full_path)
     end
 
+    def for_group
+      for_custom_model('group', @entity.full_path)
+    end
+
     def entity_audit_events_enabled?
       @entity.respond_to?(:feature_available?) && @entity.feature_available?(:audit_events)
     end
@@ -167,5 +181,23 @@ module EE
       @details.merge!(ip_address: ip_address,
                       entity_path: @entity.full_path)
     end
+
+    def custom_project_link_group_attributes(group_link)
+      case @details[:action]
+      when :destroy
+        { remove: 'project_access' }
+      when :create
+        {
+          add: 'project_access',
+          as: group_link.human_access
+        }
+      when :update
+        {
+          change: 'access_level',
+          from: @details[:old_access_level],
+          to: group_link.human_access
+        }
+      end
+    end
   end
 end

ee/app/services/ee/groups/create_service.rb

+module EE
+  module Groups
+    module CreateService
+      def execute
+        raise NotImplementedError unless defined?(super)
+
+        super.tap { |group| log_audit_event if group&.persisted? }
+      end
+
+      private
+
+      def log_audit_event
+        ::AuditEventService.new(
+          current_user,
+          group,
+          action: :create
+        ).for_group.security_event
+      end
+    end
+  end
+end

ee/app/services/ee/groups/destroy_service.rb

+module EE
+  module Groups
+    module DestroyService
+      def execute
+        raise NotImplementedError unless defined?(super)
+
+        super.tap { |group| log_audit_event unless group&.persisted? }
+      end
+
+      private
+
+      def log_audit_event
+        ::AuditEventService.new(
+          current_user,
+          group,
+          action: :destroy
+        ).for_group.security_event
+      end
+    end
+  end
+end

ee/app/services/ee/groups/update_service.rb

+module EE
+  module Groups
+    module UpdateService
+      def execute
+        raise NotImplementedError unless defined?(super)
+
+        super.tap { |success| log_audit_event if success }
+      end
+
+      private
+
+      def log_audit_event
+        EE::Audit::GroupChangesAuditor.new(current_user, group).execute
+      end
+    end
+  end
+end

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

+module EE
+  module Projects
+    module GroupLinks
+      module CreateService
+        def execute(group)
+          raise NotImplementedError unless defined?(super)
+
+          super.tap { |link| log_audit_event(link) if link && link&.persisted? }
+        end
+
+        private
+
+        def log_audit_event(group_link)
+          ::AuditEventService.new(
+            current_user,
+            group_link.group,
+            action: :create
+          ).for_project_group_link(group_link).security_event
+        end
+      end
+    end
+  end
+end

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

+module EE
+  module Projects
+    module GroupLinks
+      module DestroyService
+        def execute(group_link)
+          raise NotImplementedError unless defined?(super)
+
+          super.tap { |link| log_audit_event(link) if link && !link&.persisted? }
+        end
+
+        private
+
+        def log_audit_event(group_link)
+          ::AuditEventService.new(
+            current_user,
+            group_link.group,
+            action: :destroy
+          ).for_project_group_link(group_link).security_event
+        end
+      end
+    end
+  end
+end

lib/ee/audit/group_changes_auditor.rb

+module EE
+  module Audit
+    class GroupChangesAuditor < ProjectChangesAuditor
+      def execute
+        audit_changes(:visibility_level, as: 'visibility', model: model)
+      end
+    end
+  end
+end

spec/controllers/groups/group_members_controller_spec.rb

@@ -50,6 +50,14 @@ describe Groups::GroupMembersController do
         expect(group.users).to include group_user
       end
 
+      it 'creates an audit event' do
+        expect do
+          post :create, group_id: group,
+                        user_ids: group_user.id,
+                        access_level: Gitlab::Access::GUEST
+        end.to change(AuditEvent, :count).by(1)
+      end
+
       it 'adds no user to members' do
         post :create, group_id: group,
                       user_ids: '',
@@ -147,6 +155,10 @@ describe Groups::GroupMembersController do
           expect(response).to have_gitlab_http_status(200)
           expect(json_response['notice']).to eq "You left the \"#{group.name}\" group."
         end
+
+        it 'creates an audit event' do
+          expect { delete :leave, group_id: group }.to change(AuditEvent, :count).by(1)
+        end
       end
 
       context 'and is an owner' do
@@ -159,6 +171,10 @@ describe Groups::GroupMembersController do
 
           expect(response).to have_gitlab_http_status(403)
         end
+
+        it 'does not create an audit event' do
+          expect { delete :leave, group_id: group }.not_to change(AuditEvent, :count)
+        end
       end
 
       context 'and is a requester' do
@@ -174,6 +190,10 @@ describe Groups::GroupMembersController do
           expect(group.requesters).to be_empty
           expect(group.users).not_to include user
         end
+
+        it 'creates an audit event' do
+          expect { delete :leave, group_id: group }.to change(AuditEvent, :count).by(1)
+        end
       end
     end
   end

spec/ee/spec/services/ee/groups/create_service_spec.rb

+require 'spec_helper'
+
+describe Groups::CreateService, '#execute' do
+  let!(:user) { create :user }
+  let!(:opts) do
+    {
+      name: 'GitLab',
+      path: 'group_path',
+      visibility_level: Gitlab::VisibilityLevel::PUBLIC
+    }
+  end
+
+  context 'audit events' do
+    include_examples 'audit event logging' do
+      let(:operation) { create_group(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: 'Group',
+           details: {
+             add: 'group',
+             author_name: user.name,
+             target_id: @resource.full_path,
+             target_type: 'Group',
+             target_details: @resource.full_path
+           }
+         }
+      end
+    end
+  end
+
+  def create_group(user, opts)
+    described_class.new(user, opts).execute
+  end
+end

spec/ee/spec/services/ee/groups/destroy_service_spec.rb

+require 'spec_helper'
+
+describe Groups::DestroyService do
+  let!(:user) { create(:user) }
+  let!(:group) { create(:group) }
+
+  subject { described_class.new(group, user, {}) }
+
+  context 'audit events' do
+    include_examples 'audit event logging' do
+      let(:operation) { subject.execute }
+      let(:fail_condition!) do
+        expect_any_instance_of(Group)
+          .to receive(:really_destroy!).and_return(group)
+      end
+      let(:attributes) do
+        {
+           author_id: user.id,
+           entity_id: group.id,
+           entity_type: 'Group',
+           details: {
+             remove: 'group',
+             author_name: user.name,
+             target_id: group.full_path,
+             target_type: 'Group',
+             target_details: group.full_path
+           }
+         }
+      end
+    end
+  end
+end

spec/ee/spec/services/ee/groups/update_service_spec.rb

+require 'spec_helper'
+
+describe Groups::UpdateService, '#execute' do
+  let!(:user) { create(:user) }
+  let!(:group) { create(:group, :public) }
+
+  context 'audit events' do
+    let(:audit_event_params) do
+      {
+        author_id: user.id,
+        entity_id: group.id,
+        entity_type: 'Group',
+        details: {
+          author_name: user.name,
+          target_id: group.id,
+          target_type: 'Group',
+          target_details: group.full_path
+        }
+      }
+    end
+
+    context '#visibility' do
+      before do
+        group.add_owner(user)
+      end
+
+      include_examples 'audit event logging' do
+        let(:operation) do
+          update_group(group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        end
+        let(:fail_condition!) do
+          allow(group).to receive(:save).and_return(false)
+        end
+
+        let(:attributes) do
+          audit_event_params.tap do |param|
+            param[:details].merge!(
+              change: 'visibility',
+              from: 'Public',
+              to: 'Private'
+            )
+          end
+        end
+      end
+    end
+  end
+
+  def update_group(group, user, opts)
+    Groups::UpdateService.new(group, user, opts).execute
+  end
+end

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

+require 'spec_helper'
+
+describe Projects::GroupLinks::CreateService, '#execute' do
+  let!(:user) { create :user }
+  let!(:project) { create :project }
+  let!(:group) { create(:group, visibility_level: 0) }
+  let(:opts) do
+    {
+      link_group_access: '30',
+      expires_at: nil
+    }
+  end
+
+  context 'audit events' do
+    include_examples 'audit event logging' do
+      let(:operation) { create_group_link(user, project, group, opts) }
+      let(:fail_condition!) do
+        create(:project_group_link, project: project, group: group)
+      end
+      let(:attributes) do
+        {
+           author_id: user.id,
+           entity_id: group.id,
+           entity_type: 'Group',
+           details: {
+             add: 'project_access',
+             as: 'Developer',
+             author_name: user.name,
+             target_id: project.id,
+             target_type: 'Project',
+             target_details: project.full_path
+           }
+         }
+      end
+    end
+  end
+
+  def create_group_link(user, project, group, opts)
+    described_class.new(project, user, opts).execute(group)
+  end
+end

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

+require 'spec_helper'
+
+describe Projects::GroupLinks::DestroyService do
+  let!(:user) { create(:user) }
+  let!(:group) { create(:group) }
+  let!(:project) { create(:project) }
+  let!(:group_link) { create(:project_group_link, project: project, group: group) }
+
+  subject { described_class.new(project, user, {}) }
+
+  context 'audit events' do
+    include_examples 'audit event logging' do
+      let(:operation) { subject.execute(group_link) }
+      let(:fail_condition!) do
+        expect_any_instance_of(ProjectGroupLink)
+          .to receive(:destroy).and_return(group_link)
+      end
+      let(:attributes) do
+        {
+           author_id: user.id,
+           entity_id: group.id,
+           entity_type: 'Group',
+           details: {
+             remove: 'project_access',
+             author_name: user.name,
+             target_id: project.id,
+             target_type: 'Project',
+             target_details: project.full_path
+           }
+         }
+      end
+    end
+  end
+end

spec/lib/ee/audit/group_changes_auditor_spec.rb

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