add TOTP two-factor authentication with recovery codes#8
Merged
9cb14c1ec0 merged 6 commits intomasterfrom Feb 23, 2026
Merged
Conversation
Optional per-user TOTP 2FA with two-phase login flow. When enabled, login returns a short-lived intermediate token, frontend shows a code entry form, and a second API call verifies the TOTP code to issue real JWTs. Includes Fernet-encrypted secrets at rest, bcrypt-hashed recovery codes, admin reset capability, and a user settings page for setup/disable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds TOTP-based two-factor authentication: backend TOTP service, new auth endpoints and schemas, user model and recovery code model plus migration, frontend settings/login/admin UI for setup/verify/disable/reset, and related config and dependency additions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant TOTPService
participant Database
User->>Frontend: Enter email/password
Frontend->>Backend: POST /auth/login
Backend->>Database: Verify credentials
Database-->>Backend: Return User (totp_enabled=true)
Backend->>Backend: create_totp_token(user_id)
Backend-->>Frontend: LoginResponse {totp_required:true, totp_token}
Frontend->>User: Prompt for TOTP code
User->>Frontend: Submit 6-digit code
Frontend->>Backend: POST /auth/verify-totp {totp_token, code}
Backend->>TOTPService: verify_login_code(user, code)
TOTPService->>Database: Check TOTP secret / recovery codes
Database-->>TOTPService: Verification result
TOTPService-->>Backend: Valid
Backend-->>Frontend: Tokens {access_token, refresh_token}
Frontend->>User: Redirect to dashboard
sequenceDiagram
participant User
participant Frontend
participant Backend
participant TOTPService
participant Database
User->>Frontend: Click "Enable 2FA"
Frontend->>Backend: POST /auth/totp/setup
Backend->>TOTPService: setup_begin(user)
TOTPService->>TOTPService: generate_secret & encrypt_secret
TOTPService->>Database: Save encrypted secret and recovery codes
TOTPService->>TOTPService: generate_qr_base64()
TOTPService-->>Backend: {secret, qr_code, recovery_codes}
Backend-->>Frontend: TOTPSetupResponse
User->>Frontend: Scan QR, enter first code
Frontend->>Backend: POST /auth/totp/setup/verify {code}
Backend->>TOTPService: setup_verify(user, code)
TOTPService->>TOTPService: verify_code(secret, code)
alt Code valid
TOTPService->>Database: Set user.totp_enabled = true
TOTPService-->>Backend: Success
Backend-->>Frontend: Success
else Code invalid
Backend-->>Frontend: Error
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
backend/app/schemas/auth.py (1)
24-31:LoginResponseshould enforce its discriminated-union invariants with amodel_validator.Nothing prevents
LoginResponse(totp_required=True, totp_token=None)orLoginResponse(totp_required=False, access_token=None)from being instantiated. A server-side bug could produce such a response and the client would silently receive inconsistent data (e.g.,totpToken.value = nullstored in Pinia state, thentotp_token: nullsent to/auth/verify-totp).♻️ Proposed fix (Pydantic v2)
+from pydantic import BaseModel, EmailStr, model_validator class LoginResponse(BaseModel): totp_required: bool = False access_token: str | None = None refresh_token: str | None = None token_type: str = "bearer" totp_token: str | None = None + + `@model_validator`(mode='after') + def check_token_fields(self) -> 'LoginResponse': + if self.totp_required: + if self.totp_token is None: + raise ValueError("totp_token must be provided when totp_required is True") + else: + if self.access_token is None or self.refresh_token is None: + raise ValueError("access_token and refresh_token must be provided when totp_required is False") + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/schemas/auth.py` around lines 24 - 31, Add a Pydantic v2 model validator to LoginResponse (use `@model_validator`(mode="after")) that enforces the discriminated-union invariants: if totp_required is True then totp_token must be not None and access_token/refresh_token must be None; if totp_required is False then access_token and refresh_token must be not None and totp_token must be None; raise a ValueError with a clear message when the invariant is violated so invalid LoginResponse(...) instantiations are rejected.backend/app/services/auth.py (1)
32-32: TOTP token TTL is hardcoded; consider making it configurable.
create_access_tokenandcreate_refresh_tokenboth usesettings.*_expire_*values.create_totp_tokenhardcodes 5 minutes with no corresponding setting, making it impossible to tune without a code change.♻️ Proposed change
+ # In Settings: + # totp_token_expire_minutes: int = 5 `@staticmethod` def create_totp_token(user_id: str) -> str: - expire = datetime.now(timezone.utc) + timedelta(minutes=5) + expire = datetime.now(timezone.utc) + timedelta(minutes=settings.totp_token_expire_minutes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/auth.py` at line 32, The TOTP TTL is hardcoded in create_totp_token; make it configurable by adding/using a setting (e.g., settings.totp_expire_minutes) instead of the literal "5" and compute expire = datetime.now(timezone.utc) + timedelta(minutes=settings.totp_expire_minutes), with a sensible default fallback (5) if the setting is absent; update any docs/config schema accordingly so create_totp_token reads the new setting like create_access_token/create_refresh_token do.backend/requirements.txt (1)
11-12: Consider pinningcryptographyas a direct dependency.
totp.pyusesFernetfrom thecryptographypackage directly. Currently it only arrives transitively viapython-jose[cryptography]. If that extras declaration ever changes, Fernet usage silently breaks. The pattern in this file already explicitly lists direct deps likebcrypt.📦 Suggested addition
pyotp>=2.9.0 qrcode[pil]>=7.4.0 +cryptography>=41.0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/requirements.txt` around lines 11 - 12, totp.py imports and uses Fernet from the cryptography package but cryptography is only a transitive extra via python-jose; add cryptography as an explicit direct dependency in backend/requirements.txt so Fernet won't break if python-jose's extras change—update requirements.txt to include a pinned cryptography entry (e.g., add a cryptography version constraint compatible with your CI/runtime), ensuring Fernet usage in totp.py remains supported.backend/app/api/admin.py (1)
97-109: Address unusedadminparameter warning.Ruff flags Line 98 as unused. A conventional fix is renaming to
_adminto keep the dependency while satisfying lint.♻️ Suggested adjustment
-@router.post("/users/{user_id}/reset-totp") -async def reset_user_totp(admin: AdminUser, user_id: UUID): +@router.post("/users/{user_id}/reset-totp") +async def reset_user_totp(_admin: AdminUser, user_id: UUID):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/admin.py` around lines 97 - 109, The admin parameter in reset_user_totp is unused and flagged by Ruff; rename the parameter from admin to _admin while keeping its type annotation (AdminUser) in the function signature of reset_user_totp so the dependency injection remains but lint warnings are suppressed; update any internal references if they exist (there are none) and keep the rest of the function behavior unchanged.frontend/src/views/Login.vue (1)
103-143: Consider clearing the password after TOTP is required.Once Line 115 indicates TOTP is required, the password is no longer needed. Clearing it reduces exposure of sensitive data in memory.
🔐 Suggested tweak
if (result === 'success') { router.push('/') } else if (result === 'totp_required') { // Phase 2 will show automatically via authStore.totpRequired + password.value = '' } else { error.value = 'Invalid email or password' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/Login.vue` around lines 103 - 143, When authStore.login returns 'totp_required' in handleLogin, clear the stored password to reduce sensitive data exposure: set password.value = '' right after detecting the 'totp_required' branch; also ensure handleBack already clears totp state (authStore.clearTotpState) and consider clearing password.value in handleBack as well to be thorough. Update references in handleLogin and handleBack (function names: handleLogin, handleBack; symbols: password, authStore.login, authStore.clearTotpState) accordingly.frontend/src/views/admin/Users.vue (1)
124-252: ClearuserToResetTotpafter reset/cancel.Leaving stale references can lead to confusing dialog content if the user list changes.
🧹 Suggested cleanup
async function resetTotp() { if (!userToResetTotp.value) return resettingTotp.value = true try { await api.post(`/admin/users/${userToResetTotp.value.id}/reset-totp`) resetTotpDialog.value = false + userToResetTotp.value = null await fetchUsers() } catch (error) { console.error('Failed to reset TOTP:', error) } finally { resettingTotp.value = false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/admin/Users.vue` around lines 124 - 252, The dialog leaves a stale user reference in userToResetTotp which can show outdated data after list changes; update confirmResetTotp/resetTotp and any cancel/close flows to clear userToResetTotp (set userToResetTotp.value = null) whenever resetTotpDialog is closed (after successful reset, on error/finally if you close the dialog, and on explicit cancel), ensuring the symbols to modify are userToResetTotp, resetTotp(), confirmResetTotp(), and resetTotpDialog so the reference is always cleared when the dialog is dismissed.backend/app/models/__init__.py (1)
5-7: Optional: sort all to satisfy RUF022.If lint gates on RUF022, reordering all will resolve it.
♻️ Example ordering
-__all__ = ["User", "Team", "TeamMembership", "TeamRole", "ApiKey", "Log", "LogLevel", "RecoveryCode"] +__all__ = ["ApiKey", "Log", "LogLevel", "RecoveryCode", "Team", "TeamMembership", "TeamRole", "User"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/models/__init__.py` around lines 5 - 7, The __all__ export list is not alphabetically ordered (RUF022); reorder the __all__ list so entries are sorted (e.g., ApiKey, Log, LogLevel, RecoveryCode, Team, TeamMembership, TeamRole, User) and ensure the RecoveryCode import (RecoveryCode) remains present; update the __all__ variable in the module (the __all__ symbol) to the sorted list to satisfy the lint rule.frontend/src/views/Settings.vue (2)
209-212: Cancelled setup leaves an encrypted secret persisted on the backend.When the user cancels after
beginSetup, the encrypted TOTP secret is already saved to the user record (seetotp.pyLine 69) and recovery code hashes are stored in the DB. These orphaned artifacts persist until the next setup attempt overwrites them. Consider either calling a backend cleanup endpoint on cancel, or deferring the server-side persist until the verify step completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/Settings.vue` around lines 209 - 212, The cancelSetup client flow currently just closes the dialog and clears setupData, but beginSetup already caused server-side persistence of an encrypted TOTP secret and recovery code hashes (see totp.py behavior), leaving orphaned artifacts; modify cancelSetup to call a new backend cleanup endpoint (e.g., POST /totp/cleanup or existing auth cleanup handler) passing the setup/session identifier stored in setupData before clearing setupData and closing setupDialog, or alternately change the flow so beginSetup does not persist to the user record and persistence only occurs in the verify step (e.g., move server-side save logic from begin_setup to verify_setup in totp.py). Ensure you reference and use the setupData/session token and the cancelSetup and beginSetup functions when implementing the change.
76-83: Add basic client-side validation for the TOTP code input.The text field accepts arbitrary input without constraints. Consider adding
maxlength="6", aninputmode="numeric"hint, and disabling the Verify button when the input isn't 6 digits. This improves UX and avoids unnecessary API calls.♻️ Suggested input constraints
<v-text-field v-model="verifyCode" label="Authentication Code" + maxlength="6" + inputmode="numeric" + placeholder="000000" autofocus :error-messages="setupError" />And on the verify button:
- <v-btn color="primary" :loading="verifyLoading" `@click`="verifySetup">Verify</v-btn> + <v-btn color="primary" :loading="verifyLoading" :disabled="verifyCode.length < 6" `@click`="verifySetup">Verify</v-btn>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/Settings.vue` around lines 76 - 83, The TOTP input accepts arbitrary text; add client-side constraints in Settings.vue by updating the v-text-field bound to verifyCode: add maxlength="6" and inputmode="numeric" (and optionally a pattern to only allow digits) to the component, validate the value length/format in the verifySetup flow (or a computed like isVerifyEnabled) and disable the Verify button unless verifyCode is exactly 6 digits; keep using setupError to surface any validation messages and ensure verifySetup early-returns if the input is invalid to avoid unnecessary API calls.backend/app/services/totp.py (2)
52-55: Recovery codes are functional but lack grouping for readability.The 8-character lowercase alphanumeric codes (e.g.,
a3b7f9x2) are hard for users to transcribe. A common pattern is to add dashes for visual grouping (e.g.,a3b7-f9x2). This is purely a UX improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/totp.py` around lines 52 - 55, The recovery codes returned by generate_recovery_codes are currently 8-char strings; change generation to format each code with a dash after the 4th character (e.g., "a3b7-f9x2") for readability by grouping the random characters into two 4-char segments; update the logic inside the generate_recovery_codes function to build an 8-char token and insert a '-' between chars 4 and 5 (keeping the same alphabet, length, and return type list[str]) so callers receive the dashed, grouped codes.
61-90:setup_beginpersists the encrypted secret and recovery codes before the user verifies.If the user abandons the setup flow after this call (e.g., closes the browser), the encrypted secret and hashed recovery codes remain in the DB with
totp_enabled = False. While a subsequent setup call overwrites these, the orphaned secret could be a concern from a security hygiene standpoint. Consider deferring persistence to the verify step, or returning the encrypted secret as part of the setup response and having the frontend pass it back during verify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/totp.py` around lines 61 - 90, setup_begin currently persists the encrypted TOTP secret (user.totp_secret_encrypted) and RecoveryCode entries immediately; change it to avoid writing these to the DB until verification: generate the secret (generate_secret) and encrypted payload (encrypt_secret) and generate recovery codes (generate_recovery_codes) but do not call user.save() or RecoveryCode.create()/delete() here; instead return the encrypted secret, QR (generate_qr_base64(get_provisioning_uri(...))) and plain recovery codes in the response and update the verify flow (the method that finalizes setup) to accept the encrypted secret and recovery codes from the client and then persist user.totp_secret_encrypted, set totp_enabled=True, and create hashed RecoveryCode rows (using hash_recovery_code) only after successful verification; alternatively, if you prefer server-side cleanup, ensure setup_begin creates DB entries with an expiry/cleanup flag or schedules deletion if not verified, referencing setup_begin, encrypt_secret, generate_recovery_codes, RecoveryCode, and hash_recovery_code.backend/app/api/auth.py (1)
82-97:totp_setup_verifyhas a TOCTOU gap with thetotp_enabledcheck.Between the
if user.totp_enabledcheck (Line 85) and thesetup_verifycall (Line 91), a concurrent request could complete setup first. Sincesetup_verifysetstotp_enabled = Trueand saves, a second concurrent call would overwrite/double-enable harmlessly in this case, but the pattern is fragile. For a 2FA setup flow, this is low-risk since it requires an authenticated session, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/auth.py` around lines 82 - 97, The current totp_setup_verify handler performs a non-atomic check of user.totp_enabled before calling TOTPService.setup_verify, creating a TOCTOU race; move the responsibility for the existence check and enabling into TOTPService.setup_verify (or make setup_verify perform an atomic conditional update) so the verification + set happens in one DB transaction/conditional update. Concretely: remove or relax the pre-check in totp_setup_verify and change TOTPService.setup_verify to either (a) reload the user inside a transaction and abort if totp_enabled is now True, or (b) perform an UPDATE ... WHERE id=? AND totp_enabled=FALSE and return success only if rows_affected==1, or (c) use optimistic locking/versioning to detect concurrent enable and fail cleanly; ensure setup_verify returns a clear result that totp_setup_verify can use to raise the correct HTTPException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/auth.py`:
- Around line 41-66: The verify_totp endpoint (function verify_totp) needs
protection against brute force: implement per-token or per-user attempt counting
and a short rate limit before calling TOTPService.verify_login_code. Modify the
flow around AuthService.decode_token and TOTPService.verify_login_code to (1)
extract a unique identifier from the decoded payload (e.g., payload.jti or
payload.sub+iat), (2) increment and persist an attempts counter in a fast store
(Redis/cache) keyed by that identifier, (3) reject and raise HTTPException with
401 when attempts exceed a safe threshold (e.g., 3) or when a short TTL expires,
and only then call TOTPService.verify_login_code; also consider reducing the
totp_token TTL if configured. Ensure failed attempts increment the counter and
successful verification clears the counter and issues tokens via
AuthService.create_tokens.
In `@backend/app/config.py`:
- Around line 20-21: The totp_encryption_key default of "" will cause Fernet("")
to raise at runtime; change the config field totp_encryption_key to be optional
(str | None = None) and add a Pydantic validator (e.g.,
`@validator`("totp_encryption_key") in the config model) that ensures the value is
present and a valid Fernet key (or raise a clear ValueError instructing to set
TOTP_ENCRYPTION_KEY created via Fernet.generate_key()); alternatively, add an
explicit guard in TOTPService initialization that checks totp_encryption_key,
validates it can construct a Fernet instance, and raises a descriptive error if
missing/invalid so TOTP setup/verify/disable fail fast with operator-friendly
message.
In `@backend/app/services/totp.py`:
- Around line 80-84: The loop over plain_codes is calling the CPU-bound
cls.hash_recovery_code (which uses bcrypt) and then awaiting RecoveryCode.create
for each code, blocking the event loop; change it to offload hashing to worker
threads (import asyncio and call await asyncio.to_thread(cls.hash_recovery_code,
code) or schedule all hashes with asyncio.gather of asyncio.to_thread calls) and
then perform a single database insert with RecoveryCode.bulk_create using the
precomputed hashes instead of calling RecoveryCode.create per item; update the
method that iterates plain_codes to build RecoveryCode instances from (user,
code_hash) and pass them to bulk_create.
- Around line 106-127: The recovery-code loop in verify_login_code performs
multiple bcrypt.verify calls on the event loop (via RecoveryCode.verify_code),
which can block; move the CPU-bound comparisons into a thread: fetch
recovery_codes with RecoveryCode.filter(...).all() as before, then call
asyncio.to_thread (or loop.run_in_executor) to run a small helper that iterates
those RecoveryCode objects and returns the id/index of the first matching code
(calling rc.verify_code inside the thread). Back in the async context, if a
match id is returned, look up that RecoveryCode instance from the earlier list,
set rc.used = True and await rc.save() (keeping DB I/O on the event loop) and
return True; otherwise return False. Ensure you reference verify_login_code and
RecoveryCode.verify_code when changing the implementation.
In `@backend/migrations/005_totp_tables.sql`:
- Around line 6-16: The migration currently checks only for
totp_secret_encrypted and skips the ALTER TABLE entirely if that column exists,
so totp_enabled can be missing after a partial run; modify the migration to
perform per-column existence checks: query information_schema.columns for
totp_secret_encrypted and separately for totp_enabled and conditionally run
ALTER TABLE users ADD COLUMN totp_secret_encrypted TEXT NULL and ALTER TABLE
users ADD COLUMN totp_enabled BOOLEAN NOT NULL DEFAULT FALSE independently (and
emit appropriate RAISE NOTICE messages for each addition or skip) so each column
is added idempotently even if the other already exists.
In `@frontend/src/stores/auth.ts`:
- Around line 44-47: The login and verifyTotp flows in
frontend/src/stores/auth.ts currently persist access_token and refresh_token to
localStorage (localStorage.setItem in login and verifyTotp); remove those
localStorage writes and switch to server-managed HttpOnly SameSite=Strict
cookies by having the backend set tokens and the frontend send fetch/axios
requests with credentials included (e.g., withCredentials: true / credentials:
'include'); if you must keep localStorage temporarily, add a strong
Content-Security-Policy and limit exposure, but ideally delete the
localStorage.setItem calls in the login and verifyTotp handlers and update any
code that reads tokens client-side to rely on server cookie-based
authentication/refresh endpoints instead.
- Around line 25-26: Replace the non-null assertions on data.access_token and
data.refresh_token before calling localStorage.setItem: check the runtime values
of data.access_token and data.refresh_token (e.g., typeof === 'string' &&
data.access_token.trim().length > 0) and only call
localStorage.setItem('access_token', ...) /
localStorage.setItem('refresh_token', ...) when valid; otherwise call
localStorage.removeItem(...) for that key or handle the error path (in the same
function where localStorage.setItem('access_token', data.access_token!) and
localStorage.setItem('refresh_token', data.refresh_token!) are currently used)
to avoid storing the string "null" or "undefined".
---
Nitpick comments:
In `@backend/app/api/admin.py`:
- Around line 97-109: The admin parameter in reset_user_totp is unused and
flagged by Ruff; rename the parameter from admin to _admin while keeping its
type annotation (AdminUser) in the function signature of reset_user_totp so the
dependency injection remains but lint warnings are suppressed; update any
internal references if they exist (there are none) and keep the rest of the
function behavior unchanged.
In `@backend/app/api/auth.py`:
- Around line 82-97: The current totp_setup_verify handler performs a non-atomic
check of user.totp_enabled before calling TOTPService.setup_verify, creating a
TOCTOU race; move the responsibility for the existence check and enabling into
TOTPService.setup_verify (or make setup_verify perform an atomic conditional
update) so the verification + set happens in one DB transaction/conditional
update. Concretely: remove or relax the pre-check in totp_setup_verify and
change TOTPService.setup_verify to either (a) reload the user inside a
transaction and abort if totp_enabled is now True, or (b) perform an UPDATE ...
WHERE id=? AND totp_enabled=FALSE and return success only if rows_affected==1,
or (c) use optimistic locking/versioning to detect concurrent enable and fail
cleanly; ensure setup_verify returns a clear result that totp_setup_verify can
use to raise the correct HTTPException.
In `@backend/app/models/__init__.py`:
- Around line 5-7: The __all__ export list is not alphabetically ordered
(RUF022); reorder the __all__ list so entries are sorted (e.g., ApiKey, Log,
LogLevel, RecoveryCode, Team, TeamMembership, TeamRole, User) and ensure the
RecoveryCode import (RecoveryCode) remains present; update the __all__ variable
in the module (the __all__ symbol) to the sorted list to satisfy the lint rule.
In `@backend/app/schemas/auth.py`:
- Around line 24-31: Add a Pydantic v2 model validator to LoginResponse (use
`@model_validator`(mode="after")) that enforces the discriminated-union
invariants: if totp_required is True then totp_token must be not None and
access_token/refresh_token must be None; if totp_required is False then
access_token and refresh_token must be not None and totp_token must be None;
raise a ValueError with a clear message when the invariant is violated so
invalid LoginResponse(...) instantiations are rejected.
In `@backend/app/services/auth.py`:
- Line 32: The TOTP TTL is hardcoded in create_totp_token; make it configurable
by adding/using a setting (e.g., settings.totp_expire_minutes) instead of the
literal "5" and compute expire = datetime.now(timezone.utc) +
timedelta(minutes=settings.totp_expire_minutes), with a sensible default
fallback (5) if the setting is absent; update any docs/config schema accordingly
so create_totp_token reads the new setting like
create_access_token/create_refresh_token do.
In `@backend/app/services/totp.py`:
- Around line 52-55: The recovery codes returned by generate_recovery_codes are
currently 8-char strings; change generation to format each code with a dash
after the 4th character (e.g., "a3b7-f9x2") for readability by grouping the
random characters into two 4-char segments; update the logic inside the
generate_recovery_codes function to build an 8-char token and insert a '-'
between chars 4 and 5 (keeping the same alphabet, length, and return type
list[str]) so callers receive the dashed, grouped codes.
- Around line 61-90: setup_begin currently persists the encrypted TOTP secret
(user.totp_secret_encrypted) and RecoveryCode entries immediately; change it to
avoid writing these to the DB until verification: generate the secret
(generate_secret) and encrypted payload (encrypt_secret) and generate recovery
codes (generate_recovery_codes) but do not call user.save() or
RecoveryCode.create()/delete() here; instead return the encrypted secret, QR
(generate_qr_base64(get_provisioning_uri(...))) and plain recovery codes in the
response and update the verify flow (the method that finalizes setup) to accept
the encrypted secret and recovery codes from the client and then persist
user.totp_secret_encrypted, set totp_enabled=True, and create hashed
RecoveryCode rows (using hash_recovery_code) only after successful verification;
alternatively, if you prefer server-side cleanup, ensure setup_begin creates DB
entries with an expiry/cleanup flag or schedules deletion if not verified,
referencing setup_begin, encrypt_secret, generate_recovery_codes, RecoveryCode,
and hash_recovery_code.
In `@backend/requirements.txt`:
- Around line 11-12: totp.py imports and uses Fernet from the cryptography
package but cryptography is only a transitive extra via python-jose; add
cryptography as an explicit direct dependency in backend/requirements.txt so
Fernet won't break if python-jose's extras change—update requirements.txt to
include a pinned cryptography entry (e.g., add a cryptography version constraint
compatible with your CI/runtime), ensuring Fernet usage in totp.py remains
supported.
In `@frontend/src/views/admin/Users.vue`:
- Around line 124-252: The dialog leaves a stale user reference in
userToResetTotp which can show outdated data after list changes; update
confirmResetTotp/resetTotp and any cancel/close flows to clear userToResetTotp
(set userToResetTotp.value = null) whenever resetTotpDialog is closed (after
successful reset, on error/finally if you close the dialog, and on explicit
cancel), ensuring the symbols to modify are userToResetTotp, resetTotp(),
confirmResetTotp(), and resetTotpDialog so the reference is always cleared when
the dialog is dismissed.
In `@frontend/src/views/Login.vue`:
- Around line 103-143: When authStore.login returns 'totp_required' in
handleLogin, clear the stored password to reduce sensitive data exposure: set
password.value = '' right after detecting the 'totp_required' branch; also
ensure handleBack already clears totp state (authStore.clearTotpState) and
consider clearing password.value in handleBack as well to be thorough. Update
references in handleLogin and handleBack (function names: handleLogin,
handleBack; symbols: password, authStore.login, authStore.clearTotpState)
accordingly.
In `@frontend/src/views/Settings.vue`:
- Around line 209-212: The cancelSetup client flow currently just closes the
dialog and clears setupData, but beginSetup already caused server-side
persistence of an encrypted TOTP secret and recovery code hashes (see totp.py
behavior), leaving orphaned artifacts; modify cancelSetup to call a new backend
cleanup endpoint (e.g., POST /totp/cleanup or existing auth cleanup handler)
passing the setup/session identifier stored in setupData before clearing
setupData and closing setupDialog, or alternately change the flow so beginSetup
does not persist to the user record and persistence only occurs in the verify
step (e.g., move server-side save logic from begin_setup to verify_setup in
totp.py). Ensure you reference and use the setupData/session token and the
cancelSetup and beginSetup functions when implementing the change.
- Around line 76-83: The TOTP input accepts arbitrary text; add client-side
constraints in Settings.vue by updating the v-text-field bound to verifyCode:
add maxlength="6" and inputmode="numeric" (and optionally a pattern to only
allow digits) to the component, validate the value length/format in the
verifySetup flow (or a computed like isVerifyEnabled) and disable the Verify
button unless verifyCode is exactly 6 digits; keep using setupError to surface
any validation messages and ensure verifySetup early-returns if the input is
invalid to avoid unnecessary API calls.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
backend/app/api/admin.pybackend/app/api/auth.pybackend/app/config.pybackend/app/models/__init__.pybackend/app/models/recovery_code.pybackend/app/models/user.pybackend/app/schemas/__init__.pybackend/app/schemas/auth.pybackend/app/schemas/user.pybackend/app/services/auth.pybackend/app/services/totp.pybackend/migrations/005_totp_tables.sqlbackend/requirements.txtfrontend/src/App.vuefrontend/src/api/client.tsfrontend/src/router/index.tsfrontend/src/stores/auth.tsfrontend/src/views/Login.vuefrontend/src/views/Settings.vuefrontend/src/views/admin/Users.vue
Each totp_token now carries a unique jti claim. The verify-totp endpoint tracks failed attempts per jti in memory and rejects requests after 5 failures, forcing the user to re-authenticate. Stale entries are evicted after 5 minutes matching the token TTL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/app/api/auth.py (1)
73-104:verify_totpcontrol flow is sound; brute-force is addressed (with caveats noted above).Success path correctly clears the counter (
_totp_attempts.pop), failure path increments it. The past concern about attempt counting is addressed at the logic level — the multi-worker and TOCTOU gaps noted earlier are the remaining concerns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/auth.py` around lines 73 - 104, The verify_totp endpoint's control flow is correct (clears _totp_attempts on success, records failures on invalid codes), so no functional code change is needed; remove the duplicate review/annotation ([duplicate_comment]) and mark the approval as resolved, and if you want to address the remaining multi-worker/TOCTOU concern later, open a separate issue referencing verify_totp, AuthService.create_tokens, _totp_attempts and _record_totp_failure so it can be handled with a distributed attempt store.
🧹 Nitpick comments (1)
backend/app/schemas/auth.py (1)
25-32: Consider amodel_validatorto enforce mutual exclusivity of TOTP vs. token fields.The schema allows constructing a
LoginResponsewith bothtotp_tokenandaccess_tokensimultaneously, or neither — both invalid states. The API layer prevents this today, but a@model_validator(mode="after")would catch it at the schema boundary.♻️ Proposed model validator
+from typing_extensions import Self +from pydantic import model_validator class LoginResponse(BaseModel): totp_required: bool = False access_token: str | None = None refresh_token: str | None = None token_type: str = "bearer" totp_token: str | None = None + + `@model_validator`(mode="after") + def check_token_fields_consistency(self) -> Self: + if self.totp_required: + if self.totp_token is None: + raise ValueError("totp_token must be set when totp_required is True") + else: + if self.access_token is None or self.refresh_token is None: + raise ValueError( + "access_token and refresh_token must be set when totp_required is False" + ) + if self.totp_token is not None: + raise ValueError("totp_token must not be set when totp_required is False") + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/schemas/auth.py` around lines 25 - 32, Add a Pydantic model_validator to LoginResponse to enforce mutual exclusivity between TOTP and token fields: implement a `@model_validator`(mode="after") method (e.g., validate_totp_vs_tokens) on class LoginResponse that checks totp_required and ensures when totp_required is True then totp_token is present and access_token/refresh_token are None, and when totp_required is False then access_token and refresh_token are present and totp_token is None; raise ValueError with a clear message on violation. Ensure you import model_validator if not already imported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/auth.py`:
- Around line 15-18: The module-level in-memory tracker (_totp_attempts with
_TOTP_MAX_ATTEMPTS and _TOTP_WINDOW_SECONDS) is only per-process and will be
bypassed in multi-worker/multi-pod deployments; replace it with a shared store
(e.g., Redis) by implementing async helpers that use a Redis key per jti (e.g.,
"totp_attempts:{jti}") and perform an atomic INCR and set EXPIRE (or INCR with
TTL) to check and record failures, and update any call sites that consult or
mutate _totp_attempts to call these new async functions (e.g.,
_check_totp_attempts and _record_totp_failure); if Redis is not available, add
clear deployment documentation noting the single-worker limitation and that
running multiple workers/pods will bypass the counter.
- Around line 120-135: Add the same brute-force protection used by the existing
verify-totp flow to the totp_setup_verify route: before calling
TOTPService.setup_verify in totp_setup_verify, use the existing
jti/session-based attempt counter (the same helper or service used by the
/verify-totp endpoint) to check and increment attempts for this session/user and
reject further tries with an HTTP 429 when the limit is exceeded; on successful
verification call reset/clear of that attempt counter so legitimate setup
succeeds. Target symbols: totp_setup_verify, TOTPService.setup_verify and the
attempt-counter or rate-limiter utility used by /verify-totp (use the same
function names/mechanism rather than creating a different limiter).
- Around line 21-44: The in-memory TOCTOU with _check_totp_attempts and
_record_totp_failure must be replaced with an atomic counter in a shared store
(e.g., Redis) keyed by jti with a TTL equal to _TOTP_WINDOW_SECONDS: remove or
stop using the _totp_attempts dict, and in the TOTP verify flow use a single
Redis INCR call for the jti key (if INCR returns 1, set EXPIRE to
_TOTP_WINDOW_SECONDS) and immediately check the returned count against
_TOTP_MAX_ATTEMPTS to raise the HTTP_401; update or remove
_check_totp_attempts/_record_totp_failure to use this Redis INCR+EXPIRE pattern
(or a Lua script) so the check-and-increment is atomic even across awaited
points in verify_totp and concurrent requests.
---
Duplicate comments:
In `@backend/app/api/auth.py`:
- Around line 73-104: The verify_totp endpoint's control flow is correct (clears
_totp_attempts on success, records failures on invalid codes), so no functional
code change is needed; remove the duplicate review/annotation
([duplicate_comment]) and mark the approval as resolved, and if you want to
address the remaining multi-worker/TOCTOU concern later, open a separate issue
referencing verify_totp, AuthService.create_tokens, _totp_attempts and
_record_totp_failure so it can be handled with a distributed attempt store.
---
Nitpick comments:
In `@backend/app/schemas/auth.py`:
- Around line 25-32: Add a Pydantic model_validator to LoginResponse to enforce
mutual exclusivity between TOTP and token fields: implement a
`@model_validator`(mode="after") method (e.g., validate_totp_vs_tokens) on class
LoginResponse that checks totp_required and ensures when totp_required is True
then totp_token is present and access_token/refresh_token are None, and when
totp_required is False then access_token and refresh_token are present and
totp_token is None; raise ValueError with a clear message on violation. Ensure
you import model_validator if not already imported.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/auth.pybackend/app/schemas/auth.pybackend/app/services/auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/services/auth.py
Guard in TOTPService._get_fernet() now gives a clear error with generation instructions when TOTP_ENCRYPTION_KEY is missing or invalid. Added the env var to docker-compose.yml, .env.example, and backend/.env.example. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bcrypt hashing is CPU-bound (~200ms per call); running 8 sequentially blocked the event loop for ~1.6s. Now hashes in parallel via asyncio.to_thread and inserts all codes in a single bulk_create. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per-process counters don't work across multiple workers or pods. Replace the module-level dict with a totp_attempts table so brute-force limits are enforced globally. New migration in 006 (005 may already be applied). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bcrypt.checkpw in the recovery code loop blocks the event loop for up to ~1.6s worst case. Move the comparison loop into asyncio.to_thread while keeping the DB write on the event loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Optional per-user TOTP 2FA with two-phase login flow. When enabled, login returns a short-lived intermediate token, frontend shows a code entry form, and a second API call verifies the TOTP code to issue real JWTs. Includes Fernet-encrypted secrets at rest, bcrypt-hashed recovery codes, admin reset capability, and a user settings page for setup/disable.
Summary by CodeRabbit
New Features
Chores