From a7b25d0f6ed2fe5b8a4826e443166c0d9ac461b4 Mon Sep 17 00:00:00 2001 From: Tony Jackson Date: Wed, 13 Jan 2021 02:34:53 -0800 Subject: [PATCH] Add deprecation warnings to direct users towards using cache_handler (#630) * Refactor functions into static methods of AuthBase Functions is_token_expired was a loose function that was added to SpotifyClientCredentials, SpotifyPKCE, and SpotifyImplicitGrant classes through a method on each class that passed the call to the loose is_token_expired function. Function _is_scope_subset was duplicated on SpotifyClientCredentials, SpotifyPKCE, and SpotifyImplicitGrant classes. Refactoring is_token_expired and _is_scope_subset to be static methods on SpotifyAuthBase means both are available for all derived classes and require less boilerplate. * Create CacheHandler to abstract caching tokens Previous code only supported caching to and from json files in a given directory. In addition, the get_cached_token method mixed getting and getting the token in the same method. This change creates a CacheHandler class to abstract out the caching implementation and allow the user to cache tokens in any way they see fit. For example, the user could create a MongoCache class to store and retrieve tokens from a Mongo database and specify that cache_handler=MongoCache in creating an auth_manager object. To implement the CacheHandler abstraction, the following changes are implemented: The validation code in each get_cached_token method in SpotifyOAuth, SpotifyPKCE, and SpotifyImplicitGrant is moved into a validate_token method in each class. The CacheHandler class is created with get_cached_token and save_token_to_cache methods. Previous instances of self.get_cached_token() are now replaced with self.validate_token(self.cache_handler.get_cached_token()) to preserve the getting and validation behaviour. cache_handler is added as an argument to SpotifyOAuth, SpotifyPKCE, and SpotifyImplicitGrant. Specifying a cache_handler now overrides any specification of cache_path and/or username. To preserve backwards compatibility in handling cache files, a CacheFileHandler class extending CacheHandler is created. If no cache_handler is specified, the cache_path and username arguments are used to create an instance of CacheFileHandler. It may be worth deprecating the cache_path and username fields in favour of using CacheFileHandler. Tests are also modified and extended to cover the new functionality. A sample MemoryCache CacheHandler is created to test getting and saving to a custom CacheHandler. * Fix cache_handler subclass check for Python 2 * Split assert message to fix line over max length * Split cache handlers into cache_handler.py * flake8 and autopep fixes * Fix init to allow importing CacheHandler When spotipy is installed as a package, CacheHandler is not accessible from a `from spotipy import CacheHandler` statement because the import is not specified in the __init__.py file. This commit adds CacheHandler and CacheFileHandler to the init file so the user can import them. * flake8 fix * Add cache_path & username deprecation warning When cache_path or username are specified in the constructors of SpotifyOAuth, SpotifyPKCE, or SpotifyImplicitGrant, the constructor creates a CacheFileHandler instance under the hood. The user is currently able to create a CacheFileHandler instance in two ways: 1. By creating it outside the SpotifyOAuth (or etc.) constructor and passing it as the cache_handler 2. By passing cache_path and username to the constructor Ideally, there would be one and only one obvious way to specify a CacheFileHandler instance and the cache_handler approach allows any CacheHandler to be used, so passing the cache_path or username to the constructor should be deprecated. * Update flask example to use CacheFileHandler * Update changelog with deprecation warning info * Restore token caching methods on auth_manager Change 9550c8fd86314a15d693a3389b68468b17813156 in https://github.com/plamere/spotipy accidentally broke the caching functionality in SpotifyOAUth, SpotifyPKCE, and SpotifyImplicitGrant by removing the get_cached_token and _save_token_info methods from the auth_manager object. Users with existing codebases that use the get_cached_token and _save_token_info methods directly will experience errors if they upgrade spotipy. This commit restores the get_cached_token and _save_token_info methods on the three auth_manager classes as aliases for the corresponding methods in the cache_handler. Deprecation warnings are also added to the get_cached_token and _save_token_info methods to direct users to switch to using the new cache_handler approach. * Add deprecation warning to docstrings * Rearrange depr. warn for cache_path & username Rearrange logic so that deprecation warning always triggers if cache_path or username are specified. Previously, if cache_handler was specified, no deprecation warning would be raised if cache_path or username were specified. In addition, if both cache_handler and cache_path or username are specified, a new warning will be raised alongside the deprecation warning to let the user know that the cache_path and username fields will be ignored in favour of the cache_handler. --- CHANGELOG.md | 1 + examples/app.py | 20 +++--- spotipy/oauth2.py | 130 ++++++++++++++++++++++++++++++++++++--- tests/unit/test_oauth.py | 51 +++++++++++++++ 4 files changed, 185 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01df2f6..2ebf989 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The docs for the `auth` parameter of `Spotify.init` use the term "access token" instead of "authorization token" - Changed docs for `search` to mention that you can provide multiple types to search for - The query parameters of requests are now logged +- Deprecate specifing `cache_path` or `username` directly to `SpotifyOAuth`, `SpotifyPKCE`, and `SpotifyImplicitGrant` constructors, instead directing users to use the `CacheFileHandler` cache handler ### Added diff --git a/examples/app.py b/examples/app.py index 188b4eb..727dfba 100644 --- a/examples/app.py +++ b/examples/app.py @@ -48,8 +48,9 @@ def index(): # Step 1. Visitor is unknown, give random ID session['uuid'] = str(uuid.uuid4()) + cache_handler = spotipy.cache_handler.CacheFileHandler(cache_path=session_cache_path()) auth_manager = spotipy.oauth2.SpotifyOAuth(scope='user-read-currently-playing playlist-modify-private', - cache_path=session_cache_path(), + cache_handler=cache_handler, show_dialog=True) if request.args.get("code"): @@ -57,7 +58,7 @@ def index(): auth_manager.get_access_token(request.args.get("code")) return redirect('/') - if not auth_manager.get_cached_token(): + if not auth_manager.validate_token(cache_handler.get_cached_token()): # Step 2. Display sign in link when no token auth_url = auth_manager.get_authorize_url() return f'

Sign in

' @@ -84,8 +85,9 @@ def sign_out(): @app.route('/playlists') def playlists(): - auth_manager = spotipy.oauth2.SpotifyOAuth(cache_path=session_cache_path()) - if not auth_manager.get_cached_token(): + cache_handler = spotipy.cache_handler.CacheFileHandler(cache_path=session_cache_path()) + auth_manager = spotipy.oauth2.SpotifyOAuth(cache_handler=cache_handler) + if not auth_manager.validate_token(cache_handler.get_cached_token()): return redirect('/') spotify = spotipy.Spotify(auth_manager=auth_manager) @@ -94,8 +96,9 @@ def playlists(): @app.route('/currently_playing') def currently_playing(): - auth_manager = spotipy.oauth2.SpotifyOAuth(cache_path=session_cache_path()) - if not auth_manager.get_cached_token(): + cache_handler = spotipy.cache_handler.CacheFileHandler(cache_path=session_cache_path()) + auth_manager = spotipy.oauth2.SpotifyOAuth(cache_handler=cache_handler) + if not auth_manager.validate_token(cache_handler.get_cached_token()): return redirect('/') spotify = spotipy.Spotify(auth_manager=auth_manager) track = spotify.current_user_playing_track() @@ -106,8 +109,9 @@ def currently_playing(): @app.route('/current_user') def current_user(): - auth_manager = spotipy.oauth2.SpotifyOAuth(cache_path=session_cache_path()) - if not auth_manager.get_cached_token(): + cache_handler = spotipy.cache_handler.CacheFileHandler(cache_path=session_cache_path()) + auth_manager = spotipy.oauth2.SpotifyOAuth(cache_handler=cache_handler) + if not auth_manager.validate_token(cache_handler.get_cached_token()): return redirect('/') spotify = spotipy.Spotify(auth_manager=auth_manager) return spotify.current_user() diff --git a/spotipy/oauth2.py b/spotipy/oauth2.py index 8c17636..34403b3 100644 --- a/spotipy/oauth2.py +++ b/spotipy/oauth2.py @@ -260,9 +260,9 @@ class SpotifyOAuth(SpotifyAuthBase): getting and saving cached authorization tokens. May be supplied, will otherwise use `CacheFileHandler`. (takes precedence over `cache_path` and `username`) - * cache_path: May be supplied, will otherwise be generated + * cache_path: (deprecated) May be supplied, will otherwise be generated (takes precedence over `username`) - * username: May be supplied or set as environment variable + * username: (deprecated) May be supplied or set as environment variable (will set `cache_path` to `.cache-{username}`) * proxies: Proxy for the requests library to route through * show_dialog: Interpreted as boolean @@ -278,14 +278,32 @@ class SpotifyOAuth(SpotifyAuthBase): self.redirect_uri = redirect_uri self.state = state self.scope = self._normalize_scope(scope) + username = (username or os.getenv(CLIENT_CREDS_ENV_VARS["client_username"])) + if username or cache_path: + warnings.warn("Specifying cache_path or username as arguments to SpotifyOAuth " + + "will be deprecated. Instead, please create a CacheFileHandler " + + "instance with the desired cache_path and username and pass it " + + "to SpotifyOAuth as the cache_handler. For example:\n\n" + + "\tfrom spotipy.oauth2 import CacheFileHandler\n" + + "\thandler = CacheFileHandler(cache_path=cache_path, " + + "username=username)\n" + + "\tsp = spotipy.SpotifyOAuth(client_id, client_secret, " + + "redirect_uri," + + " cache_handler=handler)", + DeprecationWarning + ) + if cache_handler: + warnings.warn("A cache_handler has been specified along with a cache_path or " + + "username. The cache_path and username arguments will be ignored.") if cache_handler: assert issubclass(cache_handler.__class__, CacheHandler), \ "cache_handler must be a subclass of CacheHandler: " + str(type(cache_handler)) \ + " != " + str(CacheHandler) self.cache_handler = cache_handler else: + self.cache_handler = CacheFileHandler( - username=(username or os.getenv(CLIENT_CREDS_ENV_VARS["client_username"])), + username=username, cache_path=cache_path ) self.proxies = proxies @@ -554,6 +572,26 @@ class SpotifyOAuth(SpotifyAuthBase): token_info["scope"] = self.scope return token_info + def get_cached_token(self): + warnings.warn("Calling get_cached_token directly on the SpotifyOAuth object will be " + + "deprecated. Instead, please specify a CacheFileHandler instance as " + + "the cache_handler in SpotifyOAuth and use the CacheFileHandler's " + + "get_cached_token method. You can replace:\n\tsp.get_cached_token()" + + "\n\nWith:\n\tsp.validate_token(sp.cache_handler.get_cached_token())", + DeprecationWarning + ) + return self.validate_token(self.cache_handler.get_cached_token()) + + def _save_token_info(self, token_info): + warnings.warn("Calling _save_token_info directly on the SpotifyOAuth object will be " + + "deprecated. Instead, please specify a CacheFileHandler instance as " + + "the cache_handler in SpotifyOAuth and use the CacheFileHandler's " + + "save_token_to_cache method.", + DeprecationWarning + ) + self.cache_handler.save_token_to_cache(token_info) + return None + class SpotifyPKCE(SpotifyAuthBase): """ Implements PKCE Authorization Flow for client apps @@ -595,9 +633,9 @@ class SpotifyPKCE(SpotifyAuthBase): getting and saving cached authorization tokens. May be supplied, will otherwise use `CacheFileHandler`. (takes precedence over `cache_path` and `username`) - * cache_path: May be supplied, will otherwise be generated + * cache_path: (deprecated) May be supplied, will otherwise be generated (takes precedence over `username`) - * username: May be supplied or set as environment variable + * username: (deprecated) May be supplied or set as environment variable (will set `cache_path` to `.cache-{username}`) * show_dialog: Interpreted as boolean * proxies: Proxy for the requests library to route through @@ -611,13 +649,29 @@ class SpotifyPKCE(SpotifyAuthBase): self.redirect_uri = redirect_uri self.state = state self.scope = self._normalize_scope(scope) + username = (username or os.getenv(CLIENT_CREDS_ENV_VARS["client_username"])) + if username or cache_path: + warnings.warn("Specifying cache_path or username as arguments to SpotifyPKCE " + + "will be deprecated. Instead, please create a CacheFileHandler " + + "instance with the desired cache_path and username and pass it " + + "to SpotifyPKCE as the cache_handler. For example:\n\n" + + "\tfrom spotipy.oauth2 import CacheFileHandler\n" + + "\thandler = CacheFileHandler(cache_path=cache_path, " + + "username=username)\n" + + "\tsp = spotipy.SpotifyImplicitGrant(client_id, client_secret, " + + "redirect_uri, cache_handler=handler)", + DeprecationWarning + ) + if cache_handler: + warnings.warn("A cache_handler has been specified along with a cache_path or " + + "username. The cache_path and username arguments will be ignored.") if cache_handler: assert issubclass(type(cache_handler), CacheHandler), \ "type(cache_handler): " + str(type(cache_handler)) + " != " + str(CacheHandler) self.cache_handler = cache_handler else: self.cache_handler = CacheFileHandler( - username=(username or os.getenv(CLIENT_CREDS_ENV_VARS["client_username"])), + username=username, cache_path=cache_path ) self.proxies = proxies @@ -912,6 +966,26 @@ class SpotifyPKCE(SpotifyAuthBase): def parse_auth_response_url(url): return SpotifyOAuth.parse_auth_response_url(url) + def get_cached_token(self): + warnings.warn("Calling get_cached_token directly on the SpotifyPKCE object will be " + + "deprecated. Instead, please specify a CacheFileHandler instance as " + + "the cache_handler in SpotifyOAuth and use the CacheFileHandler's " + + "get_cached_token method. You can replace:\n\tsp.get_cached_token()" + + "\n\nWith:\n\tsp.validate_token(sp.cache_handler.get_cached_token())", + DeprecationWarning + ) + return self.validate_token(self.cache_handler.get_cached_token()) + + def _save_token_info(self, token_info): + warnings.warn("Calling _save_token_info directly on the SpotifyOAuth object will be " + + "deprecated. Instead, please specify a CacheFileHandler instance as " + + "the cache_handler in SpotifyOAuth and use the CacheFileHandler's " + + "save_token_to_cache method.", + DeprecationWarning + ) + self.cache_handler.save_token_to_cache(token_info) + return None + class SpotifyImplicitGrant(SpotifyAuthBase): """ Implements Implicit Grant Flow for client apps @@ -971,9 +1045,9 @@ class SpotifyImplicitGrant(SpotifyAuthBase): getting and saving cached authorization tokens. May be supplied, will otherwise use `CacheFileHandler`. (takes precedence over `cache_path` and `username`) - * cache_path: May be supplied, will otherwise be generated + * cache_path: (deprecated) May be supplied, will otherwise be generated (takes precedence over `username`) - * username: May be supplied or set as environment variable + * username: (deprecated) May be supplied or set as environment variable (will set `cache_path` to `.cache-{username}`) * show_dialog: Interpreted as boolean """ @@ -986,13 +1060,30 @@ class SpotifyImplicitGrant(SpotifyAuthBase): self.client_id = client_id self.redirect_uri = redirect_uri self.state = state + username = (username or os.getenv(CLIENT_CREDS_ENV_VARS["client_username"])) + if username or cache_path: + warnings.warn("Specifying cache_path or username as arguments to " + + "SpotifyImplicitGrant will be deprecated. Instead, please create " + + "a CacheFileHandler instance with the desired cache_path and " + + "username and pass it to SpotifyImplicitGrant as the " + + "cache_handler. For example:\n\n" + + "\tfrom spotipy.oauth2 import CacheFileHandler\n" + + "\thandler = CacheFileHandler(cache_path=cache_path, " + + "username=username)\n" + + "\tsp = spotipy.SpotifyImplicitGrant(client_id, client_secret, " + + "redirect_uri, cache_handler=handler)", + DeprecationWarning + ) + if cache_handler: + warnings.warn("A cache_handler has been specified along with a cache_path or " + + "username. The cache_path and username arguments will be ignored.") if cache_handler: assert issubclass(type(cache_handler), CacheHandler), \ "type(cache_handler): " + str(type(cache_handler)) + " != " + str(CacheHandler) self.cache_handler = cache_handler else: self.cache_handler = CacheFileHandler( - username=(username or os.getenv(CLIENT_CREDS_ENV_VARS["client_username"])), + username=username, cache_path=cache_path ) self.scope = self._normalize_scope(scope) @@ -1139,6 +1230,27 @@ class SpotifyImplicitGrant(SpotifyAuthBase): token_info["scope"] = self.scope return token_info + def get_cached_token(self): + warnings.warn("Calling get_cached_token directly on the SpotifyImplicitGrant " + + "object will be deprecated. Instead, please specify a " + + "CacheFileHandler instance as the cache_handler in SpotifyOAuth " + + "and use the CacheFileHandler's get_cached_token method. " + + "You can replace:\n\tsp.get_cached_token()" + + "\n\nWith:\n\tsp.validate_token(sp.cache_handler.get_cached_token())", + DeprecationWarning + ) + return self.validate_token(self.cache_handler.get_cached_token()) + + def _save_token_info(self, token_info): + warnings.warn("Calling _save_token_info directly on the SpotifyImplicitGrant " + + "object will be deprecated. Instead, please specify a " + + "CacheFileHandler instance as the cache_handler in SpotifyOAuth " + + "and use the CacheFileHandler's save_token_to_cache method.", + DeprecationWarning + ) + self.cache_handler.save_token_to_cache(token_info) + return None + class RequestHandler(BaseHTTPRequestHandler): def do_GET(self): diff --git a/tests/unit/test_oauth.py b/tests/unit/test_oauth.py index 4fea792..9acdcc9 100644 --- a/tests/unit/test_oauth.py +++ b/tests/unit/test_oauth.py @@ -79,9 +79,11 @@ class OAuthCacheTest(unittest.TestCase): spot = _make_oauth(scope, path) cached_tok = spot.validate_token(spot.cache_handler.get_cached_token()) + cached_tok_legacy = spot.get_cached_token() opener.assert_called_with(path) self.assertIsNotNone(cached_tok) + self.assertIsNotNone(cached_tok_legacy) self.assertEqual(refresh_access_token.call_count, 0) @patch.multiple(SpotifyOAuth, @@ -140,6 +142,21 @@ class OAuthCacheTest(unittest.TestCase): opener.assert_called_with(path, 'w') self.assertTrue(fi.write.called) + @patch('spotipy.cache_handler.open', create=True) + def test_saves_to_cache_path_legacy(self, opener): + scope = "playlist-modify-private" + path = ".cache-username" + tok = _make_fake_token(1, 1, scope) + + fi = _fake_file() + opener.return_value = fi + + spot = SpotifyOAuth("CLID", "CLISEC", "REDIR", "STATE", scope, path) + spot._save_token_info(tok) + + opener.assert_called_with(path, 'w') + self.assertTrue(fi.write.called) + def test_cache_handler(self): scope = "playlist-modify-private" tok = _make_fake_token(1, 1, scope) @@ -258,9 +275,11 @@ class ImplicitGrantCacheTest(unittest.TestCase): spot = _make_implicitgrantauth(scope, path) cached_tok = spot.cache_handler.get_cached_token() + cached_tok_legacy = spot.get_cached_token() opener.assert_called_with(path) self.assertIsNotNone(cached_tok) + self.assertIsNotNone(cached_tok_legacy) @patch.object(SpotifyImplicitGrant, "is_token_expired", DEFAULT) @patch('spotipy.cache_handler.open', create=True) @@ -311,6 +330,21 @@ class ImplicitGrantCacheTest(unittest.TestCase): opener.assert_called_with(path, 'w') self.assertTrue(fi.write.called) + @patch('spotipy.cache_handler.open', create=True) + def test_saves_to_cache_path_legacy(self, opener): + scope = "playlist-modify-private" + path = ".cache-username" + tok = _make_fake_token(1, 1, scope) + + fi = _fake_file() + opener.return_value = fi + + spot = SpotifyImplicitGrant("CLID", "REDIR", "STATE", scope, path) + spot._save_token_info(tok) + + opener.assert_called_with(path, 'w') + self.assertTrue(fi.write.called) + class TestSpotifyImplicitGrant(unittest.TestCase): @@ -378,9 +412,11 @@ class SpotifyPKCECacheTest(unittest.TestCase): spot = _make_pkceauth(scope, path) cached_tok = spot.cache_handler.get_cached_token() + cached_tok_legacy = spot.get_cached_token() opener.assert_called_with(path) self.assertIsNotNone(cached_tok) + self.assertIsNotNone(cached_tok_legacy) self.assertEqual(refresh_access_token.call_count, 0) @patch.multiple(SpotifyPKCE, @@ -439,6 +475,21 @@ class SpotifyPKCECacheTest(unittest.TestCase): opener.assert_called_with(path, 'w') self.assertTrue(fi.write.called) + @patch('spotipy.cache_handler.open', create=True) + def test_saves_to_cache_path_legacy(self, opener): + scope = "playlist-modify-private" + path = ".cache-username" + tok = _make_fake_token(1, 1, scope) + + fi = _fake_file() + opener.return_value = fi + + spot = SpotifyPKCE("CLID", "REDIR", "STATE", scope, path) + spot._save_token_info(tok) + + opener.assert_called_with(path, 'w') + self.assertTrue(fi.write.called) + class TestSpotifyPKCE(unittest.TestCase):