forgejo_admin/basketball-api

Fork 1

feat: auto Keycloak account creation + email-mismatch fix + script names fix #94

Merged

forgejo_admin merged 1 commit from 93-feat-auto-keycloak-account-creation-on-r into main

2026-03-16 21:16:15 +00:00

forgejo_admin commented

2026-03-16 21:09:42 +00:00

Owner

Summary

Three tightly coupled fixes for the registration-to-account-creation pipeline. When a parent completes paid registration, their Keycloak account is now created automatically with correct firstName/lastName and role. The email-mismatch bug that created duplicate parent records is fixed by using token-based lookup. The batch script now includes names in the payload to prevent VERIFY_PROFILE prompts.

Changes

src/basketball_api/services/keycloak.py (new): Shared Keycloak Admin API service module extracted from the batch script. All functions accept base_url parameter. Includes create_account_for_parent() high-level entry point with full error handling (never raises).
src/basketball_api/services/password.py (new): Canonical location for generate_password and extract_first_name. Moved from scripts.
src/basketball_api/routes/register.py: Token-based parent lookup when form_token is present (fixes email-mismatch bug). Auto Keycloak account creation after db.commit() for paid registrations. Credentials section on confirmation page.
src/basketball_api/config.py: Added keycloak_base_url and keycloak_admin_password settings.
scripts/create_keycloak_accounts.py: Refactored to import from shared services/keycloak module. Passes first_name/last_name to create_keycloak_user().
scripts/password_gen.py: Re-exports from basketball_api.services.password for backward compatibility.
tests/conftest.py: Default empty BASKETBALL_KEYCLOAK_ADMIN_PASSWORD env var.
tests/test_keycloak_integration.py (new): 15 tests covering all acceptance criteria.

Test Plan

All 284 existing tests pass (pytest tests/ -v)
ruff format and ruff check pass clean
New tests cover all acceptance criteria:
- Token-based parent lookup prevents email-mismatch duplicates
- Paid registration creates Keycloak user with firstName, lastName, role
- Existing Keycloak user skipped gracefully (no error, no duplicate)
- Unpaid registration does NOT trigger Keycloak calls
- Batch script payload includes firstName/lastName
- Keycloak failure does not block registration (returns None, logs error)
All Keycloak httpx calls are mocked -- no real Keycloak needed for tests

Review Checklist

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

Closes #93
plan-wkq -- Phase 11 (Girls Tryout, March 21)

## Summary Three tightly coupled fixes for the registration-to-account-creation pipeline. When a parent completes paid registration, their Keycloak account is now created automatically with correct firstName/lastName and role. The email-mismatch bug that created duplicate parent records is fixed by using token-based lookup. The batch script now includes names in the payload to prevent VERIFY_PROFILE prompts. ## Changes - `src/basketball_api/services/keycloak.py` (new): Shared Keycloak Admin API service module extracted from the batch script. All functions accept `base_url` parameter. Includes `create_account_for_parent()` high-level entry point with full error handling (never raises). - `src/basketball_api/services/password.py` (new): Canonical location for `generate_password` and `extract_first_name`. Moved from scripts. - `src/basketball_api/routes/register.py`: Token-based parent lookup when `form_token` is present (fixes email-mismatch bug). Auto Keycloak account creation after `db.commit()` for paid registrations. Credentials section on confirmation page. - `src/basketball_api/config.py`: Added `keycloak_base_url` and `keycloak_admin_password` settings. - `scripts/create_keycloak_accounts.py`: Refactored to import from shared `services/keycloak` module. Passes `first_name`/`last_name` to `create_keycloak_user()`. - `scripts/password_gen.py`: Re-exports from `basketball_api.services.password` for backward compatibility. - `tests/conftest.py`: Default empty `BASKETBALL_KEYCLOAK_ADMIN_PASSWORD` env var. - `tests/test_keycloak_integration.py` (new): 15 tests covering all acceptance criteria. ## Test Plan - [x] All 284 existing tests pass (`pytest tests/ -v`) - [x] `ruff format` and `ruff check` pass clean - [x] New tests cover all acceptance criteria: - Token-based parent lookup prevents email-mismatch duplicates - Paid registration creates Keycloak user with firstName, lastName, role - Existing Keycloak user skipped gracefully (no error, no duplicate) - Unpaid registration does NOT trigger Keycloak calls - Batch script payload includes firstName/lastName - Keycloak failure does not block registration (returns None, logs error) - [x] All Keycloak httpx calls are mocked -- no real Keycloak needed for tests ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #93 - `plan-wkq` -- Phase 11 (Girls Tryout, March 21)

forgejo_admin added 1 commit

2026-03-16 21:09:42 +00:00

feat: auto Keycloak account creation on registration + fix email-mismatch + fix script names

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

6f80fa7383

Three tightly coupled fixes for the registration-to-account pipeline:

1. Bug fix (email-mismatch): Registration POST now matches parents by
   registration_token when a token is present, falling back to email
   only for walk-up flow. Prevents duplicate parent records when Stripe
   checkout email differs from the form email.

2. Bug fix (script names): create_keycloak_accounts.py now includes
   firstName/lastName in the Keycloak user payload via the shared
   service module, preventing VERIFY_PROFILE prompts on first login.

3. Feature (auto account creation): After a paid registration completes,
   automatically creates a Keycloak account with correct name, role,
   and generated password. Credentials are displayed on the confirmation
   page. Keycloak failures are caught and logged -- they never block
   registration.

Architecture: Extracted Keycloak Admin API operations into a shared
service module (services/keycloak.py) used by both the registration
route and the batch script. Password generation moved to
services/password.py as canonical location with scripts/password_gen.py
re-exporting for backward compatibility.

Closes #93

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

forgejo_admin commented

2026-03-16 21:10:48 +00:00

Author

Owner

Self-Review

Reviewed the full diff (8 files, +919 -177 lines). No issues found.

Correctness

Email-mismatch fix: Token-based parent lookup is correct -- Parent.registration_token == form_token is checked first, email fallback only for walk-up flow. Parent email is updated to the form value after token match, which is the intended behavior.
Auto Keycloak creation: Runs after db.commit() so parent data is safe regardless of Keycloak outcome. create_account_for_parent() catches all exceptions and returns None on failure. Guard condition (is_paid and settings.keycloak_admin_password) prevents calls when password is not configured.
Script fix: firstName/lastName now included in payload. Parent name parsed into first/last correctly.

Architecture

Shared service module (services/keycloak.py) used by both route and script -- no duplication.
Password module moved to canonical services/password.py with backward-compatible re-export from scripts/password_gen.py.
All Keycloak functions accept base_url parameter (not hardcoded), enabling testing and multi-environment deployment.

Test Coverage

15 new tests cover all acceptance criteria from #93.
All Keycloak httpx calls properly mocked -- no real network calls in tests.
All 284 tests pass. Ruff format and check clean.

No Issues Found

No secrets committed
No unrelated changes
Credentials display uses escape() for XSS prevention
Error handling is defensive throughout

## Self-Review Reviewed the full diff (8 files, +919 -177 lines). No issues found. ### Correctness - **Email-mismatch fix**: Token-based parent lookup is correct -- `Parent.registration_token == form_token` is checked first, email fallback only for walk-up flow. Parent email is updated to the form value after token match, which is the intended behavior. - **Auto Keycloak creation**: Runs after `db.commit()` so parent data is safe regardless of Keycloak outcome. `create_account_for_parent()` catches all exceptions and returns None on failure. Guard condition (`is_paid and settings.keycloak_admin_password`) prevents calls when password is not configured. - **Script fix**: `firstName`/`lastName` now included in payload. Parent name parsed into first/last correctly. ### Architecture - Shared service module (`services/keycloak.py`) used by both route and script -- no duplication. - Password module moved to canonical `services/password.py` with backward-compatible re-export from `scripts/password_gen.py`. - All Keycloak functions accept `base_url` parameter (not hardcoded), enabling testing and multi-environment deployment. ### Test Coverage - 15 new tests cover all acceptance criteria from #93. - All Keycloak httpx calls properly mocked -- no real network calls in tests. - All 284 tests pass. Ruff format and check clean. ### No Issues Found - No secrets committed - No unrelated changes - Credentials display uses `escape()` for XSS prevention - Error handling is defensive throughout

forgejo_admin commented

2026-03-16 21:14:54 +00:00

Author

Owner

PR #94 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic / httpx. Keycloak Admin API integration.

Architecture assessment: Well-structured extraction of Keycloak logic into a shared service module (services/keycloak.py). The batch script and registration route now share one codepath for user creation, role assignment, and password generation. The create_account_for_parent() high-level entry point correctly wraps everything in try/except and never raises -- a Keycloak failure cannot block registration. This is the right design.

Token-based lookup (email-mismatch fix): The registration route at line 833 now checks form_token first and matches by registration_token column, falling back to email only for walk-up (no-token) flows. This correctly addresses the duplicate-parent bug where Stripe checkout email differs from form email.

PEP compliance:

PEP 484 type hints present on all public functions in services/keycloak.py and services/password.py.
PEP 257 docstrings present on all public functions with Args/Returns/Raises sections.
Module-level docstrings present on all new files.

OWASP / Security:

User input is HTML-escaped via escape() before rendering in confirmation pages (lines 311-312, 401, etc.). Good.
Token values in hidden form fields are escaped with escape(token, quote=True) (line 141). Good.
The keycloak_admin_password config defaults to empty string and is loaded via env var (BASKETBALL_ prefix). No credentials in code. Good.
The conftest.py explicitly sets BASKETBALL_KEYCLOAK_ADMIN_PASSWORD to empty to prevent test-time Keycloak calls. Good.

SQLAlchemy patterns:

db.commit() is called before Keycloak account creation (line 980), ensuring parent/player data is persisted regardless of Keycloak outcome. Correct session management.
Token lookup uses proper filter on Parent.registration_token with tenant scoping. Good.

httpx usage:

All Keycloak API calls use explicit timeout=30.0. Good.
raise_for_status() called after each request. Good.

BLOCKERS

None.

Test coverage: 15 new tests in test_keycloak_integration.py covering:

Token-based parent lookup (email-mismatch fix) -- happy path + walk-up fallback
Auto Keycloak account creation with firstName/lastName verification
Duplicate account skip (idempotency)
Unpaid registration does NOT trigger Keycloak calls
Batch script payload includes names
Service module unit tests (determine_role, create_account_for_parent, password generation)
Error handling (token acquisition failure returns None, does not raise)
Re-export backward compatibility for scripts/password_gen.py

All acceptance criteria have corresponding tests. No BLOCKER issues.

Input validation: Token is validated against the database (line 571, line 835). Form fields validated for required values. File uploads validated for extension, content type, and size. No unvalidated user input reaches Keycloak API calls.

Secrets: No credentials committed. Admin password loaded from environment. Default is empty string (disables feature). Config uses BASKETBALL_ env prefix via pydantic-settings.

NITS

random vs secrets for password generation (services/password.py line 10): random.randint is not cryptographically secure. For password generation, secrets.randbelow(90) + 10 would be more appropriate. The practical risk is low given the password pattern is Westside-{Name}-{2digits} (only 90 possible suffixes per name), but using secrets signals intent correctly. This is a pre-existing pattern from the batch script, so not introduced by this PR.
Duplicated name-parsing logic (services/keycloak.py lines 219-221 and scripts/create_keycloak_accounts.py lines 148-150): The parent_name.strip().split(None, 1) pattern for extracting first/last name is duplicated between the shared service and the batch script. Consider extracting a parse_parent_name(name: str) -> tuple[str, str] helper into the service module so both callers use one implementation. Minor DRY concern -- not in an auth/security path, so not a blocker.
Hardcoded KEYCLOAK_BASE_URL in batch script (scripts/create_keycloak_accounts.py line 52): The script has KEYCLOAK_BASE_URL = "https://keycloak.tail5b443a.ts.net" hardcoded, while the FastAPI app reads it from settings.keycloak_base_url (which defaults to the same value but is overridable via env). Consider reading from BASKETBALL_KEYCLOAK_BASE_URL env var with the same default, for consistency.
Test overlap with test_account_creation.py: The new TestKeycloakServiceModule and TestPasswordModule classes in test_keycloak_integration.py partially overlap with existing tests in test_account_creation.py (e.g., determine_role tests, generate_password pattern test, extract_first_name tests). Not harmful, but worth noting for future deduplication.
ADMIN_EMAILS re-export (scripts/create_keycloak_accounts.py line 35): The ADMIN_EMAILS import is kept with a # noqa: F401 comment "re-exported for test_account_creation.py". Now that ADMIN_EMAILS lives canonically in services/keycloak.py, test_account_creation.py could import directly from there. Low priority.

SOP COMPLIANCE

Branch named after issue (93-feat-auto-keycloak-account-creation-on-r)
PR body has: ## Summary, ## Changes, ## Test Plan, ## Related
Related section references plan-wkq
Tests exist (15 new tests covering all acceptance criteria)
No secrets, .env files, or credentials committed
No unnecessary file changes -- all changes serve the three stated goals
Commit messages are descriptive

PROCESS OBSERVATIONS

Deployment Frequency: This PR bundles three tightly coupled fixes into one changeset. Given that the email-mismatch bug, auto account creation, and script names fix all touch the registration-to-account pipeline, bundling is justified. Shipping them separately would risk intermediate broken states.
Change Failure Risk: Low. The Keycloak integration is fully behind a feature gate (settings.keycloak_admin_password must be non-empty). Tests default it to empty, and the code path is designed to never block registration on Keycloak failure. The token-based lookup is a strict superset of the old behavior (falls back to email lookup when no token present).
MTTR: Good error handling design. All Keycloak failures are caught and logged at the service layer. Registration succeeds even if Keycloak is down. Credentials are only shown on the confirmation page when account creation succeeds.

VERDICT: APPROVED

## PR #94 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic / httpx. Keycloak Admin API integration. **Architecture assessment:** Well-structured extraction of Keycloak logic into a shared service module (`services/keycloak.py`). The batch script and registration route now share one codepath for user creation, role assignment, and password generation. The `create_account_for_parent()` high-level entry point correctly wraps everything in try/except and never raises -- a Keycloak failure cannot block registration. This is the right design. **Token-based lookup (email-mismatch fix):** The registration route at line 833 now checks `form_token` first and matches by `registration_token` column, falling back to email only for walk-up (no-token) flows. This correctly addresses the duplicate-parent bug where Stripe checkout email differs from form email. **PEP compliance:** - PEP 484 type hints present on all public functions in `services/keycloak.py` and `services/password.py`. - PEP 257 docstrings present on all public functions with Args/Returns/Raises sections. - Module-level docstrings present on all new files. **OWASP / Security:** - User input is HTML-escaped via `escape()` before rendering in confirmation pages (lines 311-312, 401, etc.). Good. - Token values in hidden form fields are escaped with `escape(token, quote=True)` (line 141). Good. - The `keycloak_admin_password` config defaults to empty string and is loaded via env var (`BASKETBALL_` prefix). No credentials in code. Good. - The `conftest.py` explicitly sets `BASKETBALL_KEYCLOAK_ADMIN_PASSWORD` to empty to prevent test-time Keycloak calls. Good. **SQLAlchemy patterns:** - `db.commit()` is called before Keycloak account creation (line 980), ensuring parent/player data is persisted regardless of Keycloak outcome. Correct session management. - Token lookup uses proper filter on `Parent.registration_token` with tenant scoping. Good. **httpx usage:** - All Keycloak API calls use explicit `timeout=30.0`. Good. - `raise_for_status()` called after each request. Good. ### BLOCKERS None. **Test coverage:** 15 new tests in `test_keycloak_integration.py` covering: - Token-based parent lookup (email-mismatch fix) -- happy path + walk-up fallback - Auto Keycloak account creation with firstName/lastName verification - Duplicate account skip (idempotency) - Unpaid registration does NOT trigger Keycloak calls - Batch script payload includes names - Service module unit tests (determine_role, create_account_for_parent, password generation) - Error handling (token acquisition failure returns None, does not raise) - Re-export backward compatibility for `scripts/password_gen.py` All acceptance criteria have corresponding tests. No BLOCKER issues. **Input validation:** Token is validated against the database (line 571, line 835). Form fields validated for required values. File uploads validated for extension, content type, and size. No unvalidated user input reaches Keycloak API calls. **Secrets:** No credentials committed. Admin password loaded from environment. Default is empty string (disables feature). Config uses `BASKETBALL_` env prefix via pydantic-settings. ### NITS 1. **`random` vs `secrets` for password generation** (`services/password.py` line 10): `random.randint` is not cryptographically secure. For password generation, `secrets.randbelow(90) + 10` would be more appropriate. The practical risk is low given the password pattern is `Westside-{Name}-{2digits}` (only 90 possible suffixes per name), but using `secrets` signals intent correctly. This is a pre-existing pattern from the batch script, so not introduced by this PR. 2. **Duplicated name-parsing logic** (`services/keycloak.py` lines 219-221 and `scripts/create_keycloak_accounts.py` lines 148-150): The `parent_name.strip().split(None, 1)` pattern for extracting first/last name is duplicated between the shared service and the batch script. Consider extracting a `parse_parent_name(name: str) -> tuple[str, str]` helper into the service module so both callers use one implementation. Minor DRY concern -- not in an auth/security path, so not a blocker. 3. **Hardcoded `KEYCLOAK_BASE_URL` in batch script** (`scripts/create_keycloak_accounts.py` line 52): The script has `KEYCLOAK_BASE_URL = "https://keycloak.tail5b443a.ts.net"` hardcoded, while the FastAPI app reads it from `settings.keycloak_base_url` (which defaults to the same value but is overridable via env). Consider reading from `BASKETBALL_KEYCLOAK_BASE_URL` env var with the same default, for consistency. 4. **Test overlap with `test_account_creation.py`**: The new `TestKeycloakServiceModule` and `TestPasswordModule` classes in `test_keycloak_integration.py` partially overlap with existing tests in `test_account_creation.py` (e.g., `determine_role` tests, `generate_password` pattern test, `extract_first_name` tests). Not harmful, but worth noting for future deduplication. 5. **`ADMIN_EMAILS` re-export** (`scripts/create_keycloak_accounts.py` line 35): The `ADMIN_EMAILS` import is kept with a `# noqa: F401` comment "re-exported for test_account_creation.py". Now that `ADMIN_EMAILS` lives canonically in `services/keycloak.py`, `test_account_creation.py` could import directly from there. Low priority. ### SOP COMPLIANCE - [x] Branch named after issue (`93-feat-auto-keycloak-account-creation-on-r`) - [x] PR body has: ## Summary, ## Changes, ## Test Plan, ## Related - [x] Related section references `plan-wkq` - [x] Tests exist (15 new tests covering all acceptance criteria) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all changes serve the three stated goals - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment Frequency:** This PR bundles three tightly coupled fixes into one changeset. Given that the email-mismatch bug, auto account creation, and script names fix all touch the registration-to-account pipeline, bundling is justified. Shipping them separately would risk intermediate broken states. - **Change Failure Risk:** Low. The Keycloak integration is fully behind a feature gate (`settings.keycloak_admin_password` must be non-empty). Tests default it to empty, and the code path is designed to never block registration on Keycloak failure. The token-based lookup is a strict superset of the old behavior (falls back to email lookup when no token present). - **MTTR:** Good error handling design. All Keycloak failures are caught and logged at the service layer. Registration succeeds even if Keycloak is down. Credentials are only shown on the confirmation page when account creation succeeds. ### VERDICT: APPROVED