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

bug: token refresh doesn't work #235

Closed
insomnimus opened this issue Jul 16, 2021 · 8 comments
Closed

bug: token refresh doesn't work #235

insomnimus opened this issue Jul 16, 2021 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@insomnimus
Copy link

insomnimus commented Jul 16, 2021

rspotify::blocking::oauth2.rs line 468 called unwrap on an Err value

This bug happens when the access token needs to be refreshed (1hour).

I've inspected the error, it seems that for some reason spotify returns this as a response to token refresh:

{
	"access_token":"*************",
	"token_type":"Bearer",
	"expires_in":3600
}

The scope field is missing, which serde_json sees as an error since the corresponding rust struct field is not optional.

I've made it Option, however then the "refreshed" token just doesn't work...

I know you are working on a new release but i'd be very glad if i could have some quick fix for this that i can apply myself, otherwise i'm just going to go write this in go (github.com/insomnimus/libman)

I tried upgrading to the master but i think the branch is just not stable yet, there were a lot of inconsistencies and i'd rather spend time writing code than migrating it.

So again, you can reproduce this error by having a client connection for ~1 hour then requesting something.

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("missing field `scope`", line: 1, column: 142)', D:\home\.cargo\registry\src\github.com-1ecc6299db9ec823\rspotify-0.10.0\src\blocki
ng\oauth2.rs:468:68
@insomnimus insomnimus added bug Something isn't working help wanted Extra attention is needed labels Jul 16, 2021
@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Jul 16, 2021

I strongly suggest you to use the master branch, but as you said, if you need a quick fix I did something similar myself to run some benchmarks with the old version of rspotify. I was keeping the repo private until we released v0.11 but you can take a look:

https://github.com/marioortizmanero/rspotify-bench

Let me know if rspotify-0.10.0-patched works, though I didn't really test the refresh tokens, only the first auth steps.

But as I said, if you have any problems with the master branch please let us know, we're actively working on it and hope it makes rspotify a better library. It is indeed a bit unstable but the public interface won't be changed as much anymore. Rspotify v0.10 won't be supported after v0.11, so your program will be stuck afterwards.

@insomnimus
Copy link
Author

Thank you for the quick reply.

I will see if your patch works for me, thanks a lot for that.

I'd love to upgrade to the master branch but it would be rather painful to rush that change on my repo, it's ~3k loc and I kind of need a quick fix right now, so eventually I'll switch to master but I don't have the energy for now.

I'll try the patch and report back if it works, thanks!

@insomnimus
Copy link
Author

Sadly, the patch does not work.

It no longer panics but after about an hour, the requests return 403 and 404, so i think the token is not refreshed properly.

I wonder if this bug is present in the master branch, i'd like to switch to it but i wanna be sure it's fixed.

@feldroop
Copy link

Hello, may I ask what is the current status on this? I am using the version 0.10 and also getting this error after one hour:

thread 'main' panicked at 'Error parsing token_info response: reqwest::Error { kind: Decode, source: Error("missing field `scope`", line: 1, column: 142) }'

Is it fixed on the master branch or is there any additional info?

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 12, 2021

It's probably fixed on master (we still have to make sure). What we were trying to do is fix the previous version so that it worked as well, but ultimately failed to do so.

I'd suggest you to try master out, let us know what you think of the new changes :)

@feldroop
Copy link

I like the new changes and it works fine for me. Just one thing I stumbled upon:

In the interface of most of the Spotify functions (like start_uris_playback you now require a strongly typed ID (which seems like a good thing to me). But (some of) the structs still have members that are just strings. So I found myself doing stuff like:

let track_id = TrackId::from_id(track.id).unwrap();

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 19, 2021

Yea I know. I haven't worked on making the IDs consistent yet because I was considering whether strongly typed IDs are the best way to go in the first place. Sometimes you just can't know the kind of ID you're dealing with in compilation time, in which case they're a bit more complicated to use.

Leave your opinion in #203 if you like! I'm not sure what to do myself.

@marioortizmanero marioortizmanero mentioned this issue Aug 30, 2021
1 task
@marioortizmanero marioortizmanero added this to the v0.11 milestone Sep 1, 2021
@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Sep 25, 2021

It seems to work for @Felix-Droop, so I'm closing this. If anyone else has this problem please let us know and we'll take a look.

As far as I know this can only happen when the user performs a request with an authenticated client, but with no scopes. Then, trying to refresh the token would return a token without the scopes field, such as:

{
    "access_token": "***",
    "token_type": "Bearer",
    "expires_in": 3600,
    "refresh_token": "***"
}

However, I've tried this case and it does seem to work now, probably because of the #[default] attribute in Token.scopes:

pub struct Token {
    // ...
    #[serde(default, with = "space_separated_scopes", rename = "scope")] // <---
    pub scopes: HashSet<String>,
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants