diff --git a/app/controllers/api/v1/auth_controller.rb b/app/controllers/api/v1/auth_controller.rb index e522fb03f..f4fe22023 100644 --- a/app/controllers/api/v1/auth_controller.rb +++ b/app/controllers/api/v1/auth_controller.rb @@ -12,6 +12,12 @@ class AuthController < BaseController before_action :log_api_access, only: :enable_ai def signup + # Block signups when registration is closed (self-hosted) + if Rails.application.config.app_mode.self_hosted? && Setting.onboarding_state == "closed" + render json: { error: "Registration is currently closed" }, status: :forbidden + return + end + # Check if invite code is required if invite_code_required? && params[:invite_code].blank? render json: { error: "Invite code is required" }, status: :forbidden @@ -65,9 +71,21 @@ def signup end def login + # Enforce AuthConfig — respect SSO-only mode for API clients too + unless AuthConfig.local_login_enabled? + render json: { error: "Local login is disabled. Please use SSO." }, status: :forbidden + return + end + user = User.find_by(email: params[:email]) if user&.authenticate(params[:password]) + # Reject deactivated users + unless user.active? + render json: { error: "Account has been deactivated" }, status: :unauthorized + return + end + # Check MFA if enabled if user.otp_required? unless params[:otp_code].present? && user.verify_otp?(params[:otp_code]) @@ -258,7 +276,23 @@ def refresh return end - # Create new access token + # Reject deactivated or deleted users BEFORE issuing a fresh token. + # If we checked after creation, a concurrent request could briefly pass + # the OAuth gate in base_controller against `new_token` — and we'd also + # do pointless writes. Also matches the 401-for-missing-user shape used + # by api/v1/base_controller#authenticate_oauth (use find_by, not find). + user = User.find_by(id: access_token.resource_owner_id) + unless user&.active? + # Revoke every outstanding access token for this resource owner so + # other devices lose access immediately. + Doorkeeper::AccessToken + .where(resource_owner_id: access_token.resource_owner_id, revoked_at: nil) + .find_each(&:revoke) + render json: { error: "Account has been deactivated" }, status: :unauthorized + return + end + + # Create new access token (only after the active check passes). new_token = Doorkeeper::AccessToken.create!( application: access_token.application, resource_owner_id: access_token.resource_owner_id, @@ -272,7 +306,6 @@ def refresh access_token.revoke # 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! diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index d768bdf3f..053d5eb91 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -56,7 +56,8 @@ def authenticate_oauth # Check token validity and scope (read_write includes read access) has_sufficient_scope = access_token&.scopes&.include?("read") || access_token&.scopes&.include?("read_write") - unless access_token && !access_token.expired? && has_sufficient_scope + # Also reject revoked tokens (revocation is meaningless if we still accept them) + unless access_token && !access_token.expired? && !access_token.revoked? && has_sufficient_scope render_json({ error: "unauthorized", message: "Access token is invalid, expired, or missing required scope" }, status: :unauthorized) return false end diff --git a/app/controllers/mfa_controller.rb b/app/controllers/mfa_controller.rb index 987170a9c..ca0cf3cd8 100644 --- a/app/controllers/mfa_controller.rb +++ b/app/controllers/mfa_controller.rb @@ -1,6 +1,7 @@ class MfaController < ApplicationController layout :determine_layout skip_authentication only: [ :verify, :verify_code ] + before_action :block_sso_only_users, only: [ :new, :create, :disable ] def new redirect_to root_path if Current.user.otp_required? @@ -8,6 +9,14 @@ def new end def create + unless password_reauth_ok? + # Do NOT call disable_mfa! here — a wrong password during enable should + # not wipe the in-progress otp_secret / backup codes. Only an actual + # code mismatch below invalidates the setup. + redirect_to new_mfa_path, alert: t(".invalid_password") + return + end + if Current.user.verify_otp?(params[:code]) Current.user.enable_mfa! @backup_codes = Current.user.otp_backup_codes @@ -41,12 +50,36 @@ def verify_code end def disable + unless password_reauth_ok? + redirect_to settings_security_path, alert: t(".invalid_password") + return + end + Current.user.disable_mfa! redirect_to settings_security_path, notice: t(".success") end private + # SSO-only users can't satisfy the password prompt MFA enable/disable + # requires (F-11). Block every MFA management action for them — including + # GET /mfa/new, which would otherwise call setup_mfa! and leave an + # incomplete otp_secret the user can never finish wiring up. The view + # already hides the enable/disable UI for these users; this is the + # server-side backstop for a direct-URL visit. + def block_sso_only_users + return if Current.user.password_digest.present? + redirect_to settings_security_path, alert: t("mfa.new.sso_only_not_supported", default: "Two-factor authentication requires a local password — it is managed through your identity provider.") + end + + # Password re-auth for sensitive MFA operations. SSO-only users are + # already rejected by block_sso_only_users, but the guard here is kept + # for defense-in-depth. + def password_reauth_ok? + return false if Current.user.password_digest.blank? + Current.user.authenticate(params[:password]).present? + end + def determine_layout if action_name.in?(%w[verify verify_code]) "auth" diff --git a/app/models/api_key.rb b/app/models/api_key.rb index ae24ccec3..ef6980703 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -56,7 +56,7 @@ def expired? end def key_matches?(plain_key) - display_key == plain_key + ActiveSupport::SecurityUtils.secure_compare(display_key.to_s, plain_key.to_s) end def revoke! diff --git a/app/models/user.rb b/app/models/user.rb index ade5ee4cb..85507ec4c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -449,8 +449,9 @@ def totp def verify_backup_code?(code) return false if otp_backup_codes.blank? - # Find and remove the used backup code - if (index = otp_backup_codes.index(code)) + # Use constant-time comparison to prevent timing attacks + index = otp_backup_codes.index { |stored| ActiveSupport::SecurityUtils.secure_compare(stored.to_s, code.to_s) } + if index remaining_codes = otp_backup_codes.dup remaining_codes.delete_at(index) update!(otp_backup_codes: remaining_codes) diff --git a/app/views/mfa/new.html.erb b/app/views/mfa/new.html.erb index c8ad0892f..325b3c10b 100644 --- a/app/views/mfa/new.html.erb +++ b/app/views/mfa/new.html.erb @@ -46,7 +46,7 @@ <%= styled_form_with url: mfa_path, method: :post, class: "mt-5", data: { turbo: false } do |f| %> -
+
<%= f.text_field :code, required: true, autofocus: true, @@ -56,6 +56,12 @@ label: t(".code_label"), placeholder: t(".code_placeholder") %> + <%= f.password_field :password, + required: true, + autocomplete: "current-password", + label: t(".password_label"), + placeholder: t(".password_placeholder") %> +
<%= f.submit t(".verify_button") %>
diff --git a/app/views/settings/securities/show.html.erb b/app/views/settings/securities/show.html.erb index b7866c225..5d46ef6e9 100644 --- a/app/views/settings/securities/show.html.erb +++ b/app/views/settings/securities/show.html.erb @@ -20,19 +20,24 @@
- <% if Current.user.otp_required? %> - <%= render DS::Button.new( - text: t(".disable_mfa"), - variant: "secondary", - href: disable_mfa_path, - method: :delete, - confirm: CustomConfirm.new( - title: t(".disable_mfa_confirm"), - body: t(".disable_mfa_confirm"), - btn_text: t(".disable_mfa"), - destructive: true - ) - ) %> + <% if Current.user.password_digest.blank? %> + <%# SSO-only user — password re-auth is impossible. Surface a hint + instead of the enable/disable form they could never submit. %> +

<%= t(".mfa_sso_only") %>

+ <% elsif Current.user.otp_required? %> +
> + + <%= t(".disable_mfa") %> + + <%= styled_form_with url: disable_mfa_path, method: :delete, class: "mt-3 space-y-3", data: { turbo: false } do |f| %> + <%= f.password_field :password, + required: true, + autocomplete: "current-password", + label: t(".disable_mfa_password_label"), + placeholder: t(".disable_mfa_password_placeholder") %> + <%= f.submit t(".disable_mfa") %> + <% end %> +
<% else %> <%= render DS::Link.new( text: t(".enable_mfa"), diff --git a/config/locales/views/mfa/en.yml b/config/locales/views/mfa/en.yml index 786f52594..09ba7ea48 100644 --- a/config/locales/views/mfa/en.yml +++ b/config/locales/views/mfa/en.yml @@ -12,14 +12,19 @@ en: title: Save Your Backup Codes create: invalid_code: Invalid verification code. Please try again. + invalid_password: Incorrect password. Please try again. disable: + invalid_password: Incorrect password. Please try again. success: Two-factor authentication has been disabled new: code_label: Verification Code code_placeholder: Enter 6-digit code description: Enhance your account security by setting up two-factor authentication page_title: Two-Factor Authentication Setup + password_label: Password + password_placeholder: Enter your password scan_description: Use an authenticator app like Google Authenticator or 1Password + sso_only_not_supported: Two-factor authentication requires a local password — it is managed through your identity provider. to scan this QR code scan_title: 1. Scan QR Code secret_description: If you can't scan the QR code, enter this secret key manually diff --git a/config/locales/views/settings/de.yml b/config/locales/views/settings/de.yml index e8a64d97e..4e4af720c 100644 --- a/config/locales/views/settings/de.yml +++ b/config/locales/views/settings/de.yml @@ -102,6 +102,9 @@ de: enable_mfa: 2FA aktivieren disable_mfa: 2FA deaktivieren disable_mfa_confirm: Bist du sicher, dass du die Zwei-Faktor-Authentifizierung deaktivieren möchtest? + disable_mfa_password_label: Mit deinem Passwort bestätigen + disable_mfa_password_placeholder: Passwort eingeben + mfa_sso_only: Die Zwei-Faktor-Authentifizierung wird über deinen Identitätsanbieter verwaltet. sso_title: Verbundene Konten sso_subtitle: Verwalte deine Single Sign-On-Kontoverbindungen sso_disconnect: Trennen diff --git a/config/locales/views/settings/en.yml b/config/locales/views/settings/en.yml index 79e7c69d3..b8eec5354 100644 --- a/config/locales/views/settings/en.yml +++ b/config/locales/views/settings/en.yml @@ -138,6 +138,9 @@ en: enable_mfa: Enable 2FA disable_mfa: Disable 2FA disable_mfa_confirm: Are you sure you want to disable two-factor authentication? + disable_mfa_password_label: Confirm with your password + disable_mfa_password_placeholder: Enter your password + mfa_sso_only: Two-factor authentication is managed through your identity provider. sso_title: Connected Accounts sso_subtitle: Manage your single sign-on account connections sso_disconnect: Disconnect diff --git a/config/locales/views/settings/es.yml b/config/locales/views/settings/es.yml index b9346a547..74724940b 100644 --- a/config/locales/views/settings/es.yml +++ b/config/locales/views/settings/es.yml @@ -103,6 +103,9 @@ es: enable_mfa: Activar 2FA disable_mfa: Desactivar 2FA disable_mfa_confirm: ¿Estás seguro de que deseas desactivar la autenticación de dos factores? + disable_mfa_password_label: Confirma con tu contraseña + disable_mfa_password_placeholder: Introduce tu contraseña + mfa_sso_only: La autenticación de dos factores se gestiona a través de tu proveedor de identidad. sso_title: Cuentas Conectadas sso_subtitle: Gestiona tus conexiones de inicio de sesión único (SSO) sso_disconnect: Desconectar diff --git a/config/locales/views/settings/fr.yml b/config/locales/views/settings/fr.yml index e3179c63f..cee25821a 100644 --- a/config/locales/views/settings/fr.yml +++ b/config/locales/views/settings/fr.yml @@ -136,6 +136,9 @@ fr: enable_mfa: Activer 2FA disable_mfa: Désactiver 2FA disable_mfa_confirm: Êtes-vous sûr(e) de vouloir désactiver l'authentification à deux facteurs ? + disable_mfa_password_label: Confirmez avec votre mot de passe + disable_mfa_password_placeholder: Entrez votre mot de passe + mfa_sso_only: L'authentification à deux facteurs est gérée par votre fournisseur d'identité. sso_title: Comptes connectés sso_subtitle: Gérez vos connexions de compte à authentification unique sso_disconnect: Déconnecter diff --git a/config/locales/views/settings/nl.yml b/config/locales/views/settings/nl.yml index a74136bbe..eb9155479 100644 --- a/config/locales/views/settings/nl.yml +++ b/config/locales/views/settings/nl.yml @@ -99,6 +99,9 @@ nl: enable_mfa: 2FA inschakelen disable_mfa: 2FA uitschakelen disable_mfa_confirm: Weet u zeker dat u twee-factor authenticatie wilt uitschakelen? + disable_mfa_password_label: Bevestig met uw wachtwoord + disable_mfa_password_placeholder: Voer uw wachtwoord in + mfa_sso_only: Twee-factor authenticatie wordt beheerd via uw identiteitsprovider. sso_title: Gekoppelde Accounts sso_subtitle: Beheer uw single sign-on accountverbindingen sso_disconnect: Loskoppelen diff --git a/config/locales/views/settings/pl.yml b/config/locales/views/settings/pl.yml index 9962be6a7..2d32e06ba 100644 --- a/config/locales/views/settings/pl.yml +++ b/config/locales/views/settings/pl.yml @@ -119,6 +119,9 @@ pl: enable_mfa: Włącz 2FA disable_mfa: Wyłącz 2FA disable_mfa_confirm: Czy na pewno chcesz wyłączyć uwierzytelnianie dwuskładnikowe? + disable_mfa_password_label: Potwierdź hasłem + disable_mfa_password_placeholder: Wpisz hasło + mfa_sso_only: Uwierzytelnianie dwuskładnikowe jest zarządzane przez dostawcę tożsamości. sso_title: Połączone konta sso_subtitle: Zarządzaj połączeniami kont jednokrotnego logowania sso_disconnect: Odłącz diff --git a/config/locales/views/settings/pt-BR.yml b/config/locales/views/settings/pt-BR.yml index 26fa9fa13..1c19eeea6 100644 --- a/config/locales/views/settings/pt-BR.yml +++ b/config/locales/views/settings/pt-BR.yml @@ -121,6 +121,9 @@ pt-BR: enable_mfa: Ativar 2FA disable_mfa: Desativar 2FA disable_mfa_confirm: Tem certeza de que deseja desativar a autenticação de dois fatores? + disable_mfa_password_label: Confirme com sua senha + disable_mfa_password_placeholder: Digite sua senha + mfa_sso_only: A autenticação de dois fatores é gerenciada pelo seu provedor de identidade. sso_title: Contas Conectadas sso_subtitle: Gerencie suas conexões de contas de login único (SSO) sso_disconnect: Desconectar diff --git a/test/controllers/api/v1/auth_controller_test.rb b/test/controllers/api/v1/auth_controller_test.rb index 52207df3a..a023fbae2 100644 --- a/test/controllers/api/v1/auth_controller_test.rb +++ b/test/controllers/api/v1/auth_controller_test.rb @@ -818,4 +818,74 @@ class Api::V1::AuthControllerTest < ActionDispatch::IntegrationTest assert_equal "AI is not available for your account", response_data["error"] assert_not user.reload.ai_enabled end + + # Security tests — ported from origin/security/pentest-2026-03-02 + + test "signup is blocked when registration is closed on self-hosted" do + Rails.application.config.app_mode.stubs(:self_hosted?).returns(true) + Setting.stubs(:onboarding_state).returns("closed") + + post "/api/v1/auth/signup", params: { + user: { email: "new@example.com", password: "SecurePass123!", first_name: "New", last_name: "User" }, + device: @device_info + } + + assert_response :forbidden + assert_equal "Registration is currently closed", JSON.parse(response.body)["error"] + end + + test "login is blocked when local login disabled via AuthConfig" do + AuthConfig.stubs(:local_login_enabled?).returns(false) + + post "/api/v1/auth/login", params: { + email: users(:family_admin).email, + password: user_password_test, + device: @device_info + } + + assert_response :forbidden + assert_equal "Local login is disabled. Please use SSO.", JSON.parse(response.body)["error"] + end + + test "refresh token is rejected for deactivated user and new token is revoked" do + user = users(:family_member) + 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!(active: false) + + post "/api/v1/auth/refresh", params: { + refresh_token: initial_token.refresh_token, + device: @device_info + } + + assert_response :unauthorized + assert_equal "Account has been deactivated", JSON.parse(response.body)["error"] + + # All tokens for this user must be revoked (including any newly issued one) + assert Doorkeeper::AccessToken.where(resource_owner_id: user.id).all?(&:revoked?), + "Expected all tokens to be revoked for deactivated user" + end + + test "login is rejected for deactivated user" do + user = users(:family_member) + user.update!(active: false) + + post "/api/v1/auth/login", params: { + email: user.email, + password: user_password_test, + device: @device_info + } + + assert_response :unauthorized + assert_equal "Account has been deactivated", JSON.parse(response.body)["error"] + end end diff --git a/test/controllers/mfa_controller_test.rb b/test/controllers/mfa_controller_test.rb index b653df41a..ac1d5c170 100644 --- a/test/controllers/mfa_controller_test.rb +++ b/test/controllers/mfa_controller_test.rb @@ -33,7 +33,7 @@ def sign_out @user.setup_mfa! totp = ROTP::TOTP.new(@user.otp_secret, issuer: "Sure Finances") - post mfa_path, params: { code: totp.now } + post mfa_path, params: { code: totp.now, password: user_password_test } assert_response :success assert @user.reload.otp_required? @@ -44,13 +44,65 @@ def sign_out test "does not enable MFA with invalid code" do @user.setup_mfa! - post mfa_path, params: { code: "invalid" } + post mfa_path, params: { code: "invalid", password: user_password_test } assert_redirected_to new_mfa_path assert_not @user.reload.otp_required? assert_empty @user.otp_backup_codes end + test "enables MFA only with correct password" do + @user.setup_mfa! + totp = ROTP::TOTP.new(@user.otp_secret, issuer: "Sure Finances") + + post mfa_path, params: { code: totp.now, password: "wrongpassword" } + + @user.reload + assert_not @user.otp_required? + assert_redirected_to new_mfa_path + # A wrong password during enable must NOT wipe the in-progress setup; + # only an actual code mismatch / session expiry should do that. Prevents + # a single typo from forcing a QR re-scan. + assert @user.otp_secret.present?, "otp_secret should survive a wrong-password attempt" + end + + test "SSO-only users (no password_digest) cannot enable MFA" do + @user.setup_mfa! + totp = ROTP::TOTP.new(@user.otp_secret, issuer: "Sure Finances") + @user.update_column(:password_digest, nil) # simulate SSO-only user + + post mfa_path, params: { code: totp.now, password: "anything" } + + assert_not @user.reload.otp_required? + assert_redirected_to settings_security_path + end + + test "SSO-only users cannot reach the MFA new page" do + # Direct GET to /mfa/new would previously call setup_mfa!, leaving the + # user with an otp_secret they could never submit a password for. The + # controller now redirects them to settings before touching state. + @user.update_column(:password_digest, nil) # simulate SSO-only user + + secret_before = @user.otp_secret + + get new_mfa_path + + assert_redirected_to settings_security_path + assert_equal secret_before, @user.reload.otp_secret, + "setup_mfa! should not run for SSO-only users on the new page" + end + + test "SSO-only users cannot disable MFA via password" do + @user.setup_mfa! + @user.enable_mfa! + @user.update_column(:password_digest, nil) # simulate SSO-only user + + delete disable_mfa_path, params: { password: "anything" } + + assert @user.reload.otp_required? + assert_redirected_to settings_security_path + end + test "verify shows MFA verification page" do @user.setup_mfa! @user.enable_mfa! @@ -109,11 +161,21 @@ def sign_out @user.setup_mfa! @user.enable_mfa! - delete disable_mfa_path + delete disable_mfa_path, params: { password: user_password_test } assert_redirected_to settings_security_path assert_not @user.reload.otp_required? assert_nil @user.otp_secret assert_empty @user.otp_backup_codes end + + test "disabling MFA requires correct password" do + @user.setup_mfa! + @user.enable_mfa! + + delete disable_mfa_path, params: { password: "wrongpassword" } + + assert @user.reload.otp_required? + assert_redirected_to settings_security_path + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 00696fce6..2656a3c2d 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -584,4 +584,32 @@ def teardown assert_not Family.exists?(family.id) assert_not ActiveStorage::Attachment.exists?(export_attachment_id) end + + # Security tests — ported from origin/security/pentest-2026-03-02 + + test "backup code cannot be reused after verification" do + user = users(:family_admin) + user.setup_mfa! + user.enable_mfa! + code = user.otp_backup_codes.first + + assert user.verify_otp?(code) + assert_not user.reload.verify_otp?(code), "Backup code should not be reusable" + end + + test "backup code rejects invalid code" do + user = users(:family_admin) + user.setup_mfa! + user.enable_mfa! + + assert_not user.verify_otp?("00000000") + end + + test "backup code verification handles blank input safely" do + user = users(:family_admin) + user.setup_mfa! + user.enable_mfa! + + assert_not user.verify_otp?("") + end end