Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Error refreshing token #20

Closed
micahdlamb opened this issue Oct 7, 2019 · 5 comments
Closed

Error refreshing token #20

micahdlamb opened this issue Oct 7, 2019 · 5 comments

Comments

@micahdlamb
Copy link
Contributor

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
Copy link
Owner

mental32 commented Oct 7, 2019

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
Copy link
Owner

mental32 commented Oct 7, 2019

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

@micahdlamb
Copy link
Contributor Author

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
Copy link
Owner

mental32 commented Oct 7, 2019

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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants