Merge branch 'ignore-ip-address-input-to-audit-event-service' into 'master'

Drop IP address input when creating audit events

See merge request gitlab-org/gitlab!54187

app/controllers/concerns/authenticates_with_two_factor.rb

@@ -151,14 +151,14 @@ module AuthenticatesWithTwoFactor
 
   def handle_two_factor_failure(user, method, message)
     user.increment_failed_attempts!
-    log_failed_two_factor(user, method, request.remote_ip)
+    log_failed_two_factor(user, method)
 
     Gitlab::AppLogger.info("Failed Login: user=#{user.username} ip=#{request.remote_ip} method=#{method}")
     flash.now[:alert] = message
     prompt_for_two_factor(user)
   end
 
-  def log_failed_two_factor(user, method, ip_address)
+  def log_failed_two_factor(user, method)
     # overridden in EE
   end
 

app/controllers/concerns/authenticates_with_two_factor_for_admin_mode.rb

@@ -98,7 +98,7 @@ module AuthenticatesWithTwoFactorForAdminMode
 
   def admin_handle_two_factor_failure(user, method, message)
     user.increment_failed_attempts!
-    log_failed_two_factor(user, method, request.remote_ip)
+    log_failed_two_factor(user, method)
 
     Gitlab::AppLogger.info("Failed Admin Mode Login: user=#{user.username} ip=#{request.remote_ip} method=#{method}")
     flash.now[:alert] = message

app/services/audit_event_service.rb

@@ -16,7 +16,7 @@ class AuditEventService
     @author = build_author(author)
     @entity = entity
     @details = details
-    @ip_address = resolve_ip_address(@details, @author)
+    @ip_address = resolve_ip_address(@author)
   end
 
   # Builds the @details attribute for authentication
@@ -64,9 +64,8 @@ class AuditEventService
     end
   end
 
-  def resolve_ip_address(details, author)
-    details[:ip_address].presence ||
-      Gitlab::RequestContext.instance.client_ip ||
+  def resolve_ip_address(author)
+    Gitlab::RequestContext.instance.client_ip ||
       author.current_sign_in_ip
   end
 

ee/app/controllers/concerns/ee/authenticates_with_two_factor.rb

@@ -5,11 +5,10 @@ module EE
     extend ::Gitlab::Utils::Override
 
     override :log_failed_two_factor
-    def log_failed_two_factor(user, method, ip_address)
+    def log_failed_two_factor(user, method)
       ::AuditEventService.new(
         user,
         user,
-        ip_address: ip_address,
         with: method
       ).for_failed_login.unauth_security_event
     end

ee/app/controllers/ee/omniauth_callbacks_controller.rb

@@ -21,7 +21,6 @@ module EE
       ::AuditEventService.new(
         author,
         nil,
-        ip_address: request.remote_ip,
         with: provider
       ).for_failed_login.unauth_security_event
     end

ee/app/controllers/ee/sessions_controller.rb

@@ -64,7 +64,7 @@ module EE
     override :log_failed_login
     def log_failed_login
       login = request.filtered_parameters.dig('user', 'login')
-      audit_event_service = ::AuditEventService.new(login, nil, ip_address: request.remote_ip)
+      audit_event_service = ::AuditEventService.new(login, nil)
       audit_event_service.for_failed_login.unauth_security_event
 
       super

ee/app/services/audit_events/build_service.rb

@@ -8,13 +8,13 @@ module AuditEvents
     # @raise [MissingAttributeError] when required attributes are blank
     #
     # @return [BuildService]
-    def initialize(author:, scope:, target:, ip_address:, message:)
-      raise MissingAttributeError if missing_attribute?(author, scope, target, ip_address, message)
+    def initialize(author:, scope:, target:, message:)
+      raise MissingAttributeError if missing_attribute?(author, scope, target, message)
 
       @author = build_author(author)
       @scope = scope
       @target = target
-      @ip_address = ip_address
+      @ip_address = build_ip_address
       @message = build_message(message)
     end
 
@@ -27,7 +27,7 @@ module AuditEvents
 
     private
 
-    def missing_attribute?(author, scope, target, ip_address, message)
+    def missing_attribute?(author, scope, target, message)
       author.blank? || scope.blank? || target.blank? || message.blank?
     end
 
@@ -35,11 +35,11 @@ module AuditEvents
       if License.feature_available?(:admin_audit_log)
         base_payload.merge(
           details: base_details_payload.merge(
-            ip_address: ip_address,
+            ip_address: @ip_address,
             entity_path: @scope.full_path,
             custom_message: @message
           ),
-          ip_address: ip_address
+          ip_address: @ip_address
         )
       else
         base_payload.merge(details: base_details_payload)
@@ -78,8 +78,8 @@ module AuditEvents
       end
     end
 
-    def ip_address
-      @ip_address.presence || @author.current_sign_in_ip
+    def build_ip_address
+      Gitlab::RequestContext.instance.client_ip || @author.current_sign_in_ip
     end
   end
 end

ee/app/services/ee/resource_access_tokens/create_service.rb

@@ -33,8 +33,7 @@ module EE
           target_type: token&.class&.name,
           target_details: token&.user&.name,
           action: :custom,
-          custom_message: message,
-          ip_address: current_user.current_sign_in_ip
+          custom_message: message
         ).security_event
       end
     end

ee/app/services/ee/resource_access_tokens/revoke_service.rb

@@ -26,8 +26,7 @@ module EE
           target_type: token.class.name,
           target_details: token.user.name,
           action: :custom,
-          custom_message: message,
-          ip_address: current_user.current_sign_in_ip
+          custom_message: message
         ).security_event
       end
     end

ee/lib/gitlab/audit/auditor.rb

@@ -10,7 +10,6 @@ module Gitlab
       # @option context [User] :author the user who authors the change
       # @option context [User, Project, Group] :scope the scope which audit event belongs to
       # @option context [Object] :target the target object being audited
-      # @option context [Object] :ip_address the request IP address
       # @option context [String] :message the message describing the action
       #
       # @example Using block (useful when events are emitted deep in the call stack)
@@ -21,7 +20,6 @@ module Gitlab
       #     author: current_user,
       #     scope: project_alpha,
       #     target: merge_approval_rule,
-      #     ip_address: request.remote_ip
       #     message: 'a user has attempted to update an approval rule'
       #   }
       #
@@ -58,7 +56,6 @@ module Gitlab
         @author = @context.fetch(:author)
         @scope = @context.fetch(:scope)
         @target = @context.fetch(:target)
-        @ip_address = @context.fetch(:ip_address, nil)
         @message = @context.fetch(:message, '')
       end
 
@@ -91,7 +88,6 @@ module Gitlab
           author: @author,
           scope: @scope,
           target: @target,
-          ip_address: @ip_address,
           message: message
         ).execute
       end

ee/spec/lib/gitlab/audit/auditor_spec.rb

@@ -7,8 +7,7 @@ RSpec.describe Gitlab::Audit::Auditor do
   let(:author) { build_stubbed(:user) }
   let(:scope) { build_stubbed(:group) }
   let(:target) { build_stubbed(:project) }
-  let(:ip_address) { '192.168.8.8' }
-  let(:context) { { name: name, author: author, scope: scope, target: target, ip_address: ip_address } }
+  let(:context) { { name: name, author: author, scope: scope, target: target } }
   let(:add_message) { 'Added an interesting field from project Gotham' }
   let(:remove_message) { 'Removed an interesting field from project Gotham' }
   let(:operation) do
@@ -85,7 +84,7 @@ RSpec.describe Gitlab::Audit::Auditor do
       let(:audit!) { auditor.audit(context) }
       let(:context) do
         {
-          name: name, author: author, scope: scope, target: target, ip_address: ip_address,
+          name: name, author: author, scope: scope, target: target,
           message: 'Project has been deleted'
         }
       end

ee/spec/services/audit_event_service_spec.rb

@@ -2,16 +2,20 @@
 
 require 'spec_helper'
 
-RSpec.describe AuditEventService do
+RSpec.describe AuditEventService, :request_store do
   let(:project) { build_stubbed(:project) }
   let_it_be(:user) { create(:user, current_sign_in_ip: '192.168.68.104') }
   let_it_be(: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, ip_address: request_ip_address } }
+  let(:details) { { action: :destroy } }
   let(:service) { described_class.new(user, project, details) }
 
+  before do
+    allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(request_ip_address)
+  end
+
   describe '#for_member' do
     let(:event) { service.for_member(project_member).security_event }
     let(:event_details) { event[:details] }
@@ -103,36 +107,59 @@ RSpec.describe AuditEventService do
       end
 
       context 'for an unauthenticated user' do
-        let(:details) { { ip_address: '10.11.12.13' } }
         let(:user) { Gitlab::Audit::UnauthenticatedAuthor.new }
 
-        it 'defaults to the IP address in the details hash' do
-          event = service.security_event
+        context 'when request IP address is present' do
+          it 'has the request IP address' 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(request_ip_address)
+            expect(event.ip_address).to eq(request_ip_address)
+          end
+        end
+
+        context 'when request IP address is not present' do
+          let(:request_ip_address) { nil }
+
+          it 'has the user IP address' do
+            event = service.security_event
+
+            expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
+            expect(event.ip_address).to eq(user.current_sign_in_ip)
+          end
         end
       end
 
       context 'for an authenticated user' do
-        let(:details) { {} }
+        context 'when request IP address is present' do
+          it 'has the request IP address' do
+            event = service.security_event
 
-        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(request_ip_address)
+            expect(event.ip_address).to eq(request_ip_address)
+          end
         end
 
-        it 'tracks exceptions when the event cannot be created' do
-          allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
+        context 'when request IP address is not present' do
+          let(:request_ip_address) { nil }
+
+          it 'has the user IP address' do
+            event = service.security_event
+
+            expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
+            expect(event.ip_address).to eq(user.current_sign_in_ip)
+          end
 
-          expect(Gitlab::ErrorTracking).to(
-            receive(:track_exception)
-              .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuditEvent').and_call_original
-          )
+          it 'tracks exceptions when the event cannot be created' do
+            allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
 
-          service.security_event
+            expect(Gitlab::ErrorTracking).to(
+              receive(:track_exception)
+                .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuditEvent').and_call_original
+            )
+
+            service.security_event
+          end
         end
       end
 
@@ -141,11 +168,24 @@ RSpec.describe AuditEventService do
         let(:impersonator) { build(:user, name: 'Donald Duck', current_sign_in_ip: '192.168.88.88') }
         let(:user) { create(:user, impersonator: impersonator) }
 
-        it 'has the impersonator IP address' do
-          event = service.security_event
+        context 'when request IP address is present' do
+          it 'has the request IP address' do
+            event = service.security_event
 
-          expect(event.details[:ip_address]).to eq('192.168.88.88')
-          expect(event.ip_address).to eq('192.168.88.88')
+            expect(event.details[:ip_address]).to eq(request_ip_address)
+            expect(event.ip_address).to eq(request_ip_address)
+          end
+        end
+
+        context 'when request IP address is not present' do
+          let(:request_ip_address) { nil }
+
+          it 'has the impersonator IP address' do
+            event = service.security_event
+
+            expect(event.details[:ip_address]).to eq(impersonator.current_sign_in_ip)
+            expect(event.ip_address).to eq(impersonator.current_sign_in_ip)
+          end
         end
 
         it 'has the impersonator name' do
@@ -245,7 +285,7 @@ RSpec.describe AuditEventService do
 
   describe '#for_failed_login' do
     let(:author_name) { 'testuser' }
-    let(:service) { described_class.new(author_name, nil, ip_address: request_ip_address) }
+    let(:service) { described_class.new(author_name, nil) }
     let(:event) { service.for_failed_login.unauth_security_event }
 
     before do
@@ -520,9 +560,7 @@ RSpec.describe AuditEventService do
         expect(event.details[:entity_path]).to eq(project.full_path)
       end
 
-      context 'request IP address is provided' do
-        let(:details) { { action: :destroy, ip_address: request_ip_address } }
-
+      context 'request IP address is present' do
         it 'has the IP address in the details hash' do
           expect(event.details[:ip_address]).to eq(request_ip_address)
         end
@@ -532,8 +570,8 @@ RSpec.describe AuditEventService do
         end
       end
 
-      context 'request IP address is not provided' do
-        let(:details) { { action: :destroy } }
+      context 'request IP address is not present' do
+        let(:request_ip_address) { nil }
 
         it 'has the IP address in the details hash' do
           expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)

ee/spec/services/audit_events/build_service_spec.rb

@@ -14,14 +14,17 @@ RSpec.describe AuditEvents::BuildService do
       author: author,
       scope: scope,
       target: target,
-      ip_address: ip_address,
       message: message
     )
   end
 
-  describe '#execute' do
+  describe '#execute', :request_store do
     subject(:event) { service.execute }
 
+    before do
+      allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(ip_address)
+    end
+
     context 'when licensed' do
       before do
         stub_licensed_features(admin_audit_log: true)

ee/spec/support/shared_examples/controllers/sessions_shared_examples.rb

@@ -7,7 +7,7 @@ RSpec.shared_examples 'an auditable failed authentication' do
 
     operation
 
-    expect(AuditEventService).to have_received(:new).with(user, user, ip_address: '0.0.0.0', with: method)
+    expect(AuditEventService).to have_received(:new).with(user, user, with: method)
     expect(audit_event_service).to have_received(:for_failed_login)
     expect(audit_event_service).to have_received(:unauth_security_event)
   end

spec/services/audit_event_service_spec.rb

@@ -79,15 +79,14 @@ RSpec.describe AuditEventService do
       context 'with IP address', :request_store do
         using RSpec::Parameterized::TableSyntax
 
-        where(:from_caller, :from_context, :from_author_sign_in, :output) do
-          '192.168.0.1' | '192.168.0.2' | '192.168.0.3' | '192.168.0.1'
-          nil           | '192.168.0.2' | '192.168.0.3' | '192.168.0.2'
-          nil           | nil           | '192.168.0.3' | '192.168.0.3'
+        where(:from_context, :from_author_sign_in, :output) do
+          '192.168.0.2' | '192.168.0.3' | '192.168.0.2'
+          nil           | '192.168.0.3' | '192.168.0.3'
         end
 
         with_them do
           let(:user) { create(:user, current_sign_in_ip: from_author_sign_in) }
-          let(:audit_service) { described_class.new(user, user, with: 'standard', ip_address: from_caller) }
+          let(:audit_service) { described_class.new(user, user, with: 'standard') }
 
           before do
             allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(from_context)