From 327eed00767a08217c09addeddfed97c9b91c95f Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 21 Jun 2022 15:16:22 +0200 Subject: [PATCH] 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 | 9 ++++++- .../auth/sessions_controller_spec.rb | 26 +++++++++++++++++++ spec/fabricators/login_activity_fabricator.rb | 10 +++---- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index c4c8151e333..f9a55eb4bd0 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/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) diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 1b8fd0b7b0a..d3db7aa1ab2 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/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 } } diff --git a/spec/fabricators/login_activity_fabricator.rb b/spec/fabricators/login_activity_fabricator.rb index 931d3082ccf..686fd6483d9 100644 --- a/spec/fabricators/login_activity_fabricator.rb +++ b/spec/fabricators/login_activity_fabricator.rb @@ -1,8 +1,8 @@ Fabricator(:login_activity) do user - strategy 'password' - success true - failure_reason nil - ip { Faker::Internet.ip_v4_address } - user_agent { Faker::Internet.user_agent } + authentication_method 'password' + success true + failure_reason nil + ip { Faker::Internet.ip_v4_address } + user_agent { Faker::Internet.user_agent } end