Merge branch...

Merge branch '7225-no-audit-event-for-adding-a-user-to-a-group-or-project-through-api' into 'master'

Add audit events to the adding members to project or group API endpoint

Closes #7225

See merge request gitlab-org/gitlab!21633

changelogs/unreleased/7225-no-audit-event-for-adding-a-user-to-a-group-or-project-through-api.yml

+---
+title: Add audit events to the adding members to project or group API endpoint
+merge_request: 21633
+author:
+type: changed

ee/lib/ee/api/helpers/members_helpers.rb

@@ -32,6 +32,25 @@ module EE
         def can_view_group_identity?(members_source)
           can?(current_user, :read_group_saml_identity, members_source)
         end
+
+        override :create_member
+        def create_member(current_user, user, source, params)
+          member = source.add_user(user, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
+
+          return false unless member
+
+          log_audit_event(member) if member.persisted? && member.valid?
+
+          member
+        end
+
+        def log_audit_event(member)
+          ::AuditEventService.new(
+            current_user,
+            member.source,
+            action: :create
+          ).for_member(member).security_event
+        end
       end
     end
   end

ee/spec/lib/ee/api/helpers/members_helpers_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe EE::API::Helpers::MembersHelpers do
+  subject(:members_helpers) { Class.new.include(described_class).new }
+
+  before do
+    allow(members_helpers).to receive(:current_user).and_return(create(:user))
+  end
+
+  shared_examples 'creates security_event' do |source_type|
+    context "with :source_type == #{source_type.pluralize}" do
+      it 'creates security_event' do
+        security_event = subject.log_audit_event(member)
+
+        expect(security_event.entity_id).to eq(source.id)
+        expect(security_event.entity_type).to eq(source_type.capitalize)
+        expect(security_event.details.fetch(:target_id)).to eq(member.id)
+      end
+    end
+  end
+
+  describe '#log_audit_event' do
+    it_behaves_like 'creates security_event', 'group' do
+      let(:source) { create(:group) }
+      let(:member) { create(:group_member, :owner, group: source, user: create(:user)) }
+    end
+
+    it_behaves_like 'creates security_event', 'project' do
+      let(:source) { create(:project) }
+      let(:member) { create(:project_member, project: source, user: create(:user)) }
+    end
+  end
+end

ee/spec/requests/api/members_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 describe API::Members do
   let(:group) { create(:group) }
   let(:owner) { create(:user) }
+  let(:project) { create(:project, group: group) }
 
   before do
     group.add_owner(owner)
@@ -74,4 +75,39 @@ describe API::Members do
       end
     end
   end
+
+  shared_examples 'POST /:source_type/:id/members' do |source_type|
+    let(:stranger) { create(:user) }
+    let(:url) { "/#{source_type.pluralize}/#{source.id}/members" }
+
+    context "with :source_type == #{source_type.pluralize}" do
+      it 'creates an audit event while creating a new member' do
+        params = { user_id: stranger.id, access_level: Member::DEVELOPER }
+
+        expect do
+          post api(url, owner), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+        end.to change { AuditEvent.count }.by(1)
+      end
+
+      it 'does not create audit event if creating a new member fails' do
+        params = { user_id: 0, access_level: Member::DEVELOPER }
+
+        expect do
+          post api(url, owner), params: params
+
+          expect(response).to have_gitlab_http_status(404)
+        end.not_to change { AuditEvent.count }
+      end
+    end
+  end
+
+  it_behaves_like 'POST /:source_type/:id/members', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'POST /:source_type/:id/members', 'group' do
+    let(:source) { group }
+  end
 end

lib/api/helpers/members_helpers.rb

@@ -41,6 +41,10 @@ module API
         GroupMembersFinder.new(group).execute
       end
 
+      def create_member(current_user, user, source, params)
+        source.add_user(user, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
+      end
+
       def present_members(members)
         present members, with: Entities::Member, current_user: current_user
       end

lib/api/members.rb

@@ -101,12 +101,12 @@ module API
           user = User.find_by_id(params[:user_id])
           not_found!('User') unless user
 
-          member = source.add_user(user, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
+          member = create_member(current_user, user, source, params)
 
           if !member
             not_allowed! # This currently can only be reached in EE
           elsif member.persisted? && member.valid?
-            present_members member
+            present_members(member)
           else
             render_validation_error!(member)
           end