feat: require auth on tryout and upload endpoints

forgejo_admin commented

2026-03-16 01:49:21 +00:00

Author

Owner

PR #92 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Keycloak JWT auth (Pydantic models, dependency injection).

Auth pattern analysis: The get_current_user dependency (in auth.py) correctly returns 401 when no Bearer token is present (HTTPBearer(auto_error=False) + explicit credentials is None check). The require_role factory chains on top of get_current_user, adding 403 for insufficient permissions. This is sound -- get_current_user is the correct dependency for "any authenticated user" gating.

Endpoints modified (6 in tryouts.py, 1 in upload.py):

Endpoint	Before	After	Correct?
`GET /tryouts/roster/{tenant}`	Public	`get_current_user`	Yes
`GET /tryouts/roster/{tenant}/print`	Public	`get_current_user`	Yes
`GET /api/roster/{tenant}`	Public	`get_current_user`	Yes
`POST /api/roster/{tenant}/check-in/{id}`	Public	`get_current_user`	Yes
`POST /api/roster/{tenant}/assign-number/{id}`	Public	`get_current_user`	Yes
`GET /tryouts/admin/{tenant}`	Public	`get_current_user`	Yes
`POST /upload/photo`	Public	`get_current_user`	Yes

Endpoints correctly left unchanged:

Endpoint	Auth	Rationale
`GET /pay`	None (public)	Stripe redirect, no PII -- correct
`POST /tryouts/admin/{tenant}/assign-numbers`	`require_admin_role`	Already gated -- correct
`POST /tryouts/admin/{tenant}/checkin/{id}`	`require_admin_role`	Already gated -- correct
`POST /tryouts/admin/{tenant}/uncheckin/{id}`	`require_admin_role`	Already gated -- correct

The admin_client fixture correctly overrides BOTH require_admin_role AND get_current_user now (line 123). This is necessary because the admin dashboard endpoint moved from no auth to get_current_user, while checkin/uncheckin still use require_admin_role. Without overriding get_current_user in the admin fixture, admin tests hitting the dashboard would fail. Good catch by the dev.

Docstrings: All updated from "Public-but-unlisted" to "Requires authentication." -- accurate and consistent.

BLOCKERS

None. This PR is clean.

All 7 previously-public endpoints that expose PII (parent email, phone, registration tokens) are now auth-gated.
All 7 endpoints have corresponding test_requires_auth negative tests verifying 401/403 on unauthenticated access.
The /pay redirect correctly remains public (no PII, just a Stripe redirect).
The checkin_player and uncheckin_player endpoints retain their require_admin_role dependency and are unaffected by this change.
No secrets or credentials in code.
No unvalidated user input -- all existing input validation is unchanged.
Test coverage is comprehensive: 269 tests passing per PR description, both positive and negative cases covered.

NITS

DRY: Duplicated auth_client / public_client fixtures. The auth_client and public_client fixtures in tests/test_upload.py (lines 16-52) are identical copies of the fixtures in tests/test_tryouts.py (lines 128-164). These should be extracted to tests/conftest.py as shared fixtures. Not blocking because the duplication is in test code (not auth/security path), but it will drift.
Admin dashboard auth level. The admin dashboard (GET /tryouts/admin/{tenant}) is now gated by get_current_user (any authenticated user), but the page renders parent emails, payment status, registration tokens, and check-in buttons. The check-in buttons call endpoints gated by require_admin_role, so non-admin users would see data but get 403 on actions. Consider whether this should be require_role("admin") instead of get_current_user for defense-in-depth. The current choice is defensible (view vs. action separation), but worth a deliberate decision.
assert resp.status_code in (401, 403) pattern. All test_requires_auth tests accept both 401 and 403. Since get_current_user raises exactly 401 (and require_admin_role raises 403 after 401), the non-admin endpoints should always return 401. Tightening to assert resp.status_code == 401 for get_current_user-only endpoints would catch regressions where someone accidentally changes the auth dependency. Minor -- the current pattern is safe, just less precise.
Comment on line 432 still says "playground." The comment # --- JSON roster API (for SvelteKit playground dashboard) ---------------------- on tryouts.py line 432 still references "playground" even though the docstring was correctly updated to remove it. Cosmetic only.

SOP COMPLIANCE

Branch named after issue: Branch is feature/lock-down-public-endpoints, should be issue-91 or similar per convention. Non-blocking but noted.
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related -- all present and well-structured.
Related references plan slug: Yes, references plan-2026-03-08-tryout-prep Phase 11.
Related references issue: Yes, Closes #91.
No secrets committed: Confirmed clean.
No scope creep: All changes directly related to the auth lockdown.
Tests exist and cover positive + negative cases.

PROCESS OBSERVATIONS

Change Failure Risk: LOW. The change is mechanically simple (adding a FastAPI dependency to existing endpoints). The get_current_user dependency is battle-tested across the codebase. The westside-app frontend already passes tokens server-side, so no frontend breakage expected.
DORA impact: This is a security hardening PR that reduces attack surface. Good practice to ship this as a focused, single-purpose PR rather than bundling with feature work.
Discovered during QA of PR #90 -- this is the review-fix loop working as intended. QA found the gap, dev filed #91 and fixed it in a separate PR. Clean workflow.

VERDICT: APPROVED

## PR #92 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Keycloak JWT auth (Pydantic models, dependency injection). **Auth pattern analysis:** The `get_current_user` dependency (in `auth.py`) correctly returns 401 when no Bearer token is present (`HTTPBearer(auto_error=False)` + explicit `credentials is None` check). The `require_role` factory chains on top of `get_current_user`, adding 403 for insufficient permissions. This is sound -- `get_current_user` is the correct dependency for "any authenticated user" gating. **Endpoints modified (6 in tryouts.py, 1 in upload.py):** | Endpoint | Before | After | Correct? | |----------|--------|-------|----------| | `GET /tryouts/roster/{tenant}` | Public | `get_current_user` | Yes | | `GET /tryouts/roster/{tenant}/print` | Public | `get_current_user` | Yes | | `GET /api/roster/{tenant}` | Public | `get_current_user` | Yes | | `POST /api/roster/{tenant}/check-in/{id}` | Public | `get_current_user` | Yes | | `POST /api/roster/{tenant}/assign-number/{id}` | Public | `get_current_user` | Yes | | `GET /tryouts/admin/{tenant}` | Public | `get_current_user` | Yes | | `POST /upload/photo` | Public | `get_current_user` | Yes | **Endpoints correctly left unchanged:** | Endpoint | Auth | Rationale | |----------|------|-----------| | `GET /pay` | None (public) | Stripe redirect, no PII -- correct | | `POST /tryouts/admin/{tenant}/assign-numbers` | `require_admin_role` | Already gated -- correct | | `POST /tryouts/admin/{tenant}/checkin/{id}` | `require_admin_role` | Already gated -- correct | | `POST /tryouts/admin/{tenant}/uncheckin/{id}` | `require_admin_role` | Already gated -- correct | The `admin_client` fixture correctly overrides BOTH `require_admin_role` AND `get_current_user` now (line 123). This is necessary because the admin dashboard endpoint moved from no auth to `get_current_user`, while checkin/uncheckin still use `require_admin_role`. Without overriding `get_current_user` in the admin fixture, admin tests hitting the dashboard would fail. Good catch by the dev. **Docstrings:** All updated from "Public-but-unlisted" to "Requires authentication." -- accurate and consistent. ### BLOCKERS None. This PR is clean. - All 7 previously-public endpoints that expose PII (parent email, phone, registration tokens) are now auth-gated. - All 7 endpoints have corresponding `test_requires_auth` negative tests verifying 401/403 on unauthenticated access. - The `/pay` redirect correctly remains public (no PII, just a Stripe redirect). - The `checkin_player` and `uncheckin_player` endpoints retain their `require_admin_role` dependency and are unaffected by this change. - No secrets or credentials in code. - No unvalidated user input -- all existing input validation is unchanged. - Test coverage is comprehensive: 269 tests passing per PR description, both positive and negative cases covered. ### NITS 1. **DRY: Duplicated `auth_client` / `public_client` fixtures.** The `auth_client` and `public_client` fixtures in `tests/test_upload.py` (lines 16-52) are identical copies of the fixtures in `tests/test_tryouts.py` (lines 128-164). These should be extracted to `tests/conftest.py` as shared fixtures. Not blocking because the duplication is in test code (not auth/security path), but it will drift. 2. **Admin dashboard auth level.** The admin dashboard (`GET /tryouts/admin/{tenant}`) is now gated by `get_current_user` (any authenticated user), but the page renders parent emails, payment status, registration tokens, and check-in buttons. The check-in buttons call endpoints gated by `require_admin_role`, so non-admin users would see data but get 403 on actions. Consider whether this should be `require_role("admin")` instead of `get_current_user` for defense-in-depth. The current choice is defensible (view vs. action separation), but worth a deliberate decision. 3. **`assert resp.status_code in (401, 403)` pattern.** All `test_requires_auth` tests accept both 401 and 403. Since `get_current_user` raises exactly 401 (and `require_admin_role` raises 403 after 401), the non-admin endpoints should always return 401. Tightening to `assert resp.status_code == 401` for `get_current_user`-only endpoints would catch regressions where someone accidentally changes the auth dependency. Minor -- the current pattern is safe, just less precise. 4. **Comment on line 432 still says "playground."** The comment `# --- JSON roster API (for SvelteKit playground dashboard) ----------------------` on tryouts.py line 432 still references "playground" even though the docstring was correctly updated to remove it. Cosmetic only. ### SOP COMPLIANCE - [ ] Branch named after issue: Branch is `feature/lock-down-public-endpoints`, should be `issue-91` or similar per convention. Non-blocking but noted. - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related -- all present and well-structured. - [x] Related references plan slug: Yes, references `plan-2026-03-08-tryout-prep` Phase 11. - [x] Related references issue: Yes, `Closes #91`. - [x] No secrets committed: Confirmed clean. - [x] No scope creep: All changes directly related to the auth lockdown. - [x] Tests exist and cover positive + negative cases. ### PROCESS OBSERVATIONS - **Change Failure Risk: LOW.** The change is mechanically simple (adding a FastAPI dependency to existing endpoints). The `get_current_user` dependency is battle-tested across the codebase. The westside-app frontend already passes tokens server-side, so no frontend breakage expected. - **DORA impact:** This is a security hardening PR that reduces attack surface. Good practice to ship this as a focused, single-purpose PR rather than bundling with feature work. - **Discovered during QA of PR #90** -- this is the review-fix loop working as intended. QA found the gap, dev filed #91 and fixed it in a separate PR. Clean workflow. ### VERDICT: APPROVED

2026-03-16 01:46:56 +00:00

Summary

Lock down all previously-public endpoints that expose parent PII (email, phone, registration tokens). Tryouts are over, all 50 users have Keycloak accounts, and there is no longer a reason to serve roster data without authentication. Every endpoint now requires a valid JWT via get_current_user (any authenticated role).

Changes

src/basketball_api/routes/tryouts.py -- Added get_current_user dependency to 6 endpoints: GET /api/roster/{tenant}, GET /tryouts/roster/{tenant}, GET /tryouts/roster/{tenant}/print, GET /tryouts/admin/{tenant}, POST /api/roster/{tenant}/check-in/{player_id}, POST /api/roster/{tenant}/assign-number/{player_id}
src/basketball_api/routes/upload.py -- Added get_current_user dependency to POST /upload/photo
tests/test_tryouts.py -- Added auth_client fixture (overrides get_current_user), updated admin_client to also override get_current_user, switched all functional tests from public_client to auth_client, added test_requires_auth tests for roster, print roster, admin dashboard, check-in, and assign-number endpoints
tests/test_upload.py -- Rewrote to use auth_client/public_client fixtures with proper auth mocking, added test_requires_auth test

Test Plan

Full test suite passes (269 passed) -- pytest tests/ -v
All 7 secured endpoints return 401 without auth token (verified by test_requires_auth tests)
All endpoints work normally with valid auth token
ruff format and ruff check pass

Review Checklist

No unrelated changes
Tests cover both positive (auth works) and negative (401 without auth) cases
Existing test count preserved -- no tests removed, only updated
Frontend compatibility: westside-app passes tokens server-side via event.locals.accessToken

Forgejo issue: Closes #91
Plan: plan-2026-03-08-tryout-prep Phase 11
PR #90 (player profile endpoints -- discovered this gap during QA review)

## Summary Lock down all previously-public endpoints that expose parent PII (email, phone, registration tokens). Tryouts are over, all 50 users have Keycloak accounts, and there is no longer a reason to serve roster data without authentication. Every endpoint now requires a valid JWT via `get_current_user` (any authenticated role). ## Changes - `src/basketball_api/routes/tryouts.py` -- Added `get_current_user` dependency to 6 endpoints: `GET /api/roster/{tenant}`, `GET /tryouts/roster/{tenant}`, `GET /tryouts/roster/{tenant}/print`, `GET /tryouts/admin/{tenant}`, `POST /api/roster/{tenant}/check-in/{player_id}`, `POST /api/roster/{tenant}/assign-number/{player_id}` - `src/basketball_api/routes/upload.py` -- Added `get_current_user` dependency to `POST /upload/photo` - `tests/test_tryouts.py` -- Added `auth_client` fixture (overrides `get_current_user`), updated `admin_client` to also override `get_current_user`, switched all functional tests from `public_client` to `auth_client`, added `test_requires_auth` tests for roster, print roster, admin dashboard, check-in, and assign-number endpoints - `tests/test_upload.py` -- Rewrote to use `auth_client`/`public_client` fixtures with proper auth mocking, added `test_requires_auth` test ## Test Plan - [x] Full test suite passes (269 passed) -- `pytest tests/ -v` - [x] All 7 secured endpoints return 401 without auth token (verified by `test_requires_auth` tests) - [x] All endpoints work normally with valid auth token - [x] `ruff format` and `ruff check` pass ## Review Checklist - [x] No unrelated changes - [x] Tests cover both positive (auth works) and negative (401 without auth) cases - [x] Existing test count preserved -- no tests removed, only updated - [x] Frontend compatibility: westside-app passes tokens server-side via `event.locals.accessToken` ## Related - Forgejo issue: Closes #91 - Plan: `plan-2026-03-08-tryout-prep` Phase 11 - PR #90 (player profile endpoints -- discovered this gap during QA review)

forgejo_admin added 1 commit

feat: require auth on tryout and upload endpoints 75f5d539c7

Lock down all previously-public endpoints that expose parent PII.
Tryouts are over and all users have Keycloak accounts, so there is
no longer a reason to serve roster data without authentication.

Endpoints secured:
- GET /api/roster/{tenant} (JSON roster with parent info)
- GET /tryouts/roster/{tenant} (HTML coach roster)
- GET /tryouts/roster/{tenant}/print (print view)
- GET /tryouts/admin/{tenant} (admin dashboard)
- POST /api/roster/{tenant}/check-in/{player_id}
- POST /api/roster/{tenant}/assign-number/{player_id}
- POST /upload/photo

All endpoints use get_current_user (any authenticated user, not
role-restricted). Tests updated to use auth_client fixture and
new tests verify 401 on unauthenticated access.

Closes #91

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

Endpoint	Before	After	Correct?
`GET /tryouts/roster/{tenant}`	Public	`get_current_user`	Yes
`GET /tryouts/roster/{tenant}/print`	Public	`get_current_user`	Yes
`GET /api/roster/{tenant}`	Public	`get_current_user`	Yes
`POST /api/roster/{tenant}/check-in/{id}`	Public	`get_current_user`	Yes
`POST /api/roster/{tenant}/assign-number/{id}`	Public	`get_current_user`	Yes
`GET /tryouts/admin/{tenant}`	Public	`get_current_user`	Yes
`POST /upload/photo`	Public	`get_current_user`	Yes

Endpoint	Auth	Rationale
`GET /pay`	None (public)	Stripe redirect, no PII -- correct
`POST /tryouts/admin/{tenant}/assign-numbers`	`require_admin_role`	Already gated -- correct
`POST /tryouts/admin/{tenant}/checkin/{id}`	`require_admin_role`	Already gated -- correct
`POST /tryouts/admin/{tenant}/uncheckin/{id}`	`require_admin_role`	Already gated -- correct