Enforce account-active status across the auth lifecycle
active? was only checked at the password step of sign-in. A user disabled afterwards could (a) still complete the 2FA step and mint a valid session, and (b) keep using any existing session until natural expiry, because per-request auth only checked session expiry, not user status. Three enforcement points: - Mid-flow guard: verify_totp and webauthn_verify re-check active? before start_new_session_for, clearing the pending session and rejecting if disabled. - Request-time guard: find_session_by_cookie now uses Session.for_active_user, so a session whose user is disabled no longer authenticates (authoritative, catches any disable path including direct DB changes). - Immediate cleanup: User#revoke_sessions_when_deactivated destroys a user's sessions when status changes away from active, so access is revoked everywhere at once rather than on the next request. Tests cover the mid-flow TOTP rejection, request-time rejection of an existing session after disable, session destruction on disable, and that unrelated updates leave sessions intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -31,7 +31,7 @@ module Authentication
|
||||
end
|
||||
|
||||
def find_session_by_cookie
|
||||
Session.active.find_by(id: cookies.signed[:session_id]) if cookies.signed[:session_id]
|
||||
Session.active.for_active_user.find_by(id: cookies.signed[:session_id]) if cookies.signed[:session_id]
|
||||
end
|
||||
|
||||
def request_authentication
|
||||
|
||||
@@ -121,6 +121,16 @@ class SessionsController < ApplicationController
|
||||
return
|
||||
end
|
||||
|
||||
# Re-check account status: active? was verified at the password step, but an
|
||||
# admin may have disabled the account while the user sat on this 2FA screen.
|
||||
# Without this, a disabled account could still mint a valid session here.
|
||||
unless user.active?
|
||||
session.delete(:pending_totp_user_id)
|
||||
session.delete(:pending_remember_me)
|
||||
redirect_to signin_path, alert: "Your account is not active. Please contact an administrator."
|
||||
return
|
||||
end
|
||||
|
||||
remember_me = session.delete(:pending_remember_me) || false
|
||||
|
||||
# Try TOTP verification first (password + TOTP = 2FA)
|
||||
@@ -241,6 +251,14 @@ class SessionsController < ApplicationController
|
||||
return
|
||||
end
|
||||
|
||||
# Re-check account status: an admin may have disabled the account between the
|
||||
# password step and this passkey verification. Reject before creating a session.
|
||||
unless user.active?
|
||||
session.delete(:pending_webauthn_user_id)
|
||||
render json: {error: "Your account is not active."}, status: :unauthorized
|
||||
return
|
||||
end
|
||||
|
||||
# Get the credential and assertion from params
|
||||
credential_data = params[:credential]
|
||||
if credential_data.blank?
|
||||
|
||||
@@ -7,6 +7,9 @@ class Session < ApplicationRecord
|
||||
# Scopes
|
||||
scope :active, -> { where("expires_at > ?", Time.current) }
|
||||
scope :expired, -> { where("expires_at <= ?", Time.current) }
|
||||
# Sessions whose owning user is currently active. Used at request time so a
|
||||
# disabled account cannot continue to authenticate with an existing session.
|
||||
scope :for_active_user, -> { joins(:user).where(users: {status: User.statuses[:active]}) }
|
||||
|
||||
def expired?
|
||||
expires_at.present? && expires_at <= Time.current
|
||||
|
||||
@@ -41,6 +41,11 @@ class User < ApplicationRecord
|
||||
# Enum - automatically creates scopes (User.active, User.disabled, etc.)
|
||||
enum :status, {active: 0, disabled: 1, pending_invitation: 2}
|
||||
|
||||
# When an account stops being active (e.g. an admin disables it), immediately
|
||||
# terminate its sessions so access is revoked everywhere, not just on expiry.
|
||||
# Defence-in-depth: session lookup also filters by active status at request time.
|
||||
after_update_commit :revoke_sessions_when_deactivated
|
||||
|
||||
# Scopes
|
||||
scope :admins, -> { joins(:groups).where(groups: {admin: true}).distinct }
|
||||
|
||||
@@ -246,6 +251,13 @@ class User < ApplicationRecord
|
||||
Group.auto_assign.each { |g| groups << g }
|
||||
end
|
||||
|
||||
def revoke_sessions_when_deactivated
|
||||
return unless saved_change_to_status?
|
||||
return if active?
|
||||
|
||||
sessions.destroy_all
|
||||
end
|
||||
|
||||
def no_reserved_claim_names
|
||||
return if custom_claims.blank?
|
||||
|
||||
|
||||
@@ -1,6 +1,49 @@
|
||||
require "test_helper"
|
||||
|
||||
class SessionSecurityTest < ActionDispatch::IntegrationTest
|
||||
# ====================
|
||||
# ACCOUNT DEACTIVATION TESTS
|
||||
# ====================
|
||||
|
||||
test "TOTP verification rejects a user disabled mid-flow" do
|
||||
user = User.create!(email_address: "midflow_totp@example.com", password: "password123")
|
||||
user.enable_totp!
|
||||
code = ROTP::TOTP.new(user.totp_secret).now
|
||||
|
||||
# Phase A: password step stashes the pending 2FA user
|
||||
post signin_path, params: {email_address: "midflow_totp@example.com", password: "password123"}
|
||||
assert_redirected_to totp_verification_path
|
||||
|
||||
# Admin disables the account while the user is on the 2FA screen
|
||||
user.update!(status: :disabled)
|
||||
|
||||
# Phase B: completing TOTP must NOT create a session
|
||||
post totp_verification_path, params: {code: code}
|
||||
assert_redirected_to signin_path
|
||||
assert_equal 0, user.reload.sessions.count
|
||||
|
||||
user.destroy
|
||||
end
|
||||
|
||||
test "an existing session stops authenticating once the user is disabled" do
|
||||
user = User.create!(email_address: "disabled_session@example.com", password: "password123")
|
||||
sign_in_as(user)
|
||||
|
||||
get root_path
|
||||
assert_response :success
|
||||
|
||||
# Disable bypassing the destroy callback to isolate the request-time lookup
|
||||
# guard (find_session_by_cookie filtering on active users).
|
||||
user.update_column(:status, User.statuses[:disabled])
|
||||
|
||||
get root_path
|
||||
assert_response :redirect
|
||||
assert_match %r{/signin}, response.location
|
||||
|
||||
user.sessions.delete_all
|
||||
user.destroy
|
||||
end
|
||||
|
||||
# ====================
|
||||
# SESSION TIMEOUT TESTS
|
||||
# ====================
|
||||
|
||||
@@ -1,6 +1,27 @@
|
||||
require "test_helper"
|
||||
|
||||
class UserTest < ActiveSupport::TestCase
|
||||
test "disabling a user destroys their active sessions" do
|
||||
user = User.create!(email_address: "disable_sessions@example.com", password: "password123")
|
||||
user.sessions.create!
|
||||
user.sessions.create!
|
||||
assert_equal 2, user.sessions.count
|
||||
|
||||
user.update!(status: :disabled)
|
||||
|
||||
assert_equal 0, user.reload.sessions.count
|
||||
end
|
||||
|
||||
test "reactivating or other updates do not destroy sessions" do
|
||||
user = User.create!(email_address: "keep_sessions@example.com", password: "password123")
|
||||
user.sessions.create!
|
||||
|
||||
# An update that does not change status must leave sessions intact.
|
||||
user.update!(username: "keepsessions")
|
||||
|
||||
assert_equal 1, user.reload.sessions.count
|
||||
end
|
||||
|
||||
test "downcases and strips email_address" do
|
||||
user = User.new(email_address: " DOWNCASED@EXAMPLE.COM ")
|
||||
assert_equal("downcased@example.com", user.email_address)
|
||||
|
||||
Reference in New Issue
Block a user