starred/bambuddy

Fork 0

mirror of https://github.com/maziggy/bambuddy.git synced 2026-05-09 05:35:30 +02:00

[PR #933] [MERGED] feat: Two-Factor Authentication (TOTP, Email OTP) and OIDC/SSO – full implementation with admin UI #1133

New issue

Closed

opened 2026-05-06 12:35:15 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 12:35:15 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/maziggy/bambuddy/pull/933
Author: @netscout2001
Created: 4/10/2026
Status: ✅ Merged
Merged: 4/13/2026
Merged by: @maziggy

Base: dev ← Head: feature/2fa-oidc-authentication

📝 Commits (10+)

8020889 feat: add Two-Factor Authentication (TOTP, Email OTP) and OIDC/SSO
4663cab feat: add 2FA and OIDC settings UI with full translations
bd07bed test: add unit and integration tests for 2FA and OIDC
bba2495 fix(security): address all critical and important issues from maintainer review
9203794 fix(frontend): update UI for email OTP two-step enable, password-gated disable, and OIDC 2FA
7c313e0 test: add missing security tests and update existing tests for API changes
f7e570e fix(frontend): remove unused Toggle import in TwoFactorSettings
eb8ca2b test: add email OTP confirm endpoint tests and fix disable test
80dd277 fix(security): TOTP replay protection, rate limiting on disable/regenerate, migration cleanup
7edab65 fix(security): bind pre_auth token to HttpOnly cookie, encrypt secrets at rest

📊 Changes

44 files changed (+10473 additions, -290 deletions)

View changed files

📝 backend/app/api/routes/archives.py (+4 -4)
📝 backend/app/api/routes/auth.py (+428 -57)
📝 backend/app/api/routes/camera.py (+1 -1)
📝 backend/app/api/routes/library.py (+2 -2)
➕ backend/app/api/routes/mfa.py (+1690 -0)
📝 backend/app/api/routes/users.py (+43 -7)
📝 backend/app/core/auth.py (+256 -64)
📝 backend/app/core/database.py (+44 -0)
➕ backend/app/core/encryption.py (+88 -0)
📝 backend/app/main.py (+158 -4)
📝 backend/app/models/__init__.py (+10 -0)
➕ backend/app/models/auth_ephemeral.py (+199 -0)
➕ backend/app/models/oidc_provider.py (+93 -0)
📝 backend/app/models/user.py (+5 -1)
➕ backend/app/models/user_otp_code.py (+55 -0)
➕ backend/app/models/user_totp.py (+84 -0)
📝 backend/app/schemas/auth.py (+344 -16)
📝 backend/app/services/email_service.py (+67 -4)
📝 backend/tests/conftest.py (+4 -0)
📝 backend/tests/integration/test_advanced_auth_api.py (+111 -27)

...and 24 more files

📄 Description

Description

Adds a complete multi-factor authentication and single sign-on system to BamBuddy, accompanied by an extensive security hardening pass (~50 findings across four review rounds).

Features https://github.com/maziggy/bambuddy/issues/902

Documentation

Companion docs PRs (delete lines that don't apply):

Wiki: maziggy/bambuddy-wiki#___
Website: maziggy/bambuddy-website#___

Pick one:

Docs PR(s) linked above
No docs update required — reason: bundling with PR initially; will add wiki docs if maintainers want to proceed

Type of Change

New feature
Bug fix
Security improvement
Test addition or update

Two-Factor Authentication

Feature	Details
TOTP	Compatible with Google Authenticator, Authy, 2FAS and any standard app
Email OTP	Alternative second factor via email
Backup codes	10 single-use recovery codes per user
Opt-in	Per-user; each user can enable 2FA independently
Brute-force protection	Rate limiting on all verification and enable endpoints

OIDC / Single Sign-On

Feature	Details
Providers	PocketID, Authentik, Keycloak and any standards-compliant OIDC provider
PKCE (S256)	Works with public clients that have no client secret
Account linking	Existing users linked automatically via verified email on first SSO login (opt-in, disabled by default)
Auto-provisioning	Optional: create new BamBuddy accounts on first SSO login
Login page	SSO buttons rendered dynamically per configured provider

Security Hardening

Input validation (pbkdf2 / pyotp DoS guard)

Finding	Fix
Multiple request schemas had no `max_length` on string fields — unbounded input reached pbkdf2 or pyotp	`max_length` enforced on all user-facing fields before any hashing or OTP call: `LoginRequest`, `SetupRequest`, `UserCreate`, `UserUpdate`, `ChangePasswordRequest`, `ForgotPasswordRequest`, `TOTPSetupRequest`, `EmailOTPEnableConfirmRequest`, `OIDCProviderCreate/Update`
Email OTP setup code accepted any string	`code` now validated as exactly 6 digits before reaching any verification logic
`TwoFAVerifyRequest.code` accepted arbitrary strings up to 128 chars	Regex validator `^[A-Za-z0-9]{6,8}$` added; field capped at `max_length=8`

JWT & Session management

Finding	Fix
Tokens could not be invalidated on logout	JWT revocation via DB-backed `jti` blocklist; logout revokes the token immediately
Tokens remained valid after password change/reset	`iat` claim added to all JWTs; `password_changed_at` timestamp on User invalidates all prior sessions
`/auth/me` and `auth_middleware` skipped freshness check	`_is_token_fresh()` now enforced in all 8 JWT validation paths
No session cleanup	Background task runs hourly to purge expired revoked JTIs and rate-limit events

Authentication hardening

Finding	Fix
Login endpoint had no rate limiting	Dual rate-limit buckets: per-username (10/15 min) and per-IP (20/15 min)
IP bucket read from `client.host` (proxy-blind)	`_get_client_ip()` with `TRUSTED_PROXY_IPS` env var; rightmost non-proxy IP from `X-Forwarded-For`
Leftmost XFF IP was attacker-controlled	Fixed to walk right-to-left, skip trusted proxies
2FA verify endpoint had no rate limiting	Per-user sliding-window rate limit (5 attempts/15 min)
TOTP enable step had no rate limiting	`check_rate_limit` + `record_failed_attempt` added
Active TOTP could be silently replaced	`setup_totp` requires a valid current TOTP code before overwriting
Password change endpoint had no rate limiting	Rate-limited with same 2FA bucket; JWT revoked on success
`POST /forgot-password` only rate-limited by email address	Per-IP rate limit added (10 req/15 min) to prevent mass-reset flooding across many addresses from a single source
Weak password policy	Minimum complexity enforced: uppercase, lowercase, digit, special character
Forgot-password response time revealed account existence	`send_email()` moved to FastAPI `BackgroundTasks`; response time now constant

Pre-auth & token handling

Finding	Fix
Pre-auth tokens were stored in memory	Migrated to DB-backed single-use tokens with 5-minute TTL
Pre-auth tokens were not single-use	Atomically consumed via `DELETE … RETURNING`
Email OTP setup token was burned on wrong code	Rewrote to peek-then-consume: code verified first, token deleted only on success
TOTP codes could be replayed within same 30s window	`last_totp_counter` tracked per TOTP record; replay rejected
Slicer/camera download tokens stored in in-memory dicts	Migrated to DB-backed `AuthEphemeralToken` with TTL and single-use enforcement
Slicer token resource-binding not enforced at DB level	`nonce` (resource key) added to the `DELETE … WHERE` clause; wrong-resource calls cannot consume the token
`get_api_key` (webhook path) scanned all keys with pbkdf2	`key_prefix` pre-filter added (same O(1) fix as login API key validation)

OIDC / SSO

Finding	Fix
`email_verified` claim not checked	Unverified emails ignored to prevent account takeover via unverified provider
Issuer not validated against discovery document	Validated with trailing-slash tolerance
OIDC callback parameters had no length limits	Max-length enforced on `code`, `state`, `error` query params
Error strings passed unencoded to redirect URL	URL-encoded to prevent query-parameter injection
Discovery `token_endpoint`/`jwks_uri` could use non-HTTP schemes	Scheme validated (`https://` or `http://` only); SSRF via `file://`, `gopher://` etc. prevented
OIDC error codes passed raw from URL to frontend toast	Whitelisted set of allowed error codes; unknown values map to a generic message
`auto_link_existing_accounts` defaulted to `True`	Changed to `False`; operators must explicitly opt in to prevent silent account hijack by an attacker-controlled IdP
Auto-link did not check for existing OIDC links on target user	Before linking, existing `UserOIDCLink` rows for the target user are queried; any existing link blocks auto-link to prevent a second IdP from taking over the account
Auto-created OIDC users had `auth_source="local"`	Set to `auth_source="oidc"`; OIDC users are blocked from the password-reset flow

Information leakage

Finding	Fix
Exception detail in `reset_user_password` included `str(e)`	Replaced with static message; full error logged server-side only
Backup-code loop response time varied by code position	All codes always iterated (no early `break`); constant O(n) timing

Infrastructure

Finding	Fix
No Content-Security-Policy header	CSP header added for the React SPA (`ws:`/`wss:` for MQTT, `data:`/`blob:` for thumbnails)

Testing

1608+ passed

Suite	Coverage
Unit	TOTP secret generation, backup code hashing, PKCE derivation, OTP expiry, token freshness, rate limiting
Integration	Full TOTP enrol/verify/disable lifecycle, email OTP, backup codes, OIDC provider CRUD, OIDC callback error cases, JWT revocation, password change flow, slicer token resource binding, input length validation, per-IP rate limiting, OIDC auto-link rejection
Ruff lint/format	✅ clean

Branch based on current dev. Frontend build files not included.

Screenshots

Login page — SSO button

Settings → Authentication → Two-Factor Auth

Settings → Authentication → SSO / OIDC (admin)

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/maziggy/bambuddy/pull/933 **Author:** [@netscout2001](https://github.com/netscout2001) **Created:** 4/10/2026 **Status:** ✅ Merged **Merged:** 4/13/2026 **Merged by:** [@maziggy](https://github.com/maziggy) **Base:** `dev` ← **Head:** `feature/2fa-oidc-authentication` --- ### 📝 Commits (10+) - [`8020889`](https://github.com/maziggy/bambuddy/commit/802088931505112652f41f70068989e7c430c670) feat: add Two-Factor Authentication (TOTP, Email OTP) and OIDC/SSO - [`4663cab`](https://github.com/maziggy/bambuddy/commit/4663cab4e9d5b0fc159e4c9eef74355a8b8330b0) feat: add 2FA and OIDC settings UI with full translations - [`bd07bed`](https://github.com/maziggy/bambuddy/commit/bd07bed20404268fe461993edacb6d02e5183b98) test: add unit and integration tests for 2FA and OIDC - [`bba2495`](https://github.com/maziggy/bambuddy/commit/bba24956ab81094061eb17117cf1ae6b3181d170) fix(security): address all critical and important issues from maintainer review - [`9203794`](https://github.com/maziggy/bambuddy/commit/920379439277df80e4f29ae37e4f46355a248a28) fix(frontend): update UI for email OTP two-step enable, password-gated disable, and OIDC 2FA - [`7c313e0`](https://github.com/maziggy/bambuddy/commit/7c313e0686d48971271258c2432c6aae26d0b142) test: add missing security tests and update existing tests for API changes - [`f7e570e`](https://github.com/maziggy/bambuddy/commit/f7e570e681ddbaef6a4201f580e6f465833f03c4) fix(frontend): remove unused Toggle import in TwoFactorSettings - [`eb8ca2b`](https://github.com/maziggy/bambuddy/commit/eb8ca2bccc841005efbf033324ada0b0e34864d1) test: add email OTP confirm endpoint tests and fix disable test - [`80dd277`](https://github.com/maziggy/bambuddy/commit/80dd27756c32460fef0f3dc77d22d09dbfe4817c) fix(security): TOTP replay protection, rate limiting on disable/regenerate, migration cleanup - [`7edab65`](https://github.com/maziggy/bambuddy/commit/7edab65149e04b8b2155fac458204f8985f2918a) fix(security): bind pre_auth token to HttpOnly cookie, encrypt secrets at rest ### 📊 Changes **44 files changed** (+10473 additions, -290 deletions) <details> <summary>View changed files</summary> 📝 `backend/app/api/routes/archives.py` (+4 -4) 📝 `backend/app/api/routes/auth.py` (+428 -57) 📝 `backend/app/api/routes/camera.py` (+1 -1) 📝 `backend/app/api/routes/library.py` (+2 -2) ➕ `backend/app/api/routes/mfa.py` (+1690 -0) 📝 `backend/app/api/routes/users.py` (+43 -7) 📝 `backend/app/core/auth.py` (+256 -64) 📝 `backend/app/core/database.py` (+44 -0) ➕ `backend/app/core/encryption.py` (+88 -0) 📝 `backend/app/main.py` (+158 -4) 📝 `backend/app/models/__init__.py` (+10 -0) ➕ `backend/app/models/auth_ephemeral.py` (+199 -0) ➕ `backend/app/models/oidc_provider.py` (+93 -0) 📝 `backend/app/models/user.py` (+5 -1) ➕ `backend/app/models/user_otp_code.py` (+55 -0) ➕ `backend/app/models/user_totp.py` (+84 -0) 📝 `backend/app/schemas/auth.py` (+344 -16) 📝 `backend/app/services/email_service.py` (+67 -4) 📝 `backend/tests/conftest.py` (+4 -0) 📝 `backend/tests/integration/test_advanced_auth_api.py` (+111 -27) _...and 24 more files_ </details> ### 📄 Description ### Description Adds a complete multi-factor authentication and single sign-on system to BamBuddy, accompanied by an extensive security hardening pass (~50 findings across four review rounds). ### Related Issue Features <https://github.com/maziggy/bambuddy/issues/902> ## Documentation  **Companion docs PRs** (delete lines that don't apply): - Wiki: maziggy/bambuddy-wiki#___ - Website: maziggy/bambuddy-website#___ **Pick one**: - [ ] Docs PR(s) linked above - [ ] No docs update required — reason: bundling with PR initially; will add wiki docs if maintainers want to proceed ## Type of Change - [x] New feature - [ ] Bug fix - [x] Security improvement - [x] Test addition or update --- #### Two-Factor Authentication | Feature | Details | |---|---| | TOTP | Compatible with Google Authenticator, Authy, 2FAS and any standard app | | Email OTP | Alternative second factor via email | | Backup codes | 10 single-use recovery codes per user | | Opt-in | Per-user; each user can enable 2FA independently | | Brute-force protection | Rate limiting on all verification and enable endpoints | #### OIDC / Single Sign-On | Feature | Details | |---|---| | Providers | PocketID, Authentik, Keycloak and any standards-compliant OIDC provider | | PKCE (S256) | Works with public clients that have no client secret | | Account linking | Existing users linked automatically via verified email on first SSO login (opt-in, disabled by default) | | Auto-provisioning | Optional: create new BamBuddy accounts on first SSO login | | Login page | SSO buttons rendered dynamically per configured provider | --- #### Security Hardening **Input validation (pbkdf2 / pyotp DoS guard)** | Finding | Fix | |---|---| | Multiple request schemas had no `max_length` on string fields — unbounded input reached pbkdf2 or pyotp | `max_length` enforced on all user-facing fields before any hashing or OTP call: `LoginRequest`, `SetupRequest`, `UserCreate`, `UserUpdate`, `ChangePasswordRequest`, `ForgotPasswordRequest`, `TOTPSetupRequest`, `EmailOTPEnableConfirmRequest`, `OIDCProviderCreate/Update` | | Email OTP setup code accepted any string | `code` now validated as exactly 6 digits before reaching any verification logic | | `TwoFAVerifyRequest.code` accepted arbitrary strings up to 128 chars | Regex validator `^[A-Za-z0-9]{6,8}$` added; field capped at `max_length=8` | **JWT & Session management** | Finding | Fix | |---|---| | Tokens could not be invalidated on logout | JWT revocation via DB-backed `jti` blocklist; logout revokes the token immediately | | Tokens remained valid after password change/reset | `iat` claim added to all JWTs; `password_changed_at` timestamp on User invalidates all prior sessions | | `/auth/me` and `auth_middleware` skipped freshness check | `_is_token_fresh()` now enforced in all 8 JWT validation paths | | No session cleanup | Background task runs hourly to purge expired revoked JTIs and rate-limit events | **Authentication hardening** | Finding | Fix | |---|---| | Login endpoint had no rate limiting | Dual rate-limit buckets: per-username (10/15 min) and per-IP (20/15 min) | | IP bucket read from `client.host` (proxy-blind) | `_get_client_ip()` with `TRUSTED_PROXY_IPS` env var; rightmost non-proxy IP from `X-Forwarded-For` | | Leftmost XFF IP was attacker-controlled | Fixed to walk right-to-left, skip trusted proxies | | 2FA verify endpoint had no rate limiting | Per-user sliding-window rate limit (5 attempts/15 min) | | TOTP enable step had no rate limiting | `check_rate_limit` + `record_failed_attempt` added | | Active TOTP could be silently replaced | `setup_totp` requires a valid current TOTP code before overwriting | | Password change endpoint had no rate limiting | Rate-limited with same 2FA bucket; JWT revoked on success | | `POST /forgot-password` only rate-limited by email address | Per-IP rate limit added (10 req/15 min) to prevent mass-reset flooding across many addresses from a single source | | Weak password policy | Minimum complexity enforced: uppercase, lowercase, digit, special character | | Forgot-password response time revealed account existence | `send_email()` moved to FastAPI `BackgroundTasks`; response time now constant | **Pre-auth & token handling** | Finding | Fix | |---|---| | Pre-auth tokens were stored in memory | Migrated to DB-backed single-use tokens with 5-minute TTL | | Pre-auth tokens were not single-use | Atomically consumed via `DELETE … RETURNING` | | Email OTP setup token was burned on wrong code | Rewrote to peek-then-consume: code verified first, token deleted only on success | | TOTP codes could be replayed within same 30s window | `last_totp_counter` tracked per TOTP record; replay rejected | | Slicer/camera download tokens stored in in-memory dicts | Migrated to DB-backed `AuthEphemeralToken` with TTL and single-use enforcement | | Slicer token resource-binding not enforced at DB level | `nonce` (resource key) added to the `DELETE … WHERE` clause; wrong-resource calls cannot consume the token | | `get_api_key` (webhook path) scanned all keys with pbkdf2 | `key_prefix` pre-filter added (same O(1) fix as login API key validation) | **OIDC / SSO** | Finding | Fix | |---|---| | `email_verified` claim not checked | Unverified emails ignored to prevent account takeover via unverified provider | | Issuer not validated against discovery document | Validated with trailing-slash tolerance | | OIDC callback parameters had no length limits | Max-length enforced on `code`, `state`, `error` query params | | Error strings passed unencoded to redirect URL | URL-encoded to prevent query-parameter injection | | Discovery `token_endpoint`/`jwks_uri` could use non-HTTP schemes | Scheme validated (`https://` or `http://` only); SSRF via `file://`, `gopher://` etc. prevented | | OIDC error codes passed raw from URL to frontend toast | Whitelisted set of allowed error codes; unknown values map to a generic message | | `auto_link_existing_accounts` defaulted to `True` | Changed to `False`; operators must explicitly opt in to prevent silent account hijack by an attacker-controlled IdP | | Auto-link did not check for existing OIDC links on target user | Before linking, existing `UserOIDCLink` rows for the target user are queried; any existing link blocks auto-link to prevent a second IdP from taking over the account | | Auto-created OIDC users had `auth_source="local"` | Set to `auth_source="oidc"`; OIDC users are blocked from the password-reset flow | **Information leakage** | Finding | Fix | |---|---| | Exception detail in `reset_user_password` included `str(e)` | Replaced with static message; full error logged server-side only | | Backup-code loop response time varied by code position | All codes always iterated (no early `break`); constant O(n) timing | **Infrastructure** | Finding | Fix | |---|---| | No Content-Security-Policy header | CSP header added for the React SPA (`ws:`/`wss:` for MQTT, `data:`/`blob:` for thumbnails) | --- ### Testing 1608+ passed | Suite | Coverage | |---|---| | Unit | TOTP secret generation, backup code hashing, PKCE derivation, OTP expiry, token freshness, rate limiting | | Integration | Full TOTP enrol/verify/disable lifecycle, email OTP, backup codes, OIDC provider CRUD, OIDC callback error cases, JWT revocation, password change flow, slicer token resource binding, input length validation, per-IP rate limiting, OIDC auto-link rejection | | Ruff lint/format | ✅ clean | Branch based on current `dev`. Frontend build files not included. --- ### Screenshots **Login page — SSO button** <img width="476" height="682" alt="image" src="https://github.com/user-attachments/assets/86b372ec-540c-4b9a-bf5b-7456ef1303e8" /> **Settings → Authentication → Two-Factor Auth** <img width="1910" height="902" alt="image" src="https://github.com/user-attachments/assets/b59e2828-ebee-435b-b52c-807ceed75ec4" /> **Settings → Authentication → SSO / OIDC (admin)** <img width="1918" height="908" alt="image" src="https://github.com/user-attachments/assets/b748ee33-261d-47be-b90a-dc41c8757293" /> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>