Merge branch 'dblessing-ip-address-validator' into 'master'

Add validator for IP address/inet columns

See merge request gitlab-org/gitlab!42893

app/models/audit_event.rb

@@ -22,6 +22,7 @@ class AuditEvent < ApplicationRecord
   validates :author_id, presence: true
   validates :entity_id, presence: true
   validates :entity_type, presence: true
+  validates :ip_address, ip_address: true
 
   scope :by_entity_type, -> (entity_type) { where(entity_type: entity_type) }
   scope :by_entity_id, -> (entity_id) { where(entity_id: entity_id) }

app/models/authentication_event.rb

@@ -6,6 +6,7 @@ class AuthenticationEvent < ApplicationRecord
   belongs_to :user, optional: true
 
   validates :provider, :user_name, :result, presence: true
+  validates :ip_address, ip_address: true
 
   enum result: {
     failed: 0,

app/services/audit_event_service.rb

@@ -108,13 +108,25 @@ class AuditEventService
   def log_security_event_to_database
     return if Gitlab::Database.read_only?
 
-    AuditEvent.create(base_payload.merge(details: @details))
+    event = AuditEvent.new(base_payload.merge(details: @details))
+    save_or_track event
+
+    event
   end
 
   def log_authentication_event_to_database
     return unless Gitlab::Database.read_write? && authentication_event?
 
-    AuthenticationEvent.create(authentication_event_payload)
+    event = AuthenticationEvent.new(authentication_event_payload)
+    save_or_track event
+
+    event
+  end
+
+  def save_or_track(event)
+    event.save!
+  rescue => e
+    Gitlab::ErrorTracking.track_exception(e, audit_event_type: event.class.to_s)
   end
 end
 

app/validators/ip_address_validator.rb

+# frozen_string_literal: true
+
+# IpAddressValidator
+#
+# Validates that an IP address is a valid IPv4 or IPv6 address.
+# This should be coupled with a database column of type `inet`
+#
+# When using column type `inet` Rails will silently return the value
+# as `nil` when the value is not valid according to its type cast
+# using `IpAddr`. It's not very user friendly to return an error
+# "IP Address can't be blank" when a value was clearly given but
+# was not the right format. This validator will look at the value
+# before Rails type casts it when the value itself is `nil`.
+# This enables the validator to return a specific and useful error message.
+#
+# This validator allows `nil` values by default since the database
+# allows null values by default. To disallow `nil` values, use in conjunction
+# with `presence: true`.
+#
+# Do not use this validator with `allow_nil: true` or `allow_blank: true`.
+# Because of Rails type casting, when an invalid value is set the attribute
+# will return `nil` and Rails won't run this validator.
+#
+# Example:
+#
+#   class Group < ActiveRecord::Base
+#     validates :ip_address, presence: true, ip_address: true
+#   end
+#
+class IpAddressValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, _)
+    value = record.public_send("#{attribute}_before_type_cast") # rubocop:disable GitlabSecurity/PublicSend
+    return if value.blank?
+
+    IPAddress.parse(value.to_s)
+  rescue ArgumentError
+    record.errors.add(attribute, _('must be a valid IPv4 or IPv6 address'))
+  end
+end

changelogs/unreleased/dblessing-ip-address-validator.yml

+---
+title: Add validator for IP address/inet columns
+merge_request: 42893
+author:
+type: added

ee/spec/services/audit_event_service_spec.rb

@@ -122,12 +122,23 @@ RSpec.describe AuditEventService do
           expect(event.ip_address).to eq(user.current_sign_in_ip)
           expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
         end
+
+        it 'tracks exceptions when the event cannot be created' do
+          allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
+
+          expect(Gitlab::ErrorTracking).to(
+            receive(:track_exception)
+              .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuditEvent').and_call_original
+          )
+
+          service.security_event
+        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) }
+        let(:user) { create(:user, impersonator: impersonator) }
 
         it 'has the impersonator IP address' do
           event = service.security_event

locale/gitlab.pot

@@ -30759,6 +30759,9 @@ msgstr ""
 msgid "must be a root namespace"
 msgstr ""
 
+msgid "must be a valid IPv4 or IPv6 address"
+msgstr ""
+
 msgid "must be greater than start date"
 msgstr ""
 

spec/models/audit_event_spec.rb

@@ -6,6 +6,13 @@ RSpec.describe AuditEvent do
   let_it_be(:audit_event) { create(:project_audit_event) }
   subject { audit_event }
 
+  describe 'validations' do
+    include_examples 'validates IP address' do
+      let(:attribute) { :ip_address }
+      let(:object) { create(:audit_event) }
+    end
+  end
+
   describe '#as_json' do
     context 'ip_address' do
       subject { build(:group_audit_event, ip_address: '192.168.1.1').as_json }

spec/models/authentication_event_spec.rb

@@ -11,6 +11,11 @@ RSpec.describe AuthenticationEvent do
     it { is_expected.to validate_presence_of(:provider) }
     it { is_expected.to validate_presence_of(:user_name) }
     it { is_expected.to validate_presence_of(:result) }
+
+    include_examples 'validates IP address' do
+      let(:attribute) { :ip_address }
+      let(:object) { create(:authentication_event) }
+    end
   end
 
   describe 'scopes' do

spec/services/audit_event_service_spec.rb

@@ -57,7 +57,7 @@ RSpec.describe AuditEventService do
       let(:audit_service) { described_class.new(user, user, with: 'standard') }
 
       it 'creates an authentication event' do
-        expect(AuthenticationEvent).to receive(:create).with(
+        expect(AuthenticationEvent).to receive(:new).with(
           user: user,
           user_name: user.name,
           ip_address: user.current_sign_in_ip,
@@ -67,6 +67,17 @@ RSpec.describe AuditEventService do
 
         audit_service.for_authentication.security_event
       end
+
+      it 'tracks exceptions when the event cannot be created' do
+        allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
+
+        expect(Gitlab::ErrorTracking).to(
+          receive(:track_exception)
+            .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuthenticationEvent').and_call_original
+        )
+
+        audit_service.for_authentication.security_event
+      end
     end
   end
 

spec/support/shared_examples/validators/ip_address_validator_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'validates IP address' do
+  subject { object }
+
+  it { is_expected.to allow_value('192.168.17.43').for(attribute.to_sym) }
+  it { is_expected.to allow_value('2001:0db8:85a3:0000:0000:8a2e:0370:7334').for(attribute.to_sym) }
+
+  it { is_expected.not_to allow_value('invalid IP').for(attribute.to_sym) }
+end

spec/validators/ip_address_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IpAddressValidator do
+  let(:model) do
+    Class.new do
+      include ActiveModel::Model
+      include ActiveModel::Validations
+
+      attr_accessor :ip_address
+      alias_method :ip_address_before_type_cast, :ip_address
+
+      validates :ip_address, ip_address: true
+    end.new
+  end
+
+  using RSpec::Parameterized::TableSyntax
+
+  where(:ip_address, :validity, :errors) do
+    'invalid IP'                     | false | { ip_address: ['must be a valid IPv4 or IPv6 address'] }
+    '192.168.17.43'                  | true  | {}
+    '2001:0db8:85a3::8a2e:0370:7334' | true  | {}
+    nil                              | true  | {}
+    ''                               | true  | {}
+  end
+
+  with_them do
+    before do
+      model.ip_address = ip_address
+      model.validate
+    end
+
+    it { expect(model.valid?).to eq(validity) }
+    it { expect(model.errors.messages).to eq(errors) }
+  end
+end