Fix reviving revoked sessions and invalidating login (#16943)

Up until now, we have used Devise's Rememberable mechanism to re-log users
after the end of their browser sessions. This mechanism relies on a signed
cookie containing a token. That token was stored on the user's record,
meaning it was shared across all logged in browsers, meaning truly revoking
a browser's ability to auto-log-in involves revoking the token itself, and
revoking access from *all* logged-in browsers.

We had a session mechanism that dynamically checks whether a user's session
has been disabled, and would log out the user if so. However, this would only
clear a session being actively used, and a new one could be respawned with
the `remember_user_token` cookie.

In practice, this caused two issues:
- sessions could be revived after being closed from /auth/edit (security issue)
- auto-log-in would be disabled for *all* browsers after logging out from one
  of them

This PR removes the `remember_token` mechanism and treats the `_session_id`
cookie/token as a browser-specific `remember_token`, fixing both issues.

diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb
index 5db2668f72dde59c537e33a17697ff083298120f..2996c0431b9fa22b1f9e69e48716921503c9582a 100644 (file)
--- a/app/controllers/auth/passwords_controller.rb
+++ b/app/controllers/auth/passwords_controller.rb
@@ -10,7 +10,6 @@ class Auth::PasswordsController < Devise::PasswordsController
     super do |resource|
       if resource.errors.empty?
         resource.session_activations.destroy_all
-        resource.forget_me!
       end
     end
   end

diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb
index a3114ab2532aad6a9b201b5f0ee6ad8f15163bf3..3c1730f25f4bf3af9de2a41523d9a9148c7b7416 100644 (file)
--- a/app/controllers/auth/registrations_controller.rb
+++ b/app/controllers/auth/registrations_controller.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 class Auth::RegistrationsController < Devise::RegistrationsController
-  include Devise::Controllers::Rememberable
   include RegistrationSpamConcern
 
   layout :determine_layout
@@ -30,8 +29,6 @@ class Auth::RegistrationsController < Devise::RegistrationsController
     super do |resource|
       if resource.saved_change_to_encrypted_password?
         resource.clear_other_sessions(current_session.session_id)
-        resource.forget_me!
-        remember_me(resource)
       end
     end
   end

diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb
index d48abb707764fcbb9dd213c4fa2f620969f388d2..0184bfb52ede2bf7ea4bf32e8d08f5e5c9793d38 100644 (file)
--- a/app/controllers/auth/sessions_controller.rb
+++ b/app/controllers/auth/sessions_controller.rb
@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 class Auth::SessionsController < Devise::SessionsController
-  include Devise::Controllers::Rememberable
-
   layout 'auth'
 
   skip_before_action :require_no_authentication, only: [:create]
@@ -150,7 +148,6 @@ class Auth::SessionsController < Devise::SessionsController
     clear_attempt_from_session
 
     user.update_sign_in!(request, new_sign_in: true)
-    remember_me(user)
     sign_in(user)
     flash.delete(:notice)
 

diff --git a/app/models/user.rb b/app/models/user.rb
index 4059c96b56993d0aee1386b4beb66704e7441bd7..c4dec48133989b103d9acd009c48f6627797a171 100644 (file)
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -64,7 +64,7 @@ class User < ApplicationRecord
   devise :two_factor_backupable,
          otp_number_of_backup_codes: 10
 
-  devise :registerable, :recoverable, :rememberable, :validatable,
+  devise :registerable, :recoverable, :validatable,
          :confirmable
 
   include Omniauthable

diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb
index ef612e177de2fc7557653944f613717064eecc72..5232e6cfda9d32af076dd9bb5c93733ddfb6726a 100644 (file)
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@@ -1,3 +1,5 @@
+require 'devise/strategies/authenticatable'
+
 Warden::Manager.after_set_user except: :fetch do |user, warden|
   if user.session_active?(warden.cookies.signed['_session_id'] || warden.raw_session['auth_id'])
     session_id = warden.cookies.signed['_session_id'] || warden.raw_session['auth_id']
@@ -72,17 +74,48 @@ module Devise
   mattr_accessor :ldap_uid_conversion_replace
   @@ldap_uid_conversion_replace = nil
 
-  class Strategies::PamAuthenticatable
-    def valid?
-      super && ::Devise.pam_authentication
+  module Strategies
+    class PamAuthenticatable
+      def valid?
+        super && ::Devise.pam_authentication
+      end
+    end
+
+    class SessionActivationRememberable < Authenticatable
+      def valid?
+        @session_cookie = nil
+        session_cookie.present?
+      end
+
+      def authenticate!
+        resource = SessionActivation.find_by(session_id: session_cookie)&.user
+
+        unless resource
+          cookies.delete('_session_id')
+          return pass
+        end
+
+        if validate(resource)
+          success!(resource)
+        end
+      end
+
+      private
+
+      def session_cookie
+        @session_cookie ||= cookies.signed['_session_id']
+      end
     end
   end
 end
 
+Warden::Strategies.add(:session_activation_rememberable, Devise::Strategies::SessionActivationRememberable)
+
 Devise.setup do |config|
   config.warden do |manager|
     manager.default_strategies(scope: :user).unshift :two_factor_ldap_authenticatable if Devise.ldap_authentication
     manager.default_strategies(scope: :user).unshift :two_factor_pam_authenticatable  if Devise.pam_authentication
+    manager.default_strategies(scope: :user).unshift :session_activation_rememberable
     manager.default_strategies(scope: :user).unshift :two_factor_authenticatable
     manager.default_strategies(scope: :user).unshift :two_factor_backupable
   end