diff --git a/PLAN.md b/PLAN.md index f1ebb7a..5e29e2b 100644 --- a/PLAN.md +++ b/PLAN.md @@ -26,6 +26,15 @@ - Auth and session responses remain `no-store` so cached data is limited to app-owned clip state. - TeamSnap read queries now use cached-first stale-while-revalidate behavior on the client. +## Completed V1 Hardening +- Media and gameday mutations now stay within the authenticated session's selected team and player scope. +- Upload and clip-creation failures now clean up orphaned files before bubbling errors back to the client. + +## Completed Asset Source Cleanup +- Editable artwork sources now live in `frontend/assets/design/`, while the exported web-ready images remain in `frontend/public/`. +- The splash artwork in `frontend/public/splash-art.svg` still serves as the editable vector source for the startup images in `frontend/public/`. +- The existing `frontend/public/icon.svg` already covers the app icon artwork, so no separate raster-to-vector conversion was needed there. + ## Storage Status - Backend media persists in the `backend-media` named Docker volume. diff --git a/backend/app/routes/games.py b/backend/app/routes/games.py index 8619a18..19b0b2b 100644 --- a/backend/app/routes/games.py +++ b/backend/app/routes/games.py @@ -20,6 +20,14 @@ from ..schemas import ( router = APIRouter(prefix="/games", tags=["games"]) +def resolve_session_player_id(session: UserSession, requested_player_id: str | None = None) -> str: + if not session.external_team_id or not session.external_player_id: + raise HTTPException(status_code=422, detail="Select a team and player before using gameday") + if requested_player_id and requested_player_id != session.external_player_id: + raise HTTPException(status_code=403, detail="This player does not match your selected session") + return session.external_player_id + + def assignment_to_response(assignment: GameAssignment) -> GameAssignmentResponse: normalized_url = f"/media/files/{assignment.clip.normalized_path}" if assignment.clip.normalized_path else None return GameAssignmentResponse( @@ -57,9 +65,7 @@ def list_pins( session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> list[GameAssignmentResponse]: - player_id = external_player_id or session.external_player_id - if not player_id or not session.external_team_id: - raise HTTPException(status_code=422, detail="Provide a player to list pins") + player_id = resolve_session_player_id(session, external_player_id) query = select(GameAssignment).join(GameAssignment.clip).where( GameAssignment.external_team_id == session.external_team_id, @@ -80,15 +86,15 @@ def list_assignments( response: Response, external_game_id: str, external_player_id: str | None = Query(default=None), - _: UserSession = Depends(require_session), + session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> list[GameAssignmentResponse]: + resolve_session_player_id(session, external_player_id) query = select(GameAssignment).join(GameAssignment.clip).where( + GameAssignment.external_team_id == session.external_team_id, GameAssignment.external_game_id == external_game_id, AudioClip.hidden.is_(False), ) - if external_player_id: - query = query.where(GameAssignment.external_player_id == external_player_id) assignments = db.scalars(query.order_by(AudioClip.sort_order.asc(), GameAssignment.updated_at.desc())).all() payload = [assignment_to_response(assignment) for assignment in assignments] etag, not_modified = prepare_conditional_response(request, payload) @@ -102,38 +108,42 @@ def list_assignments( def create_assignment( external_game_id: str, payload: GameAssignmentCreate, - _: UserSession = Depends(require_session), + session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> GameAssignmentResponse: + player_id = resolve_session_player_id(session, payload.external_player_id) + if payload.external_team_id != session.external_team_id: + raise HTTPException(status_code=403, detail="This team does not match your selected session") clip = db.get(AudioClip, payload.clip_id) if clip is None or clip.normalization_status != "ready": raise HTTPException(status_code=422, detail="Clip is not ready") if clip.hidden: raise HTTPException(status_code=404, detail="Clip not found") - if clip.asset.external_team_id != payload.external_team_id: + if clip.asset.external_team_id != session.external_team_id: raise HTTPException(status_code=422, detail="Clip does not belong to this team") - if clip.asset.owner_external_player_id != payload.external_player_id: + if clip.asset.owner_external_player_id != player_id: raise HTTPException(status_code=403, detail="You can only pin clips owned by that player") assignment = db.scalar( select(GameAssignment).where( GameAssignment.external_game_id == external_game_id, GameAssignment.clip_id == payload.clip_id, + GameAssignment.external_team_id == session.external_team_id, ) ) if assignment is None: assignment = GameAssignment( - external_team_id=payload.external_team_id, + external_team_id=session.external_team_id, external_game_id=external_game_id, - external_player_id=payload.external_player_id, + external_player_id=player_id, clip_id=payload.clip_id, batting_slot=payload.batting_slot, status=payload.status, ) db.add(assignment) else: - assignment.external_team_id = payload.external_team_id - assignment.external_player_id = payload.external_player_id + assignment.external_team_id = session.external_team_id + assignment.external_player_id = player_id assignment.clip_id = payload.clip_id assignment.batting_slot = payload.batting_slot assignment.status = payload.status @@ -147,14 +157,15 @@ def delete_assignment( external_game_id: str, assignment_id: int, external_player_id: str | None = Query(default=None), - _: UserSession = Depends(require_session), + session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> None: + player_id = resolve_session_player_id(session, external_player_id) assignment = db.get(GameAssignment, assignment_id) if assignment is None or assignment.external_game_id != external_game_id: raise HTTPException(status_code=404, detail="Pin not found") - if external_player_id is not None and assignment.external_player_id != external_player_id: - raise HTTPException(status_code=403, detail="Pin does not belong to that player") + if assignment.external_team_id != session.external_team_id or assignment.external_player_id != player_id: + raise HTTPException(status_code=403, detail="Pin does not belong to your selected session") db.delete(assignment) db.commit() @@ -165,13 +176,18 @@ def prepare_game( request: Request, response: Response, external_game_id: str, - _: UserSession = Depends(require_session), + session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> GamePrepResponse: + resolve_session_player_id(session, None) assignments = db.scalars( select(GameAssignment) .join(GameAssignment.clip) - .where(GameAssignment.external_game_id == external_game_id, AudioClip.hidden.is_(False)) + .where( + GameAssignment.external_team_id == session.external_team_id, + GameAssignment.external_game_id == external_game_id, + AudioClip.hidden.is_(False), + ) .order_by(AudioClip.sort_order.asc(), GameAssignment.updated_at.desc()) ).all() external_team_id = assignments[0].external_team_id if assignments else "" diff --git a/backend/app/routes/media.py b/backend/app/routes/media.py index 900fbe9..245bffb 100644 --- a/backend/app/routes/media.py +++ b/backend/app/routes/media.py @@ -29,6 +29,31 @@ router = APIRouter(prefix="/media", tags=["media"]) DEFAULT_CLIP_LENGTH_MS = 30_000 +def resolve_media_scope( + session: UserSession, + *, + requested_team_id: str | None = None, + requested_player_id: str | None = None, + require_player: bool = False, +) -> tuple[str, str | None]: + if session.is_admin: + team_id = requested_team_id or session.external_team_id + player_id = requested_player_id if requested_player_id is not None else session.external_player_id + if not team_id: + raise HTTPException(status_code=422, detail="Select a team before managing media") + if require_player and not player_id: + raise HTTPException(status_code=422, detail="Select a player before managing media") + return team_id, player_id + + if not session.external_team_id or not session.external_player_id: + raise HTTPException(status_code=422, detail="Select a team and player before managing media") + if requested_team_id and requested_team_id != session.external_team_id: + raise HTTPException(status_code=403, detail="This team does not match your selected session") + if requested_player_id and requested_player_id != session.external_player_id: + raise HTTPException(status_code=403, detail="This player does not match your selected session") + return session.external_team_id, session.external_player_id + + def clip_to_response(clip: AudioClip) -> AudioClipResponse: normalized_url = f"/media/files/{clip.normalized_path}" if clip.normalized_path else None waveform = storage.load_or_generate_waveform(clip.asset.storage_path) @@ -61,10 +86,15 @@ def prepare_conditional_response( return etag, is_matching_etag(request, etag) -def can_manage_asset(session: UserSession, asset: AudioAsset, owner_external_player_id: str | None = None) -> bool: - if session.is_admin or asset.uploaded_by_session_id == session.id: +def can_manage_asset(session: UserSession, asset: AudioAsset) -> bool: + if session.is_admin: return True - return owner_external_player_id is not None and asset.owner_external_player_id == owner_external_player_id + return ( + session.external_team_id is not None + and session.external_player_id is not None + and asset.external_team_id == session.external_team_id + and asset.owner_external_player_id == session.external_player_id + ) def next_clip_sort_order(db: Session, *, external_team_id: str, owner_external_player_id: str) -> int: @@ -91,42 +121,52 @@ def create_asset_with_default_clip( size_bytes: int, storage_path: str, ) -> AudioAssetResponse: - asset = AudioAsset( - external_team_id=external_team_id, - owner_external_player_id=owner_external_player_id, - uploaded_by_session_id=session.id, - title=title, - original_filename=original_filename, - mime_type=mime_type, - size_bytes=size_bytes, - storage_path=storage_path, - ) - db.add(asset) - db.flush() - - clip = AudioClip( - asset_id=asset.id, - label=asset.title, - start_ms=0, - end_ms=DEFAULT_CLIP_LENGTH_MS, - sort_order=next_clip_sort_order( - db, + normalized_path: str | None = None + try: + asset = AudioAsset( external_team_id=external_team_id, owner_external_player_id=owner_external_player_id, - ), - normalization_status="processing", - ) - db.add(clip) - db.flush() + uploaded_by_session_id=session.id, + title=title, + original_filename=original_filename, + mime_type=mime_type, + size_bytes=size_bytes, + storage_path=storage_path, + ) + db.add(asset) + db.flush() - normalized_name = f"clip-{clip.id}-{secrets.token_hex(6)}{Path(asset.storage_path).suffix or '.bin'}" - clip.normalized_path = storage.normalize_clip(asset.storage_path, normalized_name) - clip.normalization_status = "ready" - storage.generate_waveform(asset.storage_path) + clip = AudioClip( + asset_id=asset.id, + label=asset.title, + start_ms=0, + end_ms=DEFAULT_CLIP_LENGTH_MS, + sort_order=next_clip_sort_order( + db, + external_team_id=external_team_id, + owner_external_player_id=owner_external_player_id, + ), + normalization_status="processing", + ) + db.add(clip) + db.flush() - db.commit() - db.refresh(asset) - return AudioAssetResponse.model_validate(asset, from_attributes=True) + normalized_name = f"clip-{clip.id}-{secrets.token_hex(6)}{Path(asset.storage_path).suffix or '.bin'}" + normalized_path = str(Path("normalized") / normalized_name) + normalized_path = storage.normalize_clip(asset.storage_path, normalized_name) + clip.normalized_path = normalized_path + clip.normalization_status = "ready" + storage.generate_waveform(asset.storage_path) + + db.commit() + db.refresh(asset) + return AudioAssetResponse.model_validate(asset, from_attributes=True) + except Exception: + db.rollback() + if normalized_path: + storage.delete_relative_path(normalized_path) + storage.delete_relative_path(storage_path) + raise def download_media_to_storage(url: str) -> tuple[str, int, str, str]: @@ -180,6 +220,12 @@ async def upload_audio( session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> AudioAssetResponse: + external_team_id, owner_external_player_id = resolve_media_scope( + session, + requested_team_id=external_team_id, + requested_player_id=owner_external_player_id, + require_player=True, + ) extension = Path(file.filename or "upload.bin").suffix or ".bin" storage_name = f"{secrets.token_hex(16)}{extension}" relative_path, size = storage.save_upload(file, storage_name) @@ -202,6 +248,12 @@ def import_audio( session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> AudioAssetResponse: + external_team_id, owner_external_player_id = resolve_media_scope( + session, + requested_team_id=payload.external_team_id, + requested_player_id=payload.owner_external_player_id, + require_player=True, + ) relative_path, size_bytes, original_filename, source_title = download_media_to_storage(payload.url) title = payload.title.strip() if payload.title else "" if not title: @@ -226,9 +278,14 @@ def list_assets( response: Response, external_team_id: str, owner_external_player_id: str | None = None, - _: UserSession = Depends(require_session), + session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> list[AudioAssetResponse]: + external_team_id, owner_external_player_id = resolve_media_scope( + session, + requested_team_id=external_team_id, + requested_player_id=owner_external_player_id, + ) query = select(AudioAsset).where(AudioAsset.external_team_id == external_team_id) if owner_external_player_id: query = query.where(AudioAsset.owner_external_player_id == owner_external_player_id) @@ -251,7 +308,7 @@ def delete_asset( asset = db.get(AudioAsset, asset_id) if asset is None: raise HTTPException(status_code=404, detail="Asset not found") - if not can_manage_asset(session, asset, owner_external_player_id): + if not can_manage_asset(session, asset): raise HTTPException(status_code=403, detail="You can only delete your own uploads") clips = db.scalars(select(AudioClip).where(AudioClip.asset_id == asset.id)).all() @@ -279,7 +336,7 @@ def update_asset( asset = db.get(AudioAsset, asset_id) if asset is None: raise HTTPException(status_code=404, detail="Asset not found") - if not can_manage_asset(session, asset, owner_external_player_id): + if not can_manage_asset(session, asset): raise HTTPException(status_code=403, detail="You can only update your own uploads") title = payload.title.strip() @@ -295,15 +352,21 @@ def update_asset( @router.post("/clips", response_model=AudioClipResponse) def create_clip( payload: AudioClipCreate, - _: UserSession = Depends(require_session), + session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> AudioClipResponse: + external_team_id, owner_external_player_id = resolve_media_scope( + session, + requested_team_id=payload.external_team_id, + requested_player_id=payload.owner_external_player_id, + require_player=True, + ) asset = db.get(AudioAsset, payload.asset_id) if asset is None: raise HTTPException(status_code=404, detail="Asset not found") - if asset.external_team_id != payload.external_team_id: + if asset.external_team_id != external_team_id: raise HTTPException(status_code=422, detail="Clip does not belong to this team") - if asset.owner_external_player_id != payload.owner_external_player_id: + if asset.owner_external_player_id != owner_external_player_id: raise HTTPException(status_code=403, detail="You can only create clips for that player") if payload.end_ms <= payload.start_ms: raise HTTPException(status_code=422, detail="Clip end must be greater than start") @@ -315,8 +378,8 @@ def create_clip( end_ms=payload.end_ms, sort_order=next_clip_sort_order( db, - external_team_id=payload.external_team_id, - owner_external_player_id=payload.owner_external_player_id, + external_team_id=external_team_id, + owner_external_player_id=owner_external_player_id, ), normalization_status="processing", ) @@ -342,7 +405,7 @@ def update_clip( clip = db.get(AudioClip, clip_id) if clip is None: raise HTTPException(status_code=404, detail="Clip not found") - if not can_manage_asset(session, clip.asset, owner_external_player_id): + if not can_manage_asset(session, clip.asset): raise HTTPException(status_code=403, detail="You can only update clips from your own uploads") if payload.end_ms <= payload.start_ms: raise HTTPException(status_code=422, detail="Clip end must be greater than start") @@ -369,7 +432,7 @@ def delete_clip( clip = db.get(AudioClip, clip_id) if clip is None: raise HTTPException(status_code=404, detail="Clip not found") - if not can_manage_asset(session, clip.asset, owner_external_player_id): + if not can_manage_asset(session, clip.asset): raise HTTPException(status_code=403, detail="You can only delete clips from your own uploads") db.execute(delete(GameAssignment).where(GameAssignment.clip_id == clip.id)) @@ -386,9 +449,14 @@ def list_clips( external_team_id: str, owner_external_player_id: str | None = None, include_hidden: bool = False, - _: UserSession = Depends(require_session), + session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> list[AudioClipResponse]: + external_team_id, owner_external_player_id = resolve_media_scope( + session, + requested_team_id=external_team_id, + requested_player_id=owner_external_player_id, + ) query = ( select(AudioClip) .join(AudioClip.asset) @@ -414,15 +482,19 @@ def reorder_clips( session: UserSession = Depends(require_session), db: Session = Depends(get_db), ) -> None: - if not session.is_admin and session.external_team_id != payload.external_team_id: - raise HTTPException(status_code=403, detail="You can only reorder clips for your selected team") + external_team_id, owner_external_player_id = resolve_media_scope( + session, + requested_team_id=payload.external_team_id, + requested_player_id=payload.owner_external_player_id, + require_player=True, + ) clips = db.scalars( select(AudioClip) .join(AudioClip.asset) .where( - AudioAsset.external_team_id == payload.external_team_id, - AudioAsset.owner_external_player_id == payload.owner_external_player_id, + AudioAsset.external_team_id == external_team_id, + AudioAsset.owner_external_player_id == owner_external_player_id, ) ).all() clips_by_id = {clip.id: clip for clip in clips} diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index 0fc4f7a..12dfc6f 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -11,7 +11,7 @@ from fastapi.testclient import TestClient from app.config import settings from app.database import Base, SessionLocal, engine from app.main import app -from app.models import AudioAsset, AudioClip, UserSession +from app.models import AudioAsset, AudioClip, GameAssignment, UserSession from app.routes.teamsnap import rewrite_teamsnap_urls @@ -344,6 +344,63 @@ def test_player_can_pin_a_clip_to_multiple_games_independently() -> None: assert [item["clip_id"] for item in pins.json()] == [first_clip.id] +def test_player_cannot_pin_another_players_clip() -> None: + db = SessionLocal() + owner_session = UserSession( + session_token="owner-session", + provider="teamsnap", + external_team_id="team-1", + external_player_id="player-1", + ) + attacker_session = UserSession( + session_token="attacker-session", + provider="teamsnap", + external_team_id="team-1", + external_player_id="player-2", + ) + db.add_all([owner_session, attacker_session]) + db.flush() + + asset = AudioAsset( + external_team_id="team-1", + owner_external_player_id="player-1", + uploaded_by_session_id=owner_session.id, + title="Song", + original_filename="song.mp3", + mime_type="audio/mpeg", + size_bytes=123, + storage_path="uploads/song.mp3", + ) + db.add(asset) + db.flush() + clip = AudioClip( + asset_id=asset.id, + label="Intro", + start_ms=0, + end_ms=10000, + normalization_status="ready", + normalized_path="clips/intro.mp3", + ) + db.add(clip) + db.commit() + db.refresh(clip) + db.close() + + client.cookies.set(settings.session_cookie_name, "attacker-session") + response = client.post( + "/games/game-1/assignments", + json={ + "external_team_id": "team-1", + "external_player_id": "player-1", + "clip_id": clip.id, + "batting_slot": 1, + "status": "ready", + }, + ) + + assert response.status_code == 403 + + def test_upload_creates_default_clip_and_clip_ranges_can_be_updated() -> None: login = client.post("/auth/admin/login", json={"username": "admin", "password": "admin"}) assert login.status_code == 200 @@ -552,7 +609,12 @@ def test_hidden_clips_are_removed_from_gameday_views_but_remain_pinnable() -> No def test_clip_updates_can_use_player_scoped_authorization() -> None: uploader_session = UserSession(session_token="uploader-session", provider="teamsnap") - editor_session = UserSession(session_token="editor-session", provider="teamsnap") + editor_session = UserSession( + session_token="editor-session", + provider="teamsnap", + external_team_id="team-3", + external_player_id="player-3", + ) db = SessionLocal() db.add_all([uploader_session, editor_session]) @@ -597,6 +659,59 @@ def test_clip_updates_can_use_player_scoped_authorization() -> None: assert updated_clip["label"] == "Player clip" +def test_clip_updates_reject_cross_player_scopes() -> None: + uploader_session = UserSession( + session_token="uploader-session", + provider="teamsnap", + external_team_id="team-3", + external_player_id="player-3", + ) + editor_session = UserSession( + session_token="editor-session", + provider="teamsnap", + external_team_id="team-3", + external_player_id="player-4", + ) + + db = SessionLocal() + db.add_all([uploader_session, editor_session]) + db.flush() + + asset = AudioAsset( + external_team_id="team-3", + owner_external_player_id="player-3", + uploaded_by_session_id=uploader_session.id, + title="Player track", + original_filename="player-track.mp3", + mime_type="audio/mpeg", + size_bytes=123, + storage_path="uploads/player-track.mp3", + ) + db.add(asset) + db.flush() + clip = AudioClip( + asset_id=asset.id, + label="Player clip", + start_ms=0, + end_ms=30000, + normalization_status="ready", + normalized_path="clips/player-clip.mp3", + ) + db.add(clip) + db.commit() + db.refresh(clip) + db.close() + + client.cookies.set(settings.session_cookie_name, "editor-session") + update = client.patch( + f"/media/clips/{clip.id}", + params={"owner_external_player_id": "player-3"}, + json={"start_ms": 1500, "end_ms": 9000}, + ) + + assert update.status_code == 403 + + def test_create_clip_uses_team_and_player_scope() -> None: login = client.post("/auth/admin/login", json={"username": "admin", "password": "admin"}) assert login.status_code == 200 @@ -634,6 +749,57 @@ def test_create_clip_uses_team_and_player_scope() -> None: assert clip["end_ms"] == 6000 +def test_create_asset_with_default_clip_cleans_up_files_on_failure(monkeypatch: pytest.MonkeyPatch) -> None: + from app.routes import media as media_routes + + db = SessionLocal() + session = UserSession( + session_token="cleanup-session", + provider="teamsnap", + external_team_id="team-clean", + external_player_id="player-clean", + ) + db.add(session) + db.commit() + db.refresh(session) + + source_relative_path = "uploads/cleanup-source.wav" + source_path = settings.media_root / source_relative_path + source_path.parent.mkdir(parents=True, exist_ok=True) + source_path.write_bytes(make_test_wav_bytes()) + normalized_path = settings.media_root / "normalized" / "cleanup-copy.wav" + + def fake_normalize_clip(source_relative_path_arg: str, clip_name: str) -> str: + normalized_path.parent.mkdir(parents=True, exist_ok=True) + normalized_path.write_bytes((settings.media_root / source_relative_path_arg).read_bytes()) + return "normalized/cleanup-copy.wav" + + def fake_generate_waveform(source_relative_path_arg: str, bins: int = media_routes.WAVEFORM_PEAK_COUNT) -> dict[str, int | list[int]]: + raise RuntimeError("waveform failed") + + monkeypatch.setattr(media_routes.storage, "normalize_clip", fake_normalize_clip) + monkeypatch.setattr(media_routes.storage, "generate_waveform", fake_generate_waveform) + + with pytest.raises(RuntimeError): + media_routes.create_asset_with_default_clip( + db=db, + session=session, + external_team_id="team-clean", + owner_external_player_id="player-clean", + title="Cleanup track", + original_filename="cleanup-source.wav", + mime_type="audio/wav", + size_bytes=source_path.stat().st_size, + storage_path=source_relative_path, + ) + + assert not source_path.exists() + assert not normalized_path.exists() + assert db.query(AudioAsset).count() == 0 + assert db.query(AudioClip).count() == 0 + db.close() + + def test_asset_title_can_be_edited() -> None: login = client.post("/auth/admin/login", json={"username": "admin", "password": "admin"}) assert login.status_code == 200