Commit 4c798c38 authored by Max Woolf's avatar Max Woolf

Establish parallel persistance for IP addresses

Establishes a new ip_address attribute
for AuditEvents and writes IP address
to this new column as well as persisting
to the existing details hash.

This intention being to remove this parallel
persistance after a milestone once we're
happy that it is working correctly.
parent b6ca6057
# frozen_string_literal: true
class AddIpAddressToAuditEvents < ActiveRecord::Migration[6.0]
DOWNTIME = false
def change
add_column :audit_events, :ip_address, :inet
end
end
......@@ -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
......@@ -13981,6 +13982,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200604174544
20200604174558
20200605003204
20200605093113
20200608072931
20200608075553
20200608214008
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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',
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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,
......
......@@ -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,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment