diff --git a/core/metadata/registry.py b/core/metadata/registry.py index a6c6ea39..946d84a3 100644 --- a/core/metadata/registry.py +++ b/core/metadata/registry.py @@ -230,14 +230,19 @@ def get_spotify_client_for_profile(profile_id: Optional[int] = None): return client try: - from core.spotify_client import SpotifyClient + from core.spotify_client import SpotifyClient, normalize_spotify_oauth_config from spotipy.oauth2 import SpotifyOAuth import spotipy + normalized_creds = normalize_spotify_oauth_config({ + "client_id": client_id, + "client_secret": client_secret, + "redirect_uri": redirect_uri, + }) auth_manager = SpotifyOAuth( - client_id=client_id, - client_secret=client_secret, - redirect_uri=redirect_uri, + client_id=normalized_creds.get("client_id", client_id), + client_secret=normalized_creds.get("client_secret", client_secret), + redirect_uri=normalized_creds.get("redirect_uri", redirect_uri), scope="user-library-read user-read-private playlist-read-private playlist-read-collaborative user-read-email user-follow-read", cache_path=cache_path, state=f"profile_{profile_id}", diff --git a/core/spotify_client.py b/core/spotify_client.py index b5318037..cf3859ca 100644 --- a/core/spotify_client.py +++ b/core/spotify_client.py @@ -13,6 +13,30 @@ from core.metadata.cache import get_metadata_cache logger = get_logger("spotify_client") + +def normalize_spotify_oauth_config(config: Optional[Dict[str, Any]]) -> Dict[str, Any]: + """Normalize Spotify OAuth config before building an auth manager. + + Spotify rejects values that include surrounding whitespace, quotes, or + newline characters. The settings UI can paste values that carry such + formatting, so we trim them before sending them to Spotify. + """ + if not isinstance(config, dict): + return {} + + normalized = {} + for key in ("client_id", "client_secret", "redirect_uri"): + value = config.get(key, "") + if isinstance(value, str): + value = value.strip().strip('"').strip("'") + if key == "redirect_uri": + value = value.rstrip("/") + normalized[key] = value + else: + normalized[key] = value + return normalized + + def _upgrade_spotify_image_url(url: str) -> str: """Upgrade a Spotify CDN image URL to the highest available resolution. @@ -697,7 +721,7 @@ class SpotifyClient: self._setup_client() def _setup_client(self): - config = config_manager.get_spotify_config() + config = normalize_spotify_oauth_config(config_manager.get_spotify_config()) if not config.get('client_id') or not config.get('client_secret'): logger.warning("Spotify credentials not configured") diff --git a/tests/metadata/test_spotify_oauth_integration.py b/tests/metadata/test_spotify_oauth_integration.py new file mode 100644 index 00000000..4bc809cd --- /dev/null +++ b/tests/metadata/test_spotify_oauth_integration.py @@ -0,0 +1,86 @@ +import unittest +from unittest.mock import patch, MagicMock +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from core.metadata.registry import get_spotify_client_for_profile + +# Mock config_manager as it's a global dependency +class MockConfigManager: + def __init__(self): + self.store = {} + + def get(self, key, default=None): + return self.store.get(key, default) + + def get_spotify_config(self): + return self.store.get('spotify', {}) + + def set(self, key, value): + self.store[key] = value + +class TestSpotifyOAuthIntegration(unittest.TestCase): + @patch('core.metadata.registry.get_spotify_client') + @patch('core.metadata.registry._profile_spotify_credentials_provider') + @patch('core.metadata.registry._get_config_value') + @patch('spotipy.oauth2.SpotifyOAuth') + @patch('core.spotify_client.normalize_spotify_oauth_config') + def test_get_spotify_client_for_profile_uses_normalized_config(self, mock_normalize, mock_spotify_oauth, mock_get_config, mock_creds_provider, mock_get_global): + # Set up mock config values + mock_creds_provider.return_value = { + "client_id": " original_client_id ", + "client_secret": " original_client_secret ", + "redirect_uri": "http://example.com/callback/" + } + + # Make sure the file exists check passes + with patch('os.path.exists', return_value=True): + # Set up mock for normalize_spotify_oauth_config to return cleaned values + mock_normalize.return_value = { + "client_id": "cleaned_client_id", + "client_secret": "cleaned_client_secret", + "redirect_uri": "http://example.com/callback" + } + + # Call the function under test with profile_id=2 (to bypass global client) + get_spotify_client_for_profile(profile_id=2) + + # Assert that normalize_spotify_oauth_config was called with the original config + mock_normalize.assert_any_call({ + "client_id": " original_client_id ", + "client_secret": " original_client_secret ", + "redirect_uri": "http://example.com/callback/" + }) + + # Assert that SpotifyOAuth was initialized with the normalized config + mock_spotify_oauth.assert_called_once() + args, kwargs = mock_spotify_oauth.call_args + self.assertEqual(kwargs['client_id'], "cleaned_client_id") + self.assertEqual(kwargs['client_secret'], "cleaned_client_secret") + self.assertEqual(kwargs['redirect_uri'], "http://example.com/callback") + self.assertEqual(kwargs['state'], 'profile_2') + + @patch('core.metadata.registry.get_spotify_client') + @patch('core.metadata.registry._profile_spotify_credentials_provider') + @patch('core.metadata.registry._get_config_value') + @patch('spotipy.oauth2.SpotifyOAuth') + @patch('core.spotify_client.normalize_spotify_oauth_config') + def test_get_spotify_client_for_profile_handles_no_config(self, mock_normalize, mock_spotify_oauth, mock_get_config, mock_creds_provider, mock_get_global): + # Simulate no spotify config + mock_creds_provider.return_value = {} + mock_get_config.side_effect = lambda key, default: "" if "client" in key else "http://127.0.0.1:8888/callback" + + # Ensure os.path.exists returns False so it doesn't try to use cache + with patch('os.path.exists', return_value=False): + # Call the function under test + get_spotify_client_for_profile(profile_id=2) + + # It still reaches get_spotify_client() which is mocked. + # In registry.py, the fallbacks for client_id/client_secret result in calls to get_spotify_client(). + mock_get_global.assert_called() + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/test_spotify_client.py b/tests/test_spotify_client.py new file mode 100644 index 00000000..cf9210ee --- /dev/null +++ b/tests/test_spotify_client.py @@ -0,0 +1,76 @@ +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from core.spotify_client import normalize_spotify_oauth_config + +def test_normalization(): + # Normal case with leading/trailing whitespace and quotes + config = { + "client_id": " client_id ", + "client_secret": " client_secret ", + "redirect_uri": "http://127.0.0.1:8888/callback/" + } + expected = { + "client_id": "client_id", + "client_secret": "client_secret", + "redirect_uri": "http://127.0.0.1:8888/callback" + } + assert normalize_spotify_oauth_config(config) == expected + +def test_empty_values(): + # Empty input values + config = { + "client_id": "", + "client_secret": None, + "redirect_uri": "" + } + # When value is None, it falls into the else branch: normalized[key] = value + # value is None, so expected is None for client_secret + expected = { + "client_id": "", + "client_secret": None, + "redirect_uri": "" + } + assert normalize_spotify_oauth_config(config) == expected + +def test_missing_keys(): + # Input dictionary with missing keys + config = { + "client_id": "client_id" + } + # .get(key, "") means missing keys become "" + expected = { + "client_id": "client_id", + "client_secret": "", + "redirect_uri": "" + } + assert normalize_spotify_oauth_config(config) == expected + +def test_non_string_values(): + # Input dictionary with non-string values for the keys + config = { + "client_id": 123, + "client_secret": True, + "redirect_uri": None + } + # When value is not a string, it falls into the else branch: normalized[key] = value + expected = { + "client_id": 123, + "client_secret": True, + "redirect_uri": None + } + assert normalize_spotify_oauth_config(config) == expected + +def test_no_input(): + # Empty input dictionary + config = {} + # .get(key, "") means missing keys become "" + expected = { + "client_id": "", + "client_secret": "", + "redirect_uri": "" + } + assert normalize_spotify_oauth_config(None) == {} + assert normalize_spotify_oauth_config(config) == expected \ No newline at end of file diff --git a/web_server.py b/web_server.py index 4db814c2..acdce88e 100644 --- a/web_server.py +++ b/web_server.py @@ -4692,11 +4692,13 @@ def _profile_spotify_oauth(profile_id_int): chooser so a user can't silently inherit whatever Spotify session is active in their browser (e.g. the admin's). Returns None if no app creds exist.""" from spotipy.oauth2 import SpotifyOAuth + from core.spotify_client import normalize_spotify_oauth_config creds = (get_database().get_profile_spotify(profile_id_int) or {}) - cfg = config_manager.get_spotify_config() - client_id = creds.get('client_id') or cfg.get('client_id') - client_secret = creds.get('client_secret') or cfg.get('client_secret') - redirect_uri = creds.get('redirect_uri') or cfg.get('redirect_uri', 'http://127.0.0.1:8888/callback') + cfg = normalize_spotify_oauth_config(config_manager.get_spotify_config()) + profile_creds = normalize_spotify_oauth_config(creds) + client_id = profile_creds.get('client_id') or cfg.get('client_id') + client_secret = profile_creds.get('client_secret') or cfg.get('client_secret') + redirect_uri = profile_creds.get('redirect_uri') or cfg.get('redirect_uri', 'http://127.0.0.1:8888/callback') if not client_id or not client_secret: return None return SpotifyOAuth( @@ -5095,7 +5097,7 @@ def spotify_callback(): pass try: - from core.spotify_client import SpotifyClient + from core.spotify_client import SpotifyClient, normalize_spotify_oauth_config from spotipy.oauth2 import SpotifyOAuth from config.settings import config_manager @@ -5125,7 +5127,7 @@ def spotify_callback(): raise Exception("Failed to exchange authorization code for access token") # Global callback (admin) - config = config_manager.get_spotify_config() + config = normalize_spotify_oauth_config(config_manager.get_spotify_config()) configured_uri = config.get('redirect_uri', "http://127.0.0.1:8888/callback") logger.info(f"Using redirect_uri for token exchange: {configured_uri}") @@ -35879,9 +35881,10 @@ def start_oauth_callback_servers(): try: from spotipy.oauth2 import SpotifyOAuth from config.settings import config_manager + from core.spotify_client import normalize_spotify_oauth_config # Get Spotify config - config = config_manager.get_spotify_config() + config = normalize_spotify_oauth_config(config_manager.get_spotify_config()) configured_uri = config.get('redirect_uri', "http://127.0.0.1:8888/callback") _oauth_logger.info(f"Using redirect_uri for token exchange: {configured_uri}")