feat: password reset flow via Gmail OAuth

forgejo_admin commented

2026-03-21 16:43:50 +00:00

Author

Owner

PR #133 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Pydantic / Keycloak Admin API / Gmail OAuth

Security (OWASP)

User enumeration prevention: Correctly implemented. Both the found and not-found email paths return the identical "If that email is registered, a reset link has been sent." message with HTTP 200. No timing side-channel concern since the DB lookup is fast relative to the email send, and the email send happens after the response message is determined.
Token security: secrets.token_urlsafe(32) produces 256 bits of entropy -- cryptographically strong and URL-safe. Tokens are single-use (used flag) and expire after 1 hour. Good.
HTML injection in email: reset_url is passed through escape() (from html stdlib) in the email template before being rendered in HTML attributes and text. The <a href="{escape(reset_url)}"> pattern is correct.
Keycloak admin API: Uses the existing reset_parent_password service function which sets temporary: False -- correct for a self-service flow where the user chose their own password.

SQLAlchemy / Alembic

Migration 014: Chain is correct (013 -> 014). Raw SQL migration uses sa.text() wrapper, includes proper indexes on email and token columns, and downgrade() drops with CASCADE. Clean.
Model: PasswordResetToken model mirrors the migration correctly. server_default=func.now() for created_at and server_default=text("false") for used match the SQL DDL.

FastAPI / Pydantic

Schemas: Clean Pydantic models. PasswordResetRequest, PasswordResetConfirm, PasswordResetResponse are well-defined.
Router registration: Mounted at /api/password-reset in main.py -- consistent with the /api/ prefix pattern used by other API routers.
CORS: The existing CORS middleware in main.py already allows https://westsidekingsandqueens.tail5b443a.ts.net -- no additional CORS config needed.

Email service

send_password_reset_email: Follows the existing pattern from other email functions -- uses get_gmail_client(tenant), _brand_wrapper(), and provides both HTML and plain text bodies. Returns gmail_message_id or None on failure. Exception is caught and logged, not re-raised -- consistent with the non-blocking email pattern.

Test coverage

Seven tests covering: happy path request, enumeration prevention, invalid token, expired token, used token, successful Keycloak reset, and Keycloak failure. All edge cases for the token lifecycle are covered. Mocking strategy is correct -- patches at the route module level. Test fixtures use the shared conftest.py tenant, client, and db fixtures.

BLOCKERS

None.

NITS

Double Keycloak user lookup: In confirm_password_reset, get_existing_user() is called at line 159 to verify the user exists, then reset_parent_password() is called at line 167 which internally calls get_existing_user() again (keycloak.py line 197). This is two HTTP roundtrips to Keycloak for the same email. Not a blocker since the function signatures are already established and used by admin.py, but worth noting for a future refactor (e.g., a reset_parent_password_by_id variant that accepts the user ID directly).
Duplicated tenant slug constant: _DEFAULT_TENANT_SLUG = "westside-kings-queens" in password_reset.py duplicates DEFAULT_TENANT_SLUG from services/registration.py. Consider importing from a single source to avoid drift. Not a blocker since the slug is stable and the duplication is in different modules, not in auth/security paths.
Confirm endpoint returns 200 for all outcomes: Invalid token, expired token, used token, and Keycloak failures all return HTTP 200 with different messages. The frontend will need to inspect the message string to determine success vs failure. A more conventional approach would use HTTP 400/422 for client errors and 503 for Keycloak failures. This is a design choice that works for the current frontend, but makes API semantics less discoverable. Not a blocker.
No new-password validation: The new_password field in PasswordResetConfirm accepts any string with no minimum length or complexity check. Keycloak itself may enforce password policies at the realm level, which would cause reset_parent_password to return False. If Keycloak does not have a policy, a user could set a 1-character password. Consider adding a Pydantic min_length constraint (e.g., new_password: str = Field(min_length=8)). Not a blocker since Keycloak is the ultimate gatekeeper.
No rate limiting on /request: The request endpoint has no rate limiting, so an attacker could trigger mass email sends. The existing codebase has no rate limiting anywhere, so this is consistent with the current state. Worth tracking as a future issue for all public endpoints.

SOP COMPLIANCE

Branch named after issue: 132-password-reset-oauth references issue #132
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present
Related references plan slug: PR body references issue #132 but no plan slug. This is consistent with basketball-api's lighter-weight process (no plan phase required per feedback_basketball_hands_off.md)
No secrets committed: No API keys, passwords, or credentials in code
No unnecessary file changes: All 6 files are directly related to the password reset feature
Tests exist and pass: 7 tests with good coverage of happy path and edge cases
Commit messages are descriptive: PR title follows conventional commit format

PROCESS OBSERVATIONS

Deployment frequency: Clean, focused feature PR. 6 files, 592 additions, 0 deletions. No unrelated changes. Ready for single-shot deployment.
Change failure risk: Low. New feature with no modifications to existing logic (only additive changes to main.py and email.py). The reset_parent_password function in keycloak.py already existed and is battle-tested via the admin route.
Cross-repo dependency: Frontend PR (westside-app #63) consumes these endpoints. Backend can merge independently since the endpoints are additive.
Tuesday blocker context: This is infrastructure for girls tryout March 24. The double Keycloak lookup (nit #1) is negligible latency and should not delay merge.

VERDICT: APPROVED

## PR #133 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic / Pydantic / Keycloak Admin API / Gmail OAuth **Security (OWASP)** 1. **User enumeration prevention**: Correctly implemented. Both the found and not-found email paths return the identical `"If that email is registered, a reset link has been sent."` message with HTTP 200. No timing side-channel concern since the DB lookup is fast relative to the email send, and the email send happens after the response message is determined. 2. **Token security**: `secrets.token_urlsafe(32)` produces 256 bits of entropy -- cryptographically strong and URL-safe. Tokens are single-use (`used` flag) and expire after 1 hour. Good. 3. **HTML injection in email**: `reset_url` is passed through `escape()` (from `html` stdlib) in the email template before being rendered in HTML attributes and text. The `<a href="{escape(reset_url)}">` pattern is correct. 4. **Keycloak admin API**: Uses the existing `reset_parent_password` service function which sets `temporary: False` -- correct for a self-service flow where the user chose their own password. **SQLAlchemy / Alembic** 5. **Migration 014**: Chain is correct (013 -> 014). Raw SQL migration uses `sa.text()` wrapper, includes proper indexes on `email` and `token` columns, and `downgrade()` drops with CASCADE. Clean. 6. **Model**: `PasswordResetToken` model mirrors the migration correctly. `server_default=func.now()` for `created_at` and `server_default=text("false")` for `used` match the SQL DDL. **FastAPI / Pydantic** 7. **Schemas**: Clean Pydantic models. `PasswordResetRequest`, `PasswordResetConfirm`, `PasswordResetResponse` are well-defined. 8. **Router registration**: Mounted at `/api/password-reset` in `main.py` -- consistent with the `/api/` prefix pattern used by other API routers. 9. **CORS**: The existing CORS middleware in `main.py` already allows `https://westsidekingsandqueens.tail5b443a.ts.net` -- no additional CORS config needed. **Email service** 10. **`send_password_reset_email`**: Follows the existing pattern from other email functions -- uses `get_gmail_client(tenant)`, `_brand_wrapper()`, and provides both HTML and plain text bodies. Returns `gmail_message_id` or `None` on failure. Exception is caught and logged, not re-raised -- consistent with the non-blocking email pattern. **Test coverage** 11. Seven tests covering: happy path request, enumeration prevention, invalid token, expired token, used token, successful Keycloak reset, and Keycloak failure. All edge cases for the token lifecycle are covered. Mocking strategy is correct -- patches at the route module level. Test fixtures use the shared `conftest.py` `tenant`, `client`, and `db` fixtures. ### BLOCKERS None. ### NITS 1. **Double Keycloak user lookup**: In `confirm_password_reset`, `get_existing_user()` is called at line 159 to verify the user exists, then `reset_parent_password()` is called at line 167 which internally calls `get_existing_user()` again (keycloak.py line 197). This is two HTTP roundtrips to Keycloak for the same email. Not a blocker since the function signatures are already established and used by `admin.py`, but worth noting for a future refactor (e.g., a `reset_parent_password_by_id` variant that accepts the user ID directly). 2. **Duplicated tenant slug constant**: `_DEFAULT_TENANT_SLUG = "westside-kings-queens"` in `password_reset.py` duplicates `DEFAULT_TENANT_SLUG` from `services/registration.py`. Consider importing from a single source to avoid drift. Not a blocker since the slug is stable and the duplication is in different modules, not in auth/security paths. 3. **Confirm endpoint returns 200 for all outcomes**: Invalid token, expired token, used token, and Keycloak failures all return HTTP 200 with different messages. The frontend will need to inspect the message string to determine success vs failure. A more conventional approach would use HTTP 400/422 for client errors and 503 for Keycloak failures. This is a design choice that works for the current frontend, but makes API semantics less discoverable. Not a blocker. 4. **No new-password validation**: The `new_password` field in `PasswordResetConfirm` accepts any string with no minimum length or complexity check. Keycloak itself may enforce password policies at the realm level, which would cause `reset_parent_password` to return `False`. If Keycloak does not have a policy, a user could set a 1-character password. Consider adding a Pydantic `min_length` constraint (e.g., `new_password: str = Field(min_length=8)`). Not a blocker since Keycloak is the ultimate gatekeeper. 5. **No rate limiting on `/request`**: The request endpoint has no rate limiting, so an attacker could trigger mass email sends. The existing codebase has no rate limiting anywhere, so this is consistent with the current state. Worth tracking as a future issue for all public endpoints. ### SOP COMPLIANCE - [x] Branch named after issue: `132-password-reset-oauth` references issue #132 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present - [ ] Related references plan slug: PR body references issue #132 but no plan slug. This is consistent with basketball-api's lighter-weight process (no plan phase required per `feedback_basketball_hands_off.md`) - [x] No secrets committed: No API keys, passwords, or credentials in code - [x] No unnecessary file changes: All 6 files are directly related to the password reset feature - [x] Tests exist and pass: 7 tests with good coverage of happy path and edge cases - [x] Commit messages are descriptive: PR title follows conventional commit format ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean, focused feature PR. 6 files, 592 additions, 0 deletions. No unrelated changes. Ready for single-shot deployment. - **Change failure risk**: Low. New feature with no modifications to existing logic (only additive changes to `main.py` and `email.py`). The `reset_parent_password` function in `keycloak.py` already existed and is battle-tested via the admin route. - **Cross-repo dependency**: Frontend PR (westside-app #63) consumes these endpoints. Backend can merge independently since the endpoints are additive. - **Tuesday blocker context**: This is infrastructure for girls tryout March 24. The double Keycloak lookup (nit #1) is negligible latency and should not delay merge. ### VERDICT: APPROVED

2026-03-21 16:36:53 +00:00

Summary

Self-service password reset endpoints that bypass Keycloak SMTP by sending reset links through the existing Gmail OAuth pipeline
Two public endpoints: request (generates token, sends branded email) and confirm (validates token, resets Keycloak password)
Returns 200 for both valid and invalid emails to prevent user enumeration

Changes

src/basketball_api/routes/password_reset.py (new): POST /api/password-reset/request generates a 1-hour token and sends a branded reset email; POST /api/password-reset/confirm validates the token and resets the Keycloak password via admin API
src/basketball_api/models.py: Added PasswordResetToken model (id, email, token, created_at, expires_at, used)
alembic/versions/014_add_password_reset_tokens.py (new): Migration to create password_reset_tokens table with indexes
src/basketball_api/services/email.py: Added send_password_reset_email() using existing _brand_wrapper() and Gmail OAuth client with Westside branding
src/basketball_api/main.py: Registered password reset router at /api/password-reset
tests/test_password_reset.py (new): 7 tests covering happy path, enumeration prevention, expiry, reuse, and Keycloak failure handling

Test Plan

Tests pass locally: pytest tests/test_password_reset.py -v -- all 7 pass
Full suite: pytest -- all 456 pass
ruff check and ruff format clean
Manual verification: frontend PR will add the /reset-password page that consumes these endpoints

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes forgejo_admin/basketball-api#132
Backend-only PR; frontend reset page will reference the same issue

## Summary - Self-service password reset endpoints that bypass Keycloak SMTP by sending reset links through the existing Gmail OAuth pipeline - Two public endpoints: request (generates token, sends branded email) and confirm (validates token, resets Keycloak password) - Returns 200 for both valid and invalid emails to prevent user enumeration ## Changes - `src/basketball_api/routes/password_reset.py` (new): POST /api/password-reset/request generates a 1-hour token and sends a branded reset email; POST /api/password-reset/confirm validates the token and resets the Keycloak password via admin API - `src/basketball_api/models.py`: Added PasswordResetToken model (id, email, token, created_at, expires_at, used) - `alembic/versions/014_add_password_reset_tokens.py` (new): Migration to create password_reset_tokens table with indexes - `src/basketball_api/services/email.py`: Added send_password_reset_email() using existing _brand_wrapper() and Gmail OAuth client with Westside branding - `src/basketball_api/main.py`: Registered password reset router at /api/password-reset - `tests/test_password_reset.py` (new): 7 tests covering happy path, enumeration prevention, expiry, reuse, and Keycloak failure handling ## Test Plan - [x] Tests pass locally: `pytest tests/test_password_reset.py -v` -- all 7 pass - [x] Full suite: `pytest` -- all 456 pass - [x] `ruff check` and `ruff format` clean - [ ] Manual verification: frontend PR will add the /reset-password page that consumes these endpoints ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/basketball-api#132 - Backend-only PR; frontend reset page will reference the same issue

forgejo_admin added 1 commit

feat: add password reset flow via Gmail OAuth (bypass Keycloak SMTP)

ci/woodpecker/pr/woodpecker Pipeline failed

Details

6aa2c81b34

New self-service password reset endpoints that send reset links via the
existing Gmail OAuth pipeline instead of relying on Keycloak's SMTP
configuration. Returns 200 for both valid and invalid emails to prevent
user enumeration.

- POST /api/password-reset/request: generates token, sends branded email
- POST /api/password-reset/confirm: validates token, resets via Keycloak
- PasswordResetToken model + migration (014)
- send_password_reset_email() in email service with Westside branding
- 7 tests covering happy path, enumeration, expiry, reuse, and errors

Ref #132

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

2026-03-21 16:38:33 +00:00

fix: use secrets.token_urlsafe for reset tokens instead of uuid4

ci/woodpecker/pr/woodpecker Pipeline was successful

2ba6e1ea1d

Matches the codebase pattern (registration_token uses secrets.token_urlsafe)
and provides cryptographically stronger tokens for the security-sensitive
password reset flow.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>