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

implement OAuth and refactor config #1321

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

eladyn
Copy link
Member

@eladyn eladyn commented Dec 30, 2024

This builds on top of #1317 and implements the missing OAuth support, which replaces the no longer available username + password method. Since this required some refactoring of the configuration anyway, I replaced structopt with the (actively developed) clap, which is the successor of structopt.

By removing the username + password method, we basically get rid of all the sensitive value handling, e.g. the keyring. (Well, one could still store the cached credentials in the keyring, but that hasn't happened before either, so this could be a future extension.)

The OAuth support is a bit clunky at the moment and surely could use some refinement in the future, but since this has been missing for quite some time now, it's probably better to put it out there and iterate on that later.

If I'm not mistaken, all configuration changes should be backwards compatible so far (or at least not error on previously working configs, of course username or password values are no longer used).

The current design of the authentication is as follows:

  • By using the spotifyd auth(enticate) subcommand, one initiates the OAuth process and is directed to accounts.spotify.com. After logging in there, we receive the token and exchange it for a longer-lived credential blob. This blob is stored at $cache_directory/oauth/credentials.json
  • If spotifyd finds an authentication blob from oauth on startup, it uses that for the first session.
  • If not disabled via --disable-discovery or disable_discovery = true (even if an oauth blob is present), discovery is started and selecting a device there ends the current session and starts a new one.
  • The last active session is also cached in $cache_directory/credentials.json and will be used on startup, when no oauth blob can be found.

Closes #800, closes #778 (due to configuration refactor).
Fixes #1293 (oauth support).
Fixes #1212 (new credential caching logic, no usernames necessary anymore).

Any testing is highly appreciated! Also, none of the changes have been documented so far so that still has to be done before a release.

@bjornfor
Copy link

If I'm not mistaken, all configuration changes should be backwards compatible so far (or at least not error on previously working configs, of course username or password values are no longer used).

I understand this as the username, password and password_cmd in the configuration file will now be ignored, and login requires some manual steps in a web browser. Correct?

@eladyn
Copy link
Member Author

eladyn commented Dec 30, 2024

I understand this as the username, password and password_cmd in the configuration file will now be ignored, and login requires some manual steps in a web browser. Correct?

Yes, that's correct. Alternatively we could maybe keep them around for now and warn the user, when they are encountered...

@mockxe
Copy link

mockxe commented Jan 14, 2025

Hi, thanks for your work here!

I just gave this a quick test, one thing I immediately noticed was:

Loading config from "/home/mockxe/.config/spotifyd/spotifyd.conf"
Error: 
   0: failed to read config file
   1: TOML parse error at line 1, column 1
   1:   |
   1: 1 | [global]
   1:   | ^^^^^^^^
   1: missing field `initial_volume`
   1: 

This can obviously fixed by adding a initial_volume=100 to the config, but I think a sensible default- / fallback-value would be good here, especially to not break existing configs.

Other than that I can only say the login process worked without a problem and so far it's running flawlessly. I keep you updated if I encounter any issue.

@eladyn
Copy link
Member Author

eladyn commented Jan 14, 2025

Hi, thanks for your work here!

I just gave this a quick test, one thing I immediately noticed was:

Loading config from "/home/mockxe/.config/spotifyd/spotifyd.conf"
Error: 
   0: failed to read config file
   1: TOML parse error at line 1, column 1
   1:   |
   1: 1 | [global]
   1:   | ^^^^^^^^
   1: missing field `initial_volume`
   1: 

This can obviously fixed by adding a initial_volume=100 to the config, but I think a sensible default- / fallback-value would be good here, especially to not break existing configs.

Other than that I can only say the login process worked without a problem and so far it's running flawlessly. I keep you updated if I encounter any issue.

Oh, huh. That should indeed not be happening. Thanks for the report.

@DocMarty84
Copy link

Hello,
Thank you for the work. A quick test worked, that's great!
Hopefully I'll be able to try a bit more intensively this week-end.

This greatly refactors and simplifies the config parsing.

Also, it removes the possiblity to specify user and password,
since this has been phased out by Spotify and will be replaced by OAuth.
@eladyn
Copy link
Member Author

eladyn commented Jan 19, 2025

@mockxe The error because of a missing initial_volume parameter should now be fixed.

@solarfl4re
Copy link

solarfl4re commented Jan 20, 2025

spotifyd is setting a higher volume than initial_volume on my system: with a value of 1, system volume is set to 58%, with 2 - 65%, and 15 gives 83%. The only value read properly in my testing is 0, which sets the volume to zero.

Whether initial_volume is set as a number or string doesn't make a difference.

This is with backend = alsa on Linux, full config is here.

@eladyn
Copy link
Member Author

eladyn commented Jan 20, 2025

That's a good point. I guess there is some weirdness around volume that I'll have to look into...

@solarfl4re
Copy link

That's a good point. I guess there is some weirdness around volume that I'll have to look into...

Maybe it was a misunderstanding on my end - I changed volume_controller to alsa_linear and now initial_volume = 2 sets my system volume to 2% as I expected.

After all the starting & stopping of spotifyd, Spotify made me reset my password, but after running spotifyd auth everything is working :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Bad credentials" error when starting spotifyd Credential cache is ignored by default
6 participants