spotify / web-api

This issue tracker is no longer used. Join us in the Spotify for Developers forum for support with the Spotify Web API ➡️ https://community.spotify.com/t5/Spotify-for-Developers/bd-p/Spotify_Developer
983 stars 79 forks source link

Access Token scopes are inconsistent after refresh with Refresh Token #814

Open nick-michael opened 6 years ago

nick-michael commented 6 years ago

Note: I'm not sure if this is the expected behaviour or a bug

Situation: I want to request 2 tokens, each with different scopes. I want 1 token to have permission to control user playback, and the other one to only read user playback.

Steps To Reproduce: 1) Go to spotify auth url with user-read-playback-state scope, get an access token, store access token and refresh token. 2) Go to spotify auth url again with user-read-playback-state user-modify-playback-state scopes, get an access token, store access token and refresh token

So far so good - the first token cannot control user playback and the second one can

3) Call the spotify token endpoint with the refresh_token grant type and the non-admin refresh token, save token response 4) Call the spotify token endpoint with the refresh_token grant type and the admin refresh token, save token response

Current Behaviour: Both tokens now have playback control access

Expected Behaviour: Token received from using the non-admin refresh token should not have access to playback control

jackcbrown89 commented 6 years ago

I just posted a related issue #825. Are you using two tokens to avoid the issue I'm having?

nick-michael commented 6 years ago

Just took a look at your issue and it's not something I have been able to reproduce - refresh tokens work fine in terms of giving me a valid token, and the fact that you are getting an 'access token expired' error implies that you may be using the old token to make the request (or some other bug).

My issue is specifically regarding the fact that scopes don't seem to be tied to a refresh token, and instead are tied to the web api client ID. I would like to have one token which has scope to view current playback, and another token which has scope to modify current playback, but if I refresh the 'non-admin' token with its' respective refresh token, the new access token has scope to modify playback when it really shouldn't

jackcbrown89 commented 6 years ago

I just closed my issue because I found it was my problem, but now I experience your issue as well. The tokens now include user-library-read playlist-modify-private playlist-modify-public user-read-playback-state instead of just playlist-modify-private user-read-playback-state. However, for me, it seems as though the extra scopes were already trumped by the original scopes.

It's slightly different from your issue, but it might shed some light on how they grant scopes.

nick-michael commented 6 years ago

The first token that I request definitely doesn't have permission to modify playback. If I refresh this token, it still doesn't have scope to modify playback. It's only after generating a new token with modify-playback scope, and THEN refreshing the original token, does it get generated with the incorrect scope.

One thing I am curious about and will try when I get a chance, is what happens if I generate the modify-playback token first and then generate the view-playback one afterwards. I can't even guess how that will behave given the strange stuff that seems to be going on behind the scenes

nick-michael commented 6 years ago

Ok so I just tried swapping the order round so that I request the modify-playback token first then the read-playback token second and again, both tokens had the correct scopes, but when I try to refresh the read-playback token it suddenly has extra scope to modify playback

happy-machine commented 6 years ago

how are you storing the tokens?

nick-michael commented 6 years ago

The refresh tokens are being encrypted and stored in a database as 'user token' and 'admin token', the auth tokens are just stored in plain text.

happy-machine commented 6 years ago

Can you show your code? I have a couple of apps that do almost exactly this with slightly different permissions and no issues

nick-michael commented 6 years ago

I'm actually away on holiday at the moment and the repo is private (for now at least) - I'll make an example project and link it in this thread when I'm back this weekend

nick-michael commented 6 years ago

I've created a demo project HERE The server side code is in 'server.js' and the front-end stuff is all in '/src/App.js'

(Sidenote: The code is completely rushed and throwaway, just something I threw together very quickly to demonstrate)

And attached a gif to demonstrate (click to enlarge): 2018-04-15-21-48-25

happy-machine commented 6 years ago

Hey

Using localstorage like that in a syncronous way is going to cause problems

I suggest using a promise based library like request-promise over axios You could have a look at JMPerez's library for Spotify which is promise based https://github.com/JMPerez/spotify-web-api-js

Ill assume you know its a bad idea storing refresh tokens in local storage in production but also in general

Unfortunately theres no api call to check token scopes. But i suspect your refresh tokens are being acted on or stored out of order

Promises would fix that or you can just add a key/value or object into the header and the stored refresh so you can match up the responses against the tokens

Id suggest using a database to store a record for each user with scope/token you could then reference an id in your calls if you have to do it that way.

Happy_Machine

⁣Sent from TypeApp ​

On 15 Apr 2018, 22:04, at 22:04, Nick Michael notifications@github.com wrote:

I've created a demo project HERE

And attached a gif to demonstrate: 2018-04-15-21-48-25

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/spotify/web-api/issues/814#issuecomment-381437833

happy-machine commented 6 years ago

Ah my bad didnt realise axios was promise based... the rest still stands!

⁣Sent from TypeApp ​

On 16 Apr 2018, 01:07, at 01:07, DJ Fresh Main thisisdjfresh@gmail.com wrote:

Hey

Using localstorage like that in a syncronous way is going to cause problems

I suggest using a promise based library like request-promise over axios You could have a look at JMPerez's library for Spotify which is promise based https://github.com/JMPerez/spotify-web-api-js

Ill assume you know its a bad idea storing refresh tokens in local storage in production but also in general

Unfortunately theres no api call to check token scopes. But i suspect your refresh tokens are being acted on or stored out of order

Promises would fix that or you can just add a key/value or object into the header and the stored refresh so you can match up the responses against the tokens

Id suggest using a database to store a record for each user with scope/token you could then reference an id in your calls if you have to do it that way.

Happy_Machine

⁣Sent from TypeApp ​

On 15 Apr 2018, 22:04, at 22:04, Nick Michael notifications@github.com wrote:

I've created a demo project HERE

And attached a gif to demonstrate: 2018-04-15-21-48-25

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/spotify/web-api/issues/814#issuecomment-381437833

happy-machine commented 6 years ago

Also JMPerez made a handy promise-throttle library which helps with spotify its on npm

⁣Sent from TypeApp ​

On 16 Apr 2018, 01:07, at 01:07, DJ Fresh Main thisisdjfresh@gmail.com wrote:

Hey

Using localstorage like that in a syncronous way is going to cause problems

I suggest using a promise based library like request-promise over axios You could have a look at JMPerez's library for Spotify which is promise based https://github.com/JMPerez/spotify-web-api-js

Ill assume you know its a bad idea storing refresh tokens in local storage in production but also in general

Unfortunately theres no api call to check token scopes. But i suspect your refresh tokens are being acted on or stored out of order

Promises would fix that or you can just add a key/value or object into the header and the stored refresh so you can match up the responses against the tokens

Id suggest using a database to store a record for each user with scope/token you could then reference an id in your calls if you have to do it that way.

Happy_Machine

⁣Sent from TypeApp ​

On 15 Apr 2018, 22:04, at 22:04, Nick Michael notifications@github.com wrote:

I've created a demo project HERE

And attached a gif to demonstrate: 2018-04-15-21-48-25

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/spotify/web-api/issues/814#issuecomment-381437833

nick-michael commented 6 years ago

I'm not too sure what you mean, based on the code I can't work out how they would possibly be acted on out of order. The process is as follows: 1) Request a basic auth code 2) Send the basic auth code to the server to request access/refresh tokens 3) Wait for response from server with the tokens 4) Store the tokens and then request an admin auth code 5) Send the admin auth code to the server 6) Wait for response from server with the admin tokens and store them

Just to highlight, nothing related to the admin tokens is even started until we have fully completed the journey for the basic auth tokens.

Happy to stand corrected, but as far as I can tell, there shouldn't be any issues with the ordering as the process has been made synchronous where needed (i.e. the promises are resolved before requesting more tokens).

happy-machine commented 6 years ago

I dont understand what the goal is here Youre trying to get a new token with boosted permissions for the same user?

Can you explain what youre trying to achieve?

Thanks

⁣Sent from TypeApp ​

On 16 Apr 2018, 13:32, at 13:32, Nick Michael notifications@github.com wrote:

I'm not too sure what you mean, based on the code I can't work out how they would possibly be acted on out of order. The process is as follows: 1) Request a basic auth code 2) Send the basic auth code to the server to request access/refresh tokens 3) Wait for response from server with the tokens 4) Store the tokens and then request an admin auth code 5) Send the admin auth code to the server 6) Wait for response from server with the admin tokens and store them

Just to highlight, nothing related to the admin tokens is even started until we have fully completed the journey for the basic auth tokens.

Happy to stand corrected, but as far as I can tell, there shouldn't be any issues with the ordering as the process has been made synchronous where needed (i.e. the promises are resolved before requesting more tokens).

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/spotify/web-api/issues/814#issuecomment-381583106

nick-michael commented 6 years ago

Sure, the idea is to generate two tokens for the same spotify user - one of them will have user-read-playback access and the intention is to share this token. The other token will have the ability to control playback + other "admin" stuff, this will not be shared.

The shared token will need to maintain it's restricted access to prevent it being used to do anything besides view playback, but I obviously want to be able to renew both tokens server side using the respective refresh tokens whilst maintaining each of their scopes.

Hope that clears it up a little. Thanks!

happy-machine commented 6 years ago

Shared in what sense?

⁣Sent from TypeApp ​

On 16 Apr 2018, 19:07, at 19:07, Nick Michael notifications@github.com wrote:

Sure, the idea is to generate two tokens - one of them will have user-read-playback access and the intention is to share this token. The other token will have the ability to control playback + other "admin" stuff, this will not be shared.

The shared token will need to maintain it's restricted access to prevent it being used to do anything besides view playback, but I obviously want to be able to renew both tokens server side using the respective refresh tokens whilst maintaining each of their scopes.

Hope that clears it up a little. Thanks!

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/spotify/web-api/issues/814#issuecomment-381697080

nick-michael commented 6 years ago

The basic token will be stored in a database and made available to a specific group of people in order for them to make direct calls to the web api.

The admin token will be stored in the same database but only available to the owner, again so that he can make direct calls to the web api, but with more scope to allow for playback controls.

As far as I'm aware there isn't any reason the access token cannot be shared with client side applications, but do correct me if I'm wrong here. (Note: refresh tokens will never be shared with any of the users)

happy-machine commented 6 years ago

Hey

Your not supposed to share users tokens between other users from what i understand, you'd have to at very least if you were doing that make it very clear that was what you were going to do. Especially in light of the new GDPR regs.

The only reason i can see to do that would be to allow people without a spotify account to access Spotify. Which is definetely against Spotify's terms of use. If you discount that being an option there is then not really a reason not to just give each user token the full breadth of permissions needed by the user. So you just have to create an auth/database flow that allows you to save a refresh token per user on a find/create basis. And if creating authorize them via redirect etc?

On Mon, Apr 16, 2018 at 11:10 PM, Nick Michael notifications@github.com wrote:

The basic token will be stored in a database and made available to a specific group of people in order for them to make direct calls to the web api.

The admin token will be stored in the same database but only available to the owner, again so that he can make direct calls to the web api, but with more scope to allow for playback controls.

As far as I'm aware there isn't any reason the access token cannot be shared with client side applications, but do correct me if I'm wrong here.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/spotify/web-api/issues/814#issuecomment-381766394, or mute the thread https://github.com/notifications/unsubscribe-auth/AfpTWbcgoq-FWj-L8k8DMVNzRJS1_Aj8ks5tpRbagaJpZM4Si2sR .

nick-michael commented 6 years ago

The only reason i can see to do that would be to allow people without a spotify account to access Spotify. Which is definetely against Spotify's terms of use.

So the actual idea behind the app is to allow multiple people who don't have spotify accounts to create a collaborative playlist with someone who does have a spotify account. By this, I mean that a playlist will be created on a spotify users account, and other people will be able to add songs to the playlist. The playlist will be created using the 'admin' account and all songs/playback will be done through the single account. No music playback will go through my app, and thus this will not allow people without spotify accounts to listen to any music. As far as I am aware this is very different to just "allow people without a spotify account to access Spotify" and don't see how or why this would be in breach of any terms of use.

Regarding sharing of the actual token, it's possible to move these request server side so that no tokens are shared with end users, but my preference would be to hit the web api directly. It would be nice to get an official word on whether this would be acceptable or not, as you said it may just need to be made clear to the end user.

nick-michael commented 6 years ago

Still a work in progress, but here's a gif to help you get a bit more of an idea what's going on: (I know it sucks that it's a gif because you can't control playback, but it's a lot easier than trying to upload an mp4)

Basically, I'm signed into spotify in the first window, which allows me to create a room with an associated playlist. Then in the incognito window, I can join the room and add songs to the shared playlist without needing a spotify account.

2018-04-17-22-44-23

happy-machine commented 6 years ago

Quite confusing on a quick video! Is it deployed anywhere? X

⁣Sent from TypeApp ​

On 17 Apr 2018, 22:48, at 22:48, Nick Michael notifications@github.com wrote:

Still a work in progress, but here's a gif to help you get a bit more of an idea what's going on:

2018-04-17-22-44-23

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/spotify/web-api/issues/814#issuecomment-382159669

nick-michael commented 6 years ago

Yeah my bad, feel free to play around! https://sharelist.space

Just to add, this version does not use multiple tokens (I didn't merge it after it didn't work) but at least you can get some idea of what the app does.

hughrawlinson commented 6 years ago

@nick-michael sorry for the delay in responding!

@happy-machine is right, it's not good practice to share user access tokens with other users. I'd recommend making these calls via your server. That gives you another advantage - you can share the result of a single call with everyone in a room, thereby increasing the amount you can scale before you run into rate limits.

I'm following up with the team to understand what the rules are about how tokens in the refresh grant get their scopes, but while we're waiting can I request that you try to add a scope=user-read-playback-state query parameter to your token refresh call?

nick-michael commented 6 years ago

@hughrawlinson No worries, yeah that suggestion makes sense - I was worried about rate limits and knew I'd have to do something about it at some point anyway!

I'll check how it works with the query param and get back to you

nick-michael commented 6 years ago

My god, it's been so long since I worked on this I forgot I've gone "serverless" to use AWS Lambda - I guess I might need to rethink that idea if I'm going to start moving more and more services over to the server 😢

nick-michael commented 6 years ago

Also:

I'm following up with the team to understand what the rules are about how tokens in the refresh grant get their scopes, but while we're waiting can I request that you try to add a scope=user-read-playback-state query parameter to your token refresh call?

Tried this and it seems to be correctly keeping the scope for the new token now. Thanks!

nick-michael commented 6 years ago

Happy to close this issue if this is the expected behaviour?

Just to clarify what that currently is:

Issue:

Workaround: