Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-refresh AuthCode flow token. #435

Merged
merged 17 commits into from Feb 10, 2020
Merged

Auto-refresh AuthCode flow token. #435

merged 17 commits into from Feb 10, 2020

Conversation

stefanondisponibile
Copy link
Contributor

I loved @stephanebruckert 's idea of having similar handlers for the two different authorization flows.

I think it would be even better having one one unique auth_manager inside the Spotify client, and infer it from the parameters. This also would allow a smooth and backward compatible pass to this.

What do you think?

@stephanebruckert
Copy link
Member

stephanebruckert commented Feb 3, 2020

Hey, thanks for that. I will review and respond properly later, but for now to fix the lint you can just do:

pip install autopep8
autopep8 --in-place --aggressive --recursive .
pip install flake8
flake8 .

It's missing from the README

Edit: cool, you already fixed it

Copy link
Member

@stephanebruckert stephanebruckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! I'm still trying to think of a scenario where being forced to be prompted for the token could be an issue. Could it be a problem when trying to use spotipy in an API? #287 (comment). In this case we would need something like prompt=True

What do you think @ritiek?

spotipy/util.py Outdated Show resolved Hide resolved
spotipy/oauth2.py Show resolved Hide resolved
@ritiek
Copy link
Collaborator

ritiek commented Feb 4, 2020

Looks cool! I'll hopefully be able to review by evening.

Implements the capability of trying to fetch the following parameters from the environment, when they're not directly passed to the authorization handler.
The affected parameters are: client_id, client_secret, redirect_uri.
An SpotifyOauthError is raised if no value gets found.
@stefanondisponibile
Copy link
Contributor Author

This is looking great! I'm still trying to think of a scenario where being forced to be prompted for the token could be an issue. Could it be a problem when trying to use spotipy in an API? #287 (comment). In this case we would need something like prompt=True

What do you think @ritiek?

@stephanebruckert in my opinion the best way to handle the authorization flow independently is using an authorization manager independently too, and just use the Spotify client by passing the auth token, or by initializing a manager once you've retrieved it programmatically for the first time.

@stefanondisponibile
Copy link
Contributor Author

stefanondisponibile commented Feb 6, 2020

Hey @ritiek, do you have any updates on this?

@ritiek
Copy link
Collaborator

ritiek commented Feb 6, 2020

Oops, sorry. It seems like review comments need to be submitted twice?

Anyway, this looks good to me. Seems to automatically refresh token nicely as well. I've left a few trivial review comments.

spotipy/oauth2.py Outdated Show resolved Hide resolved
spotipy/oauth2.py Outdated Show resolved Hide resolved
@stefanondisponibile
Copy link
Contributor Author

Cool! I pushed the updates, can't wait to have this merged 🥳

@stephanebruckert
Copy link
Member

Hey, sorry I was ill for most of the week, unable to do much. I just had a better look at how SpotifyOAuth works, I really want to merge the PR but there is still something backward-incompatible in it.

@stefanondisponibile:

in my opinion the best way to handle the authorization flow independently is using an authorization manager independently too, and just use the Spotify client by passing the auth token, or by initializing a manager once you've retrieved it programmatically for the first time.

Please confirm if I understand correctly. You would make the previous SpotifyOAuth (no prompt) a new class, something like SpotifyOAuthNoPrompt so we can use prompt in the updated SpotifyOAuth from this PR? Unfortunately the priority is to keep the previous implementation working because some people might already be using it on a server where there is no way to "prompt a user" and that a minor update would block them.

So I see two possible solutions to fix this PR:

  • use SpotifyOAuth(prompt=True) as initially suggested,
  • add a SpotifyOAuthPrompt auth manager (not SpotifyOAuthNoPrompt)

Let me know if we are both understanding it correctly?

Thanks again for the work you put in this PR, can't wait to see it merged!

@stefanondisponibile
Copy link
Contributor Author

Hey @stephanebruckert , good to have you back, sorry you was ill!
I'm not sure I explained my point properly (I wouldn't create more classes). Thinking of passing a prompt parameter is an option, but probably I'm missing part of the problem.

For now, the main compatibility issue after the changes I made, is that SpotifyOAuth.get_access_token is now returning a str instead of a dict. This might be an issue if someone is using this method by himself.

If instead, the problem is on the prompting part, could you please try to explain that to me again?
What I don't understand is this.
SCENARIO 1:
Let's say I've made a script that does something with spotipy, let's say something like examples/simple4.py:

import spotipy
import os
from pprint import pprint


def main():
    spotify = spotipy.Spotify(auth_manager=spotipy.SpotifyOAuth())
    me = spotify.me()
    pprint(me)


if __name__ == "__main__":
    main()

The first time I run this script it will tell me to login to spotify, gave my permission for the scopes, and will send me back to a url that I have to paste in the console to get the code that is passed in the current SpotifyOAuth.get_access_token method. This is cool because I don't have to this myself on the script, the spotipy package will "drive" me through that.
The second time I run the script the token's been cached, so it will work without asking anything until the end of time, since the token gets refreshed automatically too.

** SCENARIO 2 **:
Let's say I've got the exact same script as above. But this time we're on a remote server, there's no user in front of the screen to paste things. So yeah, we don't want to prompt, but the first time I ever run the script I need to get an access token, how do I do that? I need to call SpotifyOAuth.get_access_token, but for that I need a code. How to get that code is my responsibility: I will need to send the user who's interacting with the app to the authorization url, and catch back the code myself from the redirect uri. To do that I could also think of using the builtin get_authorization_code, or just parse it myself, or whatever. When I have that code I can finally pass it to the get_access_token method and forget about it forever.

So could you explain me again the issue please?

Thank you :)

@stephanebruckert
Copy link
Member

stephanebruckert commented Feb 9, 2020

Hey @stefanondisponibile! I agree with scenario 1 👍

In the scenario 2, get_access_token is an object method, I don't think you can call it this way:

SpotifyOAuth.get_access_token()

instead it should be called on an object of that class:

oauth = spotipy.SpotifyOAuth()  # prompt here, cannot avoid it
oauth.get_access_token()

So even if you get the code independently, the prompt would open before. There must be ways to adapt the way it works, but we cannot change it because it might already be used (for example in something that would look like #287 (comment))

Do you see what I mean? Could I be missing something?

@stefanondisponibile
Copy link
Contributor Author

Sorry @stephanebruckert , but I still don't really get the problem.
Of course, get_access_token is an instance method.


oauth = spotipy.SpotifyOAuth() # prompt here, cannot avoid it

why can't avoid it? You shouldn't receive any prompts by doing this.


In fact, one should actually reflect what's in @MaZderMind comment.

Try this and see if that's the expected behaviour:

"""
Of course:
0. pip install spotipy (from this branch or from my fork)
1. pip install flask
2. Export needed env variables:
  export SPOTIPY_CLIENT_ID=yourclientid
  export SPOTIPY_CLIENT_SECRET=yourclientsecret
  export SPOTIPY_CLIENT_USERNAME=yourusername
3. Add "http://localhost:5000" to your app's redirect uris.
4. Copy-paste this into an "app.py" file.
5. Run "flask run" and go to http://localhost:5000

"""

from flask import Flask, request, jsonify
import spotipy

app = Flask(__name__)

auth_manager = spotipy.oauth2.SpotifyOAuth()
spotify = spotipy.Spotify(auth_manager=auth_manager)

@app.route("/")
def index():
    if request.args.get("code"):
        auth_manager.get_access_token(request.args["code"])
    if not auth_manager.get_cached_token():
        auth_url = auth_manager.get_authorize_url()
        return f'<h2><a href="{auth_url}">Authorize this application.</a></h2>'
    return spotify.me()

@stephanebruckert
Copy link
Member

Indeed, that's my bad! For some reason I was thinking that SpotifyOAuth() would prompt, but it doesn't which makes the whole thing look very concise now. Perfect & sorry about that!


For now, the main compatibility issue after the changes I made, is that SpotifyOAuth.get_access_token is now returning a str instead of a dict. This might be an issue if someone is using this method by himself.

About this change, I think it's a good idea initially because ClientCredentials.get_access_token is returning a string too. But it looks like the dict returned by SpotifyOAuth.get_access_token was used in some places, here and possibly in more places. Is it a lot of work to use a dict again?

Also feel free to add a changelog and we should be good to merge! Cheers

@stefanondisponibile
Copy link
Contributor Author

No worries @stephanebruckert , always better overchecking than the opposite 😉


Good catch! On other's code we might break if we return a string instead of a dictionary!
However, I like the idea of keeping ClientCredentials and OAuth manager "similar", so how about this solution instead: let's return a dictionary, just like before, but add an as_dict parameter, that's going to be deprecated in the future. With this parameter, we start using as_dict=False in spotipy's internal codebase, and we'll remove that in the future, maybe with major releases.

I'm pushing these changes, so you can have a look.

@stephanebruckert
Copy link
Member

@stefanondisponibile sounds perfect!

@stephanebruckert stephanebruckert merged commit 4515446 into spotipy-dev:master Feb 10, 2020
@stephanebruckert
Copy link
Member

Very nice, merging, thank you @stefanondisponibile

@MaZderMind
Copy link
Contributor

Thank you all for following up on this 👍
Great to see the better implementation win 👌

@louhow
Copy link

louhow commented Dec 21, 2020

Just wanted to say, I was tweaking an old project that was leveraging this client and auth_manager let me get rid of so much code. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants