From 0ce1e11257488d5f221e25352a3904345b6b8510 Mon Sep 17 00:00:00 2001 From: bhagenbourger Date: Thu, 26 Feb 2026 14:26:50 +0000 Subject: [PATCH 1/5] Make refresh_url configurable --- src/mopidy_spotify/__init__.py | 1 + src/mopidy_spotify/backend.py | 1 + src/mopidy_spotify/ext.conf | 1 + src/mopidy_spotify/web.py | 4 ++-- tests/conftest.py | 1 + tests/test_backend.py | 16 ++++++++++++++++ tests/test_web.py | 7 ++++++- 7 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/mopidy_spotify/__init__.py b/src/mopidy_spotify/__init__.py index d2add995..3fd3c2b3 100644 --- a/src/mopidy_spotify/__init__.py +++ b/src/mopidy_spotify/__init__.py @@ -22,6 +22,7 @@ def get_config_schema(self): schema["client_id"] = config.String() schema["client_secret"] = config.Secret() + schema["refresh_url"] = config.String() 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 6657c6b6..46fa2e07 100644 --- a/src/mopidy_spotify/backend.py +++ b/src/mopidy_spotify/backend.py @@ -23,6 +23,7 @@ def __init__(self, config, audio): def on_start(self): self._web_client = web.SpotifyOAuthClient( + refresh_url=self._config["spotify"]["refresh_url"], client_id=self._config["spotify"]["client_id"], client_secret=self._config["spotify"]["client_secret"], proxy_config=self._config["proxy"], diff --git a/src/mopidy_spotify/ext.conf b/src/mopidy_spotify/ext.conf index 6e9cb450..919b1823 100644 --- a/src/mopidy_spotify/ext.conf +++ b/src/mopidy_spotify/ext.conf @@ -2,6 +2,7 @@ enabled = true client_id = client_secret = +refresh_url = https://auth.mopidy.com/spotify/token bitrate = 160 volume_normalization = true timeout = 10 diff --git a/src/mopidy_spotify/web.py b/src/mopidy_spotify/web.py index 0ce31a34..4398fe48 100644 --- a/src/mopidy_spotify/web.py +++ b/src/mopidy_spotify/web.py @@ -506,10 +506,10 @@ class SpotifyOAuthClient(OAuthClient): PLAYLIST_FIELDS = f"name,owner(id),type,uri,snapshot_id,tracks({TRACK_FIELDS})," DEFAULT_EXTRA_EXPIRY = 10 - def __init__(self, *, client_id, client_secret, proxy_config): + def __init__(self, *, refresh_url, client_id, client_secret, proxy_config): super().__init__( base_url="https://api.spotify.com/v1", - refresh_url="https://auth.mopidy.com/spotify/token", + refresh_url=refresh_url, client_id=client_id, client_secret=client_secret, proxy_config=proxy_config, diff --git a/tests/conftest.py b/tests/conftest.py index a329c14f..8b09003f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -33,6 +33,7 @@ def config(tmp_path): "search_track_count": 50, "client_id": "abcd1234", "client_secret": "YWJjZDEyMzQ=", + "refresh_url": "https://auth.mopidy.com/spotify/token", }, } diff --git a/tests/test_backend.py b/tests/test_backend.py index 36f9b5c8..cafb8aaa 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -72,6 +72,21 @@ def test_on_start_configures_proxy(web_mock, config): ) +def test_on_start_configures_refresh_url(web_mock, config): + config["spotify"]["refresh_url"] = "http://localhost:8080/callback" + + backend = get_backend(config) + with ThreadJoiner(): + backend.on_start() + + web_mock.SpotifyOAuthClient.assert_called_once_with( + refresh_url="http://localhost:8080/callback", + client_id=mock.ANY, + client_secret=mock.ANY, + proxy_config=mock.ANY, + ) + + def test_on_start_configures_web_client(web_mock, config): config["spotify"]["client_id"] = "1234567" config["spotify"]["client_secret"] = "AbCdEfG" # noqa: S105 @@ -81,6 +96,7 @@ def test_on_start_configures_web_client(web_mock, config): backend.on_start() web_mock.SpotifyOAuthClient.assert_called_once_with( + refresh_url="https://auth.mopidy.com/spotify/token", client_id="1234567", client_secret="AbCdEfG", # noqa: S106 proxy_config=mock.ANY, diff --git a/tests/test_web.py b/tests/test_web.py index e631e803..a0569a23 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -719,6 +719,7 @@ def test_updated_responses_changed(web_response_mock, oauth_client, mock_time): @pytest.fixture def spotify_client(config): client = web.SpotifyOAuthClient( + refresh_url=config["spotify"]["refresh_url"], client_id=config["spotify"]["client_id"], client_secret=config["spotify"]["client_secret"], proxy_config=None, @@ -839,6 +840,7 @@ def test_playlist_required_fields(self, field): def test_configures_auth(self): client = web.SpotifyOAuthClient( + refresh_url="https://auth.mopidy.com/spotify/token", client_id="1234567", client_secret="AbCdEfG", # noqa: S106 proxy_config=None, @@ -855,7 +857,10 @@ def test_configures_proxy(self): "password": "s3cret", } client = web.SpotifyOAuthClient( - client_id=None, client_secret=None, proxy_config=proxy_config + refresh_url="https://auth.mopidy.com/spotify/token", + client_id=None, + client_secret=None, + proxy_config=proxy_config, ) assert ( From 9fdda75d7ce4790c0290dd71d87ba85630966b5b Mon Sep 17 00:00:00 2001 From: bhagenbourger Date: Thu, 5 Mar 2026 16:29:44 +0000 Subject: [PATCH 2/5] Add Spotify direct authentication method --- pyproject.toml | 2 +- src/mopidy_spotify/__init__.py | 9 +++ src/mopidy_spotify/authenticate.py | 114 ++++++++++++++++++++++++++ src/mopidy_spotify/backend.py | 14 +++- src/mopidy_spotify/ext.conf | 2 + src/mopidy_spotify/search.py | 4 +- src/mopidy_spotify/translator.py | 6 +- src/mopidy_spotify/web.py | 82 +++++++++++++------ tests/conftest.py | 12 ++- tests/test_authenticate.py | 115 ++++++++++++++++++++++++++ tests/test_backend.py | 61 ++++++++++++-- tests/test_browse.py | 2 +- tests/test_playlists.py | 4 +- tests/test_search.py | 2 +- tests/test_translator.py | 8 +- tests/test_web.py | 124 ++++++++++++++++------------- 16 files changed, 454 insertions(+), 107 deletions(-) create mode 100644 src/mopidy_spotify/authenticate.py create mode 100644 tests/test_authenticate.py diff --git a/pyproject.toml b/pyproject.toml index 36ab9e3e..9c09d8e8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,7 +15,7 @@ classifiers = [ ] dynamic = ["version"] dependencies = [ - "mopidy >= 4.0.0a14", + "mopidy >= 4.0.0a15", "pykka >= 4.1", "requests >= 2.32", ] diff --git a/src/mopidy_spotify/__init__.py b/src/mopidy_spotify/__init__.py index 3fd3c2b3..2075e542 100644 --- a/src/mopidy_spotify/__init__.py +++ b/src/mopidy_spotify/__init__.py @@ -1,4 +1,5 @@ import pathlib +from enum import Enum from importlib.metadata import version from mopidy import config, ext @@ -11,6 +12,10 @@ class Extension(ext.Extension): ext_name = "spotify" version = __version__ + class Provider(Enum): + MOPIDY_PROXY = "mopidy_proxy" + SPOTIFY_DIRECT = "spotify_direct" + def get_default_config(self): return config.read(pathlib.Path(__file__).parent / "ext.conf") @@ -20,9 +25,13 @@ def get_config_schema(self): schema["username"] = config.Deprecated() # since 5.0 schema["password"] = config.Deprecated() # since 5.0 + schema["authentication_provider"] = config.String( + choices=[p.value for p in self.Provider] + ) schema["client_id"] = config.String() schema["client_secret"] = config.Secret() schema["refresh_url"] = config.String() + schema["cache_credentials_path"] = config.String(optional=True) schema["bitrate"] = config.Integer(choices=(96, 160, 320)) schema["volume_normalization"] = config.Boolean() diff --git a/src/mopidy_spotify/authenticate.py b/src/mopidy_spotify/authenticate.py new file mode 100644 index 00000000..5e773094 --- /dev/null +++ b/src/mopidy_spotify/authenticate.py @@ -0,0 +1,114 @@ +import base64 +import json +import secrets +import urllib.parse +import urllib.request +from pathlib import Path + + +class SpotifyDirectAuthentication: + def __init__( + self, + client_id: str, + client_secret: str, + refresh_url: str, + credentials: dict[str, str] | None = None, + cache_credentials_path: str | None = None, + ): + self._client_id: str = client_id + self._client_secret: str = client_secret + self._refresh_url: str = refresh_url + self._credentials: dict[str, str] | None = credentials + self._cache_credentials_path: str | None = cache_credentials_path + + def _save_token(self, token: dict[str, str]) -> None: + if self._cache_credentials_path: + with Path(self._cache_credentials_path).open("w") as f: + json.dump(token, f) + + def _load_token(self) -> dict[str, str] | None: + if self._cache_credentials_path and Path(self._cache_credentials_path).exists(): + with Path(self._cache_credentials_path).open() as f: + return json.load(f) + return None + + def _spotify_token_request(self, payload: dict[str, str]) -> dict[str, str]: + url = "https://accounts.spotify.com/api/token" + # Validate URL scheme is https + parsed_url = urllib.parse.urlparse(url) + if parsed_url.scheme != "https": + msg = f"Invalid URL scheme: {parsed_url.scheme}" + raise RuntimeError(msg) + + auth_str = f"{self._client_id}:{self._client_secret}" + auth_header = base64.b64encode(auth_str.encode()).decode() + + encoded_data = urllib.parse.urlencode(payload).encode() + + req = urllib.request.Request(url, data=encoded_data, method="POST") # noqa: S310 + req.add_header("Authorization", f"Basic {auth_header}") + req.add_header("Content-Type", "application/x-www-form-urlencoded") + + with urllib.request.urlopen(req) as response: # noqa: S310 + return json.loads(response.read().decode()) + + def _refresh_access_token(self, refresh_token: str) -> dict[str, str]: + data = { + "grant_type": "refresh_token", + "refresh_token": refresh_token, + } + new_token = self._spotify_token_request(data) + if "refresh_token" not in new_token: + new_token["refresh_token"] = refresh_token + return new_token + + def _exchange_code_for_token(self, code: str) -> dict[str, str]: + data = { + "grant_type": "authorization_code", + "code": code, + "redirect_uri": self._refresh_url, + } + return self._spotify_token_request(data) + + def _authenticate(self) -> dict[str, str]: + state = secrets.token_urlsafe(16) + + auth_params = { + "client_id": self._client_id, + "response_type": "code", + "redirect_uri": self._refresh_url, + "scope": "playlist-read-private streaming", + "state": state, + } + authorization_url = f"https://accounts.spotify.com/authorize?{urllib.parse.urlencode(auth_params)}" + response_url = input(f""" +Please go here and authorize: {authorization_url} + Paste the full redirect URL here: + """) + + parsed_url = urllib.parse.urlparse(response_url) + query_params = urllib.parse.parse_qs(parsed_url.query) + + returned_state = query_params.get("state", [None])[0] + code = query_params.get("code", [None])[0] + + if returned_state != state: + msg = "STATE MISMATCH: Possible CSRF attack detected." + raise RuntimeError(msg) + + if not code: + msg = "Authorization failed: No code returned." + raise RuntimeError(msg) + + return self._exchange_code_for_token(code) + + def fetch_token(self) -> dict[str, str]: + token = self._credentials or self._load_token() + + if not token: + token = self._authenticate() + else: + token = self._refresh_access_token(token["refresh_token"]) + + self._save_token(token) + return token diff --git a/src/mopidy_spotify/backend.py b/src/mopidy_spotify/backend.py index 46fa2e07..8f90dd77 100644 --- a/src/mopidy_spotify/backend.py +++ b/src/mopidy_spotify/backend.py @@ -23,9 +23,17 @@ def __init__(self, config, audio): def on_start(self): self._web_client = web.SpotifyOAuthClient( - refresh_url=self._config["spotify"]["refresh_url"], - client_id=self._config["spotify"]["client_id"], - client_secret=self._config["spotify"]["client_secret"], + authentication_config=web.SpotifyAuthenticationConfig( + provider=Extension.Provider( + self._config["spotify"]["authentication_provider"] + ), + client_id=self._config["spotify"]["client_id"], + client_secret=self._config["spotify"]["client_secret"], + refresh_url=self._config["spotify"]["refresh_url"], + cache_credentials_path=self._config["spotify"][ + "cache_credentials_path" + ], + ), proxy_config=self._config["proxy"], ) self._web_client.login() diff --git a/src/mopidy_spotify/ext.conf b/src/mopidy_spotify/ext.conf index 919b1823..b38b2369 100644 --- a/src/mopidy_spotify/ext.conf +++ b/src/mopidy_spotify/ext.conf @@ -1,8 +1,10 @@ [spotify] enabled = true +authentication_provider = mopidy_proxy client_id = client_secret = refresh_url = https://auth.mopidy.com/spotify/token +cache_credentials_path = bitrate = 160 volume_normalization = true timeout = 10 diff --git a/src/mopidy_spotify/search.py b/src/mopidy_spotify/search.py index 31cbd433..c727b645 100644 --- a/src/mopidy_spotify/search.py +++ b/src/mopidy_spotify/search.py @@ -90,9 +90,9 @@ def search( # noqa: PLR0913 tracks = ( [ translator.web_to_track(web_track) - for web_track in result["tracks"]["items"][: config["search_track_count"]] + for web_track in result["items"]["items"][: config["search_track_count"]] ] - if "tracks" in result + if "items" in result else [] ) tracks = [x for x in tracks if x] diff --git a/src/mopidy_spotify/translator.py b/src/mopidy_spotify/translator.py index 044200ce..35e315e2 100644 --- a/src/mopidy_spotify/translator.py +++ b/src/mopidy_spotify/translator.py @@ -94,7 +94,7 @@ def web_to_track_refs(web_tracks, *, check_playable=True): for web_track in web_tracks: ref = web_to_track_ref( # The extra level here is to also support "saved track objects". - web_track.get("track", web_track), + web_track.get("item", web_track), check_playable=check_playable, ) if ref is not None: @@ -113,7 +113,7 @@ def to_playlist( if ref is None or as_ref: return ref - web_tracks = web_playlist.get("tracks", {}).get("items") or [] + web_tracks = web_playlist.get("items", {}).get("items") or [] if as_items and not isinstance(web_tracks, list): return None @@ -121,7 +121,7 @@ def to_playlist( return list(web_to_track_refs(web_tracks)) tracks = [ - web_to_track(web_track.get("track", {}), bitrate=bitrate) + web_to_track(web_track.get("item", {}), bitrate=bitrate) for web_track in web_tracks ] tracks = [t for t in tracks if t] diff --git a/src/mopidy_spotify/web.py b/src/mopidy_spotify/web.py index 4398fe48..6af8d7f0 100644 --- a/src/mopidy_spotify/web.py +++ b/src/mopidy_spotify/web.py @@ -13,7 +13,7 @@ import requests -from mopidy_spotify import utils +from mopidy_spotify import Extension, authenticate, utils logger = logging.getLogger(__name__) @@ -38,8 +38,10 @@ def __init__( # noqa: PLR0913 *, base_url, refresh_url, + authentication_provider, client_id=None, client_secret=None, + cache_credentials_path, proxy_config=None, expiry_margin=60, timeout=10, @@ -51,6 +53,9 @@ def __init__( # noqa: PLR0913 else: self._auth = None self._access_token = None + self._credentials = None + self._authentication_provider = authentication_provider + self._cache_credentials_path = cache_credentials_path self._base_url = base_url self._refresh_url = refresh_url @@ -145,10 +150,23 @@ def _refresh_token(self): msg = "Lock must be held before calling." raise OAuthTokenRefreshError(msg) - data = {"grant_type": "client_credentials"} - result = self._request_with_retries( - "POST", self._refresh_url, auth=self._auth, data=data - ) + if self._authentication_provider == Extension.Provider.SPOTIFY_DIRECT: + if self._auth: + session = authenticate.SpotifyDirectAuthentication( + client_id=self._auth[0], + client_secret=self._auth[1], + refresh_url=self._refresh_url, + credentials=self._credentials, + cache_credentials_path=self._cache_credentials_path, + ) + result = session.fetch_token() + else: + result = None + else: + data = {"grant_type": "client_credentials"} + result = self._request_with_retries( + "POST", self._refresh_url, auth=self._auth, data=data + ) if result is None: msg = "Unknown error." @@ -164,6 +182,7 @@ def _refresh_token(self): raise OAuthTokenRefreshError(msg) self._access_token = result["access_token"] + self._credentials = result self._headers["Authorization"] = f"Bearer {self._access_token}" self._expires = time.time() + result.get("expires_in", float("Inf")) @@ -498,20 +517,36 @@ def __init__(self, message): } +@dataclass(frozen=True, slots=True) +class SpotifyAuthenticationConfig: + provider: Extension.Provider + client_id: str | None + client_secret: str | None + refresh_url: str + cache_credentials_path: str | None + + class SpotifyOAuthClient(OAuthClient): TRACK_FIELDS = ( - "next,items(track(type,uri,name,duration_ms,disc_number,track_number," + "next,items(item(type,uri,name,duration_ms,disc_number,track_number," "artists,album,is_playable,linked_from.uri))" ) - PLAYLIST_FIELDS = f"name,owner(id),type,uri,snapshot_id,tracks({TRACK_FIELDS})," + PLAYLIST_FIELDS = f"name,owner(id),type,uri,snapshot_id,items({TRACK_FIELDS})," + ALBUM_FIELDS = f"name,owner(id),type,uri,snapshot_id,items({TRACK_FIELDS})," DEFAULT_EXTRA_EXPIRY = 10 - def __init__(self, *, refresh_url, client_id, client_secret, proxy_config): + def __init__( + self, + authentication_config: SpotifyAuthenticationConfig, + proxy_config, + ): super().__init__( base_url="https://api.spotify.com/v1", - refresh_url=refresh_url, - client_id=client_id, - client_secret=client_secret, + refresh_url=authentication_config.refresh_url, + authentication_provider=authentication_config.provider, + client_id=authentication_config.client_id, + client_secret=authentication_config.client_secret, + cache_credentials_path=authentication_config.cache_credentials_path, proxy_config=proxy_config, ) self.user_id = None @@ -545,17 +580,17 @@ def logged_in(self): def get_user_playlists(self, *, refresh=False): expiry_strategy = ExpiryStrategy.FORCE_EXPIRED if refresh else None pages = self.get_all( - f"users/{self.user_id}/playlists", + "me/playlists", params={"limit": 50}, expiry_strategy=expiry_strategy, ) for page in pages: yield from page.get("items", []) - def _with_all_tracks(self, obj, params=None): + def _with_all_tracks(self, obj, params=None, tracks_key="items"): if params is None: params = {} - tracks_path = obj.get("tracks", {}).get("next") + tracks_path = obj.get(tracks_key, {}).get("next") track_pages = self.get_all( tracks_path, params=params, @@ -573,8 +608,8 @@ def _with_all_tracks(self, obj, params=None): if more_tracks: # Take a copy to avoid changing the cached response. obj = copy.deepcopy(obj) - obj.setdefault("tracks", {}).setdefault("items", []) - obj["tracks"]["items"] += more_tracks + obj.setdefault(tracks_key, {}).setdefault("items", []) + obj[tracks_key]["items"] += more_tracks return obj @@ -604,14 +639,11 @@ def get_batch(self, link_type, links): links = list(dict.fromkeys(links)) # Remove duplicates and maintain order for batch in utils.batched(links, API_MAX_IDS_PER_REQUEST[link_type]): - ids = [u.id for u in batch] ids_to_links = {u.id: u for u in batch} - data = self.get_one( - f"{link_type}s", params={"ids": ",".join(ids), "market": "from_token"} - ) - for item in data.get(f"{link_type}s") or []: - if not item: - continue + for type_id in ids_to_links: + item = self.get_one( + f"{link_type}s/{type_id}", params={"market": "from_token"} + ) # For track re-linking. if "linked_from" in item: @@ -619,7 +651,7 @@ def get_batch(self, link_type, links): else: item_id = item.get("id") if link := ids_to_links.get(item_id): - yield link, WebResponse.from_batch(data, item) + yield link, WebResponse.from_batch(item, item) else: logger.warning(f"Invalid batch item: {item}") @@ -633,7 +665,7 @@ def get_albums(self, album_links): for link in album_links: if album := result.get(link): - yield self._with_all_tracks(album) + yield self._with_all_tracks(album, tracks_key="tracks") def get_artist_albums(self, web_link, *, all_tracks=True): if web_link.type != LinkType.ARTIST: diff --git a/tests/conftest.py b/tests/conftest.py index 8b09003f..43242580 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -34,6 +34,8 @@ def config(tmp_path): "client_id": "abcd1234", "client_secret": "YWJjZDEyMzQ=", "refresh_url": "https://auth.mopidy.com/spotify/token", + "authentication_provider": "mopidy_proxy", + "cache_credentials_path": None, }, } @@ -41,7 +43,9 @@ def config(tmp_path): @pytest.fixture def web_mock(): patcher = mock.patch.object(backend, "web", spec=web) - yield patcher.start() + mocked_web = patcher.start() + mocked_web.SpotifyAuthenticationConfig = web.SpotifyAuthenticationConfig + yield mocked_web patcher.stop() @@ -50,7 +54,7 @@ def web_search_mock(web_album_mock_base, web_artist_mock, web_track_mock): return { "albums": {"items": [web_album_mock_base]}, "artists": {"items": [web_artist_mock]}, - "tracks": {"items": [web_track_mock, web_track_mock]}, + "items": {"items": [web_track_mock, web_track_mock]}, } @@ -59,7 +63,7 @@ def web_search_mock_large(web_album_mock, web_artist_mock, web_track_mock): return { "albums": {"items": [web_album_mock] * 10}, "artists": {"items": [web_artist_mock] * 10}, - "tracks": {"items": [web_track_mock] * 10}, + "items": {"items": [web_track_mock] * 10}, } @@ -174,7 +178,7 @@ def web_playlist_mock(web_track_mock): return { "owner": {"id": "alice"}, "name": "Foo", - "tracks": {"items": [{"track": web_track_mock}]}, + "items": {"items": [{"item": web_track_mock}]}, "snapshot_id": "abcderfg12364", "uri": "spotify:user:alice:playlist:foo", "type": "playlist", diff --git a/tests/test_authenticate.py b/tests/test_authenticate.py new file mode 100644 index 00000000..73b396c0 --- /dev/null +++ b/tests/test_authenticate.py @@ -0,0 +1,115 @@ +import builtins +import json +import secrets + +import pytest + +from mopidy_spotify.authenticate import SpotifyDirectAuthentication + +mock_secret = secrets.token_urlsafe(16) +mock_old_token = secrets.token_urlsafe(16) +mock_new_token = secrets.token_urlsafe(16) + + +def test_fetch_token_refreshes_and_caches(tmp_path, monkeypatch): + cache_path = tmp_path / "token.json" + auth = SpotifyDirectAuthentication( + client_id="id", + client_secret=mock_secret, + refresh_url="https://example.com/redirect", + credentials={"refresh_token": mock_old_token}, + cache_credentials_path=str(cache_path), + ) + + def fake_token_request(self, payload: dict[str, str]) -> dict[str, str]: + assert payload["grant_type"] == "refresh_token" + assert payload["refresh_token"] == mock_old_token + return {"access_token": mock_new_token} + + monkeypatch.setattr( + SpotifyDirectAuthentication, "_spotify_token_request", fake_token_request + ) + + token = auth.fetch_token() + + assert token["access_token"] == mock_new_token + assert token["refresh_token"] == mock_old_token + + assert json.loads(cache_path.read_text()) == token + + +def test_authenticate_flow_saves_token(tmp_path, monkeypatch): + cache_path = tmp_path / "token.json" + + # Ensure the state is predictable. + monkeypatch.setattr( + "mopidy_spotify.authenticate.secrets.token_urlsafe", lambda n: "fixedstate" + ) + + # Simulate browser redirect with expected state and code. + monkeypatch.setattr( + builtins, + "input", + lambda prompt="": "https://example.com/callback?code=code123&state=fixedstate", + ) + + def fake_token_request(self, payload: dict[str, str]) -> dict[str, str]: + assert payload["grant_type"] == "authorization_code" + assert payload["code"] == "code123" + return {"access_token": "tok", "refresh_token": "rtok"} + + monkeypatch.setattr( + SpotifyDirectAuthentication, "_spotify_token_request", fake_token_request + ) + + auth = SpotifyDirectAuthentication( + client_id="id", + client_secret=mock_secret, + refresh_url="https://example.com/redirect", + cache_credentials_path=str(cache_path), + ) + + token = auth.fetch_token() + + assert token == {"access_token": "tok", "refresh_token": "rtok"} + assert json.loads(cache_path.read_text()) == token + + +def test_authenticate_state_mismatch_raises(monkeypatch): + monkeypatch.setattr( + "mopidy_spotify.authenticate.secrets.token_urlsafe", lambda n: "expected" + ) + monkeypatch.setattr( + builtins, + "input", + lambda prompt="": "https://example.com/callback?code=code123&state=wrong", + ) + + auth = SpotifyDirectAuthentication( + client_id="id", + client_secret=mock_secret, + refresh_url="https://example.com/redirect", + ) + + with pytest.raises(RuntimeError, match="STATE MISMATCH"): + auth.fetch_token() + + +def test_authenticate_missing_code_raises(monkeypatch): + monkeypatch.setattr( + "mopidy_spotify.authenticate.secrets.token_urlsafe", lambda n: "expected" + ) + monkeypatch.setattr( + builtins, + "input", + lambda prompt="": "https://example.com/callback?state=expected", + ) + + auth = SpotifyDirectAuthentication( + client_id="id", + client_secret=mock_secret, + refresh_url="https://example.com/redirect", + ) + + with pytest.raises(RuntimeError, match="Authorization failed"): + auth.fetch_token() diff --git a/tests/test_backend.py b/tests/test_backend.py index cafb8aaa..d2d7cad7 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -2,7 +2,8 @@ from mopidy import backend as backend_api -from mopidy_spotify import backend, library, playlists +import mopidy_spotify +from mopidy_spotify import backend, library, playlists, web from mopidy_spotify.backend import SpotifyPlaybackProvider from tests import ThreadJoiner @@ -80,9 +81,51 @@ def test_on_start_configures_refresh_url(web_mock, config): backend.on_start() web_mock.SpotifyOAuthClient.assert_called_once_with( - refresh_url="http://localhost:8080/callback", - client_id=mock.ANY, - client_secret=mock.ANY, + authentication_config=web.SpotifyAuthenticationConfig( + provider=mopidy_spotify.Extension.Provider.MOPIDY_PROXY, + client_id=mock.ANY, + client_secret=mock.ANY, + refresh_url="http://localhost:8080/callback", + cache_credentials_path=None, + ), + proxy_config=mock.ANY, + ) + + +def test_on_start_configures_provider_spotify_direct(web_mock, config): + config["spotify"]["authentication_provider"] = "spotify_direct" + + backend = get_backend(config) + with ThreadJoiner(): + backend.on_start() + + web_mock.SpotifyOAuthClient.assert_called_once_with( + authentication_config=web.SpotifyAuthenticationConfig( + provider=mopidy_spotify.Extension.Provider.SPOTIFY_DIRECT, + client_id=mock.ANY, + client_secret=mock.ANY, + refresh_url="https://auth.mopidy.com/spotify/token", + cache_credentials_path=None, + ), + proxy_config=mock.ANY, + ) + + +def test_on_start_configures_cache_credentials_path(web_mock, config, tmp_path): + config["spotify"]["cache_credentials_path"] = str(tmp_path / "spotify_credentials") + + backend = get_backend(config) + with ThreadJoiner(): + backend.on_start() + + web_mock.SpotifyOAuthClient.assert_called_once_with( + authentication_config=web.SpotifyAuthenticationConfig( + provider=mopidy_spotify.Extension.Provider.MOPIDY_PROXY, + client_id=mock.ANY, + client_secret=mock.ANY, + refresh_url="https://auth.mopidy.com/spotify/token", + cache_credentials_path=str(tmp_path / "spotify_credentials"), + ), proxy_config=mock.ANY, ) @@ -96,9 +139,13 @@ def test_on_start_configures_web_client(web_mock, config): backend.on_start() web_mock.SpotifyOAuthClient.assert_called_once_with( - refresh_url="https://auth.mopidy.com/spotify/token", - client_id="1234567", - client_secret="AbCdEfG", # noqa: S106 + authentication_config=web.SpotifyAuthenticationConfig( + provider=mopidy_spotify.Extension.Provider.MOPIDY_PROXY, + client_id="1234567", + client_secret="AbCdEfG", # noqa: S106 + refresh_url="https://auth.mopidy.com/spotify/token", + cache_credentials_path=None, + ), proxy_config=mock.ANY, ) diff --git a/tests/test_browse.py b/tests/test_browse.py index 65e3b21c..b6c127d8 100644 --- a/tests/test_browse.py +++ b/tests/test_browse.py @@ -257,7 +257,7 @@ def test_browse_your_music_empty(web_client_mock, provider): def test_browse_your_music_tracks(web_client_mock, web_track_mock, provider): - web_saved_track_mock = {"track": web_track_mock} + web_saved_track_mock = {"item": web_track_mock} web_client_mock.get_all.return_value = [ {"items": [web_saved_track_mock, web_saved_track_mock]}, {"items": [web_saved_track_mock, web_saved_track_mock]}, diff --git a/tests/test_playlists.py b/tests/test_playlists.py index 7d321b6c..66967802 100644 --- a/tests/test_playlists.py +++ b/tests/test_playlists.py @@ -15,7 +15,7 @@ def web_client_mock(web_client_mock, web_track_mock): web_playlist1 = { "owner": {"id": "alice"}, "name": "Foo", - "tracks": {"items": [{"track": web_track_mock}]}, + "items": {"items": [{"item": web_track_mock}]}, "uri": "spotify:user:alice:playlist:foo", "type": "playlist", } @@ -28,7 +28,7 @@ def web_client_mock(web_client_mock, web_track_mock): web_playlist3 = { "owner": {"id": "alice"}, "name": "Malformed", - "tracks": {"items": []}, + "items": {"items": []}, "uri": "spotify:user:alice:playlist:malformed", "type": "bogus", } diff --git a/tests/test_search.py b/tests/test_search.py index 3ed9abe8..5e8d47db 100644 --- a/tests/test_search.py +++ b/tests/test_search.py @@ -249,7 +249,7 @@ def test_search_filters_bad_results( } web_search_mock["albums"]["items"] = [good_track] web_search_mock["artists"]["items"][0]["uri"] = None - web_search_mock["tracks"]["items"] = [{}, good_track, bad_track] + web_search_mock["items"]["items"] = [{}, good_track, bad_track] web_client_mock.get.return_value = web_search_mock result = provider.search({"any": ["ABBA"]}) diff --git a/tests/test_translator.py b/tests/test_translator.py index 0b1a2abe..71a64d84 100644 --- a/tests/test_translator.py +++ b/tests/test_translator.py @@ -186,7 +186,7 @@ def test_uri_uses_relinked_from_uri(self, web_track_mock): class TestWebToTrackRefs: def test_returns_playlist_tracks(self, web_track_mock): - web_tracks = [{"track": web_track_mock}] * 3 + web_tracks = [{"item": web_track_mock}] * 3 refs = list(translator.web_to_track_refs(web_tracks)) assert refs == [refs[0], refs[0], refs[0]] @@ -261,7 +261,7 @@ def test_successful_translation(self, web_track_mock, web_playlist_mock): assert playlist.last_modified is None def test_no_track_data(self, web_playlist_mock): - del web_playlist_mock["tracks"] + del web_playlist_mock["items"] playlist = translator.to_playlist(web_playlist_mock) @@ -276,7 +276,7 @@ def test_as_items(self, web_track_mock, web_playlist_mock): assert track_ref in items def test_as_items_no_track_data(self, web_playlist_mock): - del web_playlist_mock["tracks"] + del web_playlist_mock["items"] items = translator.to_playlist(web_playlist_mock, as_items=True) @@ -317,7 +317,7 @@ def test_without_name_uses_uri(self, web_playlist_mock): assert ref.name == "spotify:user:alice:playlist:foo" def test_success_without_track_data(self, web_playlist_mock): - del web_playlist_mock["tracks"] + del web_playlist_mock["items"] ref = translator.to_playlist_ref(web_playlist_mock) diff --git a/tests/test_web.py b/tests/test_web.py index a0569a23..36f51d36 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -16,8 +16,10 @@ def oauth_client(config): return web.OAuthClient( base_url="https://api.spotify.com/v1", refresh_url="https://auth.mopidy.com/spotify/token", + authentication_provider=mopidy_spotify.Extension.Provider.MOPIDY_PROXY, client_id=config["spotify"]["client_id"], client_secret=config["spotify"]["client_secret"], + cache_credentials_path=None, proxy_config=None, expiry_margin=60, ) @@ -719,9 +721,13 @@ def test_updated_responses_changed(web_response_mock, oauth_client, mock_time): @pytest.fixture def spotify_client(config): client = web.SpotifyOAuthClient( - refresh_url=config["spotify"]["refresh_url"], - client_id=config["spotify"]["client_id"], - client_secret=config["spotify"]["client_secret"], + authentication_config=web.SpotifyAuthenticationConfig( + provider=mopidy_spotify.Extension.Provider.MOPIDY_PROXY, + client_id=config["spotify"]["client_id"], + client_secret=config["spotify"]["client_secret"], + refresh_url=config["spotify"]["refresh_url"], + cache_credentials_path=config["spotify"]["cache_credentials_path"], + ), proxy_config=None, ) client.user_id = "alice" @@ -753,7 +759,7 @@ def playlist_tracks_parms(): def bar_playlist(playlist_parms): return { "href": url(f"playlists/bar?{playlist_parms}"), - "tracks": {"items": [0]}, + "items": {"items": [0]}, } @@ -769,7 +775,7 @@ def foo_playlist_tracks(playlist_tracks_parms): def foo_playlist(playlist_parms, foo_playlist_tracks): return { "href": url(f"playlists/foo?{playlist_parms}"), - "tracks": {"items": [1, 2], "next": foo_playlist_tracks["href"]}, + "items": {"items": [1, 2], "next": foo_playlist_tracks["href"]}, } @@ -820,7 +826,7 @@ class TestSpotifyOAuthClient: "field", [ ("next"), - ("items(track"), + ("items(item"), ("type"), ("uri"), ("name"), @@ -833,16 +839,20 @@ def test_track_required_fields(self, field): @pytest.mark.parametrize( "field", - [("name"), ("type"), ("uri"), ("snapshot_id"), ("tracks")], + [("name"), ("type"), ("uri"), ("snapshot_id"), ("items")], ) def test_playlist_required_fields(self, field): assert field in web.SpotifyOAuthClient.PLAYLIST_FIELDS def test_configures_auth(self): client = web.SpotifyOAuthClient( - refresh_url="https://auth.mopidy.com/spotify/token", - client_id="1234567", - client_secret="AbCdEfG", # noqa: S106 + authentication_config=web.SpotifyAuthenticationConfig( + provider=mopidy_spotify.Extension.Provider.MOPIDY_PROXY, + client_id="1234567", + client_secret="AbCdEfG", # noqa: S106 + refresh_url="https://auth.mopidy.com/spotify/token", + cache_credentials_path=None, + ), proxy_config=None, ) @@ -857,9 +867,13 @@ def test_configures_proxy(self): "password": "s3cret", } client = web.SpotifyOAuthClient( - refresh_url="https://auth.mopidy.com/spotify/token", - client_id=None, - client_secret=None, + authentication_config=web.SpotifyAuthenticationConfig( + provider=mopidy_spotify.Extension.Provider.MOPIDY_PROXY, + client_id=None, + client_secret=None, + refresh_url="https://auth.mopidy.com/spotify/token", + cache_credentials_path=None, + ), proxy_config=proxy_config, ) @@ -959,7 +973,7 @@ def test_get_all_none(self, spotify_client): @responses.activate def test_get_user_playlists_empty(self, spotify_client): - responses.add(responses.GET, url("users/alice/playlists"), json={}) + responses.add(responses.GET, url("me/playlists"), json={}) result = list(spotify_client.get_user_playlists()) @@ -979,7 +993,7 @@ def test_get_user_playlists_get_all(self, spotify_client, refresh, strategy): result = list(spotify_client.get_user_playlists(refresh=refresh)) spotify_client.get_all.assert_called_once_with( - "users/alice/playlists", + "me/playlists", params={"limit": 50}, expiry_strategy=strategy, ) @@ -987,29 +1001,27 @@ def test_get_user_playlists_get_all(self, spotify_client, refresh, strategy): @responses.activate def test_get_user_playlists_sets_params(self, spotify_client): - responses.add(responses.GET, url("users/alice/playlists"), json={}) + responses.add(responses.GET, url("me/playlists"), json={}) list(spotify_client.get_user_playlists()) assert len(responses.calls) == 1 encoded_params = urllib.parse.urlencode({"limit": 50}) - assert responses.calls[0].request.url == url( - f"users/alice/playlists?{encoded_params}" - ) + assert responses.calls[0].request.url == url(f"me/playlists?{encoded_params}") @responses.activate def test_get_user_playlists(self, spotify_client): responses.add( responses.GET, - url("users/alice/playlists?limit=50"), + url("me/playlists?limit=50"), json={ - "next": url("users/alice/playlists?offset=50"), + "next": url("me/playlists?offset=50"), "items": ["playlist0", "playlist1", "playlist2"], }, ) responses.add( responses.GET, - url("users/alice/playlists?limit=50&offset=50"), + url("me/playlists?limit=50&offset=50"), json={ "next": None, "items": ["playlist3", "playlist4", "playlist5"], @@ -1030,7 +1042,9 @@ def test_with_all_tracks_error(self, spotify_client, foo_album_response, caplog) json={"error": "baz"}, ) - result = spotify_client._with_all_tracks(foo_album_response) + result = spotify_client._with_all_tracks( + foo_album_response, tracks_key="tracks" + ) assert result == {} assert "Spotify Web API request failed: baz" in caplog.text @@ -1045,7 +1059,9 @@ def test_with_all_tracks( json=foo_album_next_tracks, ) - result = spotify_client._with_all_tracks(foo_album_response) + result = spotify_client._with_all_tracks( + foo_album_response, tracks_key="tracks" + ) assert len(responses.calls) == 1 assert result["tracks"]["items"] == [3, 4, 5, 6, 7, 8] @@ -1065,7 +1081,9 @@ def test_with_all_tracks_uses_cached_tracks_when_unchanged( ) mock_time.return_value = -1000 - result1 = spotify_client._with_all_tracks(foo_album_response) + result1 = spotify_client._with_all_tracks( + foo_album_response, tracks_key="tracks" + ) assert len(responses.calls) == 1 cache_keys = list(spotify_client._cache.keys()) @@ -1075,7 +1093,9 @@ def test_with_all_tracks_uses_cached_tracks_when_unchanged( mock_time.return_value = 1000 foo_album_response._status_code = 304 - result2 = spotify_client._with_all_tracks(foo_album_response) + result2 = spotify_client._with_all_tracks( + foo_album_response, tracks_key="tracks" + ) assert len(responses.calls) == 0 assert result1 == result2 @@ -1175,7 +1195,7 @@ def test_get_playlist_collates_tracks( result = spotify_client.get_playlist("spotify:playlist:foo") assert len(responses.calls) == 2 - assert result["tracks"]["items"] == [1, 2, 3, 4, 5] + assert result["items"]["items"] == [1, 2, 3, 4, 5] @pytest.mark.parametrize( ("uri", "msg"), @@ -1198,9 +1218,9 @@ def test_logged_in(self, spotify_client, user_id, expected): def test_get_albums(self, foo_album, foo_album_next_tracks, spotify_client): responses.add( responses.GET, - url("albums"), - match=[matchers.query_string_matcher("ids=foo&market=from_token")], - json={"albums": [foo_album]}, + url("albums/foo"), + match=[matchers.query_string_matcher("market=from_token")], + json=foo_album, ) responses.add( responses.GET, @@ -1374,7 +1394,7 @@ def test_get_artist_top_tracks_invalid_uri( responses.add( responses.GET, url("artists/baz/top-tracks"), - json={"tracks": [web_track_mock, web_track_mock]}, + json={"items": [web_track_mock, web_track_mock]}, ) link = web.WebLink.from_uri("spotify:artist:baz") link.type = "your" @@ -1401,16 +1421,8 @@ def test_get_batch_max_ids_per_request(self, spotify_client, link_type, max_link dict(spotify_client.get_batch(web.LinkType(link_type), links)) - assert spotify_client.get_one.call_count == 2 - - request_ids_1 = spotify_client.get_one.call_args_list[0][1]["params"]["ids"] - assert len(request_ids_1.split(",")) == max_links - - request_ids_2 = spotify_client.get_one.call_args_list[1][1]["params"]["ids"] - assert len(request_ids_2.split(",")) == 1 - assert set(request_ids_2.split(",")) == ( - {link.id for link in links} - set(request_ids_1.split(",")) - ) + assert spotify_client.get_one.call_count == 21 if link_type == "album" else 51 + assert spotify_client.get_one.call_args_list[0][0][0] == f"{link_type}s/0" def test_get_batch_removes_duplicates(self, spotify_client): spotify_client.get_one = mock.Mock(return_value={}) @@ -1419,8 +1431,7 @@ def test_get_batch_removes_duplicates(self, spotify_client): dict(spotify_client.get_batch(web.LinkType("track"), links)) assert spotify_client.get_one.call_count == 1 - request_ids_1 = spotify_client.get_one.call_args_list[0][1]["params"]["ids"] - assert request_ids_1 == links[0].id + assert spotify_client.get_one.call_args_list[0][0][0] == f"tracks/{links[0].id}" def test_get_batch_playlist(self, spotify_client, caplog): links = [web.WebLink.from_uri("spotify:playlist:foo")] @@ -1431,7 +1442,7 @@ def test_get_batch_playlist(self, spotify_client, caplog): assert "Cannot handle batched playlists" in caplog.text def test_get_batch_empty_results(self, spotify_client): - spotify_client.get_one = mock.Mock(return_value={"tracks": [None]}) + spotify_client.get_one = mock.Mock(return_value={"items": [None]}) links = [ web.WebLink.from_uri("spotify:track:foo"), web.WebLink.from_uri("spotify:track:bar"), @@ -1456,11 +1467,9 @@ def test_get_batch_web_response( mock_type = mock_res["type"] responses.add( responses.GET, - url(res_types), - match=[ - matchers.query_string_matcher(f"ids={mock_id}%2Cbar&market=from_token") - ], - json={res_types: [mock_res]}, + url(f"{res_types}/{mock_id}"), + match=[matchers.query_string_matcher("market=from_token")], + json=mock_res, ) links = [ web.WebLink.from_uri(f"spotify:{mock_type}:{mock_id}"), @@ -1478,15 +1487,22 @@ def test_get_batch_web_response( def test_get_batch_error(self, spotify_client, web_album_mock, caplog): responses.add( responses.GET, - url("albums"), - match=[matchers.query_string_matcher("ids=def&market=from_token")], - json={"albums": [web_album_mock, {"error": "bar"}, None]}, + url("albums/def"), + match=[matchers.query_string_matcher("market=from_token")], + json=web_album_mock, + ) + responses.add( + responses.GET, + url("albums/err"), + match=[matchers.query_string_matcher("market=from_token")], + json={"error": "bar"}, ) link = web.WebLink.from_uri(web_album_mock["uri"]) - results = dict(spotify_client.get_batch(link.type, [link])) + link2 = web.WebLink.from_uri("spotify:album:err") + results = dict(spotify_client.get_batch(link.type, [link, link2])) - assert len(responses.calls) == 1 + assert len(responses.calls) == 2 assert len(results) == 1 assert results[link]["name"] == "DEF 456" assert "Invalid batch item" in caplog.text From 6a9e86a3d1f560262e936c35cda53c5616e49bf9 Mon Sep 17 00:00:00 2001 From: bhagenbourger Date: Fri, 20 Mar 2026 14:41:35 +0000 Subject: [PATCH 3/5] Extract prepare_url method to utils.py --- src/mopidy_spotify/utils.py | 22 ++++++++++++++++++++++ src/mopidy_spotify/web.py | 22 +--------------------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/mopidy_spotify/utils.py b/src/mopidy_spotify/utils.py index c190c983..e3be9146 100644 --- a/src/mopidy_spotify/utils.py +++ b/src/mopidy_spotify/utils.py @@ -2,7 +2,9 @@ import itertools import logging import operator +import os import time +import urllib.parse import requests from mopidy import httpclient @@ -54,3 +56,23 @@ def batched(iterable, n): it = iter(iterable) while batch := tuple(itertools.islice(it, n)): yield batch + + +def prepare_url(base_url, url, *args, **kwargs): + b = urllib.parse.urlsplit(base_url) + u = urllib.parse.urlsplit(url.format(*args)) + + if u.scheme or u.netloc: + scheme, netloc, path = u.scheme, u.netloc, u.path + query = urllib.parse.parse_qsl(u.query, keep_blank_values=True) + else: + scheme, netloc = b.scheme, b.netloc + path = os.path.normpath(os.path.join(b.path, u.path)) # noqa: PTH118 + query = urllib.parse.parse_qsl(b.query, keep_blank_values=True) + query.extend(urllib.parse.parse_qsl(u.query, keep_blank_values=True)) + + for key, value in kwargs.items(): + query.append((key, value)) + + encoded_query = urllib.parse.urlencode(dict(query)) + return urllib.parse.urlunsplit((scheme, netloc, path, encoded_query, "")) diff --git a/src/mopidy_spotify/web.py b/src/mopidy_spotify/web.py index 6af8d7f0..3f5af226 100644 --- a/src/mopidy_spotify/web.py +++ b/src/mopidy_spotify/web.py @@ -195,7 +195,7 @@ def _refresh_token(self): def _request_with_retries(self, method, url, *args, **kwargs): prepared_request = self._session.prepare_request( - requests.Request(method, self._prepare_url(url, *args), **kwargs) + requests.Request(method, utils.prepare_url(self._base_url, url, *args), **kwargs) ) try_until = time.time() + self._timeout @@ -255,26 +255,6 @@ def _request_with_retries(self, method, url, *args, **kwargs): ) return result - def _prepare_url(self, url, *args, **kwargs): - # TODO: Move this out as a helper and unit-test it directly? - b = urllib.parse.urlsplit(self._base_url) - u = urllib.parse.urlsplit(url.format(*args)) - - if u.scheme or u.netloc: - scheme, netloc, path = u.scheme, u.netloc, u.path - query = urllib.parse.parse_qsl(u.query, keep_blank_values=True) - else: - scheme, netloc = b.scheme, b.netloc - path = os.path.normpath(os.path.join(b.path, u.path)) # noqa: PTH118 - query = urllib.parse.parse_qsl(b.query, keep_blank_values=True) - query.extend(urllib.parse.parse_qsl(u.query, keep_blank_values=True)) - - for key, value in kwargs.items(): - query.append((key, value)) - - encoded_query = urllib.parse.urlencode(dict(query)) - return urllib.parse.urlunsplit((scheme, netloc, path, encoded_query, "")) - def _normalise_query_string(self, url, params=None): u = urllib.parse.urlsplit(url) scheme, netloc, path = u.scheme, u.netloc, u.path From 9e795b3e491cccf7d130ac8517b76bb55058dafc Mon Sep 17 00:00:00 2001 From: bhagenbourger Date: Fri, 20 Mar 2026 15:48:21 +0000 Subject: [PATCH 4/5] Create WebRequest class to handle web request --- src/mopidy_spotify/web.py | 166 +++++++++++++++++++++++--------------- tests/test_web.py | 2 +- 2 files changed, 100 insertions(+), 68 deletions(-) diff --git a/src/mopidy_spotify/web.py b/src/mopidy_spotify/web.py index 3f5af226..680c802a 100644 --- a/src/mopidy_spotify/web.py +++ b/src/mopidy_spotify/web.py @@ -32,6 +32,97 @@ class OAuthClientError(Exception): pass +class WebRequest: + def __init__( + self, + session, + base_url, + timeout=10, + retries=3, + retry_statuses=(500, 502, 503, 429), + backoff_factor=0.5, + ): + self._session = session + self._base_url = base_url + self._timeout = timeout + self._number_of_retries = retries + self._retry_statuses = retry_statuses + self._backoff_factor = backoff_factor + + def execute(self, method, url, *args, **kwargs): + prepared_request = self._session.prepare_request( + requests.Request( + method, utils.prepare_url(self._base_url, url, *args), **kwargs + ) + ) + + try_until = time.time() + self._timeout + + status_code = None + result = None + backoff_time = 0 + + for i in range(self._number_of_retries): + remaining_timeout = max(try_until - time.time(), 1) + + # Give up if we don't have any timeout left after sleeping. + if backoff_time > remaining_timeout: + break + if backoff_time > 0: + time.sleep(backoff_time) + + try: + response = self._session.send( + prepared_request, timeout=remaining_timeout + ) + except requests.RequestException as e: + logger.debug(f"Fetching {prepared_request.url} failed: {e}") + status_code = None + backoff_time = 0 + result = None + else: + status_code = response.status_code + backoff_time = self._parse_retry_after(response) + result = WebResponse.from_requests(prepared_request, response) + + if status_code and 400 <= status_code < 600: # noqa: PLR2004 + logger.debug(f"Fetching {prepared_request.url} failed: {status_code}") + + # Filter out cases where we should not retry. + if status_code and status_code not in self._retry_statuses: + break + + # TODO: Provider might return invalid JSON for "OK" responses. + # This should really not happen, so ignoring for the purpose of + # retries. It would be easier if they correctly used 204, but + # instead some endpoints return 200 with no content, or true/false. + + # Decide how long to sleep in the next iteration. + backoff_time = backoff_time or (2**i * self._backoff_factor) + logger.error( + f"Retrying {prepared_request.url} in {backoff_time:.3f} seconds." + ) + + return result, status_code + + def _parse_retry_after(self, response): + """Parse Retry-After header from response if it is set.""" + value = response.headers.get("Retry-After") + + if not value: + seconds = 0 + elif re.match(r"^\s*[0-9]+\s*$", value): + seconds = int(value) + else: + now = datetime.now(tz=UTC).replace(tzinfo=None) + try: + date_tuple = parsedate_to_datetime(value) + seconds = (date_tuple - now).total_seconds() + except ValueError: + seconds = 0 + return max(0, seconds) + + class OAuthClient: def __init__( # noqa: PLR0913 self, @@ -71,6 +162,13 @@ def __init__( # noqa: PLR0913 self._headers = {"Content-Type": "application/json"} self._session = utils.get_requests_session(proxy_config or {}) + self._request = WebRequest( + session=self._session, + base_url=self._base_url, + timeout=self._timeout, + retries=self._number_of_retries, + retry_statuses=self._retry_statuses, + ) # TODO: Move _cache_mutex to the object it actually protects. self._cache_mutex = threading.Lock() # Protects get() cache param. self._refresh_mutex = threading.Lock() # Protects _headers and _expires. @@ -194,56 +292,7 @@ def _refresh_token(self): logger.debug(f"Token scopes: {result['scope']}") def _request_with_retries(self, method, url, *args, **kwargs): - prepared_request = self._session.prepare_request( - requests.Request(method, utils.prepare_url(self._base_url, url, *args), **kwargs) - ) - - try_until = time.time() + self._timeout - - status_code = None - result = None - backoff_time = 0 - - for i in range(self._number_of_retries): - remaining_timeout = max(try_until - time.time(), 1) - - # Give up if we don't have any timeout left after sleeping. - if backoff_time > remaining_timeout: - break - if backoff_time > 0: - time.sleep(backoff_time) - - try: - response = self._session.send( - prepared_request, timeout=remaining_timeout - ) - except requests.RequestException as e: - logger.debug(f"Fetching {prepared_request.url} failed: {e}") - status_code = None - backoff_time = 0 - result = None - else: - status_code = response.status_code - backoff_time = self._parse_retry_after(response) - result = WebResponse.from_requests(prepared_request, response) - - if status_code and 400 <= status_code < 600: # noqa: PLR2004 - logger.debug(f"Fetching {prepared_request.url} failed: {status_code}") - - # Filter out cases where we should not retry. - if status_code and status_code not in self._retry_statuses: - break - - # TODO: Provider might return invalid JSON for "OK" responses. - # This should really not happen, so ignoring for the purpose of - # retries. It would be easier if they correctly used 204, but - # instead some endpoints return 200 with no content, or true/false. - - # Decide how long to sleep in the next iteration. - backoff_time = backoff_time or (2**i * self._backoff_factor) - logger.error( - f"Retrying {prepared_request.url} in {backoff_time:.3f} seconds." - ) + result, status_code = self._request.execute(method, url, *args, **kwargs) if status_code == HTTPStatus.UNAUTHORIZED: self._authorization_failed = True @@ -266,23 +315,6 @@ def _normalise_query_string(self, url, params=None): encoded_query = urllib.parse.urlencode(sorted_unique_query) return urllib.parse.urlunsplit((scheme, netloc, path, encoded_query, "")) - def _parse_retry_after(self, response): - """Parse Retry-After header from response if it is set.""" - value = response.headers.get("Retry-After") - - if not value: - seconds = 0 - elif re.match(r"^\s*[0-9]+\s*$", value): - seconds = int(value) - else: - now = datetime.now(tz=UTC).replace(tzinfo=None) - try: - date_tuple = parsedate_to_datetime(value) - seconds = (date_tuple - now).total_seconds() - except ValueError: - seconds = 0 - return max(0, seconds) - @unique class ExpiryStrategy(StrEnum): diff --git a/tests/test_web.py b/tests/test_web.py index 36f51d36..05167e73 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -107,7 +107,7 @@ def test_user_agent(oauth_client): def test_parse_retry_after(oauth_client, mock_now, header, expected): mock_now.return_value = datetime(2022, 12, 7, 11, 27, 26, 0, tzinfo=UTC) mock_response = mock.Mock(headers={"Retry-After": header}) - result = oauth_client._parse_retry_after(mock_response) + result = oauth_client._request._parse_retry_after(mock_response) assert result == expected From 5aafba12dd749453dba651991f42e4c7fd020708 Mon Sep 17 00:00:00 2001 From: bhagenbourger Date: Fri, 20 Mar 2026 16:26:16 +0000 Subject: [PATCH 5/5] Centralize authentication --- src/mopidy_spotify/authenticate.py | 67 ++++++--- src/mopidy_spotify/web.py | 231 +++++++++++++++-------------- tests/test_authenticate.py | 43 +++++- 3 files changed, 199 insertions(+), 142 deletions(-) diff --git a/src/mopidy_spotify/authenticate.py b/src/mopidy_spotify/authenticate.py index 5e773094..14b2a270 100644 --- a/src/mopidy_spotify/authenticate.py +++ b/src/mopidy_spotify/authenticate.py @@ -3,25 +3,32 @@ import secrets import urllib.parse import urllib.request +from abc import ABC, abstractmethod from pathlib import Path -class SpotifyDirectAuthentication: +class Authentication(ABC): + @abstractmethod + def fetch_token(self) -> tuple[dict[str, str] | None, int | None]: + """Fetch an OAuth token.""" + + +class SpotifyDirectAuthentication(Authentication): def __init__( self, client_id: str, client_secret: str, refresh_url: str, - credentials: dict[str, str] | None = None, cache_credentials_path: str | None = None, ): self._client_id: str = client_id self._client_secret: str = client_secret self._refresh_url: str = refresh_url - self._credentials: dict[str, str] | None = credentials + self._credentials: dict[str, str] | None = None self._cache_credentials_path: str | None = cache_credentials_path def _save_token(self, token: dict[str, str]) -> None: + self._credentials = token if self._cache_credentials_path: with Path(self._cache_credentials_path).open("w") as f: json.dump(token, f) @@ -32,37 +39,32 @@ def _load_token(self) -> dict[str, str] | None: return json.load(f) return None - def _spotify_token_request(self, payload: dict[str, str]) -> dict[str, str]: + def _spotify_token_request(self, payload: dict[str, str]) -> tuple[dict[str, str], int]: url = "https://accounts.spotify.com/api/token" - # Validate URL scheme is https - parsed_url = urllib.parse.urlparse(url) - if parsed_url.scheme != "https": - msg = f"Invalid URL scheme: {parsed_url.scheme}" - raise RuntimeError(msg) auth_str = f"{self._client_id}:{self._client_secret}" auth_header = base64.b64encode(auth_str.encode()).decode() encoded_data = urllib.parse.urlencode(payload).encode() - req = urllib.request.Request(url, data=encoded_data, method="POST") # noqa: S310 + req = urllib.request.Request(url, data=encoded_data, method="POST") req.add_header("Authorization", f"Basic {auth_header}") req.add_header("Content-Type", "application/x-www-form-urlencoded") - with urllib.request.urlopen(req) as response: # noqa: S310 - return json.loads(response.read().decode()) + with urllib.request.urlopen(req) as response: + return json.loads(response.read().decode()), response.status - def _refresh_access_token(self, refresh_token: str) -> dict[str, str]: + def _refresh_access_token(self, refresh_token: str) -> tuple[dict[str, str], int]: data = { "grant_type": "refresh_token", "refresh_token": refresh_token, } - new_token = self._spotify_token_request(data) + new_token, status = self._spotify_token_request(data) if "refresh_token" not in new_token: new_token["refresh_token"] = refresh_token - return new_token + return new_token, status - def _exchange_code_for_token(self, code: str) -> dict[str, str]: + def _exchange_code_for_token(self, code: str) -> tuple[dict[str, str], int]: data = { "grant_type": "authorization_code", "code": code, @@ -70,7 +72,7 @@ def _exchange_code_for_token(self, code: str) -> dict[str, str]: } return self._spotify_token_request(data) - def _authenticate(self) -> dict[str, str]: + def _authenticate(self) -> tuple[dict[str, str], int]: state = secrets.token_urlsafe(16) auth_params = { @@ -102,13 +104,36 @@ def _authenticate(self) -> dict[str, str]: return self._exchange_code_for_token(code) - def fetch_token(self) -> dict[str, str]: + def fetch_token(self) -> tuple[dict[str, str] | None, int | None]: token = self._credentials or self._load_token() + status = None if not token: - token = self._authenticate() + token, status = self._authenticate() else: - token = self._refresh_access_token(token["refresh_token"]) + token, status = self._refresh_access_token(token["refresh_token"]) self._save_token(token) - return token + return token, status + + +class MopidyProxyAuthentication(Authentication): + def __init__( + self, + request, + auth: tuple[str, str] | None, + refresh_url: str, + ): + self._request = request + self._auth: tuple[str, str] | None = auth + self._refresh_url: str = refresh_url + + def _request_with_retries(self, method, url, *args, **kwargs) -> tuple[dict[str, str] | None, int | None]: + result, status = self._request.execute(method, url, *args, **kwargs) + return result, status + + def fetch_token(self) -> tuple[dict[str, str] | None, int | None]: + data = {"grant_type": "client_credentials"} + return self._request_with_retries( + "POST", self._refresh_url, auth=self._auth, data=data + ) diff --git a/src/mopidy_spotify/web.py b/src/mopidy_spotify/web.py index 680c802a..d8c158be 100644 --- a/src/mopidy_spotify/web.py +++ b/src/mopidy_spotify/web.py @@ -1,6 +1,5 @@ import copy import logging -import os import re import threading import time @@ -32,97 +31,6 @@ class OAuthClientError(Exception): pass -class WebRequest: - def __init__( - self, - session, - base_url, - timeout=10, - retries=3, - retry_statuses=(500, 502, 503, 429), - backoff_factor=0.5, - ): - self._session = session - self._base_url = base_url - self._timeout = timeout - self._number_of_retries = retries - self._retry_statuses = retry_statuses - self._backoff_factor = backoff_factor - - def execute(self, method, url, *args, **kwargs): - prepared_request = self._session.prepare_request( - requests.Request( - method, utils.prepare_url(self._base_url, url, *args), **kwargs - ) - ) - - try_until = time.time() + self._timeout - - status_code = None - result = None - backoff_time = 0 - - for i in range(self._number_of_retries): - remaining_timeout = max(try_until - time.time(), 1) - - # Give up if we don't have any timeout left after sleeping. - if backoff_time > remaining_timeout: - break - if backoff_time > 0: - time.sleep(backoff_time) - - try: - response = self._session.send( - prepared_request, timeout=remaining_timeout - ) - except requests.RequestException as e: - logger.debug(f"Fetching {prepared_request.url} failed: {e}") - status_code = None - backoff_time = 0 - result = None - else: - status_code = response.status_code - backoff_time = self._parse_retry_after(response) - result = WebResponse.from_requests(prepared_request, response) - - if status_code and 400 <= status_code < 600: # noqa: PLR2004 - logger.debug(f"Fetching {prepared_request.url} failed: {status_code}") - - # Filter out cases where we should not retry. - if status_code and status_code not in self._retry_statuses: - break - - # TODO: Provider might return invalid JSON for "OK" responses. - # This should really not happen, so ignoring for the purpose of - # retries. It would be easier if they correctly used 204, but - # instead some endpoints return 200 with no content, or true/false. - - # Decide how long to sleep in the next iteration. - backoff_time = backoff_time or (2**i * self._backoff_factor) - logger.error( - f"Retrying {prepared_request.url} in {backoff_time:.3f} seconds." - ) - - return result, status_code - - def _parse_retry_after(self, response): - """Parse Retry-After header from response if it is set.""" - value = response.headers.get("Retry-After") - - if not value: - seconds = 0 - elif re.match(r"^\s*[0-9]+\s*$", value): - seconds = int(value) - else: - now = datetime.now(tz=UTC).replace(tzinfo=None) - try: - date_tuple = parsedate_to_datetime(value) - seconds = (date_tuple - now).total_seconds() - except ValueError: - seconds = 0 - return max(0, seconds) - - class OAuthClient: def __init__( # noqa: PLR0913 self, @@ -144,7 +52,6 @@ def __init__( # noqa: PLR0913 else: self._auth = None self._access_token = None - self._credentials = None self._authentication_provider = authentication_provider self._cache_credentials_path = cache_credentials_path @@ -169,6 +76,24 @@ def __init__( # noqa: PLR0913 retries=self._number_of_retries, retry_statuses=self._retry_statuses, ) + + if self._auth: + if self._authentication_provider == Extension.Provider.SPOTIFY_DIRECT: + self._authentication = authenticate.SpotifyDirectAuthentication( + client_id=self._auth[0], + client_secret=self._auth[1], + refresh_url=self._refresh_url, + cache_credentials_path=self._cache_credentials_path, + ) + else: + self._authentication = authenticate.MopidyProxyAuthentication( + request=self._request, + auth=self._auth, + refresh_url=self._refresh_url, + ) + else: + self._authentication = None + # TODO: Move _cache_mutex to the object it actually protects. self._cache_mutex = threading.Lock() # Protects get() cache param. self._refresh_mutex = threading.Lock() # Protects _headers and _expires. @@ -247,24 +172,8 @@ def _refresh_token(self): if not self._refresh_mutex.locked(): msg = "Lock must be held before calling." raise OAuthTokenRefreshError(msg) - - if self._authentication_provider == Extension.Provider.SPOTIFY_DIRECT: - if self._auth: - session = authenticate.SpotifyDirectAuthentication( - client_id=self._auth[0], - client_secret=self._auth[1], - refresh_url=self._refresh_url, - credentials=self._credentials, - cache_credentials_path=self._cache_credentials_path, - ) - result = session.fetch_token() - else: - result = None - else: - data = {"grant_type": "client_credentials"} - result = self._request_with_retries( - "POST", self._refresh_url, auth=self._auth, data=data - ) + + result, status_code = self._authentication.fetch_token() if self._authentication else (None, None) if result is None: msg = "Unknown error." @@ -278,9 +187,10 @@ def _refresh_token(self): if result.get("token_type") != "Bearer": msg = f"wrong token_type: {result.get('token_type')}" raise OAuthTokenRefreshError(msg) + if status_code is not None: + self._check_unauthorized_status_code(status_code) self._access_token = result["access_token"] - self._credentials = result self._headers["Authorization"] = f"Bearer {self._access_token}" self._expires = time.time() + result.get("expires_in", float("Inf")) @@ -293,7 +203,10 @@ def _refresh_token(self): def _request_with_retries(self, method, url, *args, **kwargs): result, status_code = self._request.execute(method, url, *args, **kwargs) + self._check_unauthorized_status_code(status_code) + return result + def _check_unauthorized_status_code(self, status_code): if status_code == HTTPStatus.UNAUTHORIZED: self._authorization_failed = True logger.error( @@ -302,7 +215,6 @@ def _request_with_retries(self, method, url, *args, **kwargs): "https://www.mopidy.com/authenticate and/or restart " "Mopidy to resolve this problem." ) - return result def _normalise_query_string(self, url, params=None): u = urllib.parse.urlsplit(url) @@ -464,6 +376,99 @@ def increase_expiry(self, delta_seconds): self._expires += delta_seconds +class WebRequest: + def __init__( + self, + session, + base_url, + timeout=10, + retries=3, + retry_statuses=(500, 502, 503, 429), + backoff_factor=0.5, + ): + self._session = session + self._base_url = base_url + self._timeout = timeout + self._number_of_retries = retries + self._retry_statuses = retry_statuses + self._backoff_factor = backoff_factor + + def execute( + self, method, url, *args, **kwargs + ) -> tuple[WebResponse | None, int | None]: + prepared_request = self._session.prepare_request( + requests.Request( + method, utils.prepare_url(self._base_url, url, *args), **kwargs + ) + ) + + try_until = time.time() + self._timeout + + status_code = None + result = None + backoff_time = 0 + + for i in range(self._number_of_retries): + remaining_timeout = max(try_until - time.time(), 1) + + # Give up if we don't have any timeout left after sleeping. + if backoff_time > remaining_timeout: + break + if backoff_time > 0: + time.sleep(backoff_time) + + try: + response = self._session.send( + prepared_request, timeout=remaining_timeout + ) + except requests.RequestException as e: + logger.debug(f"Fetching {prepared_request.url} failed: {e}") + status_code = None + backoff_time = 0 + result = None + else: + status_code = response.status_code + backoff_time = self._parse_retry_after(response) + result = WebResponse.from_requests(prepared_request, response) + + if status_code and 400 <= status_code < 600: # noqa: PLR2004 + logger.debug(f"Fetching {prepared_request.url} failed: {status_code}") + + # Filter out cases where we should not retry. + if status_code and status_code not in self._retry_statuses: + break + + # TODO: Provider might return invalid JSON for "OK" responses. + # This should really not happen, so ignoring for the purpose of + # retries. It would be easier if they correctly used 204, but + # instead some endpoints return 200 with no content, or true/false. + + # Decide how long to sleep in the next iteration. + backoff_time = backoff_time or (2**i * self._backoff_factor) + logger.error( + f"Retrying {prepared_request.url} in {backoff_time:.3f} seconds." + ) + + return result, status_code + + def _parse_retry_after(self, response): + """Parse Retry-After header from response if it is set.""" + value = response.headers.get("Retry-After") + + if not value: + seconds = 0 + elif re.match(r"^\s*[0-9]+\s*$", value): + seconds = int(value) + else: + now = datetime.now(tz=UTC).replace(tzinfo=None) + try: + date_tuple = parsedate_to_datetime(value) + seconds = (date_tuple - now).total_seconds() + except ValueError: + seconds = 0 + return max(0, seconds) + + @unique class LinkType(StrEnum): TRACK = auto() diff --git a/tests/test_authenticate.py b/tests/test_authenticate.py index 73b396c0..9c0de19e 100644 --- a/tests/test_authenticate.py +++ b/tests/test_authenticate.py @@ -11,32 +11,58 @@ mock_new_token = secrets.token_urlsafe(16) -def test_fetch_token_refreshes_and_caches(tmp_path, monkeypatch): +def test_fetch_from_disk_token_refreshes_and_caches(tmp_path, monkeypatch): cache_path = tmp_path / "token.json" + cache_path.write_text(json.dumps({"refresh_token": mock_old_token})) + auth = SpotifyDirectAuthentication( client_id="id", client_secret=mock_secret, refresh_url="https://example.com/redirect", - credentials={"refresh_token": mock_old_token}, cache_credentials_path=str(cache_path), ) - def fake_token_request(self, payload: dict[str, str]) -> dict[str, str]: + def fake_token_request(self, payload: dict[str, str]) -> tuple[dict[str, str], int]: assert payload["grant_type"] == "refresh_token" assert payload["refresh_token"] == mock_old_token - return {"access_token": mock_new_token} + return {"access_token": mock_new_token}, 200 monkeypatch.setattr( SpotifyDirectAuthentication, "_spotify_token_request", fake_token_request ) - token = auth.fetch_token() + token, status_code = auth.fetch_token() + assert token is not None assert token["access_token"] == mock_new_token assert token["refresh_token"] == mock_old_token + assert status_code == 200 assert json.loads(cache_path.read_text()) == token +def test_fetch_from_memory_token_refreshes_and_caches(tmp_path, monkeypatch): + auth = SpotifyDirectAuthentication( + client_id="id", + client_secret=mock_secret, + refresh_url="https://example.com/redirect", + ) + auth._credentials = {"refresh_token": mock_old_token} + + def fake_token_request(self, payload: dict[str, str]) -> tuple[dict[str, str], int]: + assert payload["grant_type"] == "refresh_token" + assert payload["refresh_token"] == mock_old_token + return {"access_token": mock_new_token}, 200 + + monkeypatch.setattr( + SpotifyDirectAuthentication, "_spotify_token_request", fake_token_request + ) + + token, status_code = auth.fetch_token() + + assert token is not None + assert token["access_token"] == mock_new_token + assert token["refresh_token"] == mock_old_token + assert status_code == 200 def test_authenticate_flow_saves_token(tmp_path, monkeypatch): cache_path = tmp_path / "token.json" @@ -53,10 +79,10 @@ def test_authenticate_flow_saves_token(tmp_path, monkeypatch): lambda prompt="": "https://example.com/callback?code=code123&state=fixedstate", ) - def fake_token_request(self, payload: dict[str, str]) -> dict[str, str]: + def fake_token_request(self, payload: dict[str, str]) -> tuple[dict[str, str], int]: assert payload["grant_type"] == "authorization_code" assert payload["code"] == "code123" - return {"access_token": "tok", "refresh_token": "rtok"} + return {"access_token": "tok", "refresh_token": "rtok"}, 200 monkeypatch.setattr( SpotifyDirectAuthentication, "_spotify_token_request", fake_token_request @@ -69,8 +95,9 @@ def fake_token_request(self, payload: dict[str, str]) -> dict[str, str]: cache_credentials_path=str(cache_path), ) - token = auth.fetch_token() + token, status_code = auth.fetch_token() + assert status_code == 200 assert token == {"access_token": "tok", "refresh_token": "rtok"} assert json.loads(cache_path.read_text()) == token