From 6da135a493cc039d92bb5925c2a1ef66025623bf Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Sat, 6 Nov 2021 00:13:58 +0100
Subject: [PATCH] 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.
---
app/controllers/auth/passwords_controller.rb | 1 -
.../auth/registrations_controller.rb | 3 --
app/controllers/auth/sessions_controller.rb | 3 --
app/models/user.rb | 2 +-
config/initializers/devise.rb | 39 +++++++++++++++++--
5 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb
index 5db2668f72..2996c0431b 100644
--- 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 a3114ab253..3c1730f25f 100644
--- 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 d48abb7077..0184bfb52e 100644
--- 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 4059c96b56..c4dec48133 100644
--- 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 ef612e177d..5232e6cfda 100644
--- 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
--
GitLab