diff --git a/.gitignore b/.gitignore index 94b8263..1c3d9b9 100644 --- a/.gitignore +++ b/.gitignore @@ -1,9 +1,6 @@ # secrets .env -# local working material (design mocks, scratch), never shipped -.local/ - # python __pycache__/ *.pyc diff --git a/backend/app/adapters/spotify/client.py b/backend/app/adapters/spotify/client.py index c1cf9c3..b077df6 100644 --- a/backend/app/adapters/spotify/client.py +++ b/backend/app/adapters/spotify/client.py @@ -76,114 +76,47 @@ class SpotifyClient: params: dict[str, str | int] | None = None, json: dict[str, object] | None = None, ) -> httpx2.Response: - access_token = await self._access_token() - response = await self._send( - method, - path, - access_token, - params=params, - json=json, - ) - response = await self._retry_once_if_unauthorized( - response, - method, - path, - access_token, - params=params, - json=json, - ) - response = await self._retry_once_if_rate_limited( - response, - method, - path, - params=params, - json=json, - ) - return self._raise_for_error(response) + has_retried_authentication = False + has_retried_rate_limit = False - async def _send( - self, - method: str, - path: str, - access_token: str, - *, - params: dict[str, str | int] | None, - json: dict[str, object] | None, - ) -> httpx2.Response: - return await self.http.request( - method, - f"{self.settings.spotify_api_base_url.rstrip('/')}{path}", - params=params, - json=json, - headers={"Authorization": f"Bearer {access_token}"}, - ) + while True: + access_token = await self._access_token() + response = await self.http.request( + method, + f"{self.settings.spotify_api_base_url.rstrip('/')}{path}", + params=params, + json=json, + headers={"Authorization": f"Bearer {access_token}"}, + ) - async def _retry_once_if_unauthorized( - self, - response: httpx2.Response, - method: str, - path: str, - access_token: str, - *, - params: dict[str, str | int] | None, - json: dict[str, object] | None, - ) -> httpx2.Response: - if response.status_code != 401: + if response.status_code == 401: + if has_retried_authentication: + raise SpotifyAuthenticationError("Spotify rejected refreshed authentication") + await self._refresh_if_current(access_token) + has_retried_authentication = True + continue + + if response.status_code == 429: + retry_after_seconds = _parse_retry_after(response) + if ( + method == "GET" + and not has_retried_rate_limit + and retry_after_seconds is not None + and retry_after_seconds <= self.settings.spotify_retry_after_cap_seconds + ): + await asyncio.sleep(retry_after_seconds) + has_retried_rate_limit = True + continue + raise SpotifyRateLimitedError(retry_after_seconds) + + if response.status_code >= 500: + raise SpotifyUnavailableError( + f"Spotify is unavailable with status {response.status_code}" + ) + if response.status_code >= 400: + raise SpotifyRequestError(response.status_code) return response - await self._refresh_if_current(access_token) - return await self._send( - method, - path, - self.session.tokens.access_token, - params=params, - json=json, - ) - - async def _retry_once_if_rate_limited( - self, - response: httpx2.Response, - method: str, - path: str, - *, - params: dict[str, str | int] | None, - json: dict[str, object] | None, - ) -> httpx2.Response: - if response.status_code != 429: - return response - - retry_after_seconds = _parse_retry_after(response) - _, reason = _parse_error_details(response) - if ( - method != "GET" - or reason == "QUOTA_EXCEEDED" - or retry_after_seconds is None - or retry_after_seconds > self.settings.spotify_retry_after_cap_seconds - ): - return response - - await asyncio.sleep(retry_after_seconds) - return await self._send( - method, - path, - self.session.tokens.access_token, - params=params, - json=json, - ) - - def _raise_for_error(self, response: httpx2.Response) -> httpx2.Response: - if response.status_code < 400: - return response - - message, reason = _parse_error_details(response) - if response.status_code == 401: - raise SpotifyAuthenticationError("Spotify rejected refreshed authentication") - if response.status_code == 429: - raise SpotifyRateLimitedError(_parse_retry_after(response), reason) - if response.status_code >= 500: - raise SpotifyUnavailableError(response.status_code, message) - raise SpotifyRequestError(response.status_code, message) - async def _access_token(self) -> str: access_token = self.session.tokens.access_token if self.session.tokens.is_expired: @@ -210,22 +143,3 @@ def _parse_retry_after(response: httpx2.Response) -> float | None: except ValueError: return None return retry_after_seconds if retry_after_seconds >= 0 else None - - -def _parse_error_details(response: httpx2.Response) -> tuple[str | None, str | None]: - try: - payload: object = response.json() - except ValueError: - return None, None - if not isinstance(payload, dict): - return None, None - - error = payload.get("error") - if not isinstance(error, dict): - return None, None - message = error.get("message") - reason = error.get("reason") - return ( - message if isinstance(message, str) else None, - reason if isinstance(reason, str) else None, - ) diff --git a/backend/app/adapters/spotify/errors.py b/backend/app/adapters/spotify/errors.py index e45ad14..c1e82de 100644 --- a/backend/app/adapters/spotify/errors.py +++ b/backend/app/adapters/spotify/errors.py @@ -12,30 +12,20 @@ class SpotifyAuthenticationError(SpotifyError): class SpotifyRateLimitedError(SpotifyError): """Spotify rate limited a request that could not be retried.""" - def __init__(self, retry_after_seconds: float | None, reason: str | None = None) -> None: + def __init__(self, retry_after_seconds: float | None) -> None: """Record Spotify's requested delay without exposing request data.""" super().__init__("Spotify rate limit exceeded") self.retry_after_seconds = retry_after_seconds - self.reason = reason class SpotifyUnavailableError(SpotifyError): """Spotify returned a server-side failure.""" - def __init__(self, status_code: int, message: str | None = None) -> None: - """Record safe details from a Spotify server failure.""" - generic_message = f"Spotify is unavailable with status {status_code}" - super().__init__(message if message is not None else generic_message) - self.status_code = status_code - self.message = message - class SpotifyRequestError(SpotifyError): """Spotify rejected a non-authenticated API request.""" - def __init__(self, status_code: int, message: str | None = None) -> None: - """Record safe details from a rejected Spotify request.""" - generic_message = f"Spotify request failed with status {status_code}" - super().__init__(message if message is not None else generic_message) + def __init__(self, status_code: int) -> None: + """Record the response status without exposing response content.""" + super().__init__(f"Spotify request failed with status {status_code}") self.status_code = status_code - self.message = message diff --git a/backend/app/adapters/spotify/login.py b/backend/app/adapters/spotify/login.py deleted file mode 100644 index 3c6c48a..0000000 --- a/backend/app/adapters/spotify/login.py +++ /dev/null @@ -1,66 +0,0 @@ -"""Spotify login workflow independent of HTTP routing.""" - -import secrets - -import httpx2 - -from app.adapters.spotify.auth import ( - build_authorize_url, - derive_code_challenge, - exchange_authorization_code, - generate_code_verifier, -) -from app.adapters.spotify.client import SpotifyClient -from app.adapters.spotify.errors import SpotifyError -from app.adapters.spotify.session import PendingLogins, SessionStore, SpotifySession -from app.config import Settings - - -def begin_login(settings: Settings, pending_logins: PendingLogins) -> str: - """Store a pending PKCE login and return its Spotify authorization URL.""" - state = secrets.token_urlsafe(32) - code_verifier = generate_code_verifier() - pending_logins.add(state, code_verifier) - return build_authorize_url( - settings.spotify_client_id, - settings.spotify_redirect_uri, - state, - derive_code_challenge(code_verifier), - ) - - -async def complete_login( - http: httpx2.AsyncClient, - settings: Settings, - pending_logins: PendingLogins, - session_store: SessionStore, - code: str, - state: str, -) -> str | None: - """Complete a pending Spotify login and return its application session ID.""" - code_verifier = pending_logins.pop(state) - if code_verifier is None: - return None - - try: - tokens = await exchange_authorization_code( - http, - client_id=settings.spotify_client_id, - redirect_uri=settings.spotify_redirect_uri, - code=code, - code_verifier=code_verifier, - ) - bootstrap_session = SpotifySession(tokens=tokens, account_id="", display_name="") - current_user = await SpotifyClient( - http, - bootstrap_session, - settings, - ).fetch_current_user() - session = SpotifySession( - tokens=bootstrap_session.tokens, - account_id=current_user.account_id, - display_name=current_user.display_name, - ) - return session_store.create(session) - except (SpotifyError, ValueError): - return None diff --git a/backend/app/api/routes.py b/backend/app/api/routes.py index 7b72413..d641619 100644 --- a/backend/app/api/routes.py +++ b/backend/app/api/routes.py @@ -1,13 +1,21 @@ """HTTP routes for Spotify login and session management.""" +import secrets from typing import cast import httpx2 from fastapi import APIRouter, Request from fastapi.responses import JSONResponse, RedirectResponse, Response -from app.adapters.spotify.login import begin_login, complete_login -from app.adapters.spotify.session import PendingLogins, SessionStore +from app.adapters.spotify.auth import ( + build_authorize_url, + derive_code_challenge, + exchange_authorization_code, + generate_code_verifier, +) +from app.adapters.spotify.client import SpotifyClient +from app.adapters.spotify.errors import SpotifyError +from app.adapters.spotify.session import PendingLogins, SessionStore, SpotifySession from app.config import Settings SESSION_COOKIE_NAME = "discovery_session" @@ -20,7 +28,15 @@ def login(request: Request) -> RedirectResponse: """Start Spotify Authorization Code with PKCE login.""" application_settings = cast(Settings, request.app.state.settings) pending_logins = cast(PendingLogins, request.app.state.pending_logins) - authorize_url = begin_login(application_settings, pending_logins) + state = secrets.token_urlsafe(32) + code_verifier = generate_code_verifier() + pending_logins.add(state, code_verifier) + authorize_url = build_authorize_url( + application_settings.spotify_client_id, + application_settings.spotify_redirect_uri, + state, + derive_code_challenge(code_verifier), + ) return RedirectResponse(authorize_url, status_code=307) @@ -35,21 +51,37 @@ async def callback( if error is not None or code is None or state is None: return _login_error_redirect() - application_settings = cast(Settings, request.app.state.settings) - http = cast(httpx2.AsyncClient, request.app.state.http) pending_logins = cast(PendingLogins, request.app.state.pending_logins) - session_store = cast(SessionStore, request.app.state.session_store) - session_id = await complete_login( - http, - application_settings, - pending_logins, - session_store, - code, - state, - ) - if session_id is None: + code_verifier = pending_logins.pop(state) + if code_verifier is None: return _login_error_redirect() + application_settings = cast(Settings, request.app.state.settings) + http = cast(httpx2.AsyncClient, request.app.state.http) + try: + tokens = await exchange_authorization_code( + http, + client_id=application_settings.spotify_client_id, + redirect_uri=application_settings.spotify_redirect_uri, + code=code, + code_verifier=code_verifier, + ) + bootstrap_session = SpotifySession(tokens=tokens, account_id="", display_name="") + current_user = await SpotifyClient( + http, + bootstrap_session, + application_settings, + ).fetch_current_user() + except (SpotifyError, ValueError): + return _login_error_redirect() + + session = SpotifySession( + tokens=bootstrap_session.tokens, + account_id=current_user.account_id, + display_name=current_user.display_name, + ) + session_store = cast(SessionStore, request.app.state.session_store) + session_id = session_store.create(session) response = RedirectResponse("/", status_code=307) response.set_cookie( SESSION_COOKIE_NAME, diff --git a/backend/tests/test_auth_routes.py b/backend/tests/test_auth_routes.py index 523654b..8d46f9d 100644 --- a/backend/tests/test_auth_routes.py +++ b/backend/tests/test_auth_routes.py @@ -70,25 +70,5 @@ def test_unknown_callback_state_redirects_to_login_error() -> None: assert response.headers["location"] == "/?login=error" -def test_spotify_failure_during_callback_redirects_to_login_error() -> None: - async def spotify_handler(request: httpx2.Request) -> httpx2.Response: - return httpx2.Response(400) - - app = create_app( - application_settings=_live_settings(), - http_transport=httpx2.MockTransport(spotify_handler), - ) - with TestClient(app, follow_redirects=False) as client: - login_response = client.get("/api/auth/login") - authorize_query = parse_qs(urlparse(login_response.headers["location"]).query) - response = client.get( - "/callback", - params={"code": "code", "state": authorize_query["state"][0]}, - ) - - assert response.status_code == 307 - assert response.headers["location"] == "/?login=error" - - def _live_settings() -> Settings: return Settings(app_mode=AppMode.LIVE, spotify_client_id="client-id") diff --git a/backend/tests/test_spotify_client.py b/backend/tests/test_spotify_client.py index 14b86a0..156a677 100644 --- a/backend/tests/test_spotify_client.py +++ b/backend/tests/test_spotify_client.py @@ -9,7 +9,7 @@ import pytest from app.adapters.spotify.auth import TokenSet from app.adapters.spotify.client import SpotifyClient -from app.adapters.spotify.errors import SpotifyRateLimitedError, SpotifyRequestError +from app.adapters.spotify.errors import SpotifyRateLimitedError from app.adapters.spotify.session import SpotifySession from app.config import Settings from app.domain.models import Track @@ -110,85 +110,6 @@ def test_get_rate_limit_above_cap_raises_without_retry() -> None: asyncio.run(run()) -def test_quota_exhaustion_raises_without_retry_or_sleep( - monkeypatch: pytest.MonkeyPatch, -) -> None: - async def run() -> None: - api_calls = 0 - sleep_calls = 0 - - async def fake_sleep(delay: float) -> None: - nonlocal sleep_calls - sleep_calls += 1 - - async def handler(request: httpx2.Request) -> httpx2.Response: - nonlocal api_calls - api_calls += 1 - return httpx2.Response( - 429, - headers={"Retry-After": "0"}, - json={ - "error": { - "message": "Quota exhausted", - "reason": "QUOTA_EXCEEDED", - } - }, - ) - - monkeypatch.setattr("app.adapters.spotify.client.asyncio.sleep", fake_sleep) - with pytest.raises(SpotifyRateLimitedError) as error: - await _search_with_handler(handler) - - assert error.value.reason == "QUOTA_EXCEEDED" - assert api_calls == 1 - assert sleep_calls == 0 - - asyncio.run(run()) - - -def test_authentication_then_rate_limit_retries_each_policy_once() -> None: - async def run() -> None: - token_calls = 0 - api_calls = 0 - - async def handler(request: httpx2.Request) -> httpx2.Response: - nonlocal api_calls, token_calls - if request.url.host == "accounts.spotify.com": - token_calls += 1 - return _token_response() - api_calls += 1 - if api_calls == 1: - return httpx2.Response(401) - if api_calls == 2: - return httpx2.Response(429, headers={"Retry-After": "0"}) - return httpx2.Response(200, json=_search_payload()) - - tracks = await _search_with_handler(handler) - - assert len(tracks) == 1 - assert token_calls == 1 - assert api_calls == 3 - - asyncio.run(run()) - - -def test_request_error_includes_parsed_spotify_message() -> None: - async def run() -> None: - async def handler(request: httpx2.Request) -> httpx2.Response: - return httpx2.Response( - 400, - json={"error": {"message": "Invalid search request"}}, - ) - - with pytest.raises(SpotifyRequestError) as error: - await _search_with_handler(handler) - - assert error.value.message == "Invalid search request" - assert str(error.value) == "Invalid search request" - - asyncio.run(run()) - - def test_playlist_write_rate_limit_is_not_retried() -> None: async def run() -> None: api_calls = 0