mirror of
https://github.com/ramsayleung/rspotify.git
synced 2026-04-26 16:05:53 +03:00
[GH-ISSUE #276] Allow loading an expired token from the cache #82
Labels
No labels
Stale
bug
discussion
enhancement
good first issue
good first issue
help wanted
pull-request
question
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/rspotify#82
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @DusterTheFirst on GitHub (Oct 22, 2021).
Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/276
Is your feature request related to a problem? Please describe.
When writing an unattended repetitive short-lived program, that will persist its credentials in the token cache that rspotify creates and subsequently load it back on the next execution as to prevent the need for re-authentication, the current implementation of the
read_token_cachefunction is limiting. Theread_token_cachefunction will returnOk(None)if the stored token is past its expiration rather than returning the token to the caller. This prevents such an app from loading the refresh token and using it during the automatic re-authentication that rspotify does.Describe the solution you'd like
read_token_cacheshould return expired tokens rather than None. This could be as simple as the removal oftoken.is_expired()from the line https://github.com/ramsayleung/rspotify/blob/master/src/clients/oauth.rs#L54. The function signature would stay the same, returning an Option, since is expected behavior thatread_token_cachewould not load a token with mismatched scopes to that of the client.Describe alternatives you've considered
Since this functionality may be depended on, this may be a breaking change. It may be more favorable to instead have a different method that would allow the token to be invalid or even refresh it automatically on load.
Additional context
As I currently understand it, the refresh token does not have a set expiration time and instead will stay valid until the user revokes permission to the spotify app. The expiry date stored in the cache only applies to the token and not to the refresh token as the refresh token would need to be used past the expiration of the normal token to obtain a new token.
@marioortizmanero commented on GitHub (Oct 22, 2021):
Hmm, if you take automatic reauthentication into account it does make sense to be able to read expired tokens. Otherwise it makes no sense when combining cached and refreshing tokens.
This code is probably from back when we didn't have self-refreshing tokens, in which case not reading expired tokens can be helpful in order to do something different if the
read_token_cachecall fails. But it might be useful to read expired tokens even here, because that way you know you can just refresh it, rather than perform the whole auth process from scratch (which is what you have to do if the scopes don't match). And there's no way to differentiate between expired tokens and invalid tokens in order to do that.I think reading expired tokens makes sense in both use-cases, so it should be established as the default behavior. The breaking changes would be fine as long as we release it as
v0.12.0, which is completely fine (moreso knowing we are in beta and we should focus on the future interface).If we consider that the current behavior is acceptable in some cases, perhaps it'd better to add an
allow_expiredparameter to that function, and an explanation of this in the docs? That way the user is forced to check the docs and make sure it's working as intended.You are correct. But the refresh token will also "expire" after requesting a new token in PKCE.
@DusterTheFirst commented on GitHub (Oct 22, 2021):
That would make a lot of sense since when a user updates and the interface changes they will have to look into that parameter rather than the interface staying the same but a slight functionality change from under them.
If it would be helpful I could create a PR proposing the changes as mentioned here.
@marioortizmanero commented on GitHub (Oct 22, 2021):
Yup, but if it doesn't make much sense to not load expired tokens in any case then it's still a better idea to just break backward compatibility for the future's sake.
Sure!