Add optional TOTP-based 2FA for admin login - #32
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
opportunityfinder | 9584a35 | Commit Preview URL Branch Preview URL |
Aug 31 2026, 06:12 AM |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Adds an optional second factor on top of the existing admin
email/password login, gated by a new ADMIN_TOTP_SECRET env var —
unset (the default) keeps login exactly as it was before.
- app/security.py: stdlib-only RFC 6238 TOTP (hmac/hashlib/struct/
base64), matching this module's existing PBKDF2/session-token style.
Verified against RFC 6238 Appendix B test vectors before writing any
route code.
- app/routes/admin_auth.py: LoginRequest gains an optional totp_code
field, checked in the same /login request when configured. A new
public GET /login-methods lets the frontend know whether to show the
code field, without needing a guess. Failure messages stay generic
("invalid email, password, or code") so a correct-password guess
can't be confirmed via a different error. Also rejects replay of the
exact code that just succeeded.
- frontend/admin.html + js/admin.js: adds a hidden-by-default TOTP
input, shown only when /login-methods reports it's required.
- scripts/generate_admin_totp_secret.py: one-time helper generating a
base32 secret plus an otpauth:// provisioning URI, matching the
style of scripts/hash_admin_password.py.
- backend/.env.example, docs/DEPLOY.md: document ADMIN_TOTP_SECRET.
Tests: RFC 6238 vector checks, wrong/expired/future-step code
handling, unaffected-when-unset, and login-endpoint behavior both with
and without the secret configured (backend/tests/test_admin_auth.py).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019Umy9bW14TMbzW2QD7eLt3
adab-tech
force-pushed
the
security/admin-2fa-clean
branch
from
August 31, 2026 06:02
ef8605a to
75811b7
Compare
adab-tech
commented
Aug 31, 2026
adab-tech
left a comment
adab-tech
left a comment
Owner
Author
There was a problem hiding this comment.
Rebased onto main after #30 and #31 merged — both touched admin_auth.py (the namespace="admin_login" param from #30) which conflicted with this PR's own changes to the same file's login-limiter setup and test_admin_auth.py's test-state-clearing helpers. admin_auth.py itself auto-merged cleanly; test_admin_auth.py needed the two features' respective test helpers (_clear_login_lockout_state from #30, _totp_code from this PR) kept side by side, plus one test's setup_method updated to use the new DB-backed clearing helper instead of the old in-memory dict fields it was written against before #30 existed.
Re-ran everything after resolving: backend suite now at 339 tests (up from the 334 in this PR's own description, reflecting #30's additions merged in too), ruff check . clean, frontend's 27 tests unaffected.
Generated by Claude Code
CodeQL's py/clear-text-logging-sensitive-data correctly flagged this script printing the raw ADMIN_TOTP_SECRET/otpauth URI in clear text — but that's unavoidable by design, not a bug: unlike hash_admin_password.py (which only ever prints a one-way hash), there is no derived value to substitute here. An authenticator app needs the literal shared secret, so a one-time local provisioning tool for it has to show the real value at least once. Adds an inline lgtm suppression with the reasoning inline, plus a runtime caution the operator actually sees before the secret prints, telling them not to run this somewhere the output could be logged or shared (CI, a shared terminal, piped to a file). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Umy9bW14TMbzW2QD7eLt3
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
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.
Summary
This app has exactly one admin account, password-only — if that password leaks or is guessed, the login rate-limiter (5 attempts/15min lockout) was the only remaining defense. Adds an optional TOTP second factor (RFC 6238), stdlib-only (
hmac/hashlib/struct/base64— nopyotpor other new dependency).ADMIN_TOTP_SECRETenv var. Unset (the default) means login is byte-for-byte unchanged — password only. Set it, and/admin/loginalso requires a valid 6-digit code in the same request.backend/app/security.py:generate_totp_secret,totp_provisioning_uri,verify_totp_code— same style as the existing PBKDF2/session code in this file,hmac.compare_digestfor the code comparison, ±1 time-step tolerance for clock drift.backend/scripts/generate_admin_totp_secret.py, mirroringhash_admin_password.py's existing style — prints the secret plus anotpauth://URI for any authenticator app.GET /admin/login-methodsreturns{"totp_required": bool}so the login form only shows the code field when it's actually needed, instead of guessing or always showing a dead field.Test plan
ruff check .— clean.ADMIN_TOTP_SECRETis unset (explicit test case).Notes for the human reviewer
A few judgment calls worth knowing about, none of them blocking:
GET /login-methodsreveals one boolean (whether 2FA is configured) to any unauthenticated caller. Reasonable for a single-operator tool on an unlisted admin URL, but it is a small information disclosure if you want it removed in favor of "always show the field."Generated by Claude Code