Merge branch '217442-refactor-ip-address-in-auditevent' into 'master'

Establish parallel persistence for AuditEvent IP addresses

See merge request gitlab-org/gitlab!34065

db/migrate/20200605093113_add_ip_address_to_audit_events.rb

+# frozen_string_literal: true
+
+class AddIpAddressToAuditEvents < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :audit_events, :ip_address, :inet
+  end
+end

db/structure.sql

@@ -697,7 +697,8 @@ CREATE TABLE public.audit_events (
     entity_type character varying NOT NULL,
     details text,
     created_at timestamp without time zone,
-    updated_at timestamp without time zone
+    updated_at timestamp without time zone,
+    ip_address inet
 );
 
 CREATE SEQUENCE public.audit_events_id_seq
@@ -13982,6 +13983,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200604174544
 20200604174558
 20200605003204
+20200605093113
 20200608072931
 20200608075553
 20200608214008

ee/app/presenters/audit_event_presenter.rb

@@ -18,7 +18,7 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
   end
 
   def ip_address
-    details[:ip_address]
+    audit_event.ip_address || details[:ip_address]
   end
 
   def details

ee/app/services/ee/audit_event_service.rb

@@ -145,7 +145,8 @@ module EE
         author_id: @author.id,
         entity_id: @entity.respond_to?(:id) ? @entity.id : -1,
         entity_type: 'User',
-        details: @details
+        details: @details,
+        ip_address: ip_address
       )
     end
 
@@ -213,7 +214,7 @@ module EE
 
     override :base_payload
     def base_payload
-      {
+      payload = {
         author_id: @author.id,
         # `@author.respond_to?(:id)` is to support cases where we need to log events
         # that could take place even when a user is unathenticated, Eg: downloading a public repo.
@@ -221,6 +222,9 @@ module EE
         entity_id: @entity.id,
         entity_type: @entity.class.name
       }
+
+      payload[:ip_address] = ip_address if admin_audit_log_enabled?
+      payload
     end
 
     def for_custom_model(model, key_title)

ee/app/services/ee/audit_events/protected_branch_audit_event_service.rb

@@ -14,7 +14,8 @@ module EE
           target_type: protected_branch.class.name,
           target_details: protected_branch.name,
           push_access_levels: push_access_levels,
-          merge_access_levels: merge_access_levels
+          merge_access_levels: merge_access_levels,
+          ip_address: author.current_sign_in_ip
         })
       end
     end

ee/spec/factories/audit_events.rb

@@ -8,6 +8,7 @@ FactoryBot.define do
 
     entity_type { 'User' }
     entity_id   { target_user.id }
+    ip_address { IPAddr.new '127.0.0.1' }
     details do
       {
         change: 'email address',
@@ -27,6 +28,7 @@ FactoryBot.define do
 
       entity_type { 'Project' }
       entity_id   { target_project.id }
+      ip_address { IPAddr.new '127.0.0.1' }
       details do
         {
           change: 'packges_enabled',
@@ -47,6 +49,7 @@ FactoryBot.define do
 
       entity_type { 'Group' }
       entity_id   { target_group.id }
+      ip_address { IPAddr.new '127.0.0.1' }
       details do
         {
           change: 'project_creation_level',

ee/spec/presenters/audit_event_presenter_spec.rb

@@ -16,7 +16,9 @@ RSpec.describe AuditEventPresenter do
     }
   end
 
-  let(:audit_event) { create(:audit_event, details: details) }
+  let(:audit_event) do
+    create(:audit_event, ip_address: '10.2.1.1', details: details)
+  end
 
   subject(:presenter) do
     described_class.new(audit_event)
@@ -100,8 +102,16 @@ RSpec.describe AuditEventPresenter do
     expect(presenter.target).to eq(details[:target_details])
   end
 
-  it 'exposes the ip address' do
-    expect(presenter.ip_address).to eq(details[:ip_address])
+  context 'exposes the ip address' do
+    it 'exposes the database value by default' do
+      expect(presenter.ip_address).to eq('10.2.1.1')
+    end
+
+    it 'falls back to the details hash' do
+      audit_event.update(ip_address:  nil)
+
+      expect(presenter.ip_address).to eq('127.0.0.1')
+    end
   end
 
   context 'exposes the object' do

ee/spec/services/audit_event_service_spec.rb

@@ -64,11 +64,17 @@ RSpec.describe AuditEventService do
         expect(event[:details][:entity_path]).to eq(project.full_path)
       end
 
-      it 'has the IP address' do
+      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
@@ -131,6 +137,7 @@ RSpec.describe AuditEventService do
         it 'defaults to the IP address in the details hash' 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')
         end
       end
@@ -139,6 +146,7 @@ RSpec.describe AuditEventService do
         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)
         end
       end
@@ -151,6 +159,7 @@ RSpec.describe AuditEventService 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')
         end
 
         it 'has the impersonator name' do
@@ -283,6 +292,7 @@ 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)
       end
     end
@@ -293,6 +303,7 @@ RSpec.describe AuditEventService do
       end
 
       it 'does not have the ip_address' do
+        expect(event.ip_address).to be_nil
         expect(event.details).not_to have_key(:ip_address)
       end
     end

ee/spec/services/ee/audit_events/protected_branch_audit_event_service_spec.rb

@@ -3,11 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe EE::AuditEvents::ProtectedBranchAuditEventService do
-  let(:protected_branch) { create(:protected_branch, :no_one_can_push) }
   let(:merge_level) { 'Maintainers' }
   let(:push_level) { 'No one' }
-  let(:entity) { protected_branch.project }
-  let(:author) { entity.owner }
+  let(:author) { create(:user, :with_sign_ins) }
+  let(:entity) { create(:project, creator: author) }
+  let(:protected_branch) { create(:protected_branch, :no_one_can_push, project: entity) }
   let(:logger) { instance_double(Gitlab::AuditJsonLogger) }
 
   describe '#security_event' do
@@ -15,6 +15,10 @@ RSpec.describe EE::AuditEvents::ProtectedBranchAuditEventService do
       context "when a protected_branch is #{action}" do
         let(:service) { described_class.new(author, protected_branch, action) }
 
+        before do
+          stub_licensed_features(admin_audit_log: true)
+        end
+
         it 'creates an event and logs to a file with the provided details' do
           expect(service).to receive(:file_logger).and_return(logger)
 
@@ -22,12 +26,14 @@ RSpec.describe EE::AuditEvents::ProtectedBranchAuditEventService do
                                                 author_name: author.name,
                                                 entity_id: entity.id,
                                                 entity_type: 'Project',
+                                                entity_path: entity.full_path,
                                                 merge_access_levels: [merge_level],
                                                 push_access_levels: [push_level],
                                                 target_details: protected_branch.name,
                                                 target_id: protected_branch.id,
                                                 target_type: 'ProtectedBranch',
-                                                action => 'protected_branch')
+                                                action => 'protected_branch',
+                                                ip_address: '127.0.0.1')
 
           expect { service.security_event }.to change(SecurityEvent, :count).by(1)
 
@@ -36,14 +42,17 @@ RSpec.describe EE::AuditEvents::ProtectedBranchAuditEventService do
           expect(security_event.details).to eq(action => 'protected_branch',
                                                author_name: author.name,
                                                target_id:  protected_branch.id,
+                                               entity_path: entity.full_path,
                                                target_type: 'ProtectedBranch',
                                                target_details: protected_branch.name,
                                                push_access_levels: [push_level],
-                                               merge_access_levels: [merge_level])
+                                               merge_access_levels: [merge_level],
+                                               ip_address: '127.0.0.1')
 
           expect(security_event.author_id).to eq(author.id)
           expect(security_event.entity_id).to eq(entity.id)
           expect(security_event.entity_type).to eq('Project')
+          expect(security_event.ip_address).to eq('127.0.0.1')
         end
       end
     end

ee/spec/services/ee/audit_events/repository_push_audit_event_service_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe EE::AuditEvents::RepositoryPushAuditEventService do
-  let(:user) { create(:user) }
+  let(:user) { create(:user, :with_sign_ins) }
   let(:entity) { create(:project) }
   let(:entity_type) { 'Project' }
   let(:target_ref) { 'refs/heads/master' }
@@ -12,6 +12,10 @@ RSpec.describe EE::AuditEvents::RepositoryPushAuditEventService do
   let(:service) { described_class.new(user, entity, target_ref, from, to) }
 
   describe '#attributes' do
+    before do
+      stub_licensed_features(admin_audit_log: true)
+    end
+
     let(:timestamp) { Time.zone.local(2019, 10, 10) }
     let(:attrs) do
       {
@@ -21,6 +25,7 @@ RSpec.describe EE::AuditEvents::RepositoryPushAuditEventService do
         type: 'SecurityEvent',
         created_at: timestamp,
         updated_at: timestamp,
+        ip_address: '127.0.0.1',
         details: {
           updated_ref: updated_ref,
           author_name: user.name,

spec/services/audit_event_service_spec.rb

@@ -4,12 +4,16 @@ require 'spec_helper'
 
 describe AuditEventService do
   let(:project) { create(:project) }
-  let(:user) { create(:user) }
+  let(:user) { create(:user, :with_sign_ins) }
   let(:project_member) { create(:project_member, user: user) }
   let(:service) { described_class.new(user, project, { action: :destroy }) }
   let(:logger) { instance_double(Gitlab::AuditJsonLogger) }
 
   describe '#security_event' do
+    before do
+      stub_licensed_features(admin_audit_log: false)
+    end
+
     it 'creates an event and logs to a file' do
       expect(service).to receive(:file_logger).and_return(logger)
       expect(logger).to receive(:info).with(author_id: user.id,