Fix suspicious sign-in mails never being sent (#18599)

* Add tests

* Fix suspicious sign-in mails never being sent

app/controllers/auth/sessions_controller.rb

@@ -7,11 +7,18 @@ class Auth::SessionsController < Devise::SessionsController
   skip_before_action :require_functional!
   skip_before_action :update_user_sign_in
 
+  prepend_before_action :check_suspicious!, only: [:create]
+
   include TwoFactorAuthenticationConcern
 
   before_action :set_instance_presenter, only: [:new]
   before_action :set_body_classes
 
+  def check_suspicious!
+    user = find_user
+    @login_is_suspicious = suspicious_sign_in?(user) unless user.nil?
+  end
+
   def create
     super do |resource|
       # We only need to call this if this hasn't already been
@@ -142,7 +149,7 @@ class Auth::SessionsController < Devise::SessionsController
       user_agent: request.user_agent
     )
 
-    UserMailer.suspicious_sign_in(user, request.remote_ip, request.user_agent, Time.now.utc).deliver_later! if suspicious_sign_in?(user)
+    UserMailer.suspicious_sign_in(user, request.remote_ip, request.user_agent, Time.now.utc).deliver_later! if @login_is_suspicious
   end
 
   def suspicious_sign_in?(user)

spec/controllers/auth/sessions_controller_spec.rb

@@ -119,6 +119,32 @@ RSpec.describe Auth::SessionsController, type: :controller do
         end
       end
 
+      context 'using a valid password on a previously-used account with a new IP address' do
+        let(:previous_ip) { '1.2.3.4' }
+        let(:current_ip)  { '4.3.2.1' }
+
+        let!(:previous_login) { Fabricate(:login_activity, user: user, ip: previous_ip) }
+
+        before do
+          allow_any_instance_of(ActionDispatch::Request).to receive(:remote_ip).and_return(current_ip)
+          allow(UserMailer).to receive(:suspicious_sign_in).and_return(double('email', 'deliver_later!': nil))
+          user.update(current_sign_in_at: 1.month.ago)
+          post :create, params: { user: { email: user.email, password: user.password } }
+        end
+
+        it 'redirects to home' do
+          expect(response).to redirect_to(root_path)
+        end
+
+        it 'logs the user in' do
+          expect(controller.current_user).to eq user
+        end
+
+        it 'sends a suspicious sign-in mail' do
+          expect(UserMailer).to have_received(:suspicious_sign_in).with(user, current_ip, anything, anything)
+        end
+      end
+
       context 'using email with uppercase letters' do
         before do
           post :create, params: { user: { email: user.email.upcase, password: user.password } }

spec/fabricators/login_activity_fabricator.rb

@@ -1,6 +1,6 @@
 Fabricator(:login_activity) do
   user
-  strategy       'password'
+  authentication_method 'password'
   success               true
   failure_reason        nil
   ip                    { Faker::Internet.ip_v4_address }