feat: add player profile endpoints (GET + PATCH) #90

Merged

forgejo_admin merged 1 commit from feature/11a-player-profile-endpoints into main 2026-03-16 01:10:00 +00:00

Conversation 2 Commits 1 Files changed 3 +670

forgejo_admin commented 2026-03-16 00:57:18 +00:00

Owner

Copy link

Summary

Add GET /api/players/{player_id} and PATCH /api/players/{player_id} endpoints for individual player profiles with role-based permissions. This enables the frontend to render player profile pages with edit capabilities gated by user role.

Changes

• src/basketball_api/routes/players.py (new): Player profile router with GET and PATCH endpoints. Pydantic response models (PlayerProfileResponse, PlayerProfileUpdate, TeamInfo, ParentInfo). Permission model: admin updates any, player updates own (parent email match, case-insensitive), coach gets 403. Read-only fields (name, team_id, division, tryout_number, subscription_status) excluded from update schema.
• src/basketball_api/main.py: Register players router at /api/players prefix.
• tests/test_players.py (new): 18 tests covering GET fields, team+coach inclusion, parent info, 404/401, all three role permissions on PATCH, read-only field protection, partial updates, case-insensitive email matching.

Test Plan

• pytest tests/test_players.py -v -- 18/18 passed
• pytest tests/ -v -- 266/266 passed (248 existing + 18 new)
• ruff format + ruff check clean
• GET returns player + team + parent info
• PATCH permission model enforced (admin: any, player: own, coach: 403)
• Read-only fields cannot be modified via PATCH

Review Checklist

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

Related

• Closes #89
• plan-2026-03-08-tryout-prep -- Phase 11a

## Summary Add `GET /api/players/{player_id}` and `PATCH /api/players/{player_id}` endpoints for individual player profiles with role-based permissions. This enables the frontend to render player profile pages with edit capabilities gated by user role. ## Changes - `src/basketball_api/routes/players.py` (new): Player profile router with GET and PATCH endpoints. Pydantic response models (`PlayerProfileResponse`, `PlayerProfileUpdate`, `TeamInfo`, `ParentInfo`). Permission model: admin updates any, player updates own (parent email match, case-insensitive), coach gets 403. Read-only fields (name, team_id, division, tryout_number, subscription_status) excluded from update schema. - `src/basketball_api/main.py`: Register players router at `/api/players` prefix. - `tests/test_players.py` (new): 18 tests covering GET fields, team+coach inclusion, parent info, 404/401, all three role permissions on PATCH, read-only field protection, partial updates, case-insensitive email matching. ## Test Plan - [x] `pytest tests/test_players.py -v` -- 18/18 passed - [x] `pytest tests/ -v` -- 266/266 passed (248 existing + 18 new) - [x] `ruff format` + `ruff check` clean - [x] GET returns player + team + parent info - [x] PATCH permission model enforced (admin: any, player: own, coach: 403) - [x] Read-only fields cannot be modified via PATCH ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #89 - `plan-2026-03-08-tryout-prep` -- Phase 11a

forgejo_admin added 1 commit 2026-03-16 00:57:18 +00:00

feat: add player profile endpoints (GET + PATCH) for Phase 11a ... c4a2781f73

Closes #89

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

forgejo_admin commented 2026-03-16 00:57:44 +00:00

Author

Owner

Copy link

Self-Review: PASS

Reviewed the full diff (3 files, +670 lines). No issues found.

Route implementation (routes/players.py)

• Pydantic schemas correctly separate editable fields (PlayerProfileUpdate) from read-only response fields (PlayerProfileResponse). Read-only fields (name, team_id, division, tryout_number, subscription_status) cannot be modified via PATCH because they are not in the update model -- FastAPI silently drops unknown fields.
• Permission model is correct: admin bypasses ownership check, coach gets 403, player must match parent email (case-insensitive).
• _get_player_or_404 uses joinedload for team and parent relationships, avoiding N+1 queries.
• Coach name accessed via player.team.coach -- this is a lazy load but acceptable since team is already eagerly loaded and coach is a single FK relationship.
• exclude_unset=True on model_dump ensures partial updates work correctly (omitted fields are not set to None).

Router registration (main.py)

• Registered at /api/players prefix, alphabetically sorted with other imports. Clean.

Tests (test_players.py)

• 18 tests covering all acceptance criteria from the issue.
• Auth mocking follows the established pattern from test_teams.py (override get_current_user dependency).
• Data fixtures create realistic player data with parent, team, and coach relationships.
• Case-insensitive email test (PARENT@TEST.COM) validates the ownership check.
• Read-only field test sends team_id, subscription_status, tryout_number, division, and name in the PATCH body and verifies none of them changed.

No issues

• No secrets, no unnecessary files, no regressions (266/266 tests pass).
• Ruff format and lint clean.

## Self-Review: PASS Reviewed the full diff (3 files, +670 lines). No issues found. ### Route implementation (`routes/players.py`) - Pydantic schemas correctly separate editable fields (`PlayerProfileUpdate`) from read-only response fields (`PlayerProfileResponse`). Read-only fields (name, team_id, division, tryout_number, subscription_status) cannot be modified via PATCH because they are not in the update model -- FastAPI silently drops unknown fields. - Permission model is correct: admin bypasses ownership check, coach gets 403, player must match parent email (case-insensitive). - `_get_player_or_404` uses `joinedload` for team and parent relationships, avoiding N+1 queries. - Coach name accessed via `player.team.coach` -- this is a lazy load but acceptable since team is already eagerly loaded and coach is a single FK relationship. - `exclude_unset=True` on `model_dump` ensures partial updates work correctly (omitted fields are not set to None). ### Router registration (`main.py`) - Registered at `/api/players` prefix, alphabetically sorted with other imports. Clean. ### Tests (`test_players.py`) - 18 tests covering all acceptance criteria from the issue. - Auth mocking follows the established pattern from `test_teams.py` (override `get_current_user` dependency). - Data fixtures create realistic player data with parent, team, and coach relationships. - Case-insensitive email test (`PARENT@TEST.COM`) validates the ownership check. - Read-only field test sends `team_id`, `subscription_status`, `tryout_number`, `division`, and `name` in the PATCH body and verifies none of them changed. ### No issues - No secrets, no unnecessary files, no regressions (266/266 tests pass). - Ruff format and lint clean.

forgejo_admin commented 2026-03-16 00:59:51 +00:00

Author

Owner

Copy link

PR #90 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic v2 / Keycloak OIDC auth.

Route pattern consistency with teams.py:
The new players.py follows the same structural conventions -- separator comments, Pydantic schemas at top, helper functions in the middle, endpoints at bottom. Good.

Auth usage:
Uses get_current_user dependency directly (not require_role), which is correct for these endpoints since the permission model is role-checked inline (admin=any, player=own, coach=403 on PATCH; any authenticated user on GET).

SQLAlchemy patterns:

• _get_player_or_404 eager-loads Player.team and Player.parent via joinedload, but _player_profile_response accesses player.team.coach (line 97) which is NOT eager-loaded. This works because SQLAlchemy lazy-loads it in a synchronous context, but it is an implicit N+1 query. The teams.py route avoids this by loading Team.coach explicitly in its joinedload options. Add joinedload(Player.team, Team.coach) to _get_player_or_404 to match the established pattern and avoid the lazy load.

Pydantic models:

• PlayerProfileUpdate correctly uses model_dump(exclude_unset=True) for partial updates -- consistent with teams.py PATCH pattern.
• ParentInfo has model_config = {"from_attributes": True} but is constructed manually, so it is unused. Harmless but inconsistent -- TeamInfo does NOT have from_attributes. Nit-level.

Permission model:

• Admin: can PATCH any player. Correct.
• Player (parent email match, case-insensitive): can PATCH own. Correct, tested.
• Coach: gets 403 on PATCH. Correct. Edge case of coach+admin dual role is handled correctly ("admin" not in user_roles check takes priority).
• GET: any authenticated user. Correct, returns 401 without auth (tested).

BLOCKERS

1. No tenant scoping on player lookup -- cross-tenant data access

_get_player_or_404 queries Player.id globally with no tenant filter. Every other route in this codebase (teams.py, subscriptions.py, tryouts.py, roster.py) scopes queries by tenant_id. This means:

• Any authenticated user from Tenant A can view and (if admin) modify players belonging to Tenant B.
• The ownership check (_is_player_owner) only verifies parent email, not tenant membership.

This is a multi-tenancy authorization bypass. The endpoint should either accept a tenant_id parameter (consistent with teams.py) or derive tenant scope from the authenticated user's context.

Impact: A player in one organization can read profile details (including parent name, email, phone) of players in a completely different organization.

2. Parent PII exposed to all authenticated users via GET

ParentInfo in the GET response includes email and phone for every player profile. Any authenticated user (player, coach, admin) from any tenant can read this. Combined with Blocker #1, this is a cross-tenant PII leak.

Even within a single tenant, exposing parent phone/email to other parents (player role) may be undesirable. Consider:

• Restricting ParentInfo fields based on the requesting user's role
• Or at minimum, scoping GET to same-tenant users

NITS

1. Implicit lazy load on player.team.coach (medium priority)

As noted in the domain review, _get_player_or_404 does not eager-load the coach relationship through the team. Add joinedload(Player.team).joinedload(Team.coach) to match the pattern in teams.py and eliminate the implicit lazy query. This is technically a performance issue, not a correctness bug, since SQLAlchemy handles it transparently in sync code.

In file src/basketball_api/routes/players.py, the _get_player_or_404 function at approximately line 72 should use chained joinedload:

.options(
    joinedload(Player.team).joinedload(Team.coach),
    joinedload(Player.parent),
)

This would require importing Team (and possibly Coach) from basketball_api.models.

2. ParentInfo.model_config unused

ParentInfo sets from_attributes = True but is always constructed by explicit keyword arguments. TeamInfo does not set it. Pick one pattern and be consistent.

3. photo_url accepts arbitrary strings

No URL validation on the photo_url field in PlayerProfileUpdate. The existing upload route (upload.py) generates server-side paths, so this PATCH endpoint could be used to inject arbitrary strings (e.g., javascript: URIs) if the frontend renders the value in an <img> or <a> tag without sanitization. Consider adding a Pydantic HttpUrl type or at least a regex pattern validator. Low risk if the frontend already sanitizes, but defense-in-depth is preferred.

4. No test for empty PATCH body

Consider adding a test for PATCH /api/players/{id} with an empty JSON body {}. The current code should handle it correctly (no fields updated, returns current state), but it is an untested edge case.

5. Logger call after commit (minor)

In update_player_profile, the logger.info call at line 189 happens after the _get_player_or_404 reload but logs provided.keys() from the original request body. This is fine functionally, but if the reload query fails (unlikely), the log entry would never fire and you would lose the audit trail. Consider logging before the reload.

SOP COMPLIANCE

• Branch named after issue number -- Branch is feature/11a-player-profile-endpoints (references phase 11a, not issue #89). Convention expects issue number in branch name.
• PR body has: Summary, Changes, Test Plan, Related -- All present and thorough.
• Related section references the plan slug -- References plan-2026-03-08-tryout-prep Phase 11a.
• Tests exist and pass -- 18 new tests, 266 total passing. Good coverage of happy path, auth, permissions, edge cases.
• No secrets committed -- Clean.
• No unnecessary file changes (scope creep) -- 3 files changed, all directly relevant.
• Commit messages are descriptive -- PR title follows conventional commits.

PROCESS OBSERVATIONS

Change failure risk: MEDIUM. The cross-tenant data access gap (Blocker #1) is a real authorization vulnerability in a multi-tenant system. Even if there is currently only one tenant, this establishes an insecure pattern that will bite when a second tenant is onboarded. Every other route in the codebase scopes by tenant -- this one should too.

Test coverage is strong for the single-tenant case. The 18 tests cover GET fields, relationships, 404, 401, all three role permissions on PATCH, read-only field protection, partial updates, and case-insensitive email matching. However, no test verifies cross-tenant isolation because the tests only seed one tenant.

Deployment frequency impact: LOW. This is additive (new endpoints, new file, no migrations), so rollback is trivial -- remove the router registration line.

VERDICT: NOT APPROVED

Two blockers must be addressed:

1. Add tenant scoping to _get_player_or_404 (and add tenant_id parameter to endpoints, or derive from auth context) to prevent cross-tenant data access.
2. Evaluate ParentInfo PII exposure -- at minimum, tenant-scope the GET so users cannot read parent contact info across tenants. Ideally, restrict parent email/phone visibility by role.

## PR #90 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic v2 / Keycloak OIDC auth. **Route pattern consistency with `teams.py`:** The new `players.py` follows the same structural conventions -- separator comments, Pydantic schemas at top, helper functions in the middle, endpoints at bottom. Good. **Auth usage:** Uses `get_current_user` dependency directly (not `require_role`), which is correct for these endpoints since the permission model is role-checked inline (admin=any, player=own, coach=403 on PATCH; any authenticated user on GET). **SQLAlchemy patterns:** - `_get_player_or_404` eager-loads `Player.team` and `Player.parent` via `joinedload`, but `_player_profile_response` accesses `player.team.coach` (line 97) which is NOT eager-loaded. This works because SQLAlchemy lazy-loads it in a synchronous context, but it is an implicit N+1 query. The `teams.py` route avoids this by loading `Team.coach` explicitly in its `joinedload` options. **Add `joinedload(Player.team, Team.coach)` to `_get_player_or_404` to match the established pattern and avoid the lazy load.** **Pydantic models:** - `PlayerProfileUpdate` correctly uses `model_dump(exclude_unset=True)` for partial updates -- consistent with `teams.py` PATCH pattern. - `ParentInfo` has `model_config = {"from_attributes": True}` but is constructed manually, so it is unused. Harmless but inconsistent -- `TeamInfo` does NOT have `from_attributes`. Nit-level. **Permission model:** - Admin: can PATCH any player. Correct. - Player (parent email match, case-insensitive): can PATCH own. Correct, tested. - Coach: gets 403 on PATCH. Correct. Edge case of coach+admin dual role is handled correctly (`"admin" not in user_roles` check takes priority). - GET: any authenticated user. Correct, returns 401 without auth (tested). ### BLOCKERS **1. No tenant scoping on player lookup -- cross-tenant data access** `_get_player_or_404` queries `Player.id` globally with no tenant filter. Every other route in this codebase (`teams.py`, `subscriptions.py`, `tryouts.py`, `roster.py`) scopes queries by `tenant_id`. This means: - Any authenticated user from Tenant A can view and (if admin) modify players belonging to Tenant B. - The ownership check (`_is_player_owner`) only verifies parent email, not tenant membership. This is a multi-tenancy authorization bypass. The endpoint should either accept a `tenant_id` parameter (consistent with `teams.py`) or derive tenant scope from the authenticated user's context. **Impact:** A player in one organization can read profile details (including parent name, email, phone) of players in a completely different organization. **2. Parent PII exposed to all authenticated users via GET** `ParentInfo` in the GET response includes `email` and `phone` for every player profile. Any authenticated user (player, coach, admin) from any tenant can read this. Combined with Blocker #1, this is a cross-tenant PII leak. Even within a single tenant, exposing parent phone/email to other parents (player role) may be undesirable. Consider: - Restricting `ParentInfo` fields based on the requesting user's role - Or at minimum, scoping GET to same-tenant users ### NITS **1. Implicit lazy load on `player.team.coach` (medium priority)** As noted in the domain review, `_get_player_or_404` does not eager-load the coach relationship through the team. Add `joinedload(Player.team).joinedload(Team.coach)` to match the pattern in `teams.py` and eliminate the implicit lazy query. This is technically a performance issue, not a correctness bug, since SQLAlchemy handles it transparently in sync code. In file `src/basketball_api/routes/players.py`, the `_get_player_or_404` function at approximately line 72 should use chained joinedload: ```python .options( joinedload(Player.team).joinedload(Team.coach), joinedload(Player.parent), ) ``` This would require importing `Team` (and possibly `Coach`) from `basketball_api.models`. **2. `ParentInfo.model_config` unused** `ParentInfo` sets `from_attributes = True` but is always constructed by explicit keyword arguments. `TeamInfo` does not set it. Pick one pattern and be consistent. **3. `photo_url` accepts arbitrary strings** No URL validation on the `photo_url` field in `PlayerProfileUpdate`. The existing upload route (`upload.py`) generates server-side paths, so this PATCH endpoint could be used to inject arbitrary strings (e.g., `javascript:` URIs) if the frontend renders the value in an `<img>` or `<a>` tag without sanitization. Consider adding a `Pydantic` `HttpUrl` type or at least a regex pattern validator. Low risk if the frontend already sanitizes, but defense-in-depth is preferred. **4. No test for empty PATCH body** Consider adding a test for `PATCH /api/players/{id}` with an empty JSON body `{}`. The current code should handle it correctly (no fields updated, returns current state), but it is an untested edge case. **5. Logger call after commit (minor)** In `update_player_profile`, the `logger.info` call at line 189 happens after the `_get_player_or_404` reload but logs `provided.keys()` from the original request body. This is fine functionally, but if the reload query fails (unlikely), the log entry would never fire and you would lose the audit trail. Consider logging before the reload. ### SOP COMPLIANCE - [ ] Branch named after issue number -- Branch is `feature/11a-player-profile-endpoints` (references phase 11a, not issue #89). Convention expects issue number in branch name. - [x] PR body has: Summary, Changes, Test Plan, Related -- All present and thorough. - [x] Related section references the plan slug -- References `plan-2026-03-08-tryout-prep` Phase 11a. - [x] Tests exist and pass -- 18 new tests, 266 total passing. Good coverage of happy path, auth, permissions, edge cases. - [x] No secrets committed -- Clean. - [x] No unnecessary file changes (scope creep) -- 3 files changed, all directly relevant. - [x] Commit messages are descriptive -- PR title follows conventional commits. ### PROCESS OBSERVATIONS **Change failure risk: MEDIUM.** The cross-tenant data access gap (Blocker #1) is a real authorization vulnerability in a multi-tenant system. Even if there is currently only one tenant, this establishes an insecure pattern that will bite when a second tenant is onboarded. Every other route in the codebase scopes by tenant -- this one should too. **Test coverage is strong** for the single-tenant case. The 18 tests cover GET fields, relationships, 404, 401, all three role permissions on PATCH, read-only field protection, partial updates, and case-insensitive email matching. However, no test verifies cross-tenant isolation because the tests only seed one tenant. **Deployment frequency impact: LOW.** This is additive (new endpoints, new file, no migrations), so rollback is trivial -- remove the router registration line. ### VERDICT: NOT APPROVED Two blockers must be addressed: 1. Add tenant scoping to `_get_player_or_404` (and add tenant_id parameter to endpoints, or derive from auth context) to prevent cross-tenant data access. 2. Evaluate ParentInfo PII exposure -- at minimum, tenant-scope the GET so users cannot read parent contact info across tenants. Ideally, restrict parent email/phone visibility by role.

forgejo_admin merged commit 5100cbdae0 into main 2026-03-16 01:10:00 +00:00

forgejo_admin deleted branch feature/11a-player-profile-endpoints 2026-03-16 01:10:00 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-16 01:10:00 +00:00

feat: add player profile endpoints (GET + PATCH) (#90)

forgejo_admin referenced this pull request 2026-03-16 01:23:36 +00:00

Lock down public tryout endpoints — require auth #91

forgejo_admin referenced this pull request 2026-03-16 01:46:56 +00:00

feat: require auth on tryout and upload endpoints #92

forgejo_admin referenced this pull request 2026-03-16 01:49:21 +00:00

feat: require auth on tryout and upload endpoints #92

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

forgejo_admin/basketball-api!90

No description provided.