diff --git a/src/mopidy_spotify/__init__.py b/src/mopidy_spotify/__init__.py index 31b9e4db..16d3850c 100644 --- a/src/mopidy_spotify/__init__.py +++ b/src/mopidy_spotify/__init__.py @@ -27,8 +27,8 @@ def get_config_schema(self) -> config.ConfigSchema: schema["username"] = config.Deprecated() # since 5.0 schema["password"] = config.Deprecated() # since 5.0 - schema["client_id"] = config.String() - schema["client_secret"] = config.Secret() + schema["client_id"] = config.String(optional=True) + schema["client_secret"] = config.Secret(optional=True) schema["bitrate"] = config.Integer(choices=(96, 160, 320)) schema["volume_normalization"] = config.Boolean() diff --git a/src/mopidy_spotify/backend.py b/src/mopidy_spotify/backend.py index cfb2e92b..54cfa638 100644 --- a/src/mopidy_spotify/backend.py +++ b/src/mopidy_spotify/backend.py @@ -7,6 +7,8 @@ from mopidy.types import UriScheme from mopidy_spotify import Extension, library, playlists, web +from mopidy_spotify.oauth import providers, store +from mopidy_spotify.oauth.source import SpotifyAccessTokenSource if TYPE_CHECKING: from mopidy.audio import AudioProxy @@ -35,8 +37,14 @@ def __init__(self, *, config: Config, audio: AudioProxy) -> None: def on_start(self) -> None: self._web_client = web.SpotifyOAuthClient( - client_id=self._config["spotify"]["client_id"], - client_secret=self._config["spotify"]["client_secret"], + token_source=SpotifyAccessTokenSource( + auth_store=store.Store(Extension.get_auth_state_path(self._config)), + pkce=providers.PkceProvider(), + bridge=providers.BridgeProvider( + client_id=self._config["spotify"]["client_id"], + client_secret=self._config["spotify"]["client_secret"], + ), + ), proxy_config=self._config["proxy"], ) self._web_client.login() @@ -50,8 +58,8 @@ class SpotifyPlaybackProvider(backend.PlaybackProvider): def __init__(self, *args: Any, **kwargs: Any) -> None: super().__init__(*args, **kwargs) - self._cache_location = Extension().get_cache_dir(self.backend._config) - self._credentials_dir = Extension().get_credentials_dir(self.backend._config) + self._cache_location = Extension.get_cache_dir(self.backend._config) + self._credentials_dir = Extension.get_credentials_dir(self.backend._config) self._config = cast("SpotifyConfig", self.backend._config["spotify"]) @override diff --git a/src/mopidy_spotify/oauth/providers.py b/src/mopidy_spotify/oauth/providers.py index 479551dc..085917e4 100644 --- a/src/mopidy_spotify/oauth/providers.py +++ b/src/mopidy_spotify/oauth/providers.py @@ -1,60 +1,178 @@ -"""Refresh policies for Spotify Web authorization. +"""Refresh providers for Spotify Web authorization. -Providers choose grants and propose transitions from resolved runtime state. -They neither send HTTP requests nor handle persisted manifests or token storage; -those responsibilities belong to the executor and authorization store. This -keeps PKCE rotation and legacy bridge fallback separate from HTTP handling. +A provider owns grant-specific eligibility, request construction, and response +processing so the coordinator can persist transitions without knowing grant +details. Providers perform neither HTTP nor persistence. An exchange failure +never selects a different provider. """ from __future__ import annotations from dataclasses import dataclass from http import HTTPStatus -from typing import Literal, Protocol, runtime_checkable +from typing import ( + TYPE_CHECKING, + Literal, + Protocol, + TypeGuard, + override, + runtime_checkable, +) import requests -from mopidy_spotify import web from mopidy_spotify.oauth import pkce, state +from mopidy_spotify.oauth.tokens import ( + OAuthErrorResponse, + OAuthTokenRefreshError, + OAuthTokenResponse, +) + +if TYPE_CHECKING: + from pydantic import SecretStr + +BRIDGE_REFRESH_URL = "https://auth.mopidy.com/spotify/token" +SPOTIFY_REFRESH_URL = "https://accounts.spotify.com/api/token" @runtime_checkable class RefreshProvider(Protocol): - """Grant-specific policy consulted by the access-token refresh executor. + """Grant policy selected once for the current authorization snapshot.""" - Providers see resolved runtime state; the executor and store own HTTP - exchange and persisted manifests. - """ + def supports(self, auth_state: state.State | None) -> bool: ... - def request_for( + def request(self, auth_state: state.State | None) -> requests.Request: ... + + def process( self, auth_state: state.State | None, - ) -> requests.Request | None: - """Defer to the next provider when this grant does not apply.""" + response: OAuthTokenResponse | OAuthErrorResponse, + status_code: int, + ) -> state.State: + """Propose state for persistence, or raise to preserve it for retry.""" ... - def state_after_success( + +class PkceProvider(RefreshProvider): + """Refresh locally authorized PKCE tokens, retaining or rotating the secret.""" + + @override + def supports( + self, auth_state: state.State | None + ) -> TypeGuard[state.PkceAuthorized]: + return isinstance(auth_state, state.PkceAuthorized) + + @override + def request(self, auth_state: state.State | None) -> requests.Request: + if not self.supports(auth_state): + msg = "missing PKCE authorization state" + raise OAuthTokenRefreshError(msg) + + return requests.Request( + "POST", + SPOTIFY_REFRESH_URL, + data={ + "client_id": pkce.CLIENT_ID, + "grant_type": "refresh_token", + "refresh_token": auth_state.refresh_token.get_secret_value(), + }, + ) + + @override + def process( self, - response: web.OAuthTokenResponse, auth_state: state.State | None, + response: OAuthTokenResponse | OAuthErrorResponse, + status_code: int, ) -> state.State: - """Propose the next state; persistence precedes access-token installation.""" - ... + if not self.supports(auth_state): + msg = "missing PKCE authorization state" + raise OAuthTokenRefreshError(msg) + + if isinstance(response, OAuthErrorResponse): + return _state_after_error(response, status_code, mode="pkce") + + if _has_secret(response.refresh_token): + return state.PkceAuthorized(refresh_token=response.refresh_token) + + return state.PkceAuthorized(refresh_token=auth_state.refresh_token) + + +@dataclass(frozen=True) +class BridgeProvider(RefreshProvider): + """Exchange configured bridge credentials without adopting refresh tokens.""" + + client_id: str | None + client_secret: str | None - def state_after_error( + @override + def supports(self, auth_state: state.State | None) -> bool: + # Each refresh attempts the bridge again after permanent rejection, + # even with unchanged credentials, to allow configuration-based recovery. + # The credential-fingerprint follow-up will block unchanged credentials. + # PKCE rejection stays blocked until reauthorization. + # TODO: Bind bridge rejection to a safely stored credential fingerprint + # so only changed credentials permit retry; include it in stale-result checks. + first_time = auth_state is None + authorization_reset = isinstance(auth_state, state.Cleared) + bridge_configured = isinstance(auth_state, state.BridgeConfigured) + rejected_credentials_may_have_changed = ( + isinstance(auth_state, state.PermanentError) and auth_state.mode == "bridge" + ) + + return self._credentials() is not None and ( + first_time + or authorization_reset + or bridge_configured + or rejected_credentials_may_have_changed + ) + + @override + def request(self, auth_state: state.State | None) -> requests.Request: + credentials = self._credentials() + if credentials is None or not self.supports(auth_state): + msg = "bridge authorization unavailable" + raise OAuthTokenRefreshError(msg) + + return requests.Request( + "POST", + BRIDGE_REFRESH_URL, + auth=credentials, + data={"grant_type": "client_credentials"}, + ) + + @override + def process( self, - response: web.OAuthErrorResponse, auth_state: state.State | None, - status_code: int | HTTPStatus | None = None, + response: OAuthTokenResponse | OAuthErrorResponse, + status_code: int, ) -> state.State: - """Propose permanent failure or raise to preserve state for retry.""" - ... + if not self.supports(auth_state): + msg = "bridge authorization unavailable" + raise OAuthTokenRefreshError(msg) + + if isinstance(response, OAuthErrorResponse): + return _state_after_error(response, status_code, mode="bridge") + + return state.BridgeConfigured() + + def _credentials(self) -> tuple[str, str] | None: + if not self.client_id or not self.client_secret: + return None + + return self.client_id, self.client_secret + + +def _has_secret(secret: SecretStr | None) -> TypeGuard[SecretStr]: + return secret is not None and bool(secret.get_secret_value()) def _is_permanent_error( - response: web.OAuthErrorResponse, - status_code: int | HTTPStatus | None, + response: OAuthErrorResponse, + status_code: int, ) -> bool: + # Keep raw integer codes: endpoints may return statuses outside HTTPStatus. if response.error in { "temporarily_unavailable", "server_error", @@ -82,99 +200,17 @@ def _is_permanent_error( def _state_after_error( - response: web.OAuthErrorResponse, - status_code: int | HTTPStatus | None, + response: OAuthErrorResponse, + status_code: int, *, mode: Literal["pkce", "bridge"], ) -> state.PermanentError: if not _is_permanent_error(response, status_code): detail = response.error_description or response.error - raise web.OAuthTokenRefreshError(detail) + raise OAuthTokenRefreshError(detail) + return state.PermanentError( mode=mode, error_code=response.error, error_description=response.error_description, ) - - -class PkceRefreshProvider: - def request_for( - self, - auth_state: state.State | None, - ) -> requests.Request | None: - if not isinstance(auth_state, state.PkceAuthorized): - return None - - return requests.Request( - "POST", - web.SPOTIFY_REFRESH_URL, - data={ - "client_id": pkce.CLIENT_ID, - "grant_type": "refresh_token", - "refresh_token": auth_state.refresh_token.get_secret_value(), - }, - ) - - def state_after_success( - self, - response: web.OAuthTokenResponse, - auth_state: state.State | None, - ) -> state.State: - if not isinstance(auth_state, state.PkceAuthorized): - msg = "missing PKCE authorization state" - raise web.OAuthTokenRefreshError(msg) - - refresh_token = response.refresh_token - if refresh_token is None or not refresh_token.get_secret_value(): - refresh_token = auth_state.refresh_token - return state.PkceAuthorized(refresh_token=refresh_token) - - def state_after_error( - self, - response: web.OAuthErrorResponse, - auth_state: state.State | None, - status_code: int | HTTPStatus | None = None, - ) -> state.State: - _ = auth_state - return _state_after_error(response, status_code, mode="pkce") - - -@dataclass(frozen=True) -class BridgeRefreshProvider: - client_id: str | None - client_secret: str | None - - def request_for( - self, - auth_state: state.State | None, - ) -> requests.Request | None: - if isinstance(auth_state, state.PkceAuthorized) or ( - isinstance(auth_state, state.PermanentError) and auth_state.mode == "pkce" - ): - return None - if not self.client_id or not self.client_secret: - return None - - return requests.Request( - "POST", - web.BRIDGE_REFRESH_URL, - auth=(self.client_id, self.client_secret), - data={"grant_type": "client_credentials"}, - ) - - def state_after_success( - self, - response: web.OAuthTokenResponse, - auth_state: state.State | None, - ) -> state.State: - _ = response, auth_state - return state.BridgeConfigured() - - def state_after_error( - self, - response: web.OAuthErrorResponse, - auth_state: state.State | None, - status_code: int | HTTPStatus | None = None, - ) -> state.State: - _ = auth_state - return _state_after_error(response, status_code, mode="bridge") diff --git a/src/mopidy_spotify/oauth/source.py b/src/mopidy_spotify/oauth/source.py new file mode 100644 index 00000000..523cd86d --- /dev/null +++ b/src/mopidy_spotify/oauth/source.py @@ -0,0 +1,99 @@ +"""Coordinate Web authorization before exposing an access token. + +Load a snapshot, select its provider, exchange its request, then conditionally +persist the processed result. Only after persistence succeeds +may the OAuth client install the returned access token. HTTP runs without the +store lock, so logout or reauthorization can invalidate an in-flight result. +""" + +from __future__ import annotations + +from typing import assert_never + +from mopidy_spotify.oauth import providers, state, store +from mopidy_spotify.oauth.tokens import ( + OAuthErrorResponse, + OAuthPermanentRefreshError, + OAuthTokenRefreshError, + OAuthTokenResponse, + TokenExchange, +) + + +class SpotifyAccessTokenSource: + def __init__( + self, + *, + pkce: providers.PkceProvider, + bridge: providers.BridgeProvider, + auth_store: store.Store | None = None, + ) -> None: + self._store = auth_store + self._providers: tuple[providers.RefreshProvider, ...] = (pkce, bridge) + + def _load(self) -> store.Snapshot | None: + if self._store is None: + return None + + try: + return self._store.load() + + except (store.InvalidManifestError, store.Error) as exc: + msg = f"{exc}. Run `mopidy spotify auth web` to replace it." + raise OAuthPermanentRefreshError(msg) from exc + + def _save(self, expected: store.Snapshot | None, next_state: state.State) -> None: + if self._store is None: + return + + try: + saved = self._store.compare_and_set(expected, next_state) + except (store.InvalidManifestError, store.Error) as exc: + msg = "could not persist Spotify authorization state" + raise OAuthTokenRefreshError(msg) from exc + + if not saved: + msg = "Spotify authorization changed during token refresh" + raise OAuthTokenRefreshError(msg) + + def _select_provider( + self, auth_state: state.State | None + ) -> providers.RefreshProvider | None: + for provider in self._providers: + if provider.supports(auth_state): + return provider + + return None + + def refresh(self, exchange: TokenExchange) -> OAuthTokenResponse: + snapshot = self._load() + auth_state = snapshot.state if snapshot is not None else None + provider = self._select_provider(auth_state) + if provider is None: + if isinstance(auth_state, state.PermanentError): + detail = auth_state.error_description or auth_state.error_code + raise OAuthPermanentRefreshError(detail) + + msg = "No refresh provider available." + raise OAuthTokenRefreshError(msg) + + request = provider.request(auth_state) + response, status_code = exchange(request) + next_state = provider.process(auth_state, response, status_code) + self._save(snapshot, next_state) + match response: + case OAuthTokenResponse() as success: + return success + + case OAuthErrorResponse() as error: + if next_state.mode == "pkce": + detail = ( + "Spotify refresh token is no longer valid. " + "Run `mopidy spotify auth web` to reauthorize." + ) + else: + detail = error.error_description or error.error + raise OAuthPermanentRefreshError(detail) + + case _: + assert_never(response) diff --git a/src/mopidy_spotify/oauth/tokens.py b/src/mopidy_spotify/oauth/tokens.py new file mode 100644 index 00000000..385332de --- /dev/null +++ b/src/mopidy_spotify/oauth/tokens.py @@ -0,0 +1,86 @@ +"""Contracts between HTTP token exchange and authorization policy. + +An exchange parses the endpoint response and returns it with the raw HTTP status. +A token source interprets that response and settles authorization persistence +before returning success to the OAuth client for in-memory token installation. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Protocol + +from pydantic import BaseModel, ConfigDict, SecretStr + +if TYPE_CHECKING: + import requests + + +class OAuthTokenRefreshError(Exception): + """This attempt cannot supply a token; retry policy belongs to the source. + + Network, transient endpoint, and persistence failures leave authorization + intact. The OAuth client reports the error without installing a token. + """ + + def __init__(self, reason: str) -> None: + super().__init__(f"OAuth token refresh failed: {reason}") + + +class OAuthPermanentRefreshError(OAuthTokenRefreshError): + """The endpoint rejected authorization, or persisted authorization is unusable. + + PKCE rejection blocks refresh until reauthorization. Bridge credentials live + in configuration, so bridge policy permits another attempt to allow recovery + after configuration changes, without detecting whether they changed. + """ + + +class OAuthTokenResponse(BaseModel): + """Endpoint-issued access token and optional refresh-token replacement. + + PKCE policy retains its previous refresh token when no replacement is given; + bridge policy ignores replacements. The OAuth client uses expiry and scope + after persistence succeeds: absent expiry means no known expiration, while + zero means immediate expiry. Secret wrappers redact both token values. + """ + + model_config = ConfigDict(extra="ignore", strict=True) + + access_token: SecretStr + token_type: str + expires_in: int | float | None = None + refresh_token: SecretStr | None = None + scope: str | None = None + + +class OAuthErrorResponse(BaseModel): + """Endpoint rejection interpreted alongside HTTP status by grant policy. + + Transient failures preserve authorization for retry. Permanent failures + propose persisted rejection. Provider eligibility determines whether that + rejection blocks subsequent attempts or permits configuration-based recovery. + """ + + model_config = ConfigDict(extra="ignore", strict=True) + + error: str + error_description: str | None = None + + +class TokenExchange(Protocol): + """Execute one token request, returning parsed data and an unnormalized status. + + Transport, malformed-response, and invalid-token failures raise + `OAuthTokenRefreshError`. Valid OAuth rejections remain values so grant + policy can distinguish transient failure from permanent rejection. + """ + + def __call__( + self, request: requests.Request + ) -> tuple[OAuthTokenResponse | OAuthErrorResponse, int]: ... + + +class AccessTokenSource(Protocol): + def refresh(self, exchange: TokenExchange) -> OAuthTokenResponse: + """Return a token only after required authorization state is persisted.""" + ... diff --git a/src/mopidy_spotify/web.py b/src/mopidy_spotify/web.py index f24a2a0f..8426ee9a 100644 --- a/src/mopidy_spotify/web.py +++ b/src/mopidy_spotify/web.py @@ -16,9 +16,16 @@ from typing import TYPE_CHECKING, Any, ClassVar import requests -from pydantic import BaseModel, ConfigDict, SecretStr, TypeAdapter, ValidationError +from pydantic import SecretStr, TypeAdapter, ValidationError from mopidy_spotify import utils +from mopidy_spotify.oauth import providers +from mopidy_spotify.oauth.tokens import ( + AccessTokenSource, + OAuthErrorResponse, + OAuthTokenRefreshError, + OAuthTokenResponse, +) if TYPE_CHECKING: from collections.abc import Iterator, Mapping @@ -28,49 +35,18 @@ logger = logging.getLogger(__name__) -BRIDGE_REFRESH_URL = "https://auth.mopidy.com/spotify/token" -SPOTIFY_REFRESH_URL = "https://accounts.spotify.com/api/token" +BRIDGE_REFRESH_URL = providers.BRIDGE_REFRESH_URL +SPOTIFY_REFRESH_URL = providers.SPOTIFY_REFRESH_URL def _trace(*args: Any, **kwargs: Any) -> None: logger.log(utils.TRACE, *args, **kwargs) -class OAuthTokenRefreshError(Exception): - def __init__(self, reason: str) -> None: - message = f"OAuth token refresh failed: {reason}" - super().__init__(message) - - class OAuthClientError(Exception): pass -class OAuthTokenResponse(BaseModel): - """Validated in-memory success, separate from persisted authorization. - - Tokens stay redacted in representations; the authorization store owns - whether and where a refresh token is persisted. - """ - - model_config = ConfigDict(extra="ignore", strict=True) - - access_token: SecretStr - token_type: str - expires_in: int | float | None = None - refresh_token: SecretStr | None = None - scope: str | None = None - - -class OAuthErrorResponse(BaseModel): - """Validated failure whose code informs provider transition policy.""" - - model_config = ConfigDict(extra="ignore", strict=True) - - error: str - error_description: str | None = None - - OAUTH_REFRESH_RESPONSE_ADAPTER = TypeAdapter(OAuthTokenResponse | OAuthErrorResponse) @@ -105,31 +81,29 @@ def _parse_token_refresh_response( class OAuthClient: + """Shared HTTP exchange and in-memory access-token management. + + The token source settles authorization state before the client installs the + returned access token under the refresh mutex. + """ + def __init__( # noqa: PLR0913 self, *, base_url: str, - refresh_url: str, - client_id: str | None = None, - client_secret: str | None = None, + token_source: AccessTokenSource, proxy_config: ProxyConfig | None = None, expiry_margin: int = 60, timeout: int = 10, retries: int = 3, retry_statuses: tuple[int, ...] = (500, 502, 503, 429), ) -> None: - if client_id and client_secret: - self._auth = (client_id, client_secret) - else: - self._auth = None - self._access_token = None + self._access_token: SecretStr | None = None + self._token_source = token_source self._base_url = base_url - self._refresh_url = refresh_url - self._margin = expiry_margin self._expires = 0 - self._authorization_failed = False self._timeout = timeout self._number_of_retries = retries @@ -151,7 +125,10 @@ def token(self) -> str | None: logger.error(e) # noqa: TRY400 return None else: - return self._access_token + if self._access_token is None: + return None + + return self._access_token.get_secret_value() def get( self, @@ -160,10 +137,6 @@ def get( *args: Any, **kwargs: Any, ) -> WebResponse: - if self._authorization_failed: - logger.debug("Blocking request as previous authorization failed.") - return WebResponse(None, None) - params = kwargs.pop("params", None) path = self._normalise_query_string(path, params) @@ -213,43 +186,62 @@ def _should_refresh_token(self) -> bool: if not self._refresh_mutex.locked(): msg = "Lock must be held before calling." raise OAuthTokenRefreshError(msg) - return not self._auth or time.time() > self._expires - self._margin - - def _refresh_token(self) -> None: - logger.debug(f"Fetching OAuth token from {self._refresh_url}") + return ( + "Authorization" not in self._headers and self._expires == 0 + ) or time.time() > self._expires - self._margin - if not self._refresh_mutex.locked(): - msg = "Lock must be held before calling." + def _exchange_token( + self, + request: requests.Request, + ) -> tuple[OAuthTokenResponse | OAuthErrorResponse, int]: + logger.debug(f"Fetching OAuth token from {request.url}") + if request.method is None or not isinstance(request.url, str): + msg = "invalid token request" raise OAuthTokenRefreshError(msg) - - data = {"grant_type": "client_credentials"} result = self._request_with_retries( - "POST", self._refresh_url, auth=self._auth, data=data + request.method, + request.url, + auth=request.auth, + data=request.data, ) if result is None: msg = "Unknown error." raise OAuthTokenRefreshError(msg) - if result.get("error"): - msg = f"{result['error']} {result.get('error_description', '')}" - raise OAuthTokenRefreshError(msg) - if not result.get("access_token"): - msg = "missing access_token" + + response = _parse_token_refresh_response(result) + if isinstance(response, OAuthErrorResponse): + return response, result.status_code + + if not result.status_ok: + msg = f"token endpoint returned HTTP {result.status_code}" raise OAuthTokenRefreshError(msg) - if result.get("token_type") != "Bearer": - msg = f"wrong token_type: {result.get('token_type')}" + + if response.token_type != "Bearer": # noqa: S105 + msg = f"wrong token_type: {response.token_type}" raise OAuthTokenRefreshError(msg) - self._access_token = result["access_token"] - self._headers["Authorization"] = f"Bearer {self._access_token}" - self._expires = time.time() + result.get("expires_in", float("Inf")) + return response, result.status_code - if result.get("expires_in"): - logger.debug( - f"Token expires in {result['expires_in']} seconds.", - ) - if result.get("scope"): - logger.debug(f"Token scopes: {result['scope']}") + def _install_access_token(self, response: OAuthTokenResponse) -> None: + self._access_token = response.access_token + self._headers["Authorization"] = ( + f"Bearer {response.access_token.get_secret_value()}" + ) + lifetime = float("Inf") if response.expires_in is None else response.expires_in + self._expires = time.time() + lifetime + + if response.expires_in is not None: + logger.debug(f"Token expires in {response.expires_in} seconds.") + if response.scope: + logger.debug(f"Token scopes: {response.scope}") + + def _refresh_token(self) -> None: + if not self._refresh_mutex.locked(): + msg = "Lock must be held before calling." + raise OAuthTokenRefreshError(msg) + + self._install_access_token(self._token_source.refresh(self._exchange_token)) def _request_with_retries( self, @@ -310,7 +302,6 @@ def _request_with_retries( ) if status_code == HTTPStatus.UNAUTHORIZED: - self._authorization_failed = True logger.error( "Authorization failed, not attempting Spotify API " "request. Please get new credentials from " @@ -392,7 +383,7 @@ def __init__( self.url = url self._expires = expires self._etag = etag - self._status_code = status_code + self.status_code = status_code super().__init__(data or {}) _trace(f"New WebResponse {self}") @@ -424,7 +415,7 @@ def from_batch( item_json, expires=batch_response._expires, etag=None, - status_code=batch_response._status_code, + status_code=batch_response.status_code, ) @staticmethod @@ -484,11 +475,11 @@ def still_valid(self, *, expiry_strategy: ExpiryStrategy | None = None) -> bool: @property def status_unchanged(self) -> bool: - return self._from_cache or self._status_code == HTTPStatus.NOT_MODIFIED + return self._from_cache or self.status_code == HTTPStatus.NOT_MODIFIED @property def status_ok(self) -> bool: - return self._status_code >= 200 and self._status_code < 400 # noqa: PLR2004 + return self.status_code >= 200 and self.status_code < 400 # noqa: PLR2004 @property def etag_headers(self) -> dict[str, str]: @@ -506,14 +497,14 @@ def updated(self, response: WebResponse) -> bool: if not response.status_ok: logger.debug(f"ETag mismatch (bad response) for {self} {response}") return False - if response._status_code != HTTPStatus.NOT_MODIFIED: + if response.status_code != HTTPStatus.NOT_MODIFIED: _trace(f"ETag mismatch for {self} {response}") return False _trace(f"ETag match for {self} {response}") self._expires = response._expires self._etag = response._etag - self._status_code = response._status_code + self.status_code = response.status_code return True def __str__(self) -> str: @@ -611,15 +602,12 @@ class SpotifyOAuthClient(OAuthClient): def __init__( self, *, - client_id: str, - client_secret: str, + token_source: AccessTokenSource, proxy_config: ProxyConfig | None = None, ) -> None: super().__init__( base_url="https://api.spotify.com/v1", - refresh_url="https://auth.mopidy.com/spotify/token", - client_id=client_id, - client_secret=client_secret, + token_source=token_source, proxy_config=proxy_config, ) self.user_id: str | None = None diff --git a/tests/oauth/test_providers.py b/tests/oauth/test_providers.py index 05cc1799..7f4639fe 100644 --- a/tests/oauth/test_providers.py +++ b/tests/oauth/test_providers.py @@ -2,21 +2,30 @@ import pytest -from mopidy_spotify import web from mopidy_spotify.oauth import pkce, providers, state +from mopidy_spotify.oauth.tokens import ( + OAuthErrorResponse, + OAuthTokenRefreshError, + OAuthTokenResponse, +) -def test_pkce_refresh_provider_builds_refresh_token_request(): - provider = providers.PkceRefreshProvider() +@pytest.fixture +def authorized() -> state.PkceAuthorized: + return state.PkceAuthorized(refresh_token="refresh-token-1") # noqa: S106 + + +@pytest.fixture +def bridge() -> providers.BridgeProvider: + return providers.BridgeProvider("client-id", "client-secret") - request = provider.request_for( - state.PkceAuthorized( - refresh_token="refresh-token-1" # noqa: S106 - ) - ) - assert request is not None - assert request.url == web.SPOTIFY_REFRESH_URL +def test_pkce_provider_builds_refresh_token_request(authorized: state.PkceAuthorized): + provider = providers.PkceProvider() + assert provider.supports(authorized) + request = provider.request(authorized) + assert request.method == "POST" + assert request.url == providers.SPOTIFY_REFRESH_URL assert request.auth is None assert request.data == { "client_id": pkce.CLIENT_ID, @@ -25,229 +34,161 @@ def test_pkce_refresh_provider_builds_refresh_token_request(): } -def test_pkce_refresh_provider_declines_non_authorized_state(): - assert providers.PkceRefreshProvider().request_for(None) is None - - -def test_pkce_refresh_provider_keeps_existing_refresh_token_when_not_rotated(): - provider = providers.PkceRefreshProvider() - auth_state = state.PkceAuthorized( - refresh_token="refresh-token-1" # noqa: S106 - ) - - next_state = provider.state_after_success( - web.OAuthTokenResponse( - access_token="access-token-1", # noqa: S106 - token_type="Bearer", # noqa: S106 - ), - auth_state, - ) - - assert next_state == auth_state - - -def test_pkce_refresh_provider_requires_authorized_state_for_success(): - with pytest.raises(web.OAuthTokenRefreshError, match="missing PKCE"): - providers.PkceRefreshProvider().state_after_success( - web.OAuthTokenResponse( - access_token="access-token", # noqa: S106 - token_type="Bearer", # noqa: S106 - ), - None, - ) - +def test_pkce_provider_requires_authorized_state(): + provider = providers.PkceProvider() + assert not provider.supports(None) + with pytest.raises(OAuthTokenRefreshError, match="missing PKCE"): + provider.request(None) + with pytest.raises(OAuthTokenRefreshError, match="missing PKCE"): + provider.process(None, OAuthErrorResponse(error="invalid_grant"), 400) -def test_pkce_refresh_provider_persists_rotated_refresh_token(): - provider = providers.PkceRefreshProvider() - - next_state = provider.state_after_success( - web.OAuthTokenResponse( - access_token="access-token-1", # noqa: S106 - token_type="Bearer", # noqa: S106 - refresh_token="refresh-token-2", # noqa: S106 - ), - state.PkceAuthorized( - refresh_token="refresh-token-1" # noqa: S106 - ), - ) - assert next_state == state.PkceAuthorized( - refresh_token="refresh-token-2" # noqa: S106 +@pytest.mark.parametrize("replacement", [None, "", "refresh-token-2"]) +def test_pkce_provider_retains_or_rotates_refresh_token( + authorized: state.PkceAuthorized, replacement: str | None +): + response = OAuthTokenResponse( + access_token="access-token-1", # noqa: S106 + token_type="Bearer", # noqa: S106 + refresh_token=replacement, ) + expected = replacement or "refresh-token-1" + assert providers.PkceProvider().process( + authorized, response, HTTPStatus.OK + ) == state.PkceAuthorized(refresh_token=expected) -def test_pkce_refresh_provider_marks_invalid_grant_as_permanent_error(): - provider = providers.PkceRefreshProvider() - - next_state = provider.state_after_error( - web.OAuthErrorResponse( - error="invalid_grant", - error_description="Refresh token expired", - ), - state.PkceAuthorized( - refresh_token="refresh-token-1" # noqa: S106 - ), - HTTPStatus.BAD_REQUEST, +def test_pkce_provider_marks_invalid_grant_as_permanent_error( + authorized: state.PkceAuthorized, +): + response = OAuthErrorResponse( + error="invalid_grant", error_description="Refresh token expired" ) - - assert next_state == state.PermanentError( + assert providers.PkceProvider().process( + authorized, response, HTTPStatus.BAD_REQUEST + ) == state.PermanentError( mode="pkce", error_code="invalid_grant", error_description="Refresh token expired", ) -def test_pkce_refresh_provider_raises_transient_error_transient(): - provider = providers.PkceRefreshProvider() - - with pytest.raises(web.OAuthTokenRefreshError, match="errorTransient"): - provider.state_after_error( - web.OAuthErrorResponse(error="errorTransient"), - state.PkceAuthorized( - refresh_token="refresh-token-1" # noqa: S106 - ), - HTTPStatus.BAD_REQUEST, - ) - - -def test_pkce_refresh_provider_treats_temporary_unavailability_as_transient(): - with pytest.raises(web.OAuthTokenRefreshError, match="temporarily_unavailable"): - providers.PkceRefreshProvider().state_after_error( - web.OAuthErrorResponse(error="temporarily_unavailable"), - state.PkceAuthorized(refresh_token="refresh-token"), # noqa: S106 - HTTPStatus.BAD_REQUEST, - ) - - -def test_pkce_refresh_provider_treats_server_error_status_as_transient(): - provider = providers.PkceRefreshProvider() - - with pytest.raises(web.OAuthTokenRefreshError, match="invalid_grant"): - provider.state_after_error( - web.OAuthErrorResponse(error="invalid_grant"), - state.PkceAuthorized( - refresh_token="refresh-token-1" # noqa: S106 - ), - HTTPStatus.INTERNAL_SERVER_ERROR, +@pytest.mark.parametrize( + ("error", "status"), + [ + ("errorTransient", HTTPStatus.BAD_REQUEST), + ("temporarily_unavailable", HTTPStatus.BAD_REQUEST), + ("invalid_grant", HTTPStatus.INTERNAL_SERVER_ERROR), + ], +) +def test_pkce_provider_preserves_authorization_on_transient_error( + authorized: state.PkceAuthorized, error: str, status: int +): + with pytest.raises(OAuthTokenRefreshError, match=error): + providers.PkceProvider().process( + authorized, OAuthErrorResponse(error=error), status ) -def test_bridge_refresh_provider_builds_client_credentials_request(): - provider = providers.BridgeRefreshProvider( - client_id="client-id", - client_secret="client-secret", # noqa: S106 - ) - - request = provider.request_for(None) - - assert request is not None - assert request.url == web.BRIDGE_REFRESH_URL +def test_bridge_provider_builds_client_credentials_request( + bridge: providers.BridgeProvider, +): + assert bridge.supports(None) + request = bridge.request(None) + assert request.method == "POST" + assert request.url == providers.BRIDGE_REFRESH_URL assert request.auth == ("client-id", "client-secret") assert request.data == {"grant_type": "client_credentials"} -def test_bridge_refresh_provider_returns_none_without_credentials(): - provider = providers.BridgeRefreshProvider( - client_id=None, - client_secret=None, - ) - - assert provider.request_for(None) is None - - -def test_bridge_refresh_provider_ignores_unexpected_refresh_token(): - provider = providers.BridgeRefreshProvider( - client_id="client-id", - client_secret="client-secret", # noqa: S106 - ) - - next_state = provider.state_after_success( - web.OAuthTokenResponse( - access_token="access-token-1", # noqa: S106 - token_type="Bearer", # noqa: S106 - refresh_token="unexpected-refresh-token", # noqa: S106 - ), +@pytest.mark.parametrize( + ("client_id", "client_secret"), + [(None, None), ("client-id", None), (None, "client-secret")], +) +def test_bridge_provider_requires_complete_credentials( + client_id: str | None, client_secret: str | None +): + provider = providers.BridgeProvider(client_id, client_secret) + assert not provider.supports(None) + with pytest.raises( + OAuthTokenRefreshError, match="bridge authorization unavailable" + ): + provider.request(None) + + +@pytest.mark.parametrize( + "auth_state", + [ None, - ) - - assert next_state == state.BridgeConfigured() - - -def test_bridge_refresh_provider_defers_to_pkce(): - provider = providers.BridgeRefreshProvider( - client_id="client-id", - client_secret="client-secret", # noqa: S106 - ) - - assert ( - provider.request_for( - state.PkceAuthorized( - refresh_token="refresh-token-1" # noqa: S106 - ) - ) - is None - ) - - -def test_bridge_refresh_provider_does_not_fall_back_after_pkce_failure(): - provider = providers.BridgeRefreshProvider( - client_id="client-id", - client_secret="client-secret", # noqa: S106 - ) - - assert ( - provider.request_for( - state.PermanentError(mode="pkce", error_code="invalid_grant") - ) - is None - ) - - -def test_providers_implement_refresh_provider_protocol(): - assert isinstance( - providers.PkceRefreshProvider(), - providers.RefreshProvider, - ) - assert isinstance( - providers.BridgeRefreshProvider( - client_id="client-id", - client_secret="client-secret", # noqa: S106 - ), - providers.RefreshProvider, - ) - - -def test_bridge_refresh_provider_marks_invalid_client_as_permanent_error(): - provider = providers.BridgeRefreshProvider( - client_id="client-id", - client_secret="client-secret", # noqa: S106 - ) - - next_state = provider.state_after_error( - web.OAuthErrorResponse( - error="invalid_client", - error_description="Client not known.", - ), - None, - HTTPStatus.UNAUTHORIZED, - ) - - assert next_state == state.PermanentError( + state.Cleared(mode="pkce"), + state.Cleared(mode="bridge"), + state.BridgeConfigured(), + state.PermanentError(mode="bridge", error_code="invalid_client"), + ], +) +def test_bridge_provider_supports_fallback_states( + bridge: providers.BridgeProvider, auth_state: state.State | None +): + assert bridge.supports(auth_state) + + +@pytest.mark.parametrize( + "auth_state", + [ + state.PkceAuthorized(refresh_token="refresh-token-1"), # noqa: S106 + state.PermanentError(mode="pkce", error_code="invalid_grant"), + ], +) +def test_bridge_provider_rejects_pkce_authorization( + bridge: providers.BridgeProvider, auth_state: state.State +): + assert not bridge.supports(auth_state) + with pytest.raises( + OAuthTokenRefreshError, match="bridge authorization unavailable" + ): + bridge.request(auth_state) + with pytest.raises( + OAuthTokenRefreshError, match="bridge authorization unavailable" + ): + bridge.process(auth_state, OAuthErrorResponse(error="invalid_client"), 401) + + +def test_bridge_provider_ignores_unexpected_refresh_token( + bridge: providers.BridgeProvider, +): + response = OAuthTokenResponse( + access_token="access-token-1", # noqa: S106 + token_type="Bearer", # noqa: S106 + refresh_token="unexpected-refresh-token", # noqa: S106 + ) + assert bridge.process(None, response, HTTPStatus.OK) == state.BridgeConfigured() + + +def test_providers_implement_refresh_provider_protocol( + bridge: providers.BridgeProvider, +): + assert isinstance(providers.PkceProvider(), providers.RefreshProvider) + assert isinstance(bridge, providers.RefreshProvider) + + +def test_bridge_provider_marks_invalid_client_as_permanent_error( + bridge: providers.BridgeProvider, +): + response = OAuthErrorResponse( + error="invalid_client", error_description="Client not known." + ) + assert bridge.process( + None, response, HTTPStatus.UNAUTHORIZED + ) == state.PermanentError( mode="bridge", error_code="invalid_client", error_description="Client not known.", ) -def test_bridge_refresh_provider_treats_unknown_error_as_transient(): - provider = providers.BridgeRefreshProvider( - client_id="client-id", - client_secret="client-secret", # noqa: S106 - ) - - with pytest.raises(web.OAuthTokenRefreshError, match="unexpected_error"): - provider.state_after_error( - web.OAuthErrorResponse(error="unexpected_error"), - None, - HTTPStatus.BAD_REQUEST, - ) +@pytest.mark.parametrize("status", [HTTPStatus.BAD_REQUEST, 599]) +def test_bridge_provider_treats_unknown_error_as_transient( + bridge: providers.BridgeProvider, status: int +): + with pytest.raises(OAuthTokenRefreshError, match="unexpected_error"): + bridge.process(None, OAuthErrorResponse(error="unexpected_error"), status) diff --git a/tests/test_backend.py b/tests/test_backend.py index 48f790a4..9eafcdf2 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -2,11 +2,15 @@ from unittest import mock, skip import pytest +import requests from mopidy import backend as backend_api +from pydantic import SecretStr -from mopidy_spotify import backend, playlists +from mopidy_spotify import Extension, backend, playlists from mopidy_spotify.backend import SpotifyPlaybackProvider from mopidy_spotify.library import SpotifyLibraryProvider +from mopidy_spotify.oauth import pkce, state, store +from mopidy_spotify.oauth.tokens import OAuthTokenResponse from tests import ThreadJoiner @@ -69,9 +73,7 @@ def test_on_start_configures_proxy(web_mock: mock.MagicMock, config: dict[str, A assert True web_mock.SpotifyOAuthClient.assert_called_once_with( - client_id=mock.ANY, - client_secret=mock.ANY, - proxy_config=config["proxy"], + token_source=mock.ANY, proxy_config=config["proxy"] ) @@ -86,9 +88,59 @@ def test_on_start_configures_web_client( backend.on_start() web_mock.SpotifyOAuthClient.assert_called_once_with( - client_id="1234567", - client_secret="AbCdEfG", # noqa: S106 - proxy_config=mock.ANY, + token_source=mock.ANY, proxy_config=config["proxy"] + ) + source = web_mock.SpotifyOAuthClient.call_args.kwargs["token_source"] + + def exchange(request: requests.Request) -> tuple[OAuthTokenResponse, int]: + assert request.auth == ("1234567", "AbCdEfG") + return OAuthTokenResponse( + access_token="bridge-access-token", # noqa: S106 + token_type="Bearer", # noqa: S106 + ), 200 + + assert ( + source.refresh(exchange).access_token.get_secret_value() + == "bridge-access-token" + ) + snapshot = store.Store(Extension.get_auth_state_path(config)).load() + assert snapshot is not None + assert snapshot.state == state.BridgeConfigured() + + +def test_on_start_allows_pkce_without_bridge_credentials( + web_mock: mock.MagicMock, + config: dict[str, Any], +): + config["spotify"]["client_id"] = None + config["spotify"]["client_secret"] = None + store.Store(Extension.get_auth_state_path(config)).persist_pkce_authorization( + SecretStr("refresh-token") + ) + + backend = get_backend(config) + with ThreadJoiner(): + backend.on_start() + + web_mock.SpotifyOAuthClient.assert_called_once_with( + token_source=mock.ANY, proxy_config=config["proxy"] + ) + source = web_mock.SpotifyOAuthClient.call_args.kwargs["token_source"] + + def exchange(request: requests.Request) -> tuple[OAuthTokenResponse, int]: + assert request.auth is None + assert request.data == { + "client_id": pkce.CLIENT_ID, + "grant_type": "refresh_token", + "refresh_token": "refresh-token", + } + return OAuthTokenResponse( + access_token="pkce-access-token", # noqa: S106 + token_type="Bearer", # noqa: S106 + ), 200 + + assert ( + source.refresh(exchange).access_token.get_secret_value() == "pkce-access-token" ) diff --git a/tests/test_extension.py b/tests/test_extension.py index 770037b2..74b74969 100644 --- a/tests/test_extension.py +++ b/tests/test_extension.py @@ -34,6 +34,13 @@ def test_get_config_schema() -> None: assert "search_track_count" in schema +def test_bridge_credentials_are_optional() -> None: + schema = Extension().get_config_schema() + + assert schema["client_id"].deserialize("") is None + assert schema["client_secret"].deserialize("") is None + + def test_setup() -> None: registry = mock.Mock() diff --git a/tests/test_web.py b/tests/test_web.py index 2035583b..e2f7ed45 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1,5 +1,8 @@ +import base64 +import json import urllib from datetime import UTC, datetime +from pathlib import Path from typing import Any from unittest import mock @@ -7,22 +10,73 @@ import requests import responses from mopidy.types import Uri +from pydantic import SecretStr from responses import matchers import mopidy_spotify from mopidy_spotify import web +from mopidy_spotify._ext import keyring as keyring_ext +from mopidy_spotify.oauth import pkce, providers, state +from mopidy_spotify.oauth import store as auth_store +from mopidy_spotify.oauth.source import SpotifyAccessTokenSource +from mopidy_spotify.oauth.tokens import TokenExchange + + +def _spotify_client( + *, + client_id: str | None, + client_secret: str | None, + auth_state_path: Path | None = None, + proxy_config: Any = None, +) -> web.SpotifyOAuthClient: + return web.SpotifyOAuthClient( + token_source=SpotifyAccessTokenSource( + pkce=providers.PkceProvider(), + bridge=providers.BridgeProvider(client_id, client_secret), + auth_store=( + auth_store.Store(auth_state_path) + if auth_state_path is not None + else None + ), + ), + proxy_config=proxy_config, + ) @pytest.fixture def oauth_client(config: dict[str, Any]) -> web.OAuthClient: - return web.OAuthClient( - base_url="https://api.spotify.com/v1", - refresh_url="https://auth.mopidy.com/spotify/token", + return _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + proxy_config=None, + ) + + +@pytest.fixture +def refresh_token_oauth_client( + config: dict[str, Any], tmp_path: Path +) -> tuple[web.SpotifyOAuthClient, Path]: + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text( + json.dumps( + { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "inline", + "value": "refresh-token-1", + }, + } + ) + ) + client = _spotify_client( client_id=config["spotify"]["client_id"], client_secret=config["spotify"]["client_secret"], proxy_config=None, - expiry_margin=60, + auth_state_path=refresh_token_path, ) + return client, refresh_token_path @pytest.fixture @@ -93,6 +147,7 @@ def test_token_returns_refreshed_access_token( ) assert oauth_client.token() == "NgCXRK...MzYjw" + assert isinstance(oauth_client._access_token, SecretStr) @responses.activate @@ -108,7 +163,7 @@ def test_token_returns_none_when_refresh_fails( ) assert oauth_client.token() is None - assert "OAuth token refresh failed: invalid_client Client not known" in caplog.text + assert "OAuth token refresh failed: Client not known" in caplog.text def test_parse_token_refresh_response_preserves_zero_expiry(): @@ -245,6 +300,262 @@ def test_user_agent(oauth_client: web.OAuthClient): ) +@responses.activate +def test_generic_oauth_client_exchanges_token_from_injected_source(): + responses.add( + responses.POST, + "https://example.com/token", + json={"access_token": "other-provider-token", "token_type": "Bearer"}, + ) + + def refresh(exchange: TokenExchange) -> web.OAuthTokenResponse: + token, status = exchange(requests.Request("POST", "https://example.com/token")) + assert status == 200 + assert isinstance(token, web.OAuthTokenResponse) + return token + + source = mock.Mock() + source.refresh.side_effect = refresh + client = web.OAuthClient(base_url="https://example.com", token_source=source) + + assert client.token() == "other-provider-token" + assert client._headers["Authorization"] == "Bearer other-provider-token" + source.refresh.assert_called_once() + + +@responses.activate +@pytest.mark.parametrize( + "token_request", + [requests.Request(url="https://example.com/token"), requests.Request("POST")], + ids=["missing-method", "missing-url"], +) +def test_generic_oauth_client_rejects_incomplete_token_request( + token_request: requests.Request, caplog: pytest.LogCaptureFixture +): + def refresh(exchange: TokenExchange) -> web.OAuthTokenResponse: + token, _ = exchange(token_request) + assert isinstance(token, web.OAuthTokenResponse) + return token + + source = mock.Mock() + source.refresh.side_effect = refresh + client = web.OAuthClient(base_url="https://example.com", token_source=source) + + assert client.token() is None + assert not responses.calls + assert "OAuth token refresh failed: invalid token request" in caplog.text + + +@responses.activate +def test_spotify_oauth_client_requires_refresh_provider_without_credentials( + caplog: pytest.LogCaptureFixture, +): + client = _spotify_client( + client_id=None, + client_secret=None, + proxy_config=None, + ) + + assert client.token() is None + assert not responses.calls + assert "No refresh provider available" in caplog.text + + +@responses.activate +def test_spotify_oauth_client_supports_bridge_without_auth_state_file( + config: dict[str, Any], +): + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + proxy_config=None, + ) + responses.add( + responses.POST, + web.BRIDGE_REFRESH_URL, + json={ + "access_token": "access-token-1", + "token_type": "Bearer", + "expires_in": 3600, + }, + ) + + assert client.token() == "access-token-1" + + +@responses.activate +def test_spotify_oauth_client_uses_pkce_refresh_request_when_present( + config: dict[str, Any], tmp_path: Path +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text( + json.dumps( + { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "inline", + "value": "refresh-token-123", + }, + } + ), + encoding="utf-8", + ) + + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + auth_state_path=refresh_token_path, + proxy_config=None, + ) + responses.add( + responses.POST, + web.SPOTIFY_REFRESH_URL, + json={"access_token": "access-token", "token_type": "Bearer"}, + ) + + assert client.token() == "access-token" + request = responses.calls[0].request + assert request.url == web.SPOTIFY_REFRESH_URL + assert "Authorization" not in request.headers + assert urllib.parse.parse_qs(request.body) == { + "client_id": [pkce.CLIENT_ID], + "grant_type": ["refresh_token"], + "refresh_token": ["refresh-token-123"], + } + + +@pytest.mark.parametrize( + ("mode", "authorization_state"), + [("bridge", "configured"), ("pkce", "cleared")], +) +@responses.activate +def test_spotify_oauth_client_uses_bridge_for_non_authorized_state( + config: dict[str, Any], + tmp_path: Path, + mode: str, + authorization_state: str, +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text( + json.dumps({"version": 1, "mode": mode, "state": authorization_state}), + encoding="utf-8", + ) + + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + auth_state_path=refresh_token_path, + proxy_config=None, + ) + responses.add( + responses.POST, + web.BRIDGE_REFRESH_URL, + json={"access_token": "access-token", "token_type": "Bearer"}, + ) + + assert client.token() == "access-token" + request = responses.calls[0].request + assert request.url == web.BRIDGE_REFRESH_URL + assert urllib.parse.parse_qs(request.body) == {"grant_type": ["client_credentials"]} + credentials = ( + f"{config['spotify']['client_id']}:{config['spotify']['client_secret']}" + ) + expected = base64.b64encode(credentials.encode()).decode() + assert request.headers["Authorization"] == f"Basic {expected}" + + +@responses.activate +def test_spotify_oauth_client_falls_back_to_auth_proxy_after_auth_json_removed( + config: dict[str, Any], + tmp_path: Path, + mock_time: mock.Mock, + web_track_mock: dict[str, Any], +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text("stub") + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + auth_state_path=refresh_token_path, + proxy_config=None, + ) + refresh_token_path.unlink() + responses.add( + responses.POST, + "https://auth.mopidy.com/spotify/token", + json={ + "access_token": "access-token-2", + "token_type": "Bearer", + "expires_in": 3600, + }, + match=[ + matchers.urlencoded_params_matcher({"grant_type": "client_credentials"}) + ], + ) + responses.add( + responses.GET, + "https://api.spotify.com/v1/tracks/abc", + json=web_track_mock, + ) + mock_time.return_value = 1000 + + result = client.get("tracks/abc") + + assert result["uri"] == "spotify:track:abc" + assert len(responses.calls) == 2 + assert responses.calls[0].request.url == "https://auth.mopidy.com/spotify/token" + + +@responses.activate +def test_get_does_not_store_refresh_token_for_bridge_auth_state( + config: dict[str, Any], + tmp_path: Path, + mock_time: mock.Mock, + web_track_mock: dict[str, Any], +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text( + json.dumps({"version": 1, "mode": "bridge", "state": "configured"}), + encoding="utf-8", + ) + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + auth_state_path=refresh_token_path, + proxy_config=None, + ) + responses.add( + responses.POST, + "https://auth.mopidy.com/spotify/token", + json={ + "access_token": "access-token-2", + "refresh_token": "refresh-token-should-be-ignored", + "token_type": "Bearer", + "expires_in": 3600, + }, + match=[ + matchers.urlencoded_params_matcher({"grant_type": "client_credentials"}) + ], + ) + responses.add( + responses.GET, + "https://api.spotify.com/v1/tracks/abc", + json=web_track_mock, + ) + mock_time.return_value = 1000 + + result = client.get("tracks/abc") + + assert result["uri"] == "spotify:track:abc" + assert json.loads(refresh_token_path.read_text(encoding="utf-8")) == { + "version": 1, + "mode": "bridge", + "state": "configured", + } + + @pytest.mark.parametrize( ("header", "expected"), [ @@ -357,6 +668,570 @@ def test_get_uses_existing_access_token( assert result["uri"] == "spotify:track:abc" +@responses.activate +def test_get_uses_stored_refresh_token( + web_track_mock: dict[str, Any], + mock_time: mock.Mock, + refresh_token_oauth_client: tuple[web.OAuthClient, Path], +): + oauth_client, refresh_token_path = refresh_token_oauth_client + responses.add( + responses.POST, + "https://accounts.spotify.com/api/token", + json={ + "access_token": "access-token-2", + "refresh_token": "refresh-token-2", + "token_type": "Bearer", + "expires_in": 3600, + }, + match=[ + matchers.urlencoded_params_matcher( + { + "client_id": pkce.CLIENT_ID, + "grant_type": "refresh_token", + "refresh_token": "refresh-token-1", + } + ) + ], + ) + responses.add( + responses.GET, + "https://api.spotify.com/v1/tracks/abc", + json=web_track_mock, + ) + mock_time.return_value = 1000 + + result = oauth_client.get("tracks/abc") + + assert len(responses.calls) == 2 + assert ( + responses.calls[1].request.headers["Authorization"] == "Bearer access-token-2" + ) + assert json.loads(refresh_token_path.read_text()) == { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "inline", + "value": "refresh-token-2", + }, + } + assert result["uri"] == "spotify:track:abc" + + +@responses.activate +def test_get_rotates_keyring_refresh_token( + config: dict[str, Any], + tmp_path: Path, +): + path = tmp_path / "auth.json" + keyring = keyring_ext.memory() + keyring.save("original-id", "refresh-token-1") + path.write_text( + json.dumps( + { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "keyring", + "service": "mopidy-spotify", + "username": "original-id", + }, + } + ) + ) + responses.add( + responses.POST, + web.SPOTIFY_REFRESH_URL, + json={ + "access_token": "access-token", + "refresh_token": "refresh-token-2", + "token_type": "Bearer", + "expires_in": 3600, + }, + ) + + with mock.patch.object(keyring_ext, "system", return_value=keyring): + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + auth_state_path=path, + ) + assert client.token() == "access-token" + + manifest = json.loads(path.read_text()) + username = manifest["refresh_token"]["username"] + assert username != "original-id" + assert keyring.values == {username: "refresh-token-2"} + + +@responses.activate +def test_pkce_refresh_rejects_wrong_token_type( + refresh_token_oauth_client: tuple[web.SpotifyOAuthClient, Path], +): + client, refresh_token_path = refresh_token_oauth_client + original = refresh_token_path.read_text(encoding="utf-8") + responses.add( + responses.POST, + web.SPOTIFY_REFRESH_URL, + json={ + "access_token": "access-token-2", + "token_type": "MAC", + "expires_in": 3600, + }, + ) + + assert client.token() is None + assert "Authorization" not in client._headers + assert refresh_token_path.read_text(encoding="utf-8") == original + + +@responses.activate +def test_pkce_refresh_rejects_token_body_with_error_status( + refresh_token_oauth_client: tuple[web.SpotifyOAuthClient, Path], +): + client, refresh_token_path = refresh_token_oauth_client + original = refresh_token_path.read_text(encoding="utf-8") + responses.add( + responses.POST, + web.SPOTIFY_REFRESH_URL, + json={ + "access_token": "access-token-from-error", + "refresh_token": "refresh-token-from-error", + "token_type": "Bearer", + }, + status=400, + ) + + assert client.token() is None + assert "Authorization" not in client._headers + assert refresh_token_path.read_text(encoding="utf-8") == original + + +@responses.activate +def test_pkce_refresh_preserves_zero_expiry( + mock_time: mock.Mock, + refresh_token_oauth_client: tuple[web.SpotifyOAuthClient, Path], +): + client, _ = refresh_token_oauth_client + mock_time.return_value = 1000 + responses.add( + responses.POST, + web.SPOTIFY_REFRESH_URL, + json={ + "access_token": "access-token-2", + "token_type": "Bearer", + "expires_in": 0, + }, + ) + + assert client.token() == "access-token-2" + assert client._expires == 1000 + with client._refresh_mutex: + assert client._should_refresh_token() + + +@responses.activate +def test_pkce_refresh_success_does_not_overwrite_newer_authorization( + refresh_token_oauth_client: tuple[web.SpotifyOAuthClient, Path], +): + client, refresh_token_path = refresh_token_oauth_client + store = auth_store.Store(refresh_token_path) + replacement = state.PkceAuthorized(refresh_token=SecretStr("refresh-token-new")) + + def authorize_during_refresh(_: requests.PreparedRequest): + store.persist_pkce_authorization(replacement.refresh_token) + return ( + 200, + {}, + json.dumps( + { + "access_token": "access-token-old", + "refresh_token": "refresh-token-rotated", + "token_type": "Bearer", + "expires_in": 3600, + } + ), + ) + + responses.add_callback( + responses.POST, + web.SPOTIFY_REFRESH_URL, + callback=authorize_during_refresh, + content_type="application/json", + ) + + assert client.token() is None + assert "Authorization" not in client._headers + snapshot = store.load() + assert snapshot is not None + assert snapshot.state == replacement + + +@responses.activate +def test_pkce_refresh_error_does_not_overwrite_newer_authorization( + refresh_token_oauth_client: tuple[web.SpotifyOAuthClient, Path], +): + client, refresh_token_path = refresh_token_oauth_client + store = auth_store.Store(refresh_token_path) + replacement = state.PkceAuthorized(refresh_token=SecretStr("refresh-token-new")) + + def authorize_during_refresh(_: requests.PreparedRequest): + store.persist_pkce_authorization(replacement.refresh_token) + return ( + 400, + {}, + json.dumps( + { + "error": "invalid_grant", + "error_description": "Old refresh token expired", + } + ), + ) + + responses.add_callback( + responses.POST, + web.SPOTIFY_REFRESH_URL, + callback=authorize_during_refresh, + content_type="application/json", + ) + + assert client.token() is None + snapshot = store.load() + assert snapshot is not None + assert snapshot.state == replacement + + +@responses.activate +def test_pkce_refresh_does_not_cache_token_when_persistence_fails( + refresh_token_oauth_client: tuple[web.SpotifyOAuthClient, Path], +): + client, _ = refresh_token_oauth_client + responses.add( + responses.POST, + web.SPOTIFY_REFRESH_URL, + json={ + "access_token": "access-token-2", + "refresh_token": "refresh-token-2", + "token_type": "Bearer", + "expires_in": 3600, + }, + ) + + with mock.patch.object( + client._token_source._store, + "compare_and_set", + side_effect=auth_store.Error, + ): + assert client.token() is None + + assert "Authorization" not in client._headers + + +@responses.activate +def test_get_clears_expired_refresh_token_and_fails_fast( + mock_time: mock.Mock, + refresh_token_oauth_client: tuple[web.OAuthClient, Path], + caplog: pytest.LogCaptureFixture, +): + oauth_client, refresh_token_path = refresh_token_oauth_client + responses.add( + responses.POST, + "https://accounts.spotify.com/api/token", + json={ + "error": "invalid_grant", + "error_description": "Refresh token expired", + }, + status=400, + ) + mock_time.return_value = 1000 + + first_result = oauth_client.get("tracks/abc") + second_result = oauth_client.get("tracks/abc") + + assert first_result == {} + assert second_result == {} + assert len(responses.calls) == 1 + assert json.loads(refresh_token_path.read_text()) == { + "version": 1, + "mode": "pkce", + "state": "permanent_error", + "error_code": "invalid_grant", + "error_description": "Refresh token expired", + } + assert "Run `mopidy spotify auth web` to reauthorize" in caplog.text + + +@responses.activate +def test_get_keeps_refresh_token_on_transient_refresh_failure( + config: dict[str, Any], + tmp_path: Path, + mock_time: mock.Mock, +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text( + json.dumps( + { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "inline", + "value": "refresh-token-1", + }, + } + ), + encoding="utf-8", + ) + oauth_client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + proxy_config=None, + auth_state_path=refresh_token_path, + ) + oauth_client._number_of_retries = 1 + responses.add( + responses.POST, + "https://accounts.spotify.com/api/token", + json={"error": "temporarily_unavailable"}, + status=500, + ) + responses.add( + responses.POST, + "https://accounts.spotify.com/api/token", + json={"error": "temporarily_unavailable"}, + status=500, + ) + mock_time.return_value = 1000 + + first_result = oauth_client.get("tracks/abc") + second_result = oauth_client.get("tracks/abc") + + assert first_result == {} + assert second_result == {} + assert len(responses.calls) == 2 + assert refresh_token_path.exists() + + +def test_get_fails_fast_when_auth_json_is_permanent_error( + config: dict[str, Any], + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text( + json.dumps( + { + "version": 1, + "mode": "pkce", + "state": "permanent_error", + "error_code": "invalid_grant", + "error_description": "Refresh token expired", + } + ), + encoding="utf-8", + ) + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + proxy_config=None, + auth_state_path=refresh_token_path, + ) + + result = client.get("tracks/abc") + + assert result == {} + assert len(responses.calls) == 0 + assert "OAuth token refresh failed" in caplog.text + assert "Refresh token expired" in caplog.text + + +@responses.activate +def test_get_recovers_after_reauthorizing_with_new_auth_json( + web_track_mock: dict[str, Any], + mock_time: mock.Mock, + refresh_token_oauth_client: tuple[web.OAuthClient, Path], +): + oauth_client, refresh_token_path = refresh_token_oauth_client + responses.add( + responses.POST, + "https://accounts.spotify.com/api/token", + json={ + "error": "invalid_grant", + "error_description": "Refresh token expired", + }, + status=400, + ) + mock_time.return_value = 1000 + + first_result = oauth_client.get("tracks/abc") + + refresh_token_path.write_text( + json.dumps( + { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "inline", + "value": "refresh-token-2", + }, + } + ), + encoding="utf-8", + ) + responses.add( + responses.POST, + "https://accounts.spotify.com/api/token", + json={ + "access_token": "access-token-2", + "refresh_token": "refresh-token-3", + "token_type": "Bearer", + "expires_in": 3600, + }, + match=[ + matchers.urlencoded_params_matcher( + { + "client_id": pkce.CLIENT_ID, + "grant_type": "refresh_token", + "refresh_token": "refresh-token-2", + } + ) + ], + ) + responses.add( + responses.GET, + "https://api.spotify.com/v1/tracks/abc", + json=web_track_mock, + ) + + second_result = oauth_client.get("tracks/abc") + + assert first_result == {} + assert second_result["uri"] == "spotify:track:abc" + assert len(responses.calls) == 3 + assert json.loads(refresh_token_path.read_text()) == { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "inline", + "value": "refresh-token-3", + }, + } + + +@pytest.mark.parametrize( + "contents", + [ + "{", + json.dumps({"version": 2, "refresh_token": "refresh-token-1"}), + json.dumps({"version": 1, "refresh_token": "refresh-token-1"}), + json.dumps({"version": 1}), + json.dumps({"version": 1, "refresh_token": "refresh-token-1", "extra": 1}), + ], +) +def test_get_fails_fast_when_auth_json_is_invalid( + config: dict[str, Any], + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + contents: str, +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text(contents, encoding="utf-8") + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + proxy_config=None, + auth_state_path=refresh_token_path, + ) + + result = client.get("tracks/abc") + + assert result == {} + assert len(responses.calls) == 0 + assert ( + "OAuth token refresh failed: Invalid Spotify authorization state" in caplog.text + ) + assert "Run `mopidy spotify auth web` to replace it" in caplog.text + + +def test_get_fails_cleanly_when_auth_json_is_not_utf8( + config: dict[str, Any], + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_bytes(b"\xff") + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + proxy_config=None, + auth_state_path=refresh_token_path, + ) + + assert client.token() is None + assert "Could not load Spotify authorization state" in caplog.text + + +@responses.activate +def test_get_uses_stored_refresh_token_without_legacy_client_id( + web_track_mock: dict[str, Any], + mock_time: mock.Mock, + tmp_path: Path, +): + refresh_token_path = tmp_path / "auth.json" + refresh_token_path.write_text( + json.dumps( + { + "version": 1, + "mode": "pkce", + "state": "authorized", + "refresh_token": { + "storage": "inline", + "value": "refresh-token-1", + }, + } + ), + encoding="utf-8", + ) + oauth_client = _spotify_client( + client_id=None, + client_secret=None, + proxy_config=None, + auth_state_path=refresh_token_path, + ) + responses.add( + responses.POST, + "https://accounts.spotify.com/api/token", + json={ + "access_token": "access-token-2", + "token_type": "Bearer", + "expires_in": 3600, + }, + match=[ + matchers.urlencoded_params_matcher( + { + "client_id": pkce.CLIENT_ID, + "grant_type": "refresh_token", + "refresh_token": "refresh-token-1", + } + ) + ], + ) + responses.add( + responses.GET, + "https://api.spotify.com/v1/tracks/abc", + json=web_track_mock, + ) + mock_time.return_value = 1000 + + result = oauth_client.get("tracks/abc") + + assert result["uri"] == "spotify:track:abc" + + @responses.activate def test_bad_client_credentials(oauth_client: web.OAuthClient): bad_response = { @@ -662,7 +1537,7 @@ def test_web_response( assert isinstance(result, web.WebResponse) assert result.url == "https://api.spotify.com/v1/tracks/abc" - assert result._status_code == 301 + assert result.status_code == 301 assert result._expires == 2054 assert result._etag == '"12345"' assert result.still_valid() @@ -789,7 +1664,7 @@ def test_dont_cache_bad_status( ) result = oauth_client.get("https://api.spotify.com/v1/tracks/abc", cache) - assert result._status_code == 404 + assert result.status_code == 404 assert not oauth_client._should_cache_response(result) assert "https://api.spotify.com/v1/tracks/abc" not in cache @@ -902,7 +1777,7 @@ def test_increase_expiry(web_response_mock: web.WebResponse): def test_increase_expiry_skipped_for_bad_status(web_response_mock: web.WebResponse): - web_response_mock._status_code = 404 + web_response_mock.status_code = 404 web_response_mock.increase_expiry(30) @@ -968,7 +1843,7 @@ def test_updated_responses_changed( @pytest.fixture def spotify_client(config: dict[str, Any]) -> web.SpotifyOAuthClient: - client = web.SpotifyOAuthClient( + client = _spotify_client( client_id=config["spotify"]["client_id"], client_secret=config["spotify"]["client_secret"], proxy_config=None, @@ -1091,15 +1966,6 @@ def test_track_required_fields(self, field: str): def test_playlist_required_fields(self, field: str): assert field in web.SpotifyOAuthClient.PLAYLIST_FIELDS - def test_configures_auth(self): - client = web.SpotifyOAuthClient( - client_id="1234567", - client_secret="AbCdEfG", # noqa: S106 - proxy_config=None, - ) - - assert client._auth == ("1234567", "AbCdEfG") - def test_configures_proxy(self): proxy_config = { "scheme": "https", @@ -1108,7 +1974,7 @@ def test_configures_proxy(self): "username": "alice", "password": "s3cret", } - client = web.SpotifyOAuthClient( + client = _spotify_client( client_id=None, client_secret=None, proxy_config=proxy_config ) @@ -1117,9 +1983,8 @@ def test_configures_proxy(self): == "https://alice:s3cret@my-proxy.example.com:8080" ) - def test_configures_urls(self, spotify_client: web.SpotifyOAuthClient): + def test_configures_web_api_url(self, spotify_client: web.SpotifyOAuthClient): assert spotify_client._base_url == "https://api.spotify.com/v1" - assert spotify_client._refresh_url == "https://auth.mopidy.com/spotify/token" @responses.activate def test_login_alice( @@ -1356,7 +2221,7 @@ def test_with_all_tracks_uses_cached_tracks_when_unchanged( responses.calls.reset() mock_time.return_value = 1000 - foo_album_response._status_code = 304 + foo_album_response.status_code = 304 result2 = spotify_client._with_all_tracks(foo_album_response) assert len(responses.calls) == 0 @@ -1910,3 +2775,109 @@ def test_weblink_from_uri_raises(uri: Uri): web.WebLink.from_uri(uri) assert f"Could not parse {uri!r} as a Spotify URI" in str(excinfo.value) + + +@responses.activate +def test_get_persists_proxy_bridge_ready_state( + config: dict[str, Any], + tmp_path: Path, + mock_time: mock.Mock, + web_track_mock: dict[str, Any], +): + refresh_token_path = tmp_path / "auth.json" + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + auth_state_path=refresh_token_path, + proxy_config=None, + ) + responses.add( + responses.POST, + "https://auth.mopidy.com/spotify/token", + json={ + "access_token": "access-token-2", + "token_type": "Bearer", + "expires_in": 3600, + }, + match=[ + matchers.urlencoded_params_matcher({"grant_type": "client_credentials"}) + ], + ) + responses.add( + responses.GET, + "https://api.spotify.com/v1/tracks/abc", + json=web_track_mock, + ) + mock_time.return_value = 1000 + + result = client.get("tracks/abc") + + assert result["uri"] == "spotify:track:abc" + assert json.loads(refresh_token_path.read_text(encoding="utf-8")) == { + "version": 1, + "mode": "bridge", + "state": "configured", + } + + +@responses.activate +def test_get_recovers_from_bridge_error_with_corrected_credentials( + config: dict[str, Any], + tmp_path: Path, + mock_time: mock.Mock, +): + refresh_token_path = tmp_path / "auth.json" + client = _spotify_client( + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + auth_state_path=refresh_token_path, + proxy_config=None, + ) + responses.add( + responses.POST, + "https://auth.mopidy.com/spotify/token", + json={ + "error": "invalid_grant", + "error_description": "Bridge token expired", + }, + status=400, + ) + mock_time.return_value = 1000 + + first_result = client.token() + + assert first_result is None + assert json.loads(refresh_token_path.read_text(encoding="utf-8")) == { + "version": 1, + "mode": "bridge", + "state": "permanent_error", + "error_code": "invalid_grant", + "error_description": "Bridge token expired", + } + + corrected_client = _spotify_client( + client_id="corrected-client-id", + client_secret="corrected-client-secret", # noqa: S106 + auth_state_path=refresh_token_path, + proxy_config=None, + ) + responses.add( + responses.POST, + "https://auth.mopidy.com/spotify/token", + json={ + "access_token": "access-token-2", + "token_type": "Bearer", + "expires_in": 3600, + }, + ) + + assert corrected_client.token() == "access-token-2" + assert len(responses.calls) == 2 + assert responses.calls[1].request.headers["Authorization"] == ( + "Basic Y29ycmVjdGVkLWNsaWVudC1pZDpjb3JyZWN0ZWQtY2xpZW50LXNlY3JldA==" + ) + assert json.loads(refresh_token_path.read_text(encoding="utf-8")) == { + "version": 1, + "mode": "bridge", + "state": "configured", + }