-
Notifications
You must be signed in to change notification settings - Fork 251
Fix depreciated reliance on Keymaster tokens #694
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
Conversation
- Make OAuth/PKCE primary - Depreciate Keymaster - Refresh tokens on demand
7d2ea07 to
0fec6a3
Compare
|
Just tested this locally ( One thing is that the user information still shows "Disconnected" above the user name, this is probably wrong? Additionally, |
RensOliemans
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to ignore the review: I'm not well-versed in either rust or Psst's codebase and as such my comments are rather shallow.
|
We're a day further, presumably my token has expired, and when restarting psst I get 401 errors. Logging out and back in works (well psst complained that another service was listening on 8888, when I restarted psst it worked fine, but I'm not sure if this problem was on my side or not, I'll check that next time) |
Right, in this PR there isn't any token refresh logic yet. Working on that now. |
Pogodaanton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating OAuth is much neater than my idea!
psst-gui/src/webapi/client.rs
Outdated
| }; | ||
|
|
||
| // If unauthorized/forbidden, refresh once (if possible) and retry. | ||
| if matches!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, ureq sends a ureq::Error::StatusCode if the status code is an error. The refresh logic below this line will thus not be called, as the function errors out in line 128
|
I downloaded the latest binary from https://github.com/jpochyla/psst/actions/runs/17954044878 (built after 2f7c543, I believe, |
| clear_refresh_if_none: bool, | ||
| save: bool, | ||
| ) { | ||
| log::info!("TokenUtils: persist save={save} clear_refresh_if_none={clear_refresh_if_none}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these TokenUtil logs seem better suited to debug, right? I guess it is useful information to see when the token is refreshed. Aside: how do I change the logging level when running the binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, for now while I'm testing I've included the logs. I think the compile removes them from the binary (though I'm not completely sure)...
|
Hmm, after a while this happens: If I then restart psst, I get the following logs: and I see "Error: failed to refresh token" in the GUI. Logging out and back in worked. I'm not exactly sure why. I tried compiling psst locally, either with |
|
Yeah, I'm finding this as well. I don't know if this technique will actually work to keep refreshing for tokens... Grr. This approach could potentially be a dead end and login5 might be the solution? |
|
@jacksongoode feel free to ignore this if you don't think that's it but what about rotating the 6 access points that it fetches? They might be rate limited by access point. So for example if this one fails:
To try a different one? |
|
Hmm. I noticed that the refreshing didn't work. After some local testing, I'm pretty sure that spotify rotates their refresh token on each use. See f.e.: This means that everywhere we call |
|
Closed in favor of #693 |
There are properly a handful of other things here. I didn't want to get rid of all the Keymaster stuff, but it might be that we should eventually do that too. I'd like to follow librespot on this and as I said before this is more of a bandaid. I think these problems ought to be resolved by integrating librespot entirely but that would be a big effort.