feat: add dual-auth dependency for jersey and checkout routes (#255) #258

Merged

forgejo_admin merged 1 commit from 255-dual-auth-jersey-checkout into main 2026-03-30 21:00:53 +00:00

Conversation 2 Commits 1 Files changed 4 +385 -48

forgejo_admin commented 2026-03-30 16:15:42 +00:00

Contributor

Copy link

Summary

Add a reusable get_parent_dual_auth dependency that resolves a Parent using either a ?token= registration token (email-link flow) or a Keycloak Bearer JWT with email lookup (SPA flow). This unblocks self-service jersey ordering from the authenticated SPA without breaking existing email-link flows.

Changes

• src/basketball_api/auth.py — Added _get_optional_user (non-erroring Bearer resolver) and get_parent_dual_auth dependency that tries registration token first, falls back to Keycloak JWT email match. Eagerly loads Parent.players via joinedload.
• src/basketball_api/routes/jersey.py — Updated jersey_player_info and jersey_checkout to use get_parent_dual_auth instead of inline Parent.registration_token lookup. Removed unused joinedload import.
• src/basketball_api/routes/checkout.py — Updated create_checkout_session to use get_parent_dual_auth instead of inline token lookup. Removed unused joinedload import.
• tests/test_dual_auth.py — 13 new tests covering: valid token path, invalid token 404, valid JWT path, JWT email-no-match 404, no-auth 401, and integration tests for jersey and checkout routes with both auth methods.

Test Plan

• pytest tests/ -x -q — 702 tests pass (689 existing + 13 new)
• pytest tests/ -k "dual_auth or jersey or checkout" — all dual-auth, jersey, and checkout tests pass
• register.py and tryouts.py have zero diff (confirmed via git diff)
• ruff format and ruff check clean

Review Checklist

• No unrelated changes
• register.py and tryouts.py untouched (zero blast radius)
• Token query param remains optional for backwards compatibility
• Eagerly loads Parent.players relationship via joinedload
• Follows existing dependency injection pattern from auth.py
• All 702 tests pass
• ruff format and ruff check clean

Related Notes

• None

Related

• Closes #255
• Parent spike: forgejo_admin/westside-landing#196
• Story: WS-S18 — parent self-service jersey ordering

## Summary Add a reusable `get_parent_dual_auth` dependency that resolves a Parent using either a `?token=` registration token (email-link flow) or a Keycloak Bearer JWT with email lookup (SPA flow). This unblocks self-service jersey ordering from the authenticated SPA without breaking existing email-link flows. ## Changes - `src/basketball_api/auth.py` — Added `_get_optional_user` (non-erroring Bearer resolver) and `get_parent_dual_auth` dependency that tries registration token first, falls back to Keycloak JWT email match. Eagerly loads `Parent.players` via `joinedload`. - `src/basketball_api/routes/jersey.py` — Updated `jersey_player_info` and `jersey_checkout` to use `get_parent_dual_auth` instead of inline `Parent.registration_token` lookup. Removed unused `joinedload` import. - `src/basketball_api/routes/checkout.py` — Updated `create_checkout_session` to use `get_parent_dual_auth` instead of inline token lookup. Removed unused `joinedload` import. - `tests/test_dual_auth.py` — 13 new tests covering: valid token path, invalid token 404, valid JWT path, JWT email-no-match 404, no-auth 401, and integration tests for jersey and checkout routes with both auth methods. ## Test Plan - `pytest tests/ -x -q` — 702 tests pass (689 existing + 13 new) - `pytest tests/ -k "dual_auth or jersey or checkout"` — all dual-auth, jersey, and checkout tests pass - `register.py` and `tryouts.py` have zero diff (confirmed via `git diff`) - `ruff format` and `ruff check` clean ## Review Checklist - [x] No unrelated changes - [x] `register.py` and `tryouts.py` untouched (zero blast radius) - [x] Token query param remains optional for backwards compatibility - [x] Eagerly loads `Parent.players` relationship via `joinedload` - [x] Follows existing dependency injection pattern from `auth.py` - [x] All 702 tests pass - [x] ruff format and ruff check clean ## Related Notes - None ## Related - Closes #255 - Parent spike: `forgejo_admin/westside-landing#196` - Story: WS-S18 — parent self-service jersey ordering

forgejo_admin added 1 commit 2026-03-30 16:15:42 +00:00

feat: add dual-auth dependency for jersey and checkout routes (#255) ... d9332abb23

Add get_parent_dual_auth dependency in auth.py that resolves a Parent
using either a registration_token query param (email-link flow) or a
Keycloak Bearer token with email lookup (SPA flow). Update jersey.py
and checkout.py routes to use this dependency instead of inline
token-only lookups. register.py and tryouts.py are untouched.

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

forgejo_admin commented 2026-03-30 16:16:27 +00:00

Author

Contributor

Copy link

QA Review -- PR #258

Summary

Adds get_parent_dual_auth dependency in auth.py that resolves a Parent via either registration token or Keycloak JWT email lookup. Updates jersey and checkout routes to use it. 13 new tests. 702 total pass.

Findings

Architecture (PASS)

• _get_optional_user + get_parent_dual_auth properly uses FastAPI DI chain. _get_optional_user is testable via app.dependency_overrides, which the tests demonstrate.
• Token-first priority is correct: email-link flow stays unchanged, JWT is fallback.
• Deferred import of Parent model inside get_parent_dual_auth avoids circular import between auth.py and models.py. Acceptable pattern.

Backwards Compatibility (PASS)

• ?token= query param remains optional (Query(None)), so existing email-link callers work unchanged.
• Existing test_jersey.py (token-based tests) and test_checkout.py (token-based tests) all pass without modification, confirming no regression.

Blast Radius (PASS)

• register.py and tryouts.py have zero diff, confirmed.
• Only 4 files changed, all scoped to the ticket.

Test Coverage (PASS)

• Token valid, token invalid (404), JWT valid, JWT email-no-match (404), no-auth (401) -- all five acceptance criteria covered.
• Integration tests cover jersey player-info, jersey checkout, and checkout create-session with both auth methods.
• 702 tests pass (689 existing + 13 new).

Code Quality (PASS)

• joinedload(Parent.players) eager loading in the dependency satisfies the constraint.
• Removed unused joinedload imports from jersey.py and checkout.py -- clean.
• Functions correctly changed to async def since they await the dual-auth dependency.
• ruff format and ruff check clean.

Nits (non-blocking)

• None identified.

VERDICT: APPROVED

## QA Review -- PR #258 ### Summary Adds `get_parent_dual_auth` dependency in `auth.py` that resolves a Parent via either registration token or Keycloak JWT email lookup. Updates jersey and checkout routes to use it. 13 new tests. 702 total pass. ### Findings **Architecture (PASS)** - `_get_optional_user` + `get_parent_dual_auth` properly uses FastAPI DI chain. `_get_optional_user` is testable via `app.dependency_overrides`, which the tests demonstrate. - Token-first priority is correct: email-link flow stays unchanged, JWT is fallback. - Deferred import of `Parent` model inside `get_parent_dual_auth` avoids circular import between `auth.py` and `models.py`. Acceptable pattern. **Backwards Compatibility (PASS)** - `?token=` query param remains optional (`Query(None)`), so existing email-link callers work unchanged. - Existing test_jersey.py (token-based tests) and test_checkout.py (token-based tests) all pass without modification, confirming no regression. **Blast Radius (PASS)** - `register.py` and `tryouts.py` have zero diff, confirmed. - Only 4 files changed, all scoped to the ticket. **Test Coverage (PASS)** - Token valid, token invalid (404), JWT valid, JWT email-no-match (404), no-auth (401) -- all five acceptance criteria covered. - Integration tests cover jersey player-info, jersey checkout, and checkout create-session with both auth methods. - 702 tests pass (689 existing + 13 new). **Code Quality (PASS)** - `joinedload(Parent.players)` eager loading in the dependency satisfies the constraint. - Removed unused `joinedload` imports from jersey.py and checkout.py -- clean. - Functions correctly changed to `async def` since they `await` the dual-auth dependency. - ruff format and ruff check clean. **Nits (non-blocking)** - None identified. ### VERDICT: APPROVED

forgejo_admin commented 2026-03-30 16:19:14 +00:00

Author

Contributor

Copy link

PR #258 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic (with Stripe integration)

Auth logic (get_parent_dual_auth):

• Token-first, JWT-fallback ordering is correct. When ?token= is present, it takes precedence regardless of whether a Bearer token is also sent. This prevents ambiguity.
• _get_optional_user correctly wraps get_current_user and returns None instead of raising 401 when no Bearer token is present. The existing _bearer_scheme = HTTPBearer(auto_error=False) already returns None for missing auth headers, so the chain is sound.
• The deferred from basketball_api.models import Parent inside the function body correctly avoids a circular import (auth.py does not import models at module level, and models.py does not import auth).
• ilike for email matching follows the established codebase pattern (used identically in coaches_api.py:113, account.py:57, public.py:290).
• joinedload(Parent.players) ensures the eager load happens in both paths, matching what the removed inline code was doing.

Route changes:

• jersey_player_info correctly drops the db parameter since all DB work is now in the dependency. The parent.players access works because of the eager load.
• jersey_checkout and create_checkout_session correctly retain db: Session = Depends(get_db) since they perform additional DB operations downstream.
• FastAPI caches Depends(get_db) by callable identity within a single request, so the db session in get_parent_dual_auth and in the route handler is the same instance. The Parent object returned from the dependency is usable in the route handler's session context.

Sync-to-async change: All three route handlers changed from def to async def. These handlers still do synchronous SQLAlchemy operations, which will now run on the event loop rather than in a threadpool. This matches the existing pattern in this codebase (submit_registration, check_email, etc. are already async def with sync DB ops). Acceptable for this API's concurrency profile.

Test coverage (test_dual_auth.py):

• 13 tests across 5 test classes covering:
  • Token path: valid token, invalid token (404)
  • JWT path: valid JWT with matching email, JWT with non-matching email (404)
  • No auth: 401 when neither method provided
  • Integration: jersey player-info, jersey checkout, and checkout create-session with both auth methods
• Mock pattern (_make_jwt_client overriding _get_optional_user) correctly bypasses JWKS fetch while still exercising the get_parent_dual_auth logic.
• Fixture pattern (parent_with_player, _seed_tenant, opt_out_product) follows established test conventions from conftest.py, test_jersey.py, and test_checkout.py.
• finally: app.dependency_overrides.clear() in JWT tests prevents test pollution.

Security:

• No auth bypass: the 401 fallthrough when neither token nor JWT is present is correct and tested.
• Registration token lookup uses exact == match (no injection risk).
• Email matching uses ilike (case-insensitive) which is the established pattern and appropriate for email addresses.
• No secrets, credentials, or hardcoded tokens in the diff.

BLOCKERS

None.

NITS

1. NIT: Missing return type annotation on get_parent_dual_auth. The function returns Parent but the signature is async def get_parent_dual_auth(...): without -> Parent. Other dependencies in auth.py (get_current_user -> User, _get_optional_user -> User | None) have return type annotations. Adding -> "Parent" (string annotation to avoid the import at module level) would maintain consistency.

2. NIT: token parameter naming overlap. The get_parent_dual_auth dependency uses token: str | None = Query(None, alias="token") where the parameter name and alias are the same. The alias is technically redundant here but does make the query param name explicit. No functional issue -- just noting for clarity.

SOP COMPLIANCE

• Branch named after issue: 255-dual-auth-jersey-checkout follows {issue-number}-{kebab-case-purpose} convention
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
• Related references story: WS-S18, parent spike, and closes #255
• No secrets committed
• No unrelated changes (register.py and tryouts.py confirmed untouched)
• Commit messages descriptive (PR title matches convention)

PROCESS OBSERVATIONS

• This is a clean DRY improvement: three identical inline parent-lookup blocks consolidated into one reusable dependency. The centralization reduces divergence risk for future auth changes.
• The PR correctly identifies discovered scope in issue #256 (multi-player parent player_id param) rather than expanding this PR's scope.
• Test count increase (689 -> 702) with zero test failures reported. Coverage is solid across both auth paths and all affected routes.

VERDICT: APPROVED

## PR #258 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic (with Stripe integration) **Auth logic (`get_parent_dual_auth`)**: - Token-first, JWT-fallback ordering is correct. When `?token=` is present, it takes precedence regardless of whether a Bearer token is also sent. This prevents ambiguity. - `_get_optional_user` correctly wraps `get_current_user` and returns `None` instead of raising 401 when no Bearer token is present. The existing `_bearer_scheme = HTTPBearer(auto_error=False)` already returns `None` for missing auth headers, so the chain is sound. - The deferred `from basketball_api.models import Parent` inside the function body correctly avoids a circular import (auth.py does not import models at module level, and models.py does not import auth). - `ilike` for email matching follows the established codebase pattern (used identically in `coaches_api.py:113`, `account.py:57`, `public.py:290`). - `joinedload(Parent.players)` ensures the eager load happens in both paths, matching what the removed inline code was doing. **Route changes**: - `jersey_player_info` correctly drops the `db` parameter since all DB work is now in the dependency. The `parent.players` access works because of the eager load. - `jersey_checkout` and `create_checkout_session` correctly retain `db: Session = Depends(get_db)` since they perform additional DB operations downstream. - FastAPI caches `Depends(get_db)` by callable identity within a single request, so the `db` session in `get_parent_dual_auth` and in the route handler is the same instance. The Parent object returned from the dependency is usable in the route handler's session context. **Sync-to-async change**: All three route handlers changed from `def` to `async def`. These handlers still do synchronous SQLAlchemy operations, which will now run on the event loop rather than in a threadpool. This matches the existing pattern in this codebase (`submit_registration`, `check_email`, etc. are already `async def` with sync DB ops). Acceptable for this API's concurrency profile. **Test coverage (`test_dual_auth.py`)**: - 13 tests across 5 test classes covering: - Token path: valid token, invalid token (404) - JWT path: valid JWT with matching email, JWT with non-matching email (404) - No auth: 401 when neither method provided - Integration: jersey player-info, jersey checkout, and checkout create-session with both auth methods - Mock pattern (`_make_jwt_client` overriding `_get_optional_user`) correctly bypasses JWKS fetch while still exercising the `get_parent_dual_auth` logic. - Fixture pattern (`parent_with_player`, `_seed_tenant`, `opt_out_product`) follows established test conventions from `conftest.py`, `test_jersey.py`, and `test_checkout.py`. - `finally: app.dependency_overrides.clear()` in JWT tests prevents test pollution. **Security**: - No auth bypass: the 401 fallthrough when neither token nor JWT is present is correct and tested. - Registration token lookup uses exact `==` match (no injection risk). - Email matching uses `ilike` (case-insensitive) which is the established pattern and appropriate for email addresses. - No secrets, credentials, or hardcoded tokens in the diff. ### BLOCKERS None. ### NITS 1. **NIT: Missing return type annotation on `get_parent_dual_auth`**. The function returns `Parent` but the signature is `async def get_parent_dual_auth(...):` without `-> Parent`. Other dependencies in `auth.py` (`get_current_user -> User`, `_get_optional_user -> User | None`) have return type annotations. Adding `-> "Parent"` (string annotation to avoid the import at module level) would maintain consistency. 2. **NIT: `token` parameter naming overlap**. The `get_parent_dual_auth` dependency uses `token: str | None = Query(None, alias="token")` where the parameter name and alias are the same. The alias is technically redundant here but does make the query param name explicit. No functional issue -- just noting for clarity. ### SOP COMPLIANCE - [x] Branch named after issue: `255-dual-auth-jersey-checkout` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references story: WS-S18, parent spike, and closes #255 - [x] No secrets committed - [x] No unrelated changes (register.py and tryouts.py confirmed untouched) - [x] Commit messages descriptive (PR title matches convention) ### PROCESS OBSERVATIONS - This is a clean DRY improvement: three identical inline parent-lookup blocks consolidated into one reusable dependency. The centralization reduces divergence risk for future auth changes. - The PR correctly identifies discovered scope in issue #256 (multi-player parent `player_id` param) rather than expanding this PR's scope. - Test count increase (689 -> 702) with zero test failures reported. Coverage is solid across both auth paths and all affected routes. ### VERDICT: APPROVED

forgejo_admin merged commit 83188444d8 into main 2026-03-30 21:00:53 +00:00

forgejo_admin deleted branch 255-dual-auth-jersey-checkout 2026-03-30 21:00:53 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-30 21:00:55 +00:00

feat: add dual-auth dependency for jersey and checkout routes (#255) (#258)

forgejo_admin referenced this pull request 2026-03-30 21:01:51 +00:00

Clean up QA nits from dual-auth and jersey profile PRs #260

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/basketball-api!258

No description provided.