mental32 / spotify.py

🌐 API wrapper for Spotify 🎶
https://spotifypy.readthedocs.io/en/latest/
MIT License
150 stars 38 forks source link

Error refreshing token #20

Closed micahdlamb closed 4 years ago

micahdlamb commented 4 years ago

I noticed there is an error in the token refreshing code.

You are using the "Authorization": "Bearer " + access_token header but spotify wants you to use the "Authorization": f"Basic {token.decode()}" like is used your HttpClient.get_bearer_info function.

I worked around the problem by overloading User._refreshing_token with this code:

    async def _refreshing_token(self, expires: int, token: str):
        while True:
            import asyncio
            await asyncio.sleep(expires-1)
            REFRESH_TOKEN_URL = "https://accounts.spotify.com/api/token?grant_type=refresh_token&refresh_token={refresh_token}"
            route = ("POST", REFRESH_TOKEN_URL.format(refresh_token=token))
            from base64 import b64encode
            auth = b64encode(":".join((os.environ['SPOTIFY_CLIENT_ID'], os.environ['SPOTIFY_CLIENT_SECRET'])).encode())
            try:
                data = await self.client.http.request(
                    route,
                    headers={"Content-Type": "application/x-www-form-urlencoded",
                             "Authorization": f"Basic {auth.decode()}"}
                )

                expires = data["expires_in"]
                self.http.token = data["access_token"]
                print('token refreshed', data["access_token"])
            except:
                import traceback
                traceback.print_exc()

Also in I believe you want to change: "Content-Type": kwargs.get("content_type", "application/json"),

to

"Content-Type": kwargs.pop("content_type", "application/json"),

in HTTPClient.request otherwise it errors when the content_type kwd is passed on.

I could do a pull request if you'd like. I really like your library, its very pythonic and much easier to use than the other spotify libraries I tried.

mental32 commented 4 years ago

Damn nice catch, yeah if you can submit a pr I can merge it immediately.

I don't have access to my machine atm so I won't be able to fix this for a couple of hours

mental32 commented 4 years ago

Keep in mind, If I merge, the changes will still have to wait until I can release them to pypi

micahdlamb commented 4 years ago

Great, I'll take a stab at a pull request. I've never done one before. It will probably take me a day or two.

I was also thinking about the way you are implementing this... If I had lots of users connect to my app then I believe this would keep refreshing all their tokens indefinitely. Is there a way to stop the refreshing if I'm done with the user? I was thinking another way this could be done is to detect when an api call fails and do the refreshing then. I'm curious how other oauth applications handle this.

mental32 commented 4 years ago

User objects have a refresh attribute that is a read only reference to the task handling the session refreshing, you can stop refreshing a user's token by cancelling the task with User.refresh.cancel()

Currently there is no mechanism in place to deal with a refresh task failing part way, I'd recommend just creating a new user oauth http session and discarding the old user object in this case.

I haven't looked into how other apps handle this situation but if you can find alternatives I'm happy to consider them :)

micahdlamb commented 4 years ago

Oh awesome, I didn't realize user.refresh was an _asyncio.Task and I can call cancel on it. In any case I think its a mistake for me to be holding onto the User objects for so long. I'm gonna think about it more next weekend.