From 8a9b02550eed60ce86e8872ae71f834143690e3a Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Fri, 15 May 2020 17:13:21 +0100
Subject: [PATCH] Log AuditEvent when GroupSAML adds member to group

Brings GroupSaml::MembershipUpdater in line with
EE::Members::CreateService. Without this the audit log for group
membership is missing changes that come from SAML.
---
 .../jej-audit-log-group-saml-membership.yml          |  5 +++++
 ee/lib/gitlab/auth/group_saml/membership_updater.rb  | 12 +++++++++++-
 .../groups/omniauth_callbacks_controller_spec.rb     | 12 ++++++++++++
 .../auth/group_saml/membership_updater_spec.rb       |  8 ++++++++
 4 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100644 ee/changelogs/unreleased/jej-audit-log-group-saml-membership.yml

diff --git a/ee/changelogs/unreleased/jej-audit-log-group-saml-membership.yml b/ee/changelogs/unreleased/jej-audit-log-group-saml-membership.yml
new file mode 100644
index 00000000000..0473d3d0724
--- /dev/null
+++ b/ee/changelogs/unreleased/jej-audit-log-group-saml-membership.yml
@@ -0,0 +1,5 @@
+---
+title: Log Audit Event when Group SAML adds a user to a group
+merge_request: 32333
+author:
+type: changed
diff --git a/ee/lib/gitlab/auth/group_saml/membership_updater.rb b/ee/lib/gitlab/auth/group_saml/membership_updater.rb
index b41f0d2a668..feba5f8ac39 100644
--- a/ee/lib/gitlab/auth/group_saml/membership_updater.rb
+++ b/ee/lib/gitlab/auth/group_saml/membership_updater.rb
@@ -16,7 +16,9 @@ module Gitlab
         def execute
           return if group.member?(@user)
 
-          group.add_user(@user, default_membership_level)
+          member = group.add_user(@user, default_membership_level)
+
+          log_audit_event(member: member)
         end
 
         private
@@ -24,6 +26,14 @@ module Gitlab
         def default_membership_level
           :guest
         end
+
+        def log_audit_event(member:)
+          ::AuditEventService.new(
+            @user,
+            member.source,
+            action: :create
+          ).for_member(member).security_event
+        end
       end
     end
   end
diff --git a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
index b594927c9f1..334bd13ad9d 100644
--- a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
+++ b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
@@ -71,6 +71,7 @@ describe Groups::OmniauthCallbacksController do
       it 'logs group audit event for authentication' do
         audit_event_service = instance_double(AuditEventService)
 
+        allow(AuditEventService).to receive(:new).and_call_original
         expect(AuditEventService).to receive(:new).with(user, group, with: provider)
           .and_return(audit_event_service)
         expect(audit_event_service).to receive_message_chain(:for_authentication, :security_event)
@@ -160,6 +161,17 @@ describe Groups::OmniauthCallbacksController do
           expect(flash[:notice]).to match(/SAML for .* was added/)
         end
 
+        it 'logs group audit event for being added to the group' do
+          audit_event_service = instance_double(AuditEventService)
+
+          expect(AuditEventService).to receive(:new).ordered.and_call_original
+          expect(AuditEventService).to receive(:new).ordered.with(user, group, action: :create)
+            .and_return(audit_event_service)
+          expect(audit_event_service).to receive_message_chain(:for_member, :security_event)
+
+          post provider, params: { group_id: group }
+        end
+
         context 'with IdP initiated request' do
           let(:last_request_id) { '99999' }
 
diff --git a/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb
index 75b74eaa193..a96382e6253 100644
--- a/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb
+++ b/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb
@@ -28,4 +28,12 @@ describe Gitlab::Auth::GroupSaml::MembershipUpdater do
 
     expect(group.members.pluck(:access_level)).to eq([Gitlab::Access::MAINTAINER])
   end
+
+  it "logs an audit event" do
+    expect do
+      described_class.new(user, saml_provider).execute
+    end.to change { AuditEvent.by_entity('Group', group).count }.by(1)
+
+    expect(AuditEvent.last.details).to include(add: 'user_access', target_details: user.name, as: 'Guest')
+  end
 end
-- 
2.30.9