diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index ab40d8d..8a69343 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -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 diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 3248844..b8bc2ec 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -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? diff --git a/app/models/session.rb b/app/models/session.rb index 6156cd1..934a2a6 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -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 diff --git a/app/models/user.rb b/app/models/user.rb index 0fe24ee..95601f0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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? diff --git a/test/integration/session_security_test.rb b/test/integration/session_security_test.rb index fb7cea3..4f2a71c 100644 --- a/test/integration/session_security_test.rb +++ b/test/integration/session_security_test.rb @@ -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 # ==================== diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a143988..4071c16 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -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)