Merge branch '217439-use-request-ip-address-audit-events' into 'master'

Use IP address from request on audit events

See merge request gitlab-org/gitlab!35834

app/services/audit_event_service.rb

@@ -16,6 +16,7 @@ class AuditEventService
     @author = build_author(author)
     @entity = entity
     @details = details
+    @ip_address = (@details[:ip_address].presence || @author.current_sign_in_ip)
   end
 
   # Builds the @details attribute for authentication
@@ -49,6 +50,8 @@ class AuditEventService
 
   private
 
+  attr_reader :ip_address
+
   def build_author(author)
     case author
     when User

ee/app/services/ee/audit_event_service.rb

@@ -84,14 +84,12 @@ module EE
     #
     # @return [AuditEventService]
     def for_failed_login
-      ip = @details[:ip_address]
       auth = @details[:with] || 'STANDARD'
 
       @details = {
         failed_login: auth.upcase,
         author_name: @author.name,
-        target_details: @author.name,
-        ip_address: ip
+        target_details: @author.name
       }
 
       self
@@ -125,10 +123,8 @@ module EE
     end
 
     def prepare_security_event
-      if admin_audit_log_enabled?
-        add_security_event_admin_details!
-        add_impersonation_details!
-      end
+      add_security_event_admin_details!
+      add_impersonation_details!
     end
 
     # Creates an event record in DB
@@ -138,16 +134,18 @@ module EE
     def unauth_security_event
       return unless audit_events_enabled?
 
-      @details.delete(:ip_address) unless admin_audit_log_enabled?
-      @details[:entity_path] = @entity&.full_path if admin_audit_log_enabled?
+      add_security_event_admin_details!
 
-      SecurityEvent.create(
+      payload = {
         author_id: @author.id,
         entity_id: @entity.respond_to?(:id) ? @entity.id : -1,
         entity_type: 'User',
-        details: @details,
-        ip_address: ip_address
-      )
+        details: @details
+      }
+
+      payload[:ip_address] = ip_address if admin_audit_log_enabled?
+
+      SecurityEvent.create(payload)
     end
 
     # Builds the @details attribute for user
@@ -248,26 +246,25 @@ module EE
             author_name: @author&.name,
             target_id: key_title,
             target_type: model_class,
-            target_details: key_title,
-            ip_address: @details[:ip_address]
+            target_details: key_title
           }
         end
 
       self
     end
 
-    def ip_address
-      @author.current_sign_in_ip || @details[:ip_address]
-    end
-
     def add_security_event_admin_details!
+      return unless admin_audit_log_enabled?
+
       @details.merge!(
         ip_address: ip_address,
-        entity_path: @entity.full_path
+        entity_path: @entity&.full_path
       )
     end
 
     def add_impersonation_details!
+      return unless admin_audit_log_enabled?
+
       if @author.is_a?(::Gitlab::Audit::ImpersonatedAuthor)
         @details.merge!(impersonated_by: @author.impersonated_by)
       end

ee/changelogs/unreleased/217439-use-request-ip-address-audit-events.yml

+---
+title: Use IP address from request on audit events
+merge_request: 35834
+author:
+type: changed

ee/spec/services/audit_event_service_spec.rb

@@ -6,8 +6,9 @@ RSpec.describe AuditEventService do
   let(:project) { create(:project) }
   let(:user) { create(:user, current_sign_in_ip: '192.168.68.104') }
   let(:project_member) { create(:project_member, user: user, expires_at: 1.day.from_now) }
+  let(:request_ip_address) { '127.0.0.1' }
 
-  let(:details) { { action: :destroy } }
+  let(:details) { { action: :destroy, ip_address: request_ip_address } }
   let(:service) { described_class.new(user, project, details) }
 
   describe '#for_member' do
@@ -58,48 +59,6 @@ RSpec.describe AuditEventService do
         expect(event[:details][:expiry_to]).to eq(1.day.from_now.to_date)
       end
     end
-
-    context 'admin audit log licensed' do
-      before do
-        stub_licensed_features(admin_audit_log: true)
-      end
-
-      it 'has the entity full path' do
-        event = service.for_member(project_member).security_event
-
-        expect(event[:details][:entity_path]).to eq(project.full_path)
-      end
-
-      it 'has the IP address in the details hash' do
-        event = service.for_member(project_member).security_event
-
-        expect(event[:details][:ip_address]).to eq(user.current_sign_in_ip)
-      end
-
-      it 'has the IP address stored in a separate attribute' do
-        event = service.for_member(project_member).security_event
-
-        expect(event.ip_address).to eq(user.current_sign_in_ip)
-      end
-    end
-
-    context 'admin audit log unlicensed' do
-      before do
-        stub_licensed_features(admin_audit_log: false)
-      end
-
-      it 'does not have the entity full path' do
-        event = service.for_member(project_member).security_event
-
-        expect(event[:details]).not_to have_key(:entity_path)
-      end
-
-      it 'does not have the ip_address' do
-        event = service.for_member(project_member).security_event
-
-        expect(event[:details]).not_to have_key(:ip_address)
-      end
-    end
   end
 
   describe '#security_event' do
@@ -144,34 +103,37 @@ RSpec.describe AuditEventService do
           event = service.security_event
 
           expect(event.ip_address).to eq('10.11.12.13')
-          expect(event[:details][:ip_address]).to eq('10.11.12.13')
+          expect(event.details[:ip_address]).to eq('10.11.12.13')
         end
       end
 
       context 'for an authenticated user' do
+        let(:details) { {} }
+
         it 'has the user IP address' do
           event = service.security_event
 
           expect(event.ip_address).to eq(user.current_sign_in_ip)
-          expect(event[:details][:ip_address]).to eq(user.current_sign_in_ip)
+          expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
         end
       end
 
       context 'for an impersonated user' do
+        let(:details) { {} }
         let(:impersonator) { build(:user, name: 'Donald Duck', current_sign_in_ip: '192.168.88.88') }
         let(:user) { build(:user, impersonator: impersonator) }
 
         it 'has the impersonator IP address' do
           event = service.security_event
 
-          expect(event[:details][:ip_address]).to eq('192.168.88.88')
+          expect(event.details[:ip_address]).to eq('192.168.88.88')
           expect(event.ip_address).to eq('192.168.88.88')
         end
 
         it 'has the impersonator name' do
           event = service.security_event
 
-          expect(event[:details][:impersonated_by]).to eq('Donald Duck')
+          expect(event.details[:impersonated_by]).to eq('Donald Duck')
         end
       end
     end
@@ -265,8 +227,7 @@ RSpec.describe AuditEventService do
 
   describe '#for_failed_login' do
     let(:author_name) { 'testuser' }
-    let(:ip_address) { '127.0.0.1' }
-    let(:service) { described_class.new(author_name, nil, ip_address: ip_address) }
+    let(:service) { described_class.new(author_name, nil, ip_address: request_ip_address) }
     let(:event) { service.for_failed_login.unauth_security_event }
 
     before do
@@ -287,7 +248,7 @@ RSpec.describe AuditEventService do
     end
 
     it 'has the right auth method for OAUTH' do
-      oauth_service = described_class.new(author_name, nil, ip_address: ip_address, with: 'ldap')
+      oauth_service = described_class.new(author_name, nil, ip_address: request_ip_address, with: 'ldap')
       event = oauth_service.for_failed_login.unauth_security_event
 
       expect(event.details[:failed_login]).to eq('LDAP')
@@ -299,8 +260,8 @@ RSpec.describe AuditEventService do
       end
 
       it 'has the right IP address' do
-        expect(event.ip_address).to eq(ip_address)
-        expect(event.details[:ip_address]).to eq(ip_address)
+        expect(event.ip_address).to eq(request_ip_address)
+        expect(event.details[:ip_address]).to eq(request_ip_address)
       end
     end
 
@@ -322,8 +283,7 @@ RSpec.describe AuditEventService do
     let(:target_user_full_path) { 'ejohn' }
     let(:user) { instance_spy(User, full_path: target_user_full_path) }
     let(:custom_message) { 'Some strange event has occurred' }
-    let(:ip_address) { '127.0.0.1' }
-    let(:options) { { action: action, custom_message: custom_message, ip_address: ip_address } }
+    let(:options) { { action: action, custom_message: custom_message } }
 
     subject(:service) { described_class.new(current_user, user, options).for_user }
 
@@ -364,8 +324,7 @@ RSpec.describe AuditEventService do
           author_name: author_name,
           target_id: target_user_full_path,
           target_type: 'User',
-          target_details: target_user_full_path,
-          ip_address: ip_address
+          target_details: target_user_full_path
         )
       end
     end
@@ -412,8 +371,28 @@ RSpec.describe AuditEventService do
         expect(event.details[:entity_path]).to eq(project.full_path)
       end
 
-      it 'has the user IP address' do
-        expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
+      context 'request IP address is provided' do
+        let(:details) { { action: :destroy, ip_address: request_ip_address } }
+
+        it 'has the IP address in the details hash' do
+          expect(event.details[:ip_address]).to eq(request_ip_address)
+        end
+
+        it 'has the IP address stored in a separate attribute' do
+          expect(event.ip_address).to eq(request_ip_address)
+        end
+      end
+
+      context 'request IP address is not provided' do
+        let(:details) { { action: :destroy } }
+
+        it 'has the IP address in the details hash' do
+          expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
+        end
+
+        it 'has the IP address stored in a separate attribute' do
+          expect(event.ip_address).to eq(user.current_sign_in_ip)
+        end
       end
     end