diff --git a/.gitignore b/.gitignore index 1c3d9b9..94b8263 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,9 @@ # 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 b077df6..c1cf9c3 100644 --- a/backend/app/adapters/spotify/client.py +++ b/backend/app/adapters/spotify/client.py @@ -76,47 +76,114 @@ class SpotifyClient: params: dict[str, str | int] | None = None, json: dict[str, object] | None = None, ) -> httpx2.Response: - has_retried_authentication = False - has_retried_rate_limit = False + 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) - 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 _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}"}, + ) - 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) + 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: 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: @@ -143,3 +210,22 @@ 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 c1e82de..e45ad14 100644 --- a/backend/app/adapters/spotify/errors.py +++ b/backend/app/adapters/spotify/errors.py @@ -12,20 +12,30 @@ class SpotifyAuthenticationError(SpotifyError): class SpotifyRateLimitedError(SpotifyError): """Spotify rate limited a request that could not be retried.""" - def __init__(self, retry_after_seconds: float | None) -> None: + def __init__(self, retry_after_seconds: float | None, reason: str | None = 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) -> None: - """Record the response status without exposing response content.""" - super().__init__(f"Spotify request failed with status {status_code}") + 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) self.status_code = status_code + self.message = message diff --git a/backend/app/adapters/spotify/login.py b/backend/app/adapters/spotify/login.py new file mode 100644 index 0000000..3c6c48a --- /dev/null +++ b/backend/app/adapters/spotify/login.py @@ -0,0 +1,66 @@ +"""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 d641619..7b72413 100644 --- a/backend/app/api/routes.py +++ b/backend/app/api/routes.py @@ -1,21 +1,13 @@ """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.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.adapters.spotify.login import begin_login, complete_login +from app.adapters.spotify.session import PendingLogins, SessionStore from app.config import Settings SESSION_COOKIE_NAME = "discovery_session" @@ -28,15 +20,7 @@ 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) - 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), - ) + authorize_url = begin_login(application_settings, pending_logins) return RedirectResponse(authorize_url, status_code=307) @@ -51,37 +35,21 @@ async def callback( if error is not None or code is None or state is None: return _login_error_redirect() - pending_logins = cast(PendingLogins, request.app.state.pending_logins) - 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): + 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: 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 8d46f9d..523654b 100644 --- a/backend/tests/test_auth_routes.py +++ b/backend/tests/test_auth_routes.py @@ -70,5 +70,25 @@ 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 156a677..14b86a0 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 +from app.adapters.spotify.errors import SpotifyRateLimitedError, SpotifyRequestError from app.adapters.spotify.session import SpotifySession from app.config import Settings from app.domain.models import Track @@ -110,6 +110,85 @@ 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