ramsayleung / rspotify

Spotify Web API SDK implemented on Rust
MIT License
639 stars 123 forks source link

Scopes!() Disclaimer in documentation #389

Closed Inheritor-Vision closed 1 year ago

Inheritor-Vision commented 1 year ago

Hi!

First of all, I am new to Rust, so sorry in advance for any imprecision or mistake. I also do not know if this is a bug or simply disclaimer to add to the documentation.

Summary

scopes!() accept the old way of making scopes (i.e with spaces likescopes!("user-read-recently-played playlist-modify-public playlist-modify-private")) without errors. The mistake will only appears when prompt_for_token() will keep asking for authorization, even if the token is cached. prompt_for_token is probably used once in the code, when creating the Spotify client for the first time. And at this time, it is hard to debug and find that scopes!() is the issue, because everything seems OK on that side.

Long story

It seems like one of the breaking change between 0.10 and 0.11 was to make the creation of scopes behave differently.

0.10:

let mut oauth = SpotifyOAuth::default()
.scope("user-read-recently-played playlist-modify-public playlist-modify-private")
.build();

0.11:

let oauth = OAuth{
    scopes: scopes!("user-read-recently-played", "playlist-modify-public", "playlist-modify-private"),
    ..Default::default()
};
let spot = AuthCodeSpotify::new(creds, oauth);

However, doing the following in 0.11 do not produce any visible error:

let oauth = OAuth{
    scopes: scopes!("user-read-recently-played playlist-modify-public playlist-modify-private"),
    ..Default::default()
};
let spot = AuthCodeSpotify::new(creds, oauth);

Worse than that, it produces the exact same valid URL from get_authorize_url. So, when used with prompt_for_token, everything works fine for the current session.

If the token is stored:

let config = Config{
    cache_path: path,
    token_cached: true,
    ..Default::default()
};

The difference will then appears when the token will be retreived from the cache. Scopes from cache and scopes from the code will be then built differently:

From cache:
{
    "playlist-modify-private",
    "ugc-image-upload",
    "playlist-modify-public",
}
From scopes!
{
    "playlist-modify-public playlist-modify-private ugc-image-upload",
}

Then self.get_oauth().scopes.is_subset() inside read_token_cache inside prompt_for_token will silently return false and act like the token do not have the same scope, hence re-asking for authorization.

Potential fixes

I would see 3 different potential fixes:

Again, I'm Rust newbie, I hope it helps!

ramsayleung commented 1 year ago

Hi @Inheritor-Vision, thanks for your contribution, since the scopes! macro doesn't reject any inputs with space, then it will cause some subtle bugs here.

In my opinion, I prefer the combination of first solution and third solution, because the second fix doesn't actually fix this problem, it generates some message to help developer to address this subtle problem. As for the third fix, some developers might miss the documentation.

I think the best way to solve a problem is to prevent it from happening in the first place.

PS: I create a PR to fix this problem :)