From bb9bce440e1aa3afb77bc3cf55118102b5db2cbb Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 26 Jul 2026 20:24:57 +0200 Subject: [PATCH] fix(admin): surface silent Management API auth failures, add link-state health checks (#585) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - #269's stuck Spotify presets traced to the account link never completing, invisible because the admin UI silently swallowed 401s from `/api/mgmt/*` instead of prompting for a retry. The browser's own Basic Auth caching already works correctly here (verified live); the bug was purely missing feedback. - Adds two Health-tab checks: Spotify configured but no account linked, and Management API credentials still at the published default. Refs #269, #419. ## Test plan - [x] `make check` (fmt, vet, unit tests) clean - [x] `make lint` clean - [x] Live-tested end to end with a headless Chrome (chromedp) against a local build: confirmed the new failure-path messages render correctly for `fetchSpotifyStatus`, `fetchAccountList`, and `linkSpotify` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5 --- pkg/service/handlers/server.go | 15 ++++++ pkg/service/handlers/web/js/script.js | 38 +++++++++++--- .../health/checks_mgmt_default_creds.go | 51 +++++++++++++++++++ .../health/checks_mgmt_default_creds_test.go | 27 ++++++++++ pkg/service/health/checks_spotify_linked.go | 48 +++++++++++++++++ .../health/checks_spotify_linked_test.go | 28 ++++++++++ 6 files changed, 201 insertions(+), 6 deletions(-) create mode 100644 pkg/service/health/checks_mgmt_default_creds.go create mode 100644 pkg/service/health/checks_mgmt_default_creds_test.go create mode 100644 pkg/service/health/checks_spotify_linked.go create mode 100644 pkg/service/health/checks_spotify_linked_test.go diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 0616950..7880feb 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -196,6 +196,21 @@ func NewServer(ds *datastore.DataStore, sm *setup.Manager, serverURL string, red }, s.GetDNSRunning, ) + health.RegisterSpotifyAccountLinkedCheck( + s.healthRegistry, + func() bool { return s.spotifyService != nil }, + func() int { + if s.spotifyService == nil { + return 0 + } + + return len(s.spotifyService.GetAccounts()) + }, + ) + health.RegisterMgmtDefaultCredentialsCheck( + s.healthRegistry, + func() (string, string) { return s.mgmtUsername, s.mgmtPassword }, + ) // Health QuickFix executor for the empty-margeAccountUUID // finding from RegisterSpeakerInfoReachable. Lives here (not in diff --git a/pkg/service/handlers/web/js/script.js b/pkg/service/handlers/web/js/script.js index b03d5e3..416a703 100644 --- a/pkg/service/handlers/web/js/script.js +++ b/pkg/service/handlers/web/js/script.js @@ -112,6 +112,8 @@ async function fetchSpotifyStatus() { const settingsResponse = await fetch("/api/setup/settings"); const settings = await settingsResponse.json(); const header = document.getElementById("spotify-status-header"); + const nameEl = document.getElementById("spotify-account-name"); + const linkBtn = document.getElementById("link-spotify-btn"); if (!settings.spotify_configured) { if (header) header.style.display = "none"; @@ -121,10 +123,22 @@ async function fetchSpotifyStatus() { if (header) header.style.display = "flex"; const response = await fetch("/api/mgmt/spotify/accounts"); - if (!response.ok) return; + if (!response.ok) { + // Don't fail silently: a stale/hidden Spotify status here is exactly + // what made #269 look like a Spotify bug instead of a Management API + // login that never completed. + if (header) { + header.style.background = "#f8d7da"; + header.style.border = "1px solid #dc3545"; + } + if (nameEl) { + nameEl.innerText = response.status === 401 + ? "Unable to check (Management login required — retry, or reload the page)" + : `Unable to check (HTTP ${response.status})`; + } + return; + } const data = await response.json(); - const nameEl = document.getElementById("spotify-account-name"); - const linkBtn = document.getElementById("link-spotify-btn"); if (data.accounts && data.accounts.length > 0) { header.style.background = "#e6ffed"; @@ -162,8 +176,12 @@ async function linkSpotify() { try { const response = await fetch("/api/mgmt/spotify/init", {method: "POST"}); if (!response.ok) { - const err = await response.text(); - alert("Failed to initialize Spotify link: " + err); + if (response.status === 401) { + alert("Management login failed or was cancelled. Please try again — your browser should prompt for the Management API username/password."); + } else { + const err = await response.text(); + alert("Failed to initialize Spotify link: " + err); + } return; } const data = await response.json(); @@ -837,7 +855,15 @@ async function fetchVersion() { async function fetchAccountList() { try { const response = await fetch("/api/mgmt/accounts"); - if (!response.ok) return; + if (!response.ok) { + const selector = document.getElementById("account-selector"); + if (selector) { + selector.innerHTML = response.status === 401 + ? `` + : ``; + } + return; + } const data = await response.json(); const selector = document.getElementById("account-selector"); if (selector) { diff --git a/pkg/service/health/checks_mgmt_default_creds.go b/pkg/service/health/checks_mgmt_default_creds.go new file mode 100644 index 0000000..8193b0c --- /dev/null +++ b/pkg/service/health/checks_mgmt_default_creds.go @@ -0,0 +1,51 @@ +package health + +// CheckIDMgmtDefaultCredentials is the registry id of the +// management-credentials check. It fires when the Management API +// (Basic Auth in front of /api/mgmt/*, guarding Spotify/Amazon +// account linking and Local Accounts) is still using the published +// default username/password. Those defaults are documented publicly, +// so anyone who has read the docs can use them. +// +// This is a visibility-only nudge (see issue #419): it does not gate +// anything and does not change default behavior. A future release is +// expected to require an explicit choice (set a password, or opt out) +// before /admin is reachable; until then this just surfaces the gap. +const CheckIDMgmtDefaultCredentials = "mgmt_default_credentials" + +// DefaultMgmtUsername and DefaultMgmtPassword are the published +// defaults (cmd/soundtouch-service flags --mgmt-username/--mgmt-password). +// Duplicated here (rather than imported) to keep this package's +// dependency surface small; keep in sync if the CLI defaults change. +const ( + DefaultMgmtUsername = "admin" + DefaultMgmtPassword = "change_me!" +) + +// RegisterMgmtDefaultCredentialsCheck registers the check. +// getMgmtCredentials returns the currently-configured management +// username and password (typically Server's mgmtUsername/mgmtPassword). +func RegisterMgmtDefaultCredentialsCheck(r *Registry, getMgmtCredentials func() (username, password string)) { + r.Register(Check{ + ID: CheckIDMgmtDefaultCredentials, + Title: "Management API credentials have been changed from the published default", + Run: func() []Finding { + username, password := getMgmtCredentials() + return runMgmtDefaultCredentialsCheck(username, password) + }, + }) +} + +func runMgmtDefaultCredentialsCheck(username, password string) []Finding { + if username != DefaultMgmtUsername || password != DefaultMgmtPassword { + return nil + } + + return []Finding{{ + Severity: SeverityInfo, + Message: "The Management API (Spotify/Amazon account linking, Local Accounts) is still using " + + "the published default credentials (admin / change_me!). Anyone who has read the docs can use them.", + Details: "Set MGMT_USERNAME and MGMT_PASSWORD (env vars or the matching CLI flags) to your own " + + "values and restart the service.", + }} +} diff --git a/pkg/service/health/checks_mgmt_default_creds_test.go b/pkg/service/health/checks_mgmt_default_creds_test.go new file mode 100644 index 0000000..8462c22 --- /dev/null +++ b/pkg/service/health/checks_mgmt_default_creds_test.go @@ -0,0 +1,27 @@ +package health + +import "testing" + +func TestMgmtDefaultCredentialsCheck_NoFindingWhenChanged(t *testing.T) { + for _, tc := range []struct{ username, password string }{ + {"admin", "somethingElse"}, + {"someoneElse", "change_me!"}, + {"customadmin", "customsecret"}, + } { + got := runMgmtDefaultCredentialsCheck(tc.username, tc.password) + if len(got) != 0 { + t.Errorf("expected no findings for %+v, got %+v", tc, got) + } + } +} + +func TestMgmtDefaultCredentialsCheck_WarnsWhenStillDefault(t *testing.T) { + got := runMgmtDefaultCredentialsCheck(DefaultMgmtUsername, DefaultMgmtPassword) + if len(got) != 1 { + t.Fatalf("expected one finding for unchanged default credentials, got %+v", got) + } + + if got[0].Severity != SeverityInfo { + t.Errorf("expected SeverityInfo (visibility-only nudge, not a gate), got %v", got[0].Severity) + } +} diff --git a/pkg/service/health/checks_spotify_linked.go b/pkg/service/health/checks_spotify_linked.go new file mode 100644 index 0000000..2af4705 --- /dev/null +++ b/pkg/service/health/checks_spotify_linked.go @@ -0,0 +1,48 @@ +package health + +// CheckIDSpotifyAccountLinked is the registry id of the Spotify +// account-link check. It fires when a Spotify developer app is +// configured (client_id/secret set) but no account has completed the +// OAuth link — the state in which every speaker's Spotify token +// request 503s ("No Spotify accounts linked"), and Spotify presets +// only appear to work right after a manual Spotify Connect session +// (which bypasses AfterTouch's token proxy entirely and doesn't +// survive a reboot). +// +// See issue #269: this state is otherwise invisible outside the +// service log / a diagnostic export, so operators mistake it for a +// per-speaker Spotify limitation instead of a link they never +// finished. +const CheckIDSpotifyAccountLinked = "spotify_account_linked" + +// RegisterSpotifyAccountLinkedCheck registers the check. +// getSpotifyConfigured reports whether a Spotify client_id/secret is +// set (typically Server's spotifyService != nil); getLinkedAccountCount +// returns how many Spotify accounts have completed the OAuth link +// (typically len(spotifyService.GetAccounts())). +func RegisterSpotifyAccountLinkedCheck(r *Registry, getSpotifyConfigured func() bool, getLinkedAccountCount func() int) { + r.Register(Check{ + ID: CheckIDSpotifyAccountLinked, + Title: "Spotify account is linked", + Run: func() []Finding { + return runSpotifyAccountLinkedCheck(getSpotifyConfigured(), getLinkedAccountCount()) + }, + }) +} + +func runSpotifyAccountLinkedCheck(configured bool, linkedCount int) []Finding { + if !configured || linkedCount > 0 { + return nil + } + + return []Finding{{ + Severity: SeverityWarning, + Message: "Spotify is configured (client ID/secret set) but no account is linked yet. " + + "Every speaker's Spotify token request will fail (503 \"No Spotify accounts linked\"), " + + "so stored Spotify presets stay dead until you manually Spotify Connect from a phone — " + + "and that only primes the one speaker you connected to, only until its next reboot.", + Details: "Finish the link on the Admin page: Settings → Spotify → \"Link Spotify Account\", " + + "then complete the consent screen. Once linked, AfterTouch re-primes each speaker " + + "automatically on power-on.", + }} +} diff --git a/pkg/service/health/checks_spotify_linked_test.go b/pkg/service/health/checks_spotify_linked_test.go new file mode 100644 index 0000000..89e705f --- /dev/null +++ b/pkg/service/health/checks_spotify_linked_test.go @@ -0,0 +1,28 @@ +package health + +import "testing" + +func TestSpotifyAccountLinkedCheck_NoFindingWhenNotConfigured(t *testing.T) { + got := runSpotifyAccountLinkedCheck(false, 0) + if len(got) != 0 { + t.Errorf("expected no findings when Spotify isn't configured, got %+v", got) + } +} + +func TestSpotifyAccountLinkedCheck_NoFindingWhenLinked(t *testing.T) { + got := runSpotifyAccountLinkedCheck(true, 1) + if len(got) != 0 { + t.Errorf("expected no findings when an account is linked, got %+v", got) + } +} + +func TestSpotifyAccountLinkedCheck_WarnsWhenConfiguredButUnlinked(t *testing.T) { + got := runSpotifyAccountLinkedCheck(true, 0) + if len(got) != 1 { + t.Fatalf("expected one finding for configured-but-unlinked, got %+v", got) + } + + if got[0].Severity != SeverityWarning { + t.Errorf("expected SeverityWarning, got %v", got[0].Severity) + } +}