-
Notifications
You must be signed in to change notification settings - Fork 2k
Remove expired Spotify credentials and log error for missing configuration #6271
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
base: master
Are you sure you want to change the base?
Conversation
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.
Hey - I've found 1 issue, and left some high level feedback:
- The
_authenticatemethod now both logs an error and raises aui.UserErrorwith the same message; consider either logging at a different level with more context or relying on the exception message alone to avoid duplicate messaging. - The error message for missing
client_id/client_secretcould be more actionable by briefly indicating where/how to configure these values (e.g., pointing to the relevant config section or option names).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_authenticate` method now both logs an error and raises a `ui.UserError` with the same message; consider either logging at a different level with more context or relying on the exception message alone to avoid duplicate messaging.
- The error message for missing `client_id`/`client_secret` could be more actionable by briefly indicating where/how to configure these values (e.g., pointing to the relevant config section or option names).
## Individual Comments
### Comment 1
<location> `docs/changelog.rst:50-51` </location>
<code_context>
Bug fixes:
+- :doc:`/plugins/spotify`: Remove expired Spotify credentials and log error when
+ the Spotify credentials are missing. :bug:`6270`
- :doc:`/plugins/smartplaylist`: Fixed an issue where multiple queries in a
playlist configuration were not preserving their order, causing items to
</code_context>
<issue_to_address>
**nitpick (typo):** Consider changing "log error" to "log an error" for grammatical correctness
Change this to "log an error when the Spotify credentials are missing" for grammatical correctness.
```suggestion
- :doc:`/plugins/spotify`: Remove expired Spotify credentials and log an error when
the Spotify credentials are missing. :bug:`6270`
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Pull request overview
This PR removes expired Spotify API credentials from the default configuration and implements proper error handling when credentials are missing. Users must now provide their own Spotify API credentials from the Spotify developer dashboard.
- Replaces hardcoded expired credentials with empty strings in default config
- Adds validation and error logging when credentials are not configured
- Updates tests to explicitly configure credentials and adds test coverage for the missing credentials error case
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| beetsplug/spotify.py | Removes expired hardcoded credentials from defaults and adds validation that raises a UserError when client_id or client_secret are missing |
| test/plugins/test_spotify.py | Sets preload_plugin = False, configures credentials in setUp and configure_plugin calls, and adds test for missing credentials error |
| docs/plugins/spotify.rst | Documents the requirement for users to provide their own Spotify API credentials |
| docs/changelog.rst | Adds changelog entry describing the credential removal and error handling improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6271 +/- ##
==========================================
- Coverage 68.69% 68.68% -0.02%
==========================================
Files 138 138
Lines 18532 18532
Branches 3061 3061
==========================================
- Hits 12731 12729 -2
- Misses 5148 5150 +2
Partials 653 653
🚀 New features to boost your workflow:
|
|
Rough! Do we know if it just expired or was blocked? |
snejus
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.
Before we merge this, I want to make sure we've considered the full implications. The shared credentials are expired - a temporary problem that's fixable with a refresh. I've already contacted Adrian about this (see #3123 for context on how they were originally set up), so that could be a near-term solution.
However, this PR permanently breaks the Spotify plugin for all users until they manually set up their own credentials. This is a significant shift in the plugin's usability model, and raises several concerns:
- Barrier to entry: Many users may not have Spotify accounts at all
- API requirements: Do we know if Spotify requires a premium subscription for API access? Is the API access to non-paying users limited in any way?
- Setup friction: Every user will now need to navigate Spotify's developer dashboard, create an app, and configure credentials
This converts a plugin that worked out-of-the-box into one that requires technical setup from every single user. Have we explored renewing the shared credentials as an alternative?
Immediate action: Regardless of which direction we take, if we haven't done so already, we should respond to users who've reported this issue with a comment showing them they can override the shared credentials with their own right now. That gives them a working solution while we decide on the long-term approach.
This is indeed a change, but honestly, it should have been like this from the beginning. There is some additional friction, but that is not too different from most other plugins or other apps that use the Spotify API and require users to provide their own credentials. Spotify does have slightly elevated privileges for paid users, but we are not using any of the paid features. What we need is included in the free API. Users have reported being unable to create new applications (this may be due to the recent scraping attempt at Spotify). Irrespective, I feel that we should just refer the users to Spotify docs (as they are likely to change in the future). The plugin assumes a valid client_id and client_secret; we can clarify this in the logs. |
That's why I was asking if it just expired or was revoked. I still have a few valid api credentials I created some time ago (before spotify disabled creating new developer apps). If it is just expired it should be easy enough to replace with a valid one. |
Only @sampsyo can answer that question. My impression is that API keys don't have an expiration date (at least I didn't see one on my developer dashboard). |
|
Just leaving this here as reference (I do not think we fit their planned customer anymore): The big issue we have compared to other apps is that we share our api secrets with users which they in turn use on their home deployment. I can see how this could have been an issue in the first place and gives third parties an easy way to abuse our application and in turn the spotify api. |
|
I found this nice article that explains what's happening with Spotify API currently. See the following section:
It implies user credentials is a workaround, that's exactly how we use the API! It makes sense, because the strict 25-user limit mentioned in
applies to apps where users must authenticate with their own Spotify accounts to use the app's functionality (presumably, such functionality uses 'personal' Spotify APIs, like playback control, playlist curation etc.). Since our use case is limited to searching, which does not require user data, we can get away with shared client credentials. Warning Just realised that Spotify won't allow to create any new apps, which, disappointingly, makes the current approach in this PR pointless. It seems like our only option is to refresh/replace the credentials, as @semohr mentioned:
I've also got a few apps created a while ago. Have just verified that these creds work fine: spotify:
client_id: 78f38736bff14e3cafb16b93ed35113d
client_secret: 5c33d3e75bbc4d31a080ec0ef092d15c@arsaboo I'd say undo what we've got here and just replace the creds instead? We can merge immediately and it should hopefully revive the plugin for everyone. Meanwhile, I will wait for Adrian's response and monitor the API usage for any weirdness. |
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.
New security issues found
|
@snejus I reverted all the changes and updated the API credentials. |
|
Could you just confirm that it works fine on your end? |
|
Yes, verified it works. Creates a new token as expected. |
Looks like our hard-coded API credentials expired. This PR removes expired credentials and makes it clear to the user that they must provide their credentials.
Fixes #6270
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)