Fix 2FA/sign-in token sessions being valid after password change (#14802)

If someone tries logging in to an account and is prompted for a 2FA
code or sign-in token, even if the account's password or e-mail is
updated in the meantime, the session will show the prompt and allow
the login process to complete with a valid 2FA code or sign-in token

diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb
index e962c4e97f6042c228a745259c4ec8dc5b61452c..fe199e689a0a9cca276391eb798fce5a1a60af20 100644 (file)
--- a/app/controllers/api/base_controller.rb
+++ b/app/controllers/api/base_controller.rb
@@ -103,7 +103,7 @@ class Api::BaseController < ApplicationController
     elsif !current_user.functional?
       render json: { error: 'Your login is currently disabled' }, status: 403
     else
-      set_user_activity
+      update_user_sign_in
     end
   end
 

diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb
index c1ea702adc9d86fd08f9be795d8c846d92960776..13d158c67613a8ed9f0ab8b450eff2e31db6d271 100644 (file)
--- a/app/controllers/auth/sessions_controller.rb
+++ b/app/controllers/auth/sessions_controller.rb
@@ -7,6 +7,7 @@ class Auth::SessionsController < Devise::SessionsController
 
   skip_before_action :require_no_authentication, only: [:create]
   skip_before_action :require_functional!
+  skip_before_action :update_user_sign_in
 
   include TwoFactorAuthenticationConcern
   include SignInTokenAuthenticationConcern
@@ -24,6 +25,7 @@ class Auth::SessionsController < Devise::SessionsController
 
   def create
     super do |resource|
+      resource.update_sign_in!(request, new_sign_in: true)
       remember_me(resource)
       flash.delete(:notice)
     end
@@ -57,7 +59,7 @@ class Auth::SessionsController < Devise::SessionsController
 
   def find_user
     if session[:attempt_user_id]
-      User.find(session[:attempt_user_id])
+      User.find_by(id: session[:attempt_user_id])
     else
       user   = User.authenticate_with_ldap(user_params) if Devise.ldap_authentication
       user ||= User.authenticate_with_pam(user_params) if Devise.pam_authentication
@@ -90,6 +92,7 @@ class Auth::SessionsController < Devise::SessionsController
 
   def require_no_authentication
     super
+
     # Delete flash message that isn't entirely useful and may be confusing in
     # most cases because /web doesn't display/clear flash messages.
     flash.delete(:alert) if flash[:alert] == I18n.t('devise.failure.already_authenticated')
@@ -107,13 +110,30 @@ class Auth::SessionsController < Devise::SessionsController
 
   def home_paths(resource)
     paths = [about_path]
+
     if single_user_mode? && resource.is_a?(User)
       paths << short_account_path(username: resource.account)
     end
+
     paths
   end
 
   def continue_after?
     truthy_param?(:continue)
   end
+
+  def restart_session
+    clear_attempt_from_session
+    redirect_to new_user_session_path, alert: I18n.t('devise.failure.timeout')
+  end
+
+  def set_attempt_session(user)
+    session[:attempt_user_id]         = user.id
+    session[:attempt_user_updated_at] = user.updated_at.to_s
+  end
+
+  def clear_attempt_from_session
+    session.delete(:attempt_user_id)
+    session.delete(:attempt_user_updated_at)
+  end
 end

diff --git a/app/controllers/concerns/sign_in_token_authentication_concern.rb b/app/controllers/concerns/sign_in_token_authentication_concern.rb
index 91f813acc3508c06d88de491734290d6a92a6ab2..3c95a4afd2cf664cd465c0a40f2e3b9683bf3f7f 100644 (file)
--- a/app/controllers/concerns/sign_in_token_authentication_concern.rb
+++ b/app/controllers/concerns/sign_in_token_authentication_concern.rb
@@ -18,7 +18,9 @@ module SignInTokenAuthenticationConcern
   def authenticate_with_sign_in_token
     user = self.resource = find_user
 
-    if user_params[:sign_in_token_attempt].present? && session[:attempt_user_id]
+    if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s
+      restart_session
+    elsif user_params.key?(:sign_in_token_attempt) && session[:attempt_user_id]
       authenticate_with_sign_in_token_attempt(user)
     elsif user.present? && user.external_or_valid_password?(user_params[:password])
       prompt_for_sign_in_token(user)
@@ -27,7 +29,7 @@ module SignInTokenAuthenticationConcern
 
   def authenticate_with_sign_in_token_attempt(user)
     if valid_sign_in_token_attempt?(user)
-      session.delete(:attempt_user_id)
+      clear_attempt_from_session
       remember_me(user)
       sign_in(user)
     else
@@ -42,10 +44,10 @@ module SignInTokenAuthenticationConcern
       UserMailer.sign_in_token(user, request.remote_ip, request.user_agent, Time.now.utc.to_s).deliver_later!
     end
 
-    set_locale do
-      session[:attempt_user_id] = user.id
-      @body_classes = 'lighter'
-      render :sign_in_token
-    end
+    set_attempt_session(user)
+
+    @body_classes = 'lighter'
+
+    set_locale { render :sign_in_token }
   end
 end

diff --git a/app/controllers/concerns/two_factor_authentication_concern.rb b/app/controllers/concerns/two_factor_authentication_concern.rb
index 8a2a86a025e3a13d32a8b914d65c6a2987283f0f..4d4ccf49c8ce67d8f9778563f9190335d98b9edd 100644 (file)
--- a/app/controllers/concerns/two_factor_authentication_concern.rb
+++ b/app/controllers/concerns/two_factor_authentication_concern.rb
@@ -37,9 +37,11 @@ module TwoFactorAuthenticationConcern
   def authenticate_with_two_factor
     user = self.resource = find_user
 
-    if user.webauthn_enabled? && user_params[:credential].present? && session[:attempt_user_id]
+    if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s
+      restart_session
+    elsif user.webauthn_enabled? && user_params.key?(:credential) && session[:attempt_user_id]
       authenticate_with_two_factor_via_webauthn(user)
-    elsif user_params[:otp_attempt].present? && session[:attempt_user_id]
+    elsif user_params.key?(:otp_attempt) && session[:attempt_user_id]
       authenticate_with_two_factor_via_otp(user)
     elsif user.present? && user.external_or_valid_password?(user_params[:password])
       prompt_for_two_factor(user)
@@ -50,7 +52,7 @@ module TwoFactorAuthenticationConcern
     webauthn_credential = WebAuthn::Credential.from_get(user_params[:credential])
 
     if valid_webauthn_credential?(user, webauthn_credential)
-      session.delete(:attempt_user_id)
+      clear_attempt_from_session
       remember_me(user)
       sign_in(user)
       render json: { redirect_path: root_path }, status: :ok
@@ -61,7 +63,7 @@ module TwoFactorAuthenticationConcern
 
   def authenticate_with_two_factor_via_otp(user)
     if valid_otp_attempt?(user)
-      session.delete(:attempt_user_id)
+      clear_attempt_from_session
       remember_me(user)
       sign_in(user)
     else
@@ -71,16 +73,18 @@ module TwoFactorAuthenticationConcern
   end
 
   def prompt_for_two_factor(user)
-    set_locale do
-      session[:attempt_user_id] = user.id
-      @body_classes = 'lighter'
-      @webauthn_enabled = user.webauthn_enabled?
-      @scheme_type = if user.webauthn_enabled? && user_params[:otp_attempt].blank?
-                       'webauthn'
-                     else
-                       'totp'
-                     end
-      render :two_factor
+    set_attempt_session(user)
+
+    @body_classes     = 'lighter'
+    @webauthn_enabled = user.webauthn_enabled?
+    @scheme_type      = begin
+      if user.webauthn_enabled? && user_params[:otp_attempt].blank?
+        'webauthn'
+      else
+        'totp'
+      end
     end
+
+    set_locale { render :two_factor }
   end
 end

diff --git a/app/controllers/concerns/user_tracking_concern.rb b/app/controllers/concerns/user_tracking_concern.rb
index be10705fcc1d8b69bfef7ae672ae5b1494513bdb..efda37fae75cb7649b960c1748dc173c81c0a260 100644 (file)
--- a/app/controllers/concerns/user_tracking_concern.rb
+++ b/app/controllers/concerns/user_tracking_concern.rb
@@ -6,14 +6,13 @@ module UserTrackingConcern
   UPDATE_SIGN_IN_HOURS = 24
 
   included do
-    before_action :set_user_activity
+    before_action :update_user_sign_in
   end
 
   private
 
-  def set_user_activity
-    return unless user_needs_sign_in_update?
-    current_user.update_tracked_fields!(request)
+  def update_user_sign_in
+    current_user.update_sign_in!(request) if user_needs_sign_in_update?
   end
 
   def user_needs_sign_in_update?

diff --git a/app/models/user.rb b/app/models/user.rb
index 7c8124fedb9c1ebc496be5507d6685e88775bcb3..94fee999ab3c21a95d67968852bd4e8b472769c1 100644 (file)
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -63,7 +63,7 @@ class User < ApplicationRecord
   devise :two_factor_backupable,
          otp_number_of_backup_codes: 10
 
-  devise :registerable, :recoverable, :rememberable, :trackable, :validatable,
+  devise :registerable, :recoverable, :rememberable, :validatable,
          :confirmable
 
   include Omniauthable
@@ -165,6 +165,24 @@ class User < ApplicationRecord
     prepare_new_user! if new_user && approved?
   end
 
+  def update_sign_in!(request, new_sign_in: false)
+    old_current, new_current = current_sign_in_at, Time.now.utc
+    self.last_sign_in_at     = old_current || new_current
+    self.current_sign_in_at  = new_current
+
+    old_current, new_current = current_sign_in_ip, request.remote_ip
+    self.last_sign_in_ip     = old_current || new_current
+    self.current_sign_in_ip  = new_current
+
+    if new_sign_in
+      self.sign_in_count ||= 0
+      self.sign_in_count  += 1
+    end
+
+    save(validate: false) unless new_record?
+    prepare_returning_user!
+  end
+
   def pending?
     !approved?
   end
@@ -196,11 +214,6 @@ class User < ApplicationRecord
     prepare_new_user!
   end
 
-  def update_tracked_fields!(request)
-    super
-    prepare_returning_user!
-  end
-
   def otp_enabled?
     otp_required_for_login
   end

diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb
index 8ad9e74fcf04ebbcd6f5395bc39f7b4890992f1e..d3a9a11ebbd71b105361aac465399f2213a6ebec 100644 (file)
--- a/spec/controllers/auth/sessions_controller_spec.rb
+++ b/spec/controllers/auth/sessions_controller_spec.rb
@@ -219,7 +219,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
 
         context 'using a valid OTP' do
           before do
-            post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id }
+            post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }
           end
 
           it 'redirects to home' do
@@ -234,7 +234,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
         context 'when the server has an decryption error' do
           before do
             allow_any_instance_of(User).to receive(:validate_and_consume_otp!).and_raise(OpenSSL::Cipher::CipherError)
-            post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id }
+            post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }
           end
 
           it 'shows a login error' do
@@ -248,7 +248,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
 
         context 'using a valid recovery code' do
           before do
-            post :create, params: { user: { otp_attempt: recovery_codes.first } }, session: { attempt_user_id: user.id }
+            post :create, params: { user: { otp_attempt: recovery_codes.first } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }
           end
 
           it 'redirects to home' do
@@ -262,7 +262,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
 
         context 'using an invalid OTP' do
           before do
-            post :create, params: { user: { otp_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id }
+            post :create, params: { user: { otp_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }
           end
 
           it 'shows a login error' do
@@ -334,7 +334,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
           before do
             @controller.session[:webauthn_challenge] = challenge
 
-            post :create, params: { user: { credential: fake_credential } }, session: { attempt_user_id: user.id }
+            post :create, params: { user: { credential: fake_credential } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }
           end
 
           it 'instructs the browser to redirect to home' do
@@ -383,7 +383,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
       context 'using a valid sign in token' do
         before do
           user.generate_sign_in_token && user.save
-          post :create, params: { user: { sign_in_token_attempt: user.sign_in_token } }, session: { attempt_user_id: user.id }
+          post :create, params: { user: { sign_in_token_attempt: user.sign_in_token } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }
         end
 
         it 'redirects to home' do
@@ -397,7 +397,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
 
       context 'using an invalid sign in token' do
         before do
-          post :create, params: { user: { sign_in_token_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id }
+          post :create, params: { user: { sign_in_token_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }
         end
 
         it 'shows a login error' do