Merge pull request #942 from HellRa1SeR/fix-spotify-oauth

Fix spotify oauth: credential normalization

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}",

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")

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()

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

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}")