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

ignore credential cache if no username is specified #1019

Closed
wants to merge 1 commit into from

Conversation

eladyn
Copy link
Member

@eladyn eladyn commented Nov 20, 2021

As mentioned in #948 (thanks @aanno and @mas-4), zeroconf authentication fails if caching is enabled and credentials of a different user are cached. This adds extra handling for this case.

This PR has not yet been tested!

@eladyn eladyn marked this pull request as draft November 20, 2021 15:48
@mas-4
Copy link

mas-4 commented Nov 20, 2021

I'm not confident at all that I understand what's going on (I have only ever written a rust hello world) but here's what I think after a trial:

  1. The original issue is that when user b connects to the device after user a has disconnected the client crashes (exits?) and systemd restarts it (which takes 12 seconds because RestartSec=12 in my systemd service). This clears the in memory cache and allows a new user to connect.
  2. To test whether the same problem persists here I checked out your PR and did a clean build cargo build.
  3. I started spotifyd the same way as within my systemd service (./target/debug/spotifyd --no-daemon --config-path ~/spotifyd.conf where I took my spotifyd.conf from my raspberry pi and just changed the name to debug so I don't clash with my local.

Given all that, the problem persists in this branch. I connect with phone a, disconnect, connect with phone b, crash.

@aanno
Copy link

aanno commented Nov 20, 2021

I have just built and tested the branch eladyn:zeroconf_cache_fix and I'm able to say that using spotifyd with more than one user is possible now, even with caching enabled.

Thank you for fixing this, @eladyn !

aanno added a commit to aanno/linux-config that referenced this pull request Nov 20, 2021
@eladyn eladyn marked this pull request as ready for review November 20, 2021 21:54
@eladyn
Copy link
Member Author

eladyn commented Nov 20, 2021

Thank you both for having a look!

@mas-4: You're right, this PR does not try to fix the re-usability issue without restarting spotifyd. It only prevents the credential cache from clashing with zeroconf authentication.

@aanno, thanks for testing it. Marked as ready for review so that it hopefully can be merged soon.

@slondr slondr requested a review from a team January 14, 2022 02:50
@slondr slondr added the pinned Apply to make stale bot ignore issue/PR. label Jan 14, 2022
@eladyn
Copy link
Member Author

eladyn commented Sep 9, 2022

I think, this is no longer necessary and is now working as intended on master.

@eladyn eladyn closed this Sep 9, 2022
@eladyn eladyn deleted the zeroconf_cache_fix branch September 9, 2022 00:02
@aanno
Copy link

aanno commented Sep 10, 2022

Well, I tried this compiling the code on master. It's working when the spotifyd audio cache feature is off (no_audio_cache = true) with a working/used cache_path = "cache_directory".

However, when the spotifyd audio cache feature is on (no_audio_cache = false), my other family members could not even see the device (on the spotify app).

I guess that this is not the intended outcome (and it comes to me as surprise as well!).

@eladyn
Copy link
Member Author

eladyn commented Sep 11, 2022

Thanks for your report, I'm going to take a look, if I can find out what's going on there soon!

@eladyn
Copy link
Member Author

eladyn commented Sep 24, 2022

@aanno I just got around to trying it myself and I can confirm this bug. For me, it's even happening with no_audio_cache set to true. I'll try to push a fix soon. Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Apply to make stale bot ignore issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants