diff --git a/app/controllers/admin/sso_identity_blocks_controller.rb b/app/controllers/admin/sso_identity_blocks_controller.rb new file mode 100644 index 000000000..ce9e6dd41 --- /dev/null +++ b/app/controllers/admin/sso_identity_blocks_controller.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Admin + class SsoIdentityBlocksController < Admin::BaseController + def destroy + block = SsoIdentityBlock.find(params[:id]) + SsoIdentityBlock.transaction do + SsoAuditLog.log_identity_unblocked!(block: block, actor: Current.user, request: request) + block.destroy! + end + redirect_to admin_users_path, notice: t(".success") + end + end +end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index a86fda917..853faf6a1 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -2,7 +2,7 @@ module Admin class UsersController < Admin::BaseController - before_action :set_user, only: %i[update] + before_action :set_user, only: %i[update deletion destroy] def index authorize User @@ -43,6 +43,11 @@ module Admin .where(status: :trialing) .where(trial_ends_at: Time.current..7.days.from_now) .count + @sso_identity_blocks = SsoIdentityBlock.order(created_at: :desc) + + # Used by the view to hide the "remove" action for the sole remaining + # active super admin, computed once here instead of per-row. + @active_super_admin_count = User.where(role: :super_admin, active: true).count end def update @@ -61,6 +66,50 @@ module Admin end end + def deletion + # Same self-removal short-circuit as #destroy. UserPolicy#destroy? already + # denies it, but Pundit::NotAuthorizedError is not rescued anywhere in this + # app, so opening the confirmation modal for yourself (the index view hides + # the button, but the URL is guessable) would 500 instead of redirecting. + if @user.id == Current.user.id + redirect_to admin_users_path, alert: t("admin.users.destroy.cannot_remove_self") + return + end + + authorize @user, :destroy? + render layout: false + end + + def destroy + # Self-removal is also denied by UserPolicy#destroy?, but checking it here + # first turns it into a friendly redirect instead of an unhandled + # Pundit::NotAuthorizedError (there is no rescue_from for it in this app). + if @user.id == Current.user.id + redirect_to admin_users_path, alert: t(".cannot_remove_self") + return + end + + authorize @user + + unless ActiveSupport::SecurityUtils.secure_compare(params[:confirmation_email].to_s, @user.email) + redirect_to admin_users_path, alert: t(".confirmation_mismatch") + return + end + + removed = @user.transaction do + next false unless @user.permanently_remove! + + SsoAuditLog.log_user_removed!(user: @user, actor: Current.user, request: request) + true + end + + if removed + redirect_to admin_users_path, notice: t(".success") + else + redirect_to admin_users_path, alert: @user.errors.full_messages.to_sentence.presence || t(".failure") + end + end + private def set_user diff --git a/app/controllers/api/v1/auth_controller.rb b/app/controllers/api/v1/auth_controller.rb index 0b79d6873..4ced6ae52 100644 --- a/app/controllers/api/v1/auth_controller.rb +++ b/app/controllers/api/v1/auth_controller.rb @@ -10,6 +10,7 @@ module Api before_action :ensure_write_scope, only: :enable_ai before_action :check_api_key_rate_limit, only: :enable_ai before_action :log_api_access, only: :enable_ai + rescue_from SsoIdentityBlock::BlockedIdentity, with: :render_removed_identity def signup # Check if invite code is required @@ -153,7 +154,7 @@ module Api user = User.authenticate_by(email: params[:email], password: params[:password]) - unless user + unless user&.active? render json: { error: "Invalid email or password" }, status: :unauthorized return end @@ -291,23 +292,37 @@ module Api return end - # Create new access token - new_token = Doorkeeper::AccessToken.create!( - application: access_token.application, - resource_owner_id: access_token.resource_owner_id, - mobile_device_id: access_token.mobile_device_id, - expires_in: 30.days.to_i, - scopes: access_token.scopes, - use_refresh_token: true - ) + user = User.find_by(id: access_token.resource_owner_id) + new_token = begin + user&.with_lock do + next false unless user.active? - # Revoke old access token - access_token.revoke + access_token.with_lock do + next false if access_token.revoked? - # Update device last seen - user = User.find(access_token.resource_owner_id) - device = user.mobile_devices.find_by(device_id: params[:device][:device_id]) - device&.update_last_seen! + token = Doorkeeper::AccessToken.create!( # pipelock:ignore Credential in URL + application: access_token.application, + resource_owner_id: access_token.resource_owner_id, + mobile_device_id: access_token.mobile_device_id, + expires_in: 30.days.to_i, + scopes: access_token.scopes, + use_refresh_token: true + ) + + access_token.revoke + device = user.mobile_devices.find_by(device_id: params.dig(:device, :device_id)) + device&.update_last_seen! + token + end + end + rescue ActiveRecord::RecordNotFound + false + end + + unless new_token + render json: { error: "Invalid refresh token" }, status: :unauthorized + return + end render json: { access_token: new_token.plaintext_token, @@ -403,9 +418,19 @@ module Api return nil end + if SsoIdentityBlock.blocked?(provider: cached[:provider], uid: cached[:uid]) + Rails.cache.delete(cache_key) + render json: { error: "SSO identity was removed by an administrator" }, status: :forbidden + return nil + end + cached end + def render_removed_identity + render json: { error: "SSO identity was removed by an administrator" }, status: :forbidden + end + # Atomically deletes the linking code from cache. # Returns true only for the first caller; subsequent callers get false. def consume_linking_code!(linking_code) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 28758d9d9..b724720f7 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -29,18 +29,28 @@ module Authentication def find_session_by_cookie cookie_value = cookies.signed[:session_token] + return if cookie_value.blank? - if cookie_value.present? - Session.find_by(id: cookie_value) - else - nil - end + session_record = Session.includes(:user).find_by(id: cookie_value) + return session_record if session_record&.user&.active? + + session_record&.destroy! + cookies.delete(:session_token) + nil end def create_session_for(user) - session = user.sessions.create! - cookies.signed.permanent[:session_token] = { value: session.id, httponly: true } - session + return false unless user&.persisted? + + user.with_lock do + next false unless user.active? + + session = user.sessions.create! + cookies.signed.permanent[:session_token] = { value: session.id, httponly: true } + session + end + rescue ActiveRecord::RecordNotFound + false end def self_hosted_first_login? diff --git a/app/controllers/mfa_controller.rb b/app/controllers/mfa_controller.rb index 51154ac51..16f348de0 100644 --- a/app/controllers/mfa_controller.rb +++ b/app/controllers/mfa_controller.rb @@ -31,7 +31,10 @@ class MfaController < ApplicationController @user = User.find_by(id: session[:mfa_user_id]) if @user&.verify_otp?(params[:code]) - complete_mfa_sign_in(@user) + unless complete_mfa_sign_in(@user) + redirect_to new_session_path + return + end redirect_to root_path else flash.now[:alert] = t(".invalid_code") @@ -86,7 +89,9 @@ class MfaController < ApplicationController last_used_at: Time.current ) end - complete_mfa_sign_in(@user) + unless complete_mfa_sign_in(@user) + return render json: { error: t(".invalid_credential") }, status: :unprocessable_entity + end render json: { redirect_url: root_path } rescue WebAuthn::Error, ActionController::BadRequest, ActionController::ParameterMissing @@ -113,6 +118,9 @@ class MfaController < ApplicationController def complete_mfa_sign_in(user) session.delete(:mfa_user_id) @session = create_session_for(user) + return false unless @session + flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user) + true end end diff --git a/app/controllers/oidc_accounts_controller.rb b/app/controllers/oidc_accounts_controller.rb index 3d26432fe..e218edfeb 100644 --- a/app/controllers/oidc_accounts_controller.rb +++ b/app/controllers/oidc_accounts_controller.rb @@ -1,5 +1,7 @@ class OidcAccountsController < ApplicationController skip_authentication only: [ :link, :create_link, :new_user, :create_user ] + before_action :reject_removed_identity, only: [ :link, :create_link, :new_user, :create_user ] + rescue_from SsoIdentityBlock::BlockedIdentity, with: :reject_removed_identity_after_lock layout "auth" def link @@ -33,19 +35,31 @@ class OidcAccountsController < ApplicationController # Verify user's password to confirm identity user = User.authenticate_by(email: params[:email], password: params[:password]) - if user - # Create the OIDC identity link - oidc_identity = OidcIdentity.create_from_omniauth( - build_auth_hash(@pending_auth), - user - ) + if user&.active? + linked = user.transaction do + OidcIdentity.create_from_omniauth( + build_auth_hash(@pending_auth), + user + ) - # Log account linking - SsoAuditLog.log_link!( - user: user, - provider: @pending_auth["provider"], - request: request - ) + SsoAuditLog.log_link!( + user: user, + provider: @pending_auth["provider"], + request: request + ) + + unless user.otp_required? + @session = create_session_for(user) + raise ActiveRecord::Rollback unless @session + end + + true + end + + unless linked + redirect_to new_session_path, alert: t("sessions.openid_connect.failed") + return + end # Clear pending auth from session session.delete(:pending_oidc_auth) @@ -54,7 +68,6 @@ class OidcAccountsController < ApplicationController session[:mfa_user_id] = user.id redirect_to verify_mfa_path else - @session = create_session_for(user) notice = if accept_pending_invitation_for(user) t("invitations.accept_choice.joined_household") else @@ -160,6 +173,9 @@ class OidcAccountsController < ApplicationController @user ) + @session = create_session_for(@user) + raise ActiveRecord::Rollback unless @session + true end rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique => e @@ -182,7 +198,6 @@ class OidcAccountsController < ApplicationController # Clear pending auth from session session.delete(:pending_oidc_auth) - @session = create_session_for(@user) notice = if invitation.present? t("invitations.accept_choice.joined_household") elsif accept_pending_invitation_for(@user) @@ -198,6 +213,20 @@ class OidcAccountsController < ApplicationController private + def reject_removed_identity_after_lock + session.delete(:pending_oidc_auth) + redirect_to new_session_path, alert: t("sessions.openid_connect.failed") + end + + def reject_removed_identity + pending_auth = session[:pending_oidc_auth] + return unless pending_auth.present? + return unless SsoIdentityBlock.blocked?(provider: pending_auth["provider"], uid: pending_auth["uid"]) + + session.delete(:pending_oidc_auth) + redirect_to new_session_path, alert: t("sessions.openid_connect.failed") + end + # Convert pending auth hash to OmniAuth-like structure def build_auth_hash(pending_auth) OpenStruct.new( diff --git a/app/controllers/passkey_sessions_controller.rb b/app/controllers/passkey_sessions_controller.rb index 72dacaf2b..dc56978f0 100644 --- a/app/controllers/passkey_sessions_controller.rb +++ b/app/controllers/passkey_sessions_controller.rb @@ -59,7 +59,7 @@ class PasskeySessionsController < ApplicationController ) end - complete_sign_in(user) + return render_invalid unless complete_sign_in(user) render json: { redirect_url: root_path } rescue WebAuthn::Error, ActionController::BadRequest, ActionController::ParameterMissing @@ -81,7 +81,10 @@ class PasskeySessionsController < ApplicationController session.delete(:mfa_user_id) @session = create_session_for(user) + return false unless @session + flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user) + true end def render_invalid diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 14cdc4851..8cdff932d 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -77,6 +77,8 @@ class RegistrationsController < ApplicationController # policy so the user sees the accounts the family shares. @user.family.auto_share_existing_accounts_with(@user) @session = create_session_for(@user) + raise ActiveRecord::Rollback unless @session + success = true end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 7db6d098f..076dc1096 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -72,6 +72,10 @@ class SessionsController < ApplicationController else log_super_admin_override_login(user) @session = create_session_for(user) + unless @session + redirect_to new_session_path, alert: t("sessions.openid_connect.failed") + return + end flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user) redirect_to root_path end @@ -201,7 +205,12 @@ class SessionsController < ApplicationController end user = User.find_by(id: data[:user_id]) - unless user + # This code is minted during the OIDC callback and redeemed up to two minutes + # later, so an administrator can permanently remove the user inside that + # window. User#revoke_all_credentials! cannot reach a session that does not + # exist yet, and this path never re-consults the SSO identity (the code + # carries only a user id), so re-check the account here before minting one. + unless user&.active? redirect_to new_session_path, alert: t("sessions.openid_connect.failed") return end @@ -211,6 +220,10 @@ class SessionsController < ApplicationController redirect_to verify_mfa_path else @session = create_session_for(user) + unless @session + redirect_to new_session_path, alert: t("sessions.openid_connect.failed") + return + end flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user) redirect_to root_path end @@ -225,6 +238,11 @@ class SessionsController < ApplicationController return end + if SsoIdentityBlock.blocked?(provider: auth.provider, uid: auth.uid) + reject_removed_sso_identity(auth.provider) + return + end + # Security fix: Look up by provider + uid, not just email oidc_identity = OidcIdentity.find_by(provider: auth.provider, uid: auth.uid) @@ -266,6 +284,10 @@ class SessionsController < ApplicationController redirect_to verify_mfa_path else @session = create_session_for(user) + unless @session + redirect_to new_session_path, alert: t("sessions.openid_connect.failed") + return + end flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user) redirect_to root_path end @@ -315,7 +337,7 @@ class SessionsController < ApplicationController # Mobile SSO: redirect back to the app with error instead of web login page if session[:mobile_sso].present? session.delete(:mobile_sso) - mobile_sso_redirect(error: sanitized_reason, message: "SSO authentication failed") + mobile_sso_redirect(error: sanitized_reason, message: t("sessions.failure.sso_failed")) return end @@ -340,6 +362,22 @@ class SessionsController < ApplicationController end private + def reject_removed_sso_identity(provider) + SsoAuditLog.log_login_failed!( + provider: provider, + request: request, + reason: "removed_identity" + ) + + if session.delete(:mobile_sso).present? + mobile_sso_redirect(error: "sso_failed", message: t("sessions.failure.sso_failed")) + elsif session.delete(:desktop_sso).present? + redirect_to "sure://sso/callback?error=sso_failed", allow_other_host: true + else + redirect_to new_session_path, alert: t("sessions.openid_connect.failed") + end + end + def handle_mobile_sso_callback(user) device_info = session.delete(:mobile_sso) diff --git a/app/models/mobile_device.rb b/app/models/mobile_device.rb index 035643ce3..ad6fa0625 100644 --- a/app/models/mobile_device.rb +++ b/app/models/mobile_device.rb @@ -67,24 +67,34 @@ class MobileDevice < ApplicationRecord # previous tokens. Returns a hash with token details ready for an API # response or deep-link callback. def issue_token! - revoke_all_tokens! + user.with_lock do + unless user.active? + errors.add(:base, "User is inactive") + raise ActiveRecord::RecordInvalid, self + end - access_token = Doorkeeper::AccessToken.create!( - application: self.class.shared_oauth_application, - resource_owner_id: user_id, - mobile_device_id: id, - expires_in: 30.days.to_i, - scopes: "read_write", - use_refresh_token: true - ) + revoke_all_tokens! - { - access_token: access_token.plaintext_token, - refresh_token: access_token.plaintext_refresh_token, - token_type: "Bearer", - expires_in: access_token.expires_in, - created_at: access_token.created_at.to_i - } + access_token = Doorkeeper::AccessToken.create!( # pipelock:ignore Credential in URL + application: self.class.shared_oauth_application, + resource_owner_id: user_id, + mobile_device_id: id, + expires_in: 30.days.to_i, + scopes: "read_write", + use_refresh_token: true + ) + + { + access_token: access_token.plaintext_token, + refresh_token: access_token.plaintext_refresh_token, + token_type: "Bearer", + expires_in: access_token.expires_in, + created_at: access_token.created_at.to_i + } + end + rescue ActiveRecord::RecordNotFound + errors.add(:base, "User is inactive") + raise ActiveRecord::RecordInvalid, self end private diff --git a/app/models/oidc_identity.rb b/app/models/oidc_identity.rb index 6aae6a7e6..e3c2dcba1 100644 --- a/app/models/oidc_identity.rb +++ b/app/models/oidc_identity.rb @@ -78,22 +78,28 @@ class OidcIdentity < ApplicationRecord # Extract and store relevant info from OmniAuth auth hash def self.create_from_omniauth(auth, user) - # Extract issuer from OIDC auth response if available - issuer = auth.extra&.raw_info&.iss || auth.extra&.raw_info&.[]("iss") + SsoIdentityBlock.with_identity_lock(provider: auth.provider, uid: auth.uid) do + if SsoIdentityBlock.blocked?(provider: auth.provider, uid: auth.uid) + raise SsoIdentityBlock::BlockedIdentity + end - create!( - user: user, - provider: auth.provider, - uid: auth.uid, - issuer: issuer, - info: { - email: auth.info&.email, - name: auth.info&.name, - first_name: auth.info&.first_name, - last_name: auth.info&.last_name - }, - last_authenticated_at: Time.current - ) + # Extract issuer from OIDC auth response if available + issuer = auth.extra&.raw_info&.iss || auth.extra&.raw_info&.[]("iss") + + create!( + user: user, + provider: auth.provider, + uid: auth.uid, + issuer: issuer, + info: { + email: auth.info&.email, + name: auth.info&.name, + first_name: auth.info&.first_name, + last_name: auth.info&.last_name + }, + last_authenticated_at: Time.current + ) + end end # Find the configured provider for this identity diff --git a/app/models/sso_audit_log.rb b/app/models/sso_audit_log.rb index 21aa4e05e..100176112 100644 --- a/app/models/sso_audit_log.rb +++ b/app/models/sso_audit_log.rb @@ -12,6 +12,8 @@ class SsoAuditLog < ApplicationRecord link unlink jit_account_created + identity_unblocked + user_removed ].freeze validates :event_type, presence: true, inclusion: { in: EVENT_TYPES } @@ -104,5 +106,33 @@ class SsoAuditLog < ApplicationRecord metadata: metadata ) end + + def log_user_removed!(user:, actor:, request:) + create!( + user: user, + event_type: "user_removed", + provider: nil, + ip_address: request.remote_ip, + user_agent: request.user_agent&.truncate(500), + metadata: { + actor_user_id: actor.id, + target_user_id: user.id + } + ) + end + + def log_identity_unblocked!(block:, actor:, request:) + create!( + user: nil, + event_type: "identity_unblocked", + provider: block.provider, + ip_address: request.remote_ip, + user_agent: request.user_agent&.truncate(500), + metadata: { + actor_user_id: actor.id, + identity_block_id: block.id + } + ) + end end end diff --git a/app/models/sso_identity_block.rb b/app/models/sso_identity_block.rb new file mode 100644 index 000000000..c78b66a69 --- /dev/null +++ b/app/models/sso_identity_block.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +class SsoIdentityBlock < ApplicationRecord + include Encryptable + + class BlockedIdentity < StandardError; end + + encrypts :identity_label if encryption_ready? + + before_validation :redact_identity_label_unless_encrypted + + validates :provider, :uid_digest, :identity_label, presence: true + validates :uid_digest, uniqueness: { scope: :provider } + + class << self + def blocked?(provider:, uid:) + exists?(provider: provider, uid_digest: digest(uid)) + end + + def block_all!(identities, identity_label:) + identities.find_each do |identity| + with_identity_lock(provider: identity.provider, uid: identity.uid) do + find_or_create_by!( + provider: identity.provider, + uid_digest: digest(identity.uid) + ) { |block| block.identity_label = identity_label } + end + end + end + + def with_identity_lock(provider:, uid:) + transaction do + lock_key = advisory_lock_key(provider: provider, uid: uid) + connection.execute(sanitize_sql_array([ "SELECT pg_advisory_xact_lock(?)", lock_key ])) + yield + end + end + + def digest(uid) + OpenSSL::HMAC.hexdigest("SHA256", digest_key, uid.to_s) + end + + private + + def digest_key + Rails.application.key_generator.generate_key("sso_identity_block_uid", 32) + end + + def advisory_lock_key(provider:, uid:) + OpenSSL::HMAC.digest("SHA256", digest_key, "#{provider}\0#{uid}").unpack1("q>") + end + end + + private + + def redact_identity_label_unless_encrypted + return if self.class.encryption_ready? + + self.identity_label = "Removed identity #{uid_digest.to_s.first(12)}" + end +end diff --git a/app/models/user.rb b/app/models/user.rb index cd541331b..b87d7c3e4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -233,7 +233,57 @@ class User < ApplicationRecord after_update_commit :purge_later, if: -> { saved_change_to_active?(from: true, to: false) } def deactivate - update active: false, email: deactivated_email + return true unless active? + + transaction do + if super_admin? + active_super_admins = User.where(role: :super_admin, active: true).lock.to_a + if active_super_admins.one? + errors.add(:base, :cannot_remove_last_super_admin) + raise ActiveRecord::Rollback + end + end + + update(active: false, email: deactivated_email) + end || false + end + + # Permanent removal of another user, initiated by a super admin from the + # instance users page. Reuses the sanctioned deactivate -> UserPurgeJob path + # (which reassigns owned accounts, or destroys the family when this is its + # last member) for the heavy data cleanup, but additionally revokes every + # live authentication vector *synchronously* so there is no window in which + # the removed user can keep acting or re-authenticate before the async purge + # runs. Returns false (with errors populated) when the user cannot be + # deactivated, e.g. an admin who still has co-members in their family. + def permanently_remove! + was_active = active? + removed = transaction do + identity_label = email + raise ActiveRecord::Rollback unless deactivate + + SsoIdentityBlock.block_all!(oidc_identities, identity_label: identity_label) + revoke_all_credentials! + true + end || false + + purge_later if removed && !was_active + removed + end + + # Destroys every credential/session that can authenticate as this user. + # Web sessions and the SSO identity re-auth path (OidcIdentity lookup by + # provider+uid) are not gated on #active?, so they must be torn down here for + # revocation to be immediate; the async purge would otherwise leave a window. + def revoke_all_credentials! + Doorkeeper::AccessToken + .where(resource_owner_id: id, revoked_at: nil) + .update_all(revoked_at: Time.current) + sessions.destroy_all + api_keys.destroy_all + mobile_devices.destroy_all + webauthn_credentials.destroy_all + oidc_identities.destroy_all end def can_deactivate diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index c40bf6007..abab05920 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -12,6 +12,14 @@ class UserPolicy < ApplicationPolicy user.id != record.id end + # Permanent removal of a user from the instance. Super-admin only, and never + # the acting user themselves (self-removal is blocked here and re-checked in + # the controller so it surfaces a friendly message instead of a hard 403). + def destroy? + return false unless user&.super_admin? + user.id != record.id + end + class Scope < ApplicationPolicy::Scope def resolve if user&.super_admin? diff --git a/app/views/admin/users/deletion.html.erb b/app/views/admin/users/deletion.html.erb new file mode 100644 index 000000000..18b0cea68 --- /dev/null +++ b/app/views/admin/users/deletion.html.erb @@ -0,0 +1,30 @@ +<%= render DS::Dialog.new(width: "sm") do |dialog| %> + <% dialog.with_header(title: t(".title"), subtitle: t(".warning")) %> + + <% dialog.with_body do %> +

<%= t(".instruction", email: @user.email) %>

+ + <%= styled_form_with url: admin_user_path(@user), method: :delete, data: { turbo_frame: "_top" } do |form| %> + <%= form.text_field :confirmation_email, + label: t(".email_label"), + autocomplete: "off", + spellcheck: false, + required: true, + class: "mb-4" %> + +
+ <%= render DS::Button.new( + text: t(".cancel"), + type: "button", + variant: :secondary, + data: { action: "DS--dialog#close" } + ) %> + <%= render DS::Button.new( + text: t(".submit"), + type: "submit", + variant: :destructive + ) %> +
+ <% end %> + <% end %> +<% end %> diff --git a/app/views/admin/users/index.html.erb b/app/views/admin/users/index.html.erb index a89b4d675..326a0206b 100644 --- a/app/views/admin/users/index.html.erb +++ b/app/views/admin/users/index.html.erb @@ -122,18 +122,30 @@ <% if user.id == Current.user.id %> <%= t(".you") %> <% else %> - <%= form_with model: [:admin, user], method: :patch, class: "flex items-center justify-end gap-2", data: { controller: "auto-submit-form" } do |form| %> - <%= form.select :role, - options_for_select([ - [t(".roles.guest"), "guest"], - [t(".roles.member"), "member"], - [t(".roles.admin"), "admin"], - [t(".roles.super_admin"), "super_admin"] - ], user.role), - {}, - class: "text-sm rounded-lg border border-primary bg-container text-primary px-2 py-1", - data: { auto_submit_form_target: "auto" } %> - <% end %> +
+ <%= form_with model: [:admin, user], method: :patch, class: "flex items-center", data: { controller: "auto-submit-form" } do |form| %> + <%= form.select :role, + options_for_select([ + [t(".roles.guest"), "guest"], + [t(".roles.member"), "member"], + [t(".roles.admin"), "admin"], + [t(".roles.super_admin"), "super_admin"] + ], user.role), + {}, + class: "text-sm rounded-lg border border-primary bg-container text-primary px-2 py-1", + data: { auto_submit_form_target: "auto" } %> + <% end %> + <% unless user.super_admin? && user.active? && @active_super_admin_count <= 1 %> + <%= render DS::Button.new( + text: t(".table.remove"), + href: deletion_admin_user_path(user), + method: :get, + frame: "modal", + variant: :outline_destructive, + size: :sm + ) %> + <% end %> +
<% end %> @@ -195,6 +207,31 @@ <% end %> + <% if @sso_identity_blocks.any? %> + <%= settings_section title: t(".removed_sso_identities.title"), collapsible: true, open: false do %> +

<%= t(".removed_sso_identities.description") %>

+
+ <% @sso_identity_blocks.each do |block| %> +
+
+

<%= block.identity_label %>

+

<%= block.provider %> · <%= l(block.created_at, format: :long) %>

+
+ <%= form_with url: admin_sso_identity_block_path(block), method: :delete do %> + <%= render DS::Button.new( + text: t(".removed_sso_identities.allow_again"), + type: "submit", + variant: :outline_destructive, + size: :sm, + data: { turbo_confirm: t(".removed_sso_identities.confirm", email: block.identity_label) } + ) %> + <% end %> +
+ <% end %> +
+ <% end %> + <% end %> + <%= settings_section title: t(".role_descriptions_title"), collapsible: true, open: true do %>
diff --git a/config/locales/models/user/en.yml b/config/locales/models/user/en.yml index c94bbad77..3be84c0a2 100644 --- a/config/locales/models/user/en.yml +++ b/config/locales/models/user/en.yml @@ -17,5 +17,7 @@ en: base: cannot_deactivate_admin_with_other_users: Admin cannot delete account while other users are present. Please delete all members first. + cannot_remove_last_super_admin: Cannot remove the last active super + admin. Promote another user to super admin first. profile_image: invalid_file_size: file size must be less than %{max_megabytes}MB diff --git a/config/locales/views/admin/sso_identity_blocks/en.yml b/config/locales/views/admin/sso_identity_blocks/en.yml new file mode 100644 index 000000000..07b226c73 --- /dev/null +++ b/config/locales/views/admin/sso_identity_blocks/en.yml @@ -0,0 +1,5 @@ +en: + admin: + sso_identity_blocks: + destroy: + success: "The SSO identity can sign in or create an account again." diff --git a/config/locales/views/admin/users/en.yml b/config/locales/views/admin/users/en.yml index c14a243d5..a29816a4b 100644 --- a/config/locales/views/admin/users/en.yml +++ b/config/locales/views/admin/users/en.yml @@ -32,7 +32,13 @@ en: session_count: "Session count" never: "Never" role: "Role" + remove: "Remove" role_descriptions_title: "Role Descriptions" + removed_sso_identities: + title: "Removed SSO identities" + description: "These identities cannot create or reconnect an account. Allow an identity again only when the removal was accidental." + allow_again: "Allow again" + confirm: "Allow %{email} to sign in or create an account through SSO again?" roles: guest: "Guest" member: "Member" @@ -51,3 +57,16 @@ en: update: success: "User role updated successfully." failure: "Failed to update user role." + destroy: + success: "User access revoked and permanent deletion scheduled." + failure: "Failed to remove user." + cannot_remove_self: "You cannot remove your own account from here." + cannot_remove_last_super_admin: "You cannot remove the last active super admin." + confirmation_mismatch: "The email address did not match. The user was not removed." + deletion: + title: "Permanently remove user" + warning: "This immediately revokes access and schedules permanent deletion of the user and their data. This cannot be undone." + instruction: "Type %{email} to confirm." + email_label: "User email" + cancel: "Cancel" + submit: "Permanently remove user" diff --git a/config/routes.rb b/config/routes.rb index 5208eac10..52eec6b28 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -856,7 +856,10 @@ Rails.application.routes.draw do post :test_connection end end - resources :users, only: [ :index, :update ] + resources :users, only: [ :index, :update, :destroy ] do + get :deletion, on: :member + end + resources :sso_identity_blocks, only: [ :destroy ] resources :invitations, only: [ :destroy ] resources :families, only: [] do member do diff --git a/db/migrate/20260822130000_create_sso_identity_blocks.rb b/db/migrate/20260822130000_create_sso_identity_blocks.rb new file mode 100644 index 000000000..005f46420 --- /dev/null +++ b/db/migrate/20260822130000_create_sso_identity_blocks.rb @@ -0,0 +1,13 @@ +class CreateSsoIdentityBlocks < ActiveRecord::Migration[7.2] + def change + create_table :sso_identity_blocks, id: :uuid do |t| + t.string :provider, null: false + t.string :uid_digest, null: false + t.text :identity_label, null: false + + t.timestamps + end + + add_index :sso_identity_blocks, [ :provider, :uid_digest ], unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 7e63ad32b..cbf7a2d86 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_08_20_120000) do +ActiveRecord::Schema[7.2].define(version: 2026_08_22_130000) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -2056,6 +2056,15 @@ ActiveRecord::Schema[7.2].define(version: 2026_08_20_120000) do t.index ["user_id"], name: "index_sso_audit_logs_on_user_id" end + create_table "sso_identity_blocks", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.datetime "created_at", null: false + t.text "identity_label", null: false + t.string "provider", null: false + t.string "uid_digest", null: false + t.datetime "updated_at", null: false + t.index ["provider", "uid_digest"], name: "index_sso_identity_blocks_on_provider_and_uid_digest", unique: true + end + create_table "sso_providers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "strategy", null: false t.string "name", null: false diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 3d5edb333..73fabdd6b 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -3644,6 +3644,12 @@ paths: oneOf: - "$ref": "#/components/schemas/ErrorResponse" - "$ref": "#/components/schemas/MfaRequiredResponse" + '403': + description: SSO identity removed by an administrator + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse" requestBody: content: application/json: @@ -3725,7 +3731,7 @@ paths: schema: "$ref": "#/components/schemas/ErrorResponse" '403': - description: account creation disabled + description: SSO identity removed or account creation disabled content: application/json: schema: diff --git a/spec/requests/api/v1/auth_spec.rb b/spec/requests/api/v1/auth_spec.rb index 11366acf8..38bb01496 100644 --- a/spec/requests/api/v1/auth_spec.rb +++ b/spec/requests/api/v1/auth_spec.rb @@ -267,6 +267,42 @@ RSpec.describe 'API V1 Auth', type: :request do ] run_test! end + + response '403', 'SSO identity removed by an administrator' do + schema '$ref' => '#/components/schemas/ErrorResponse' + + let(:linking_code) { 'rswag-removed-identity' } + let(:removed_uid) { 'rswag-removed-subject' } + let(:body) do + { + linking_code: linking_code, + email: 'removed@example.com', + password: 'unused-password' + } + end + + around do |example| + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + example.run + ensure + Rails.cache = original_cache + end + + before do + Rails.cache.write("mobile_sso_link:#{linking_code}", { + provider: 'openid_connect', + uid: removed_uid + }) + SsoIdentityBlock.create!( + provider: 'openid_connect', + uid_digest: SsoIdentityBlock.digest(removed_uid), + identity_label: 'removed@example.com' + ) + end + + run_test! + end end end @@ -319,7 +355,7 @@ RSpec.describe 'API V1 Auth', type: :request do run_test! end - response '403', 'account creation disabled' do + response '403', 'SSO identity removed or account creation disabled' do schema '$ref' => '#/components/schemas/ErrorResponse' run_test! end diff --git a/test/controllers/admin/sso_identity_blocks_controller_test.rb b/test/controllers/admin/sso_identity_blocks_controller_test.rb new file mode 100644 index 000000000..f9b30fe68 --- /dev/null +++ b/test/controllers/admin/sso_identity_blocks_controller_test.rb @@ -0,0 +1,40 @@ +require "test_helper" + +class Admin::SsoIdentityBlocksControllerTest < ActionDispatch::IntegrationTest + setup do + @block = SsoIdentityBlock.create!( + provider: "openid_connect", + uid_digest: SsoIdentityBlock.digest("removed-subject"), + identity_label: "removed-user@example.com" + ) + end + + test "super admin can allow a removed SSO identity again" do + sign_in users(:sure_support_staff) + + assert_difference -> { SsoIdentityBlock.count }, -1 do + assert_difference -> { SsoAuditLog.by_event("identity_unblocked").count }, 1 do + delete admin_sso_identity_block_url(@block) + end + end + + assert_redirected_to admin_users_path + + audit_log = SsoAuditLog.by_event("identity_unblocked").last + assert_equal users(:sure_support_staff).id, audit_log.metadata["actor_user_id"] + assert_equal @block.id, audit_log.metadata["identity_block_id"] + assert_equal @block.provider, audit_log.provider + end + + test "regular user cannot allow a removed SSO identity again" do + sign_in users(:family_member) + + assert_no_difference -> { SsoIdentityBlock.count } do + assert_no_difference -> { SsoAuditLog.by_event("identity_unblocked").count } do + delete admin_sso_identity_block_url(@block) + end + end + + assert_redirected_to root_path + end +end diff --git a/test/controllers/admin/users_controller_test.rb b/test/controllers/admin/users_controller_test.rb index 1273c4b18..5c39c2494 100644 --- a/test/controllers/admin/users_controller_test.rb +++ b/test/controllers/admin/users_controller_test.rb @@ -1,6 +1,8 @@ require "test_helper" class Admin::UsersControllerTest < ActionDispatch::IntegrationTest + include ActiveJob::TestHelper + setup do sign_in users(:sure_support_staff) end @@ -46,4 +48,149 @@ class Admin::UsersControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_match(/No subscription/, response.body, "Page should show 'No subscription' for families without one") end + + test "index shows removed SSO identities with a recovery action" do + block = SsoIdentityBlock.create!( + provider: "openid_connect", + uid_digest: SsoIdentityBlock.digest("blocked-subject"), + identity_label: "removed-user@example.com" + ) + + get admin_users_url + + assert_response :success + assert_select "form[action=?]", admin_sso_identity_block_path(block) + assert_match block.identity_label, response.body + end + + test "super admin permanently removes a user and revokes their credentials" do + target = users(:family_member) + target_email = target.email + removed_identity = target.oidc_identities.first! + removed_provider = removed_identity.provider + removed_uid = removed_identity.uid + target.sessions.create! + oauth_app = Doorkeeper::Application.create!( + name: "Removal test", + redirect_uri: "https://app.example/callback", + confidential: false + ) + oauth_token = Doorkeeper::AccessToken.create!( + application: oauth_app, + resource_owner_id: target.id, + scopes: "read_write", + use_refresh_token: true + ) + assert target.oidc_identities.exists? + assert target.api_keys.exists? + + assert_difference -> { SsoAuditLog.by_event("user_removed").count }, 1 do + assert_enqueued_with(job: UserPurgeJob, args: [ target ]) do + delete admin_user_url(target), params: { confirmation_email: target_email } + end + end + + assert_redirected_to admin_users_path + target.reload + assert_not target.active? + assert_empty target.sessions + assert_empty target.oidc_identities + assert_empty target.api_keys + assert oauth_token.reload.revoked? + assert SsoIdentityBlock.blocked?(provider: removed_provider, uid: removed_uid) + identity_block = SsoIdentityBlock.find_by!(provider: removed_provider) + if SsoIdentityBlock.encryption_ready? + assert_equal target_email, identity_block.identity_label + else + assert_not_equal target_email, identity_block.identity_label + end + audit_log = SsoAuditLog.by_event("user_removed").order(:created_at).last + assert_equal target.id, audit_log.metadata.fetch("target_user_id") + assert_not audit_log.metadata.key?("target_email") + assert_equal users(:sure_support_staff).id, audit_log.metadata.fetch("actor_user_id") + end + + test "super admin cannot remove themselves" do + me = users(:sure_support_staff) + + assert_no_enqueued_jobs only: UserPurgeJob do + delete admin_user_url(me) + end + + assert_redirected_to admin_users_path + assert User.exists?(me.id) + assert me.reload.active? + end + + test "audit failure rolls back user removal" do + target = users(:family_member) + target_email = target.email + SsoAuditLog.expects(:log_user_removed!).raises("audit failure") + + assert_no_enqueued_jobs only: UserPurgeJob do + assert_raises(RuntimeError, match: /audit failure/) do + delete admin_user_url(target), params: { confirmation_email: target_email } + end + end + + assert target.reload.active? + assert target.oidc_identities.exists? + end + + test "inactive super admin cannot use an existing session to remove the last active super admin" do + target = users(:family_admin) + target.update!(role: :super_admin) + User.where(role: :super_admin).where.not(id: target.id).update_all(active: false) + + assert_no_enqueued_jobs only: UserPurgeJob do + delete admin_user_url(target), params: { confirmation_email: target.email } + end + + assert_redirected_to new_session_path + assert User.exists?(target.id) + assert target.reload.active? + end + + test "deletion page redirects instead of erroring when targeting yourself" do + me = users(:sure_support_staff) + + get deletion_admin_user_url(me) + + assert_redirected_to admin_users_path + assert_match(/cannot remove your own account/i, flash[:alert].to_s) + end + + test "deletion confirmation requires the target email" do + target = users(:family_member) + + assert_no_enqueued_jobs only: UserPurgeJob do + delete admin_user_url(target), params: { confirmation_email: "wrong@example.com" } + end + + assert_redirected_to admin_users_path + assert target.reload.active? + end + + test "deletion page renders a typed email confirmation dialog" do + target = users(:family_member) + + get deletion_admin_user_url(target) + + assert_response :success + assert_select "dialog" + assert_select "input[name=confirmation_email][required]" + assert_includes response.body, target.email + end + + test "non super admin cannot remove a user" do + sign_in users(:family_member) + target = users(:family_admin) + + assert_no_enqueued_jobs only: UserPurgeJob do + delete admin_user_url(target) + end + + assert_redirected_to root_path + assert User.exists?(target.id) + end end diff --git a/test/controllers/api/v1/auth_controller_test.rb b/test/controllers/api/v1/auth_controller_test.rb index 4c39adfed..6b6858792 100644 --- a/test/controllers/api/v1/auth_controller_test.rb +++ b/test/controllers/api/v1/auth_controller_test.rb @@ -463,6 +463,30 @@ class Api::V1::AuthControllerTest < ActionDispatch::IntegrationTest assert_equal "Invalid refresh token", response_data["error"] end + test "should not refresh a token after its user is deactivated" do + user = users(:family_admin) + device = user.mobile_devices.create!(@device_info) + initial_token = Doorkeeper::AccessToken.create!( + application: @shared_app, + resource_owner_id: user.id, + mobile_device_id: device.id, + expires_in: 30.days.to_i, + scopes: "read_write", + use_refresh_token: true + ) + user.update_column(:active, false) + + assert_no_difference("Doorkeeper::AccessToken.count") do + post "/api/v1/auth/refresh", params: { + refresh_token: initial_token.refresh_token, + device: @device_info + } + end + + assert_response :unauthorized + assert_equal "Invalid refresh token", JSON.parse(response.body)["error"] + end + test "should not refresh without refresh token" do post "/api/v1/auth/refresh", params: { device: @device_info @@ -577,6 +601,31 @@ class Api::V1::AuthControllerTest < ActionDispatch::IntegrationTest assert Rails.cache.read("mobile_sso_link:#{linking_code}").present?, "Expected linking code to survive a failed attempt" end + test "should reject SSO link for an inactive user" do + user = users(:family_admin) + user.update_column(:active, false) + linking_code = SecureRandom.urlsafe_base64(32) + Rails.cache.write("mobile_sso_link:#{linking_code}", { + provider: "google_oauth2", + uid: "google-uid-inactive", + email: "inactive@example.com", + device_info: @device_info.stringify_keys, + allow_account_creation: true + }, expires_in: 10.minutes) + + assert_no_difference "OidcIdentity.count" do + post "/api/v1/auth/sso_link", params: { + linking_code: linking_code, + email: user.email, + password: user_password_test + } + end + + assert_response :unauthorized + assert_equal "Invalid email or password", JSON.parse(response.body)["error"] + assert Rails.cache.read("mobile_sso_link:#{linking_code}").present? + end + test "should reject SSO link when user has MFA enabled" do user = users(:family_admin) user.update!(otp_required: true, otp_secret: ROTP::Base32.random(32)) @@ -960,6 +1009,30 @@ class Api::V1::AuthControllerTest < ActionDispatch::IntegrationTest AccountShare.where(user: invitee).pluck(:account_id).sort end + test "mobile SSO cannot use a cached linking code after its identity is removed" do + identity = oidc_identities(:bob_google) + SsoIdentityBlock.block_all!(OidcIdentity.where(id: identity.id), identity_label: identity.user.email) + linking_code = SecureRandom.urlsafe_base64(32) + Rails.cache.write("mobile_sso_link:#{linking_code}", { + provider: identity.provider, + uid: identity.uid, + email: "removed-mobile-sso@example.com", + device_info: @device_info.stringify_keys, + allow_account_creation: true + }, expires_in: 10.minutes) + + assert_no_difference("User.count") do + post "/api/v1/auth/sso_create_account", params: { + linking_code: linking_code, + first_name: "Removed", + last_name: "User" + } + end + + assert_response :forbidden + assert_nil Rails.cache.read("mobile_sso_link:#{linking_code}") + end + test "mobile SSO onboarding via invitation shares nothing when family sharing is private" do family = families(:dylan_family) family.update!(default_account_sharing: "private") diff --git a/test/controllers/mfa_controller_test.rb b/test/controllers/mfa_controller_test.rb index f43fe3555..39aa2953a 100644 --- a/test/controllers/mfa_controller_test.rb +++ b/test/controllers/mfa_controller_test.rb @@ -91,6 +91,9 @@ class MfaControllerTest < ActionDispatch::IntegrationTest sign_out post sessions_path, params: { email: @user.email, password: user_password_test } + assert_redirected_to verify_mfa_path + assert_equal @user.id, session[:mfa_user_id] + totp = ROTP::TOTP.new(@user.otp_secret, issuer: "Sure Finances") post verify_mfa_path, params: { code: totp.now } @@ -99,6 +102,24 @@ class MfaControllerTest < ActionDispatch::IntegrationTest assert Session.exists?(user_id: @user.id) end + test "verify_code cannot create a session after the user is deactivated" do + @user.setup_mfa! + @user.enable_mfa! + sign_out + + post sessions_path, params: { email: @user.email, password: user_password_test } + assert_redirected_to verify_mfa_path + assert_equal @user.id, session[:mfa_user_id] + + totp = ROTP::TOTP.new(@user.otp_secret, issuer: "Sure Finances") + @user.update_column(:active, false) + + assert_no_difference -> { Session.where(user_id: @user.id).count } do + post verify_mfa_path, params: { code: totp.now } + end + assert_redirected_to new_session_path + end + test "verify_code authenticates with valid backup code" do @user.setup_mfa! backup_code = @user.enable_mfa!.first @@ -161,6 +182,30 @@ class MfaControllerTest < ActionDispatch::IntegrationTest assert_operator stored_credential.sign_count, :>, 0 end + test "verify_webauthn rejects authentication when session creation fails" do + @user.setup_mfa! + @user.enable_mfa! + client = register_webauthn_credential + stored_credential = @user.webauthn_credentials.first + sign_out + + post sessions_path, params: { email: @user.email, password: user_password_test } + post webauthn_options_mfa_path, as: :json + options = JSON.parse(response.body) + assertion = client.get( + challenge: options.fetch("challenge"), + rp_id: "www.example.com", + allow_credentials: [ stored_credential.credential_id ] + ) + MfaController.any_instance.stubs(:create_session_for).returns(false) + + post verify_webauthn_mfa_path, params: { credential: assertion }, as: :json + + assert_response :unprocessable_entity + assert_equal I18n.t("mfa.verify_webauthn.invalid_credential"), JSON.parse(response.body).fetch("error") + assert_not Session.exists?(user_id: @user.id) + end + test "verify_webauthn authenticates with configured relying party id" do with_webauthn_config(rp_id: "example.test", allowed_origins: [ "https://app.example.test" ]) do @user.setup_mfa! diff --git a/test/controllers/oidc_accounts_controller_test.rb b/test/controllers/oidc_accounts_controller_test.rb index 94873521f..4e255c936 100644 --- a/test/controllers/oidc_accounts_controller_test.rb +++ b/test/controllers/oidc_accounts_controller_test.rb @@ -46,6 +46,22 @@ class OidcAccountsControllerTest < ActionController::TestCase ) end + test "rolls back identity linking when session creation fails" do + session[:pending_oidc_auth] = pending_auth + @controller.stubs(:create_session_for).returns(false) + + assert_no_difference [ "OidcIdentity.count", "SsoAuditLog.count" ] do + post :create_link, + params: { + email: @user.email, + password: user_password_test + } + end + + assert_redirected_to new_session_path + assert session[:pending_oidc_auth].present? + end + test "should reject linking with invalid password" do session[:pending_oidc_auth] = pending_auth @@ -61,6 +77,22 @@ class OidcAccountsControllerTest < ActionController::TestCase assert_equal "Invalid email or password", flash[:alert] end + test "should reject linking an identity to an inactive user" do + @user.update_column(:active, false) + session[:pending_oidc_auth] = pending_auth + + assert_no_difference "OidcIdentity.count" do + post :create_link, + params: { + email: @user.email, + password: user_password_test + } + end + + assert_response :unprocessable_entity + assert_equal "Invalid email or password", flash[:alert] + end + test "should redirect to MFA when user has MFA enabled" do @user.setup_mfa! @user.enable_mfa! @@ -261,6 +293,19 @@ class OidcAccountsControllerTest < ActionController::TestCase assert_nil User.find_by(email: auth["email"]) end + test "create_user rolls back onboarding when session creation fails" do + session[:pending_oidc_auth] = new_user_auth + @controller.stubs(:create_session_for).returns(false) + + assert_no_difference [ "User.count", "OidcIdentity.count", "Family.count" ] do + post :create_user + end + + assert_response :unprocessable_entity + assert_nil User.find_by(email: new_user_auth["email"]) + assert session[:pending_oidc_auth].present? + end + test "should create session after OIDC registration" do session[:pending_oidc_auth] = new_user_auth @@ -341,4 +386,55 @@ class OidcAccountsControllerTest < ActionController::TestCase assert_equal family.id, invitee.family_id assert_equal 0, AccountShare.where(user: invitee).count end + + # A pending_oidc_auth stashed in the session outlives the removal that blocks + # its identity, so every action that consumes it has to re-check the block. + + test "create_user refuses a pending auth whose identity was removed" do + SsoIdentityBlock.create!( + provider: pending_auth["provider"], + uid_digest: SsoIdentityBlock.digest(pending_auth["uid"]), + identity_label: pending_auth["email"] + ) + session[:pending_oidc_auth] = pending_auth.merge("email" => "blocked-jit@example.com") + + assert_no_difference "User.count" do + post :create_user + end + + assert_redirected_to new_session_path + assert_nil session[:pending_oidc_auth] + end + + test "create_link refuses a pending auth whose identity was removed" do + SsoIdentityBlock.create!( + provider: pending_auth["provider"], + uid_digest: SsoIdentityBlock.digest(pending_auth["uid"]), + identity_label: pending_auth["email"] + ) + session[:pending_oidc_auth] = pending_auth + + assert_no_difference "OidcIdentity.count" do + post :create_link, params: { email: @user.email, password: user_password_test } + end + + assert_redirected_to new_session_path + assert_nil session[:pending_oidc_auth] + end + + test "link and new_user refuse a pending auth whose identity was removed" do + SsoIdentityBlock.create!( + provider: pending_auth["provider"], + uid_digest: SsoIdentityBlock.digest(pending_auth["uid"]), + identity_label: pending_auth["email"] + ) + + session[:pending_oidc_auth] = pending_auth + get :link + assert_redirected_to new_session_path + + session[:pending_oidc_auth] = pending_auth + get :new_user + assert_redirected_to new_session_path + end end diff --git a/test/controllers/pages_controller_test.rb b/test/controllers/pages_controller_test.rb index 434b76192..757cf9227 100644 --- a/test/controllers/pages_controller_test.rb +++ b/test/controllers/pages_controller_test.rb @@ -14,6 +14,16 @@ class PagesControllerTest < ActionDispatch::IntegrationTest assert_response :ok end + test "inactive user's existing session is revoked" do + session_record = @user.sessions.order(:created_at).last + @user.update_column(:active, false) + + get root_path + + assert_redirected_to new_session_path + assert_not Session.exists?(session_record.id) + end + test "update_preferences persists dashboard section layout height" do patch "/dashboard/preferences", params: { preferences: { dashboard_section_layout: { net_worth_chart: { height: "compact" } } } diff --git a/test/controllers/passkey_sessions_controller_test.rb b/test/controllers/passkey_sessions_controller_test.rb index f38787822..07b1d8e23 100644 --- a/test/controllers/passkey_sessions_controller_test.rb +++ b/test/controllers/passkey_sessions_controller_test.rb @@ -25,6 +25,16 @@ class PasskeySessionsControllerTest < ActionDispatch::IntegrationTest assert_operator @stored_credential.sign_count, :>, 0 end + test "rejects passkey authentication when session creation fails" do + PasskeySessionsController.any_instance.stubs(:create_session_for).returns(false) + + post passkey_session_path, params: { credential: passkey_assertion }, as: :json + + assert_response :unprocessable_entity + assert_equal I18n.t("passkey_sessions.invalid_credential"), JSON.parse(response.body).fetch("error") + assert_not Session.exists?(user_id: @user.id) + end + # The pending invitation lives in the Rack session, and complete_sign_in reads # it immediately after creating the session. Anything that clears the session # in between — a reset_session added to "fix" session fixation, say — drops the diff --git a/test/controllers/registrations_controller_test.rb b/test/controllers/registrations_controller_test.rb index fb14a7b48..8f40f7f1b 100644 --- a/test/controllers/registrations_controller_test.rb +++ b/test/controllers/registrations_controller_test.rb @@ -14,6 +14,19 @@ class RegistrationsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to root_url end + test "create rolls back registration when session creation fails" do + RegistrationsController.any_instance.stubs(:create_session_for).returns(false) + + assert_no_difference "User.count" do + post registration_url, params: { user: { + email: "session-failure@example.com", + password: "Password1!" } } + end + + assert_response :unprocessable_entity + assert_nil User.find_by(email: "session-failure@example.com") + end + test "first user of instance becomes super_admin" do # Clear all users to simulate fresh instance User.destroy_all diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 56291a645..1625c0486 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -72,6 +72,18 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_response :success end + test "does not issue a session when user is deleted before row locking" do + @user.stubs(:with_lock).raises(ActiveRecord::RecordNotFound) + User.stubs(:authenticate_by).returns(@user) + + assert_no_difference("Session.count") do + post sessions_url, params: { email: @user.email, password: user_password_test } + end + + assert_redirected_to new_session_url + assert cookies[:session_token].blank? + end + test "fails to sign in with bad password" do post sessions_url, params: { email: @user.email, password: "bad" } assert_response :unprocessable_entity @@ -188,6 +200,27 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert Session.exists?(user_id: @user.id) end + test "rejects an SSO identity that an administrator permanently removed" do + oidc_identity = oidc_identities(:bob_google) + SsoIdentityBlock.block_all!(OidcIdentity.where(id: oidc_identity.id), identity_label: @user.email) + @user.sessions.destroy_all + + setup_omniauth_mock( + provider: oidc_identity.provider, + uid: oidc_identity.uid, + email: @user.email, + name: "Bob Dylan" + ) + + assert_difference -> { SsoAuditLog.by_event("login_failed").count }, 1 do + get "/auth/openid_connect/callback" + end + + assert_redirected_to new_session_path + assert_not Session.exists?(user_id: @user.id) + assert_equal "removed_identity", SsoAuditLog.by_event("login_failed").order(:created_at).last.metadata.fetch("reason") + end + test "redirects to MFA when user has MFA and uses OIDC" do @user.setup_mfa! @user.enable_mfa! @@ -768,6 +801,36 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest Rails.cache = original_cache end + test "desktop SSO exchange refuses a code minted before the user was removed" do + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + + verifier = SecureRandom.hex(32) + challenge = Base64.urlsafe_encode64(Digest::SHA256.digest(verifier), padding: false) + oidc_identity = oidc_identities(:bob_google) + + Rails.configuration.x.auth.stubs(:sso_providers).returns([ + { name: "openid_connect", strategy: "openid_connect", label: "Google" } + ]) + setup_omniauth_mock(provider: oidc_identity.provider, uid: oidc_identity.uid, email: @user.email, name: "Bob Dylan") + + get "/auth/desktop/openid_connect", params: { code_challenge: challenge } + get "/auth/openid_connect/callback" + code = Rack::Utils.parse_query(URI.parse(@response.redirect_url).query)["code"] + assert code.present? + + # The removal lands after the code was minted. Nothing revoked the session + # this exchange is about to create, because it does not exist yet. + oidc_identity.user.update_column(:active, false) + + assert_no_difference -> { oidc_identity.user.sessions.count } do + post desktop_sso_exchange_path, params: { code: code, code_verifier: verifier } + end + assert_redirected_to new_session_path + ensure + Rails.cache = original_cache + end + test "desktop_sso_start rejects a missing PKCE code_challenge" do Rails.configuration.x.auth.stubs(:sso_providers).returns([ { name: "openid_connect", strategy: "openid_connect", label: "Google" } diff --git a/test/models/mobile_device_test.rb b/test/models/mobile_device_test.rb index 4e2d777bd..7745a2a59 100644 --- a/test/models/mobile_device_test.rb +++ b/test/models/mobile_device_test.rb @@ -20,4 +20,18 @@ class MobileDeviceTest < ActiveSupport::TestCase assert_not app.confidential end end + + test "inactive users cannot receive new mobile tokens" do + user = users(:family_member) + device = user.mobile_devices.create!( + device_id: "inactive-token-test", + device_name: "Inactive test device", + device_type: "ios" + ) + user.update_column(:active, false) + + assert_no_difference "Doorkeeper::AccessToken.count" do + assert_raises(ActiveRecord::RecordInvalid) { device.issue_token! } + end + end end diff --git a/test/models/oidc_identity_test.rb b/test/models/oidc_identity_test.rb index 95da294f7..e6d6a9a10 100644 --- a/test/models/oidc_identity_test.rb +++ b/test/models/oidc_identity_test.rb @@ -201,4 +201,21 @@ class OidcIdentityTest < ActiveSupport::TestCase assert_equal @user, identity.user assert_not_nil identity.last_authenticated_at end + + test "refuses to create an identity after it is blocked" do + auth = OmniAuth::AuthHash.new({ + provider: "google_oauth2", + uid: "blocked-google-subject", + info: { email: "blocked@example.com" } + }) + existing = OidcIdentity.create_from_omniauth(auth, @user) + SsoIdentityBlock.block_all!(OidcIdentity.where(id: existing.id), identity_label: @user.email) + existing.destroy! + + assert_no_difference "OidcIdentity.count" do + assert_raises(SsoIdentityBlock::BlockedIdentity) do + OidcIdentity.create_from_omniauth(auth, @user) + end + end + end end diff --git a/test/models/sso_identity_block_test.rb b/test/models/sso_identity_block_test.rb new file mode 100644 index 000000000..63bb48581 --- /dev/null +++ b/test/models/sso_identity_block_test.rb @@ -0,0 +1,41 @@ +require "test_helper" + +class SsoIdentityBlockTest < ActiveSupport::TestCase + test "blocks an identity without storing its raw subject identifier" do + identity = oidc_identities(:bob_google) + + SsoIdentityBlock.block_all!(OidcIdentity.where(id: identity.id), identity_label: identity.user.email) + + assert SsoIdentityBlock.blocked?(provider: identity.provider, uid: identity.uid) + assert_not_equal identity.uid, SsoIdentityBlock.last.uid_digest + end + + test "blocking the same identity is idempotent" do + identity = oidc_identities(:bob_google) + + assert_difference -> { SsoIdentityBlock.count }, 1 do + 2.times { SsoIdentityBlock.block_all!(OidcIdentity.where(id: identity.id), identity_label: identity.user.email) } + end + end + + test "uses a keyed digest instead of a plain subject hash" do + uid = "predictable-subject" + + assert_equal SsoIdentityBlock.digest(uid), SsoIdentityBlock.digest(uid) + assert_not_equal Digest::SHA256.hexdigest(uid), SsoIdentityBlock.digest(uid) + end + + test "does not store the identity label in plaintext without encryption" do + SsoIdentityBlock.stubs(:encryption_ready?).returns(false) + raw_label = "removed-user@example.com" + + block = SsoIdentityBlock.create!( + provider: "openid_connect", + uid_digest: SsoIdentityBlock.digest("removed-subject"), + identity_label: raw_label + ) + + assert_not_equal raw_label, block.identity_label + assert_match(/Removed identity/, block.identity_label) + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index c8a4d9bd0..0daaf5143 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -792,6 +792,59 @@ class UserTest < ActiveSupport::TestCase assert_not ActiveStorage::Attachment.exists?(attachment_id) end + # Admin-initiated permanent removal (super-admin action) + test "permanently_remove! deactivates, revokes all credentials, and schedules purge" do + target = users(:family_member) + target.sessions.create! + assert target.sessions.exists? + assert target.api_keys.exists? + assert target.oidc_identities.exists? + + assert target.permanently_remove! + + target.reload + assert_not target.active? + assert_empty target.sessions + assert_empty target.api_keys + assert_empty target.oidc_identities + end + + test "permanently_remove! is blocked (fail-closed) for an admin with co-members and keeps credentials" do + target = users(:family_admin) + target.sessions.create! + assert_operator target.family.users.count, :>, 1 + + assert_not target.permanently_remove! + + assert target.reload.active? + assert target.sessions.exists? + assert target.oidc_identities.exists? + end + + test "permanently_remove! schedules purge for an already inactive user" do + target = users(:family_member) + target.update_column(:active, false) + + assert_enqueued_with(job: UserPurgeJob, args: [ target ]) do + assert target.permanently_remove! + end + end + + test "deactivate refuses the last active super admin" do + family = Family.create!(name: "Sole admin family", locale: "en", date_format: "%m-%d-%Y", currency: "USD") + target = User.create!( + family: family, + email: "sole-super-admin@example.com", + password: user_password_test, + role: :super_admin + ) + User.where(role: :super_admin).where.not(id: target.id).update_all(active: false) + + assert_not target.deactivate + assert target.reload.active? + assert_match(/last active super admin/, target.errors.full_messages.to_sentence) + end + test "purging the last user cascades to remove family and its export attachments" do family = Family.create!(name: "Solo Family", locale: "en", date_format: "%m-%d-%Y", currency: "USD") user = User.create!(family: family, email: "solo@example.com", password: "password123") diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index c4d471d94..aecb2f8d3 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -42,6 +42,22 @@ class UserPolicyTest < ActiveSupport::TestCase assert_not UserPolicy.new(nil, @regular_user).update? end + test "super admin can destroy another user" do + assert UserPolicy.new(@super_admin, @regular_user).destroy? + end + + test "super admin cannot destroy themselves" do + assert_not UserPolicy.new(@super_admin, @super_admin).destroy? + end + + test "regular user cannot destroy anyone" do + assert_not UserPolicy.new(@regular_user, @other_user).destroy? + end + + test "nil user cannot destroy anyone" do + assert_not UserPolicy.new(nil, @regular_user).destroy? + end + test "scope returns all users for super admin" do scope = UserPolicy::Scope.new(@super_admin, User).resolve assert_equal User.count, scope.count diff --git a/test/system/admin_user_removals_test.rb b/test/system/admin_user_removals_test.rb new file mode 100644 index 000000000..30733bfd9 --- /dev/null +++ b/test/system/admin_user_removals_test.rb @@ -0,0 +1,51 @@ +require "application_system_test_case" + +class AdminUserRemovalsTest < ApplicationSystemTestCase + include ActiveJob::TestHelper + + setup do + @admin = users(:sure_support_staff) + @target = users(:family_member) + @target_email = @target.email + @identity = @target.oidc_identities.first! + @provider = @identity.provider + @uid = @identity.uid + end + + test "super admin confirms removal and the SSO identity cannot return" do + sign_in @admin + visit admin_users_path + + find("details", text: @target.family.name).find("summary").click + + within find("tr", text: @target_email) do + click_on "Remove" + end + + within "dialog[open]" do + assert_text "This immediately revokes access" + fill_in "User email", with: @target_email + click_on "Permanently remove user" + end + + assert_text "User access revoked and permanent deletion scheduled." + assert_not @target.reload.active? + assert_empty @target.sessions + assert_empty @target.oidc_identities + assert SsoIdentityBlock.blocked?(provider: @provider, uid: @uid) + + OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new( + provider: @provider, + uid: @uid, + info: { email: @target_email, name: "Removed SSO User" } + ) + + visit "/auth/openid_connect/callback" + + assert_current_path new_session_path + assert_text "Could not authenticate via OpenID Connect." + assert_not User.exists?(email: @target_email) + ensure + OmniAuth.config.mock_auth[:openid_connect] = nil + end +end