doc: auth architecture with management paths and key decisions

forgejo_admin commented

2026-03-28 11:54:57 +00:00

Author

Owner

PR #200 Review

DOMAIN REVIEW

Stack: Documentation only (Markdown). No code changes. The document describes a Python/FastAPI/Keycloak auth architecture.

Since this is a pure documentation PR, the review focuses on accuracy against the codebase and completeness against the issue's acceptance criteria.

Accuracy verification -- I read the referenced source files and cross-checked every factual claim:

Claim in document	Source file	Verified
`get_current_user` returns `User(sub, email, username, roles)`	`auth.py:27-33, 75-157`	Correct
`require_role("admin")` factory pattern	`auth.py:160-181`	Correct
Dual-issuer: internal URL for JWKS, external URL accepted in `iss`	`auth.py:128-143`	Correct
Key rotation: clear cache + re-fetch on unknown `kid`	`auth.py:111-119`	Correct
`create_account_for_parent()` flow: token, create (no password), emailVerified:True, role, execute-actions-email	`services/keycloak.py:253-332`	Correct
ADMIN_EMAILS: `draneylucas@gmail.com`, `mldraney3@gmail.com`	`services/keycloak.py:23`	Correct
`determine_role()` returns 'admin' or 'player'	`services/keycloak.py:184-197`	Correct
`secrets.token_urlsafe(32)` for reset tokens	`routes/password_reset.py:86`	Correct
`password_reset_tokens` table, 1-hour expiry, single-use	`models.py:371`, `routes/password_reset.py:35,190`	Correct
`reset_parent_password()` via Admin API PUT with `temporary: false`	`services/keycloak.py:200-250`	Correct
`send_password_reset_email()` in `services/email.py`	`services/email.py:1016`	Correct

All factual claims in the document match the codebase. The architecture diagram, flow descriptions, URL table, role matrix, and decision rationale are all accurate.

BLOCKERS

None.

This is a documentation-only PR (182 additions, 0 deletions, 1 file: docs/auth-architecture.md). No code changes means the standard BLOCKER criteria (test coverage, input validation, secrets, DRY) do not apply. No secrets or credentials appear in the document -- only Tailscale funnel URLs and email addresses already present in source code.

NITS

Minor URL inconsistency: The document states the internal cluster URL as http://keycloak.keycloak.svc.cluster.local:80 (Key URLs table), but config.py line 16 defines keycloak_admin_url as http://keycloak.keycloak.svc.cluster.local (no explicit :80). Meanwhile keycloak_realm_url on config.py line 10-12 does include :80. Functionally identical (port 80 is default HTTP), but the document could note both forms exist in config to prevent future confusion.
Account lifecycle trigger simplification: The document says "Parent completes Stripe checkout" as the trigger for create_account_for_parent(). In reality, it is called from three places: routes/register.py (direct paid registration), routes/register.py (promo code paid registration), and routes/webhooks.py (Stripe webhook for pending registrations). The summary is not wrong, but a parenthetical noting "called from both registration routes and the Stripe webhook handler" would be more precise for future developers.
Missing coach role assignment path: The document correctly states "The coach role exists in Keycloak but is assigned manually via the admin console, not during auto-provisioning." This is accurate per determine_role() which only returns 'admin' or 'player'. Good documentation of a non-obvious gap.

SOP COMPLIANCE

Branch named after issue (145-doc-auth-architecture matches issue #145)
PR body has Summary, Changes, Test Plan, Related sections
Related section references closing issue (Closes #145) and related issues (#132, #144)
No secrets committed (URLs are public Tailscale funnels, emails are in source code)
No unnecessary file changes (1 new file, documentation scope only)
Commit message is descriptive

PROCESS OBSERVATIONS

Change failure risk: Zero. Documentation-only change cannot break production.
Knowledge capture: High value. This document captures tribal knowledge about three auth management paths, the Gmail OAuth vs SMTP decision, and dual-issuer validation. These are exactly the kind of decisions that cause hours of wasted rework when undocumented (as the PR description notes re: custom password reset).
Sync risk: The document notes a canonical home in pal-e-docs (arch-auth-westside-basketball) and states "keep both in sync." This creates a future maintenance burden. The PR test plan includes Betty Sue porting to pal-e-docs, which addresses the initial sync.

VERDICT: APPROVED

## PR #200 Review ### DOMAIN REVIEW **Stack**: Documentation only (Markdown). No code changes. The document describes a Python/FastAPI/Keycloak auth architecture. Since this is a pure documentation PR, the review focuses on **accuracy** against the codebase and **completeness** against the issue's acceptance criteria. **Accuracy verification** -- I read the referenced source files and cross-checked every factual claim: | Claim in document | Source file | Verified | |---|---|---| | `get_current_user` returns `User(sub, email, username, roles)` | `auth.py:27-33, 75-157` | Correct | | `require_role("admin")` factory pattern | `auth.py:160-181` | Correct | | Dual-issuer: internal URL for JWKS, external URL accepted in `iss` | `auth.py:128-143` | Correct | | Key rotation: clear cache + re-fetch on unknown `kid` | `auth.py:111-119` | Correct | | `create_account_for_parent()` flow: token, create (no password), emailVerified:True, role, execute-actions-email | `services/keycloak.py:253-332` | Correct | | ADMIN_EMAILS: `draneylucas@gmail.com`, `mldraney3@gmail.com` | `services/keycloak.py:23` | Correct | | `determine_role()` returns 'admin' or 'player' | `services/keycloak.py:184-197` | Correct | | `secrets.token_urlsafe(32)` for reset tokens | `routes/password_reset.py:86` | Correct | | `password_reset_tokens` table, 1-hour expiry, single-use | `models.py:371`, `routes/password_reset.py:35,190` | Correct | | `reset_parent_password()` via Admin API PUT with `temporary: false` | `services/keycloak.py:200-250` | Correct | | `send_password_reset_email()` in `services/email.py` | `services/email.py:1016` | Correct | All factual claims in the document match the codebase. The architecture diagram, flow descriptions, URL table, role matrix, and decision rationale are all accurate. ### BLOCKERS None. This is a documentation-only PR (182 additions, 0 deletions, 1 file: `docs/auth-architecture.md`). No code changes means the standard BLOCKER criteria (test coverage, input validation, secrets, DRY) do not apply. No secrets or credentials appear in the document -- only Tailscale funnel URLs and email addresses already present in source code. ### NITS 1. **Minor URL inconsistency**: The document states the internal cluster URL as `http://keycloak.keycloak.svc.cluster.local:80` (Key URLs table), but `config.py` line 16 defines `keycloak_admin_url` as `http://keycloak.keycloak.svc.cluster.local` (no explicit `:80`). Meanwhile `keycloak_realm_url` on config.py line 10-12 does include `:80`. Functionally identical (port 80 is default HTTP), but the document could note both forms exist in config to prevent future confusion. 2. **Account lifecycle trigger simplification**: The document says "Parent completes Stripe checkout" as the trigger for `create_account_for_parent()`. In reality, it is called from three places: `routes/register.py` (direct paid registration), `routes/register.py` (promo code paid registration), and `routes/webhooks.py` (Stripe webhook for pending registrations). The summary is not wrong, but a parenthetical noting "called from both registration routes and the Stripe webhook handler" would be more precise for future developers. 3. **Missing `coach` role assignment path**: The document correctly states "The `coach` role exists in Keycloak but is assigned manually via the admin console, not during auto-provisioning." This is accurate per `determine_role()` which only returns 'admin' or 'player'. Good documentation of a non-obvious gap. ### SOP COMPLIANCE - [x] Branch named after issue (`145-doc-auth-architecture` matches issue #145) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related section references closing issue (`Closes #145`) and related issues (#132, #144) - [x] No secrets committed (URLs are public Tailscale funnels, emails are in source code) - [x] No unnecessary file changes (1 new file, documentation scope only) - [x] Commit message is descriptive ### PROCESS OBSERVATIONS - **Change failure risk**: Zero. Documentation-only change cannot break production. - **Knowledge capture**: High value. This document captures tribal knowledge about three auth management paths, the Gmail OAuth vs SMTP decision, and dual-issuer validation. These are exactly the kind of decisions that cause hours of wasted rework when undocumented (as the PR description notes re: custom password reset). - **Sync risk**: The document notes a canonical home in pal-e-docs (`arch-auth-westside-basketball`) and states "keep both in sync." This creates a future maintenance burden. The PR test plan includes Betty Sue porting to pal-e-docs, which addresses the initial sync. ### VERDICT: APPROVED

2026-03-28 11:51:17 +00:00

Summary

Documents the complete auth architecture for Westside Basketball, including the three auth management paths and key architectural decisions. Prevents future sessions from rebuilding what Keycloak already provides (hours spent on custom password reset before realizing Keycloak's admin/account consoles exist).

Changes

docs/auth-architecture.md — New file. Complete auth architecture document covering:
- Token validation (JWKS, dual-issuer, key rotation)
- Account lifecycle (Stripe checkout to Keycloak provisioning)
- Three auth management paths (self-service reset, account console, admin console)
- Key decisions (Gmail OAuth over SMTP, custom forgot-password, dual-URL issuer)
- Key URLs and role matrix
- "When to use which" decision table

Test Plan

Review document accuracy against codebase (auth.py, password_reset.py, keycloak.py, config.py)
Verify all URLs are current
Betty Sue ports content to pal-e-docs note arch-auth-westside-basketball via create_block / update_block

Review Checklist

No code changes -- documentation only
Content derived from actual codebase (auth.py, keycloak.py, password_reset.py, config.py, account.py)
All three auth paths documented per acceptance criteria
Key decisions documented per acceptance criteria
URLs and roles documented per acceptance criteria
Betty Sue to verify pal-e-docs port accuracy after merge

Closes #145
basketball-api #132 -- Custom password reset flow implementation
basketball-api #144 -- Marcus admin access + account console links
pal-e-docs target: arch-auth-westside-basketball (Auth Management Paths + Key Decisions subsections)

## Summary Documents the complete auth architecture for Westside Basketball, including the three auth management paths and key architectural decisions. Prevents future sessions from rebuilding what Keycloak already provides (hours spent on custom password reset before realizing Keycloak's admin/account consoles exist). ## Changes - `docs/auth-architecture.md` — New file. Complete auth architecture document covering: - Token validation (JWKS, dual-issuer, key rotation) - Account lifecycle (Stripe checkout to Keycloak provisioning) - Three auth management paths (self-service reset, account console, admin console) - Key decisions (Gmail OAuth over SMTP, custom forgot-password, dual-URL issuer) - Key URLs and role matrix - "When to use which" decision table ## Test Plan - [ ] Review document accuracy against codebase (`auth.py`, `password_reset.py`, `keycloak.py`, `config.py`) - [ ] Verify all URLs are current - [ ] Betty Sue ports content to pal-e-docs note `arch-auth-westside-basketball` via `create_block` / `update_block` ## Review Checklist - [x] No code changes -- documentation only - [x] Content derived from actual codebase (auth.py, keycloak.py, password_reset.py, config.py, account.py) - [x] All three auth paths documented per acceptance criteria - [x] Key decisions documented per acceptance criteria - [x] URLs and roles documented per acceptance criteria - [ ] Betty Sue to verify pal-e-docs port accuracy after merge ## Related Notes - Closes #145 - basketball-api #132 -- Custom password reset flow implementation - basketball-api #144 -- Marcus admin access + account console links - pal-e-docs target: `arch-auth-westside-basketball` (Auth Management Paths + Key Decisions subsections)

forgejo_admin added 1 commit

doc: auth architecture with management paths and key decisions

ci/woodpecker/pr/woodpecker Pipeline failed

Details

17a2262b70

Documents the three auth management paths (self-service password reset,
Keycloak account console, Keycloak admin console) and key architectural
decisions (Gmail OAuth over Keycloak SMTP, custom forgot-password flow,
dual-issuer JWT validation). Content is repo-local in docs/ for Betty Sue
to port to pal-e-docs as arch-auth-westside-basketball.

Closes #145

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

2026-03-28 11:51:56 +00:00

fix: clarify coach role is manually assigned, not auto-provisioned

7b7d1f8b3c

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

Claim in document	Source file	Verified
`get_current_user` returns `User(sub, email, username, roles)`	`auth.py:27-33, 75-157`	Correct
`require_role("admin")` factory pattern	`auth.py:160-181`	Correct
Dual-issuer: internal URL for JWKS, external URL accepted in `iss`	`auth.py:128-143`	Correct
Key rotation: clear cache + re-fetch on unknown `kid`	`auth.py:111-119`	Correct
`create_account_for_parent()` flow: token, create (no password), emailVerified:True, role, execute-actions-email	`services/keycloak.py:253-332`	Correct
ADMIN_EMAILS: `draneylucas@gmail.com`, `mldraney3@gmail.com`	`services/keycloak.py:23`	Correct
`determine_role()` returns 'admin' or 'player'	`services/keycloak.py:184-197`	Correct
`secrets.token_urlsafe(32)` for reset tokens	`routes/password_reset.py:86`	Correct
`password_reset_tokens` table, 1-hour expiry, single-use	`models.py:371`, `routes/password_reset.py:35,190`	Correct
`reset_parent_password()` via Admin API PUT with `temporary: false`	`services/keycloak.py:200-250`	Correct
`send_password_reset_email()` in `services/email.py`	`services/email.py:1016`	Correct