From 938549764110e7873d6fa6e23a00507377167578 Mon Sep 17 00:00:00 2001 From: Charles Harries Date: Wed, 26 Aug 2026 12:43:34 +0100 Subject: [PATCH 1/4] feat: Only clear refresh token if Spotify tells us to --- service/spotify/spotify.go | 29 ++++++- service/spotify/spotify_test.go | 147 ++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+), 4 deletions(-) diff --git a/service/spotify/spotify.go b/service/spotify/spotify.go index 8d25445..ecf01d4 100644 --- a/service/spotify/spotify.go +++ b/service/spotify/spotify.go @@ -285,11 +285,15 @@ func (s *Service) refreshTokenForUser(user *models.User) (string, error) { s.mu.Lock() delete(s.userTokens, userID) s.mu.Unlock() - // Also clear the bad refresh token from the DB - updateErr := s.DB.UpdateUserToken(userID, "", "", time.Now().UTC()) // Clear tokens - if updateErr != nil { - s.logger.Printf("Failed to clear bad refresh token for user %d: %v", userID, updateErr) + + // Only discard the refresh token when Spotify says it is genuinely dead. + if isRefreshTokenRejected(resp.StatusCode, body) { + if updateErr := s.DB.UpdateUserToken(userID, "", "", time.Now().UTC()); updateErr != nil { + s.logger.Printf("Failed to clear bad refresh token for user %d: %v", userID, updateErr) + } + return "", fmt.Errorf("spotify refresh token rejected for user %d (%d): %s", userID, resp.StatusCode, string(body)) } + return "", fmt.Errorf("spotify token refresh failed (%d): %s", resp.StatusCode, string(body)) } @@ -326,6 +330,23 @@ func (s *Service) refreshTokenForUser(user *models.User) (string, error) { return tokenResponse.AccessToken, nil } +// isRefreshTokenRejected reports whether Spotify permanently rejected the +// refresh token, as opposed to failing for a transient reason. +func isRefreshTokenRejected(statusCode int, body []byte) bool { + if statusCode != http.StatusBadRequest && statusCode != http.StatusUnauthorized { + return false + } + + var errorResponse struct { + Error string `json:"error"` + } + if err := json.Unmarshal(body, &errorResponse); err != nil { + return false + } + + return errorResponse.Error == "invalid_grant" +} + // RefreshToken attempts to refresh the token for a given user ID. // It's less commonly needed now refreshTokenInner handles fetching the user. func (s *Service) RefreshToken(userID int64) error { diff --git a/service/spotify/spotify_test.go b/service/spotify/spotify_test.go index fce5da5..8cdf973 100644 --- a/service/spotify/spotify_test.go +++ b/service/spotify/spotify_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/spf13/viper" "github.com/teal-fm/piper/db" "github.com/teal-fm/piper/models" "github.com/teal-fm/piper/session" @@ -1413,3 +1414,149 @@ func TestGenerateLocalHash(t *testing.T) { } }) } + +// ===== Token Refresh Tests ===== + +// stubRoundTripper answers every request with a canned response, standing in +// for accounts.spotify.com. +type stubRoundTripper struct { + statusCode int + body string +} + +func (s stubRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: s.statusCode, + Body: io.NopCloser(strings.NewReader(s.body)), + Header: make(http.Header), + Request: req, + }, nil +} + +func newRefreshTestService(t *testing.T, database *db.DB, statusCode int, body string) *Service { + t.Helper() + + viper.Set("spotify.client_id", "id") + viper.Set("spotify.client_secret", "secret") + t.Cleanup(func() { + viper.Set("spotify.client_id", "") + viper.Set("spotify.client_secret", "") + }) + + service := newTestService(database, &mockPlayingNowService{}) + service.httpClient = &http.Client{ + Transport: stubRoundTripper{statusCode: statusCode, body: body}, + } + return service +} + +// A 502 from Spotify is transient -- we should keep the refresh token & retry later. +func TestRefreshTokenForUser_TransientFailureKeepsRefreshToken(t *testing.T) { + database := setupTestDB(t) + userID := createTestUser(t, database) + + user, err := database.AddSpotifySession(userID, "RUSH", "moving@pict.ures", "rush", "access", "refresh", time.Now().UTC().Add(-time.Hour)) + if err != nil { + t.Fatalf("Failed to link Spotify session: %v", err) + } + + service := newRefreshTestService(t, database, http.StatusBadGateway, "502 Server Error") + service.userTokens[userID] = "access" + + if _, err := service.refreshTokenForUser(user); err == nil { + t.Fatal("expected the refresh to fail") + } + + reloaded, err := database.GetUserByID(userID) + if err != nil { + t.Fatalf("Failed to reload user: %v", err) + } + if reloaded.RefreshToken == nil || *reloaded.RefreshToken != "refresh" { + t.Errorf("RefreshToken = %v, want it kept for the next retry", reloaded.RefreshToken) + } + + if _, exists := service.userTokens[userID]; exists { + t.Error("expected the stale cached access token to be dropped") + } +} + +// A legitimately bad refresh token should clear the token from the DB. +func TestRefreshTokenForUser_InvalidGrantClearsRefreshToken(t *testing.T) { + database := setupTestDB(t) + userID := createTestUser(t, database) + + user, err := database.AddSpotifySession(userID, "YES", "close@to.the.edge", "yes", "access", "refresh", time.Now().UTC().Add(-time.Hour)) + if err != nil { + t.Fatalf("Failed to link Spotify session: %v", err) + } + + service := newRefreshTestService(t, database, http.StatusBadRequest, `{"error":"invalid_grant","error_description":"Refresh token revoked"}`) + service.userTokens[userID] = "access" + + if _, err := service.refreshTokenForUser(user); err == nil { + t.Fatal("expected the refresh to fail") + } + + reloaded, err := database.GetUserByID(userID) + if err != nil { + t.Fatalf("Failed to reload user: %v", err) + } + if reloaded.RefreshToken != nil && *reloaded.RefreshToken != "" { + t.Errorf("RefreshToken = %v, want the dead token cleared", *reloaded.RefreshToken) + } +} + +func TestIsRefreshTokenRejected(t *testing.T) { + testCases := []struct { + name string + statusCode int + body string + expected bool + }{ + { + name: "revoked refresh token", + statusCode: http.StatusBadRequest, + body: `{"error":"invalid_grant","error_description":"Refresh token revoked"}`, + expected: true, + }, + { + name: "bad gateway HTML page", + statusCode: http.StatusBadGateway, + body: "502 Server Error", + expected: false, + }, + { + name: "service unavailable", + statusCode: http.StatusServiceUnavailable, + body: "", + expected: false, + }, + { + name: "rate limited", + statusCode: http.StatusTooManyRequests, + body: `{"error":"too_many_requests"}`, + expected: false, + }, + { + // Our credentials are wrong, not the user's token. + name: "client misconfigured", + statusCode: http.StatusUnauthorized, + body: `{"error":"invalid_client"}`, + expected: false, + }, + { + name: "bad request with unparseable body", + statusCode: http.StatusBadRequest, + body: "not json", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := isRefreshTokenRejected(tc.statusCode, []byte(tc.body)); got != tc.expected { + t.Errorf("isRefreshTokenRejected() = %v, want %v", got, tc.expected) + } + }) + } +} From ecd3a9bbd6a99ff9abffd90913bff6026bb204d4 Mon Sep 17 00:00:00 2001 From: Charles Harries Date: Wed, 26 Aug 2026 12:47:12 +0100 Subject: [PATCH 2/4] feat: Spotify only reports bad tokens with HTTP 400 --- service/spotify/spotify.go | 2 +- service/spotify/spotify_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/service/spotify/spotify.go b/service/spotify/spotify.go index ecf01d4..9958361 100644 --- a/service/spotify/spotify.go +++ b/service/spotify/spotify.go @@ -333,7 +333,7 @@ func (s *Service) refreshTokenForUser(user *models.User) (string, error) { // isRefreshTokenRejected reports whether Spotify permanently rejected the // refresh token, as opposed to failing for a transient reason. func isRefreshTokenRejected(statusCode int, body []byte) bool { - if statusCode != http.StatusBadRequest && statusCode != http.StatusUnauthorized { + if statusCode != http.StatusBadRequest { return false } diff --git a/service/spotify/spotify_test.go b/service/spotify/spotify_test.go index 8cdf973..7b53f87 100644 --- a/service/spotify/spotify_test.go +++ b/service/spotify/spotify_test.go @@ -1544,6 +1544,14 @@ func TestIsRefreshTokenRejected(t *testing.T) { body: `{"error":"invalid_client"}`, expected: false, }, + { + // Spotify only documents the 400 for a dead token, so an + // invalid_grant under any other status stays retryable. + name: "invalid_grant under an undocumented status", + statusCode: http.StatusUnauthorized, + body: `{"error":"invalid_grant"}`, + expected: false, + }, { name: "bad request with unparseable body", statusCode: http.StatusBadRequest, From b17b78188e672c57d0aecdbfdcf737b9763c7d6c Mon Sep 17 00:00:00 2001 From: Charles Harries Date: Wed, 26 Aug 2026 15:08:07 +0100 Subject: [PATCH 3/4] feat: Restore previous config during test teardown --- service/spotify/spotify_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/service/spotify/spotify_test.go b/service/spotify/spotify_test.go index 7b53f87..ce336fa 100644 --- a/service/spotify/spotify_test.go +++ b/service/spotify/spotify_test.go @@ -1436,13 +1436,16 @@ func (s stubRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { func newRefreshTestService(t *testing.T, database *db.DB, statusCode int, body string) *Service { t.Helper() - viper.Set("spotify.client_id", "id") - viper.Set("spotify.client_secret", "secret") + previousID := viper.Get("spotify.client_id") + previousSecret := viper.Get("spotify.client_secret") t.Cleanup(func() { - viper.Set("spotify.client_id", "") - viper.Set("spotify.client_secret", "") + viper.Set("spotify.client_id", previousID) + viper.Set("spotify.client_secret", previousSecret) }) + viper.Set("spotify.client_id", "id") + viper.Set("spotify.client_secret", "secret") + service := newTestService(database, &mockPlayingNowService{}) service.httpClient = &http.Client{ Transport: stubRoundTripper{statusCode: statusCode, body: body}, From f217b7344c45ff683d6ea6fde64275c5822199cc Mon Sep 17 00:00:00 2001 From: Charles Harries Date: Thu, 27 Aug 2026 08:30:33 +0100 Subject: [PATCH 4/4] fix: Ensure tokens are nulled on expiry --- db/db.go | 15 ++++++++++++++- service/spotify/spotify.go | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/db/db.go b/db/db.go index d393768..1cefd99 100644 --- a/db/db.go +++ b/db/db.go @@ -367,6 +367,19 @@ func (db *DB) UpdateUserToken(userID int64, accessToken, refreshToken string, ex return err } +// ClearUserSpotifyTokens removes the stored Spotify tokens for a user, so +// queries that look for usable tokens skip them. +func (db *DB) ClearUserSpotifyTokens(userID int64) error { + now := time.Now().UTC() + _, err := db.Exec(` + UPDATE users + SET access_token = NULL, refresh_token = NULL, token_expiry = NULL, updated_at = ? + WHERE id = ?`, + now, userID) + + return err +} + func (db *DB) UpdateAppleMusicUserToken(userID int64, userToken string) error { now := time.Now().UTC() _, err := db.Exec(` @@ -633,7 +646,7 @@ func (db *DB) GetUsersWithExpiredTokens() ([]*models.User, error) { rows, err := db.Query(` SELECT id, username, email, spotify_id, access_token, refresh_token, token_expiry, created_at, updated_at FROM users - WHERE refresh_token IS NOT NULL AND token_expiry < ? + WHERE refresh_token IS NOT NULL AND refresh_token != '' AND token_expiry < ? ORDER BY id`, time.Now().UTC()) if err != nil { diff --git a/service/spotify/spotify.go b/service/spotify/spotify.go index 9958361..90f355e 100644 --- a/service/spotify/spotify.go +++ b/service/spotify/spotify.go @@ -288,7 +288,7 @@ func (s *Service) refreshTokenForUser(user *models.User) (string, error) { // Only discard the refresh token when Spotify says it is genuinely dead. if isRefreshTokenRejected(resp.StatusCode, body) { - if updateErr := s.DB.UpdateUserToken(userID, "", "", time.Now().UTC()); updateErr != nil { + if updateErr := s.DB.ClearUserSpotifyTokens(userID); updateErr != nil { s.logger.Printf("Failed to clear bad refresh token for user %d: %v", userID, updateErr) } return "", fmt.Errorf("spotify refresh token rejected for user %d (%d): %s", userID, resp.StatusCode, string(body))