Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions app/controllers/api/v1/auth_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -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,
Expand All @@ -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!

Expand Down
3 changes: 2 additions & 1 deletion app/controllers/api/v1/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 23 additions & 0 deletions app/controllers/mfa_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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
Expand Down Expand Up @@ -41,12 +49,27 @@ 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

# Password re-auth for sensitive MFA operations. SSO-only users (no local
# password) cannot satisfy a password prompt, so we treat their request as
# a failed re-auth here and rely on the view to hide the UI / SSO re-auth
# flow to surface a meaningful next step (app/views/settings/securities
# already gates the MFA block on `password_digest.present?`).
def password_reauth_ok?
return false if Current.user.password_digest.blank?
Current.user.authenticate(params[:password]).present?
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
end

def determine_layout
if action_name.in?(%w[verify verify_code])
"auth"
Expand Down
2 changes: 1 addition & 1 deletion app/models/api_key.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
5 changes: 3 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 7 additions & 1 deletion app/views/mfa/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
</div>

<%= styled_form_with url: mfa_path, method: :post, class: "mt-5", data: { turbo: false } do |f| %>
<div>
<div class="space-y-4">
<%= f.text_field :code,
required: true,
autofocus: true,
Expand All @@ -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") %>

<div class="flex justify-end mt-4">
<%= f.submit t(".verify_button") %>
</div>
Expand Down
31 changes: 18 additions & 13 deletions app/views/settings/securities/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,24 @@
</div>

<div class="mt-4 md:mt-0">
<% 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. %>
<p class="text-sm text-subdued"><%= t(".mfa_sso_only") %></p>
<% elsif Current.user.otp_required? %>
<details class="group" <%= "open" if flash[:alert].present? %>>
<summary class="cursor-pointer text-sm font-medium text-destructive">
<%= t(".disable_mfa") %>
</summary>
<%= 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 %>
</details>
<% else %>
<%= render DS::Link.new(
text: t(".enable_mfa"),
Expand Down
4 changes: 4 additions & 0 deletions config/locales/views/mfa/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@ 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
to scan this QR code
scan_title: 1. Scan QR Code
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/settings/de.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/settings/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +141 to +142
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find sibling locale files for the settings view and check which are missing the new keys.
fd -t f . config/locales/views/settings
for f in $(fd -t f . config/locales/views/settings); do
  echo "=== $f ==="
  rg -n 'disable_mfa_password_label|disable_mfa_password_placeholder' "$f" || echo "  MISSING"
done

Repository: we-promise/sure

Length of output: 6401


🏁 Script executed:

#!/bin/bash
# Check the YAML structure around lines 141-142
echo "=== Structure of en.yml around lines 141-142 ==="
sed -n '130,150p' config/locales/views/settings/en.yml

echo -e "\n=== Looking for disable_mfa references in views ==="
fd -e erb -e html.erb . app/views | xargs rg -l 'disable.mfa|disable_mfa' 2>/dev/null || echo "No matches found"

Repository: we-promise/sure

Length of output: 1195


🏁 Script executed:

#!/bin/bash
# Check if other securities keys are translated in non-English locales
echo "=== Checking fr.yml for other securities keys ==="
rg -A 5 'securities:' config/locales/views/settings/fr.yml | head -20

echo -e "\n=== Checking es.yml for other securities keys ==="
rg -A 5 'securities:' config/locales/views/settings/es.yml | head -20

echo -e "\n=== Git log to understand PR type ==="
git log --oneline --all -20 | grep -i 'mfa\|security\|disable' || echo "No recent commits with those keywords"

Repository: we-promise/sure

Length of output: 972


Add parallel keys to all supported locale files for the securities section.

The two new keys disable_mfa_password_label and disable_mfa_password_placeholder under securities.show in config/locales/views/settings/en.yml must be added to the corresponding sections in all other locale files (ca, de, es, fr, nb, nl, pl, pt-BR, ro, tr, zh-CN, zh-TW) to maintain consistency. The securities section is already translated in these locales, so missing these keys will cause the password confirmation fields to display in English when viewed in other languages.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@config/locales/views/settings/en.yml` around lines 141 - 142, Add the two new
keys disable_mfa_password_label and disable_mfa_password_placeholder under the
existing securities.show section in every locale file that already contains a
securities translation (ca, de, es, fr, nb, nl, pl, pt-BR, ro, tr, zh-CN, zh-TW)
so those languages display the localized password-confirmation label and
placeholder instead of English; open each locale's YAML, locate the
securities.show mapping and add the two keys with appropriate translations (or
placeholders if translators will fill them later) mirroring the structure used
in config/locales/views/settings/en.yml.

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
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/settings/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/settings/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/settings/nl.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/settings/pl.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/settings/pt-BR.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
70 changes: 70 additions & 0 deletions test/controllers/api/v1/auth_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]", 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
Loading
Loading