Merge 3d558bb855 into 28284b8452

app/controllers/application_controller.rb

@@ -6,6 +6,7 @@ class ApplicationController < ActionController::Base
   protect_from_forgery with: :exception
 
   include Localized
+  include CurrentRequest
   include UserTrackingConcern
   include SessionTrackingConcern
   include CacheConcern

app/controllers/auth/omniauth_callbacks_controller.rb

@@ -41,9 +41,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController
     @user.login_activities.create(
       success: true,
       authentication_method: :omniauth,
-      provider: @provider,
-      ip: request.remote_ip,
-      user_agent: request.user_agent
+      provider: @provider
     )
   end
 

app/controllers/auth/sessions_controller.rb

@@ -141,10 +141,8 @@ class Auth::SessionsController < Devise::SessionsController
     flash.delete(:notice)
 
     user.login_activities.create(
-      request_details.merge(
-        authentication_method: security_measure,
-        success: true
-      )
+      authentication_method: security_measure,
+      success: true
     )
 
     UserMailer.suspicious_sign_in(user, request.remote_ip, request.user_agent, Time.now.utc).deliver_later! if @login_is_suspicious
@@ -156,11 +154,9 @@ class Auth::SessionsController < Devise::SessionsController
 
   def on_authentication_failure(user, security_measure, failure_reason)
     user.login_activities.create(
-      request_details.merge(
-        authentication_method: security_measure,
-        failure_reason: failure_reason,
-        success: false
-      )
+      authentication_method: security_measure,
+      failure_reason: failure_reason,
+      success: false
     )
 
     # Only send a notification email every hour at most
@@ -169,13 +165,6 @@ class Auth::SessionsController < Devise::SessionsController
     UserMailer.failed_2fa(user, request.remote_ip, request.user_agent, Time.now.utc).deliver_later!
   end
 
-  def request_details
-    {
-      ip: request.remote_ip,
-      user_agent: request.user_agent,
-    }
-  end
-
   def second_factor_attempts_key(user)
     "2fa_auth_attempts:#{user.id}:#{Time.now.utc.hour}"
   end

app/controllers/concerns/current_request.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+module CurrentRequest
+  extend ActiveSupport::Concern
+
+  included do
+    before_action do
+      Current.ip_address = request.ip
+      Current.user_agent = request.user_agent
+    end
+  end
+end

app/models/current.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class Current < ActiveSupport::CurrentAttributes
+  attribute :ip_address, :user_agent
+end

app/models/login_activity.rb

@@ -23,4 +23,17 @@ class LoginActivity < ApplicationRecord
   belongs_to :user
 
   validates :authentication_method, inclusion: { in: authentication_methods.keys }
+
+  before_validation :set_ip_address
+  before_validation :set_user_agent
+
+  private
+
+  def set_ip_address
+    self.ip ||= Current.ip_address
+  end
+
+  def set_user_agent
+    self.user_agent ||= Current.user_agent
+  end
 end

spec/controllers/auth/sessions_controller_spec.rb

@@ -107,6 +107,11 @@ RSpec.describe Auth::SessionsController do
         it 'redirects to home and logs the user in' do
           expect { subject }
             .to change(user.login_activities.where(success: true), :count).by(1)
+          expect(user.login_activities.last)
+            .to have_attributes(
+              ip: be_present.and(eq(IPAddr.new(request.env['REMOTE_ADDR']))),
+              user_agent: be_present.and(eq(request.env['HTTP_USER_AGENT']))
+            )
 
           expect(response).to redirect_to(root_path)
 

spec/models/login_activity_spec.rb

@@ -14,4 +14,32 @@ RSpec.describe LoginActivity do
 
     it { is_expected.to define_enum_for(:authentication_method).backed_by_column_of_type(:string) }
   end
+
+  describe 'Callbacks' do
+    describe 'setting IP from request attributes' do
+      subject { Fabricate :login_activity, ip: nil }
+
+      before { Current.ip_address = ip }
+
+      let(:ip) { '123.123.123.123' }
+
+      it 'sets IP address from current attributes' do
+        expect(subject)
+          .to have_attributes(ip: IPAddr.new(ip))
+      end
+    end
+
+    describe 'setting user agent from request attributes' do
+      subject { Fabricate :login_activity, user_agent: nil }
+
+      before { Current.user_agent = user_agent }
+
+      let(:user_agent) { 'Browser 1.2.3' }
+
+      it 'sets user agent from current attributes' do
+        expect(subject)
+          .to have_attributes(user_agent:)
+      end
+    end
+  end
 end