diff --git a/Makefile b/Makefile index e87bbff..efa559d 100644 --- a/Makefile +++ b/Makefile @@ -32,7 +32,7 @@ live-smoke: test-live-smoke: ./scripts/live-smoke-test.sh - go test -tags=keyring_nopassage,spotify_live ./internal/livesmoke -run '^$$' + go test -tags=keyring_nopassage,spotify_live ./internal/client ./internal/credentials -run '^$$' lint: golangci-lint run diff --git a/cmd/sptfy/main_test.go b/cmd/sptfy/main_test.go index 145e746..be4438a 100644 --- a/cmd/sptfy/main_test.go +++ b/cmd/sptfy/main_test.go @@ -10,27 +10,16 @@ import ( "testing" "time" - "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/cli-common/statedir" "github.com/open-cli-collective/cli-common/statedirtest" "github.com/open-cli-collective/spotify-cli/internal/cmd/root" "github.com/open-cli-collective/spotify-cli/internal/config" "github.com/open-cli-collective/spotify-cli/internal/credentials" + "github.com/open-cli-collective/spotify-cli/internal/credstoretest" "github.com/open-cli-collective/spotify-cli/internal/exitcode" ) -type processFailStore struct{ err error } - -func (s processFailStore) Backend() (credstore.Backend, credstore.Source) { - return credstore.BackendMemory, credstore.SourceExplicit -} -func (s processFailStore) Close() error { return nil } -func (s processFailStore) Get(string, string) (string, error) { return "", s.err } -func (s processFailStore) Set(string, string, string, ...credstore.SetOpt) error { return s.err } -func (s processFailStore) Delete(string, string) error { return s.err } -func (s processFailStore) Exists(string, string) (bool, error) { return false, s.err } - func TestUnknownCommandsExitUsage(t *testing.T) { for _, args := range [][]string{{"frobnicate"}, {"config", "frobnicate"}} { var out, errOut bytes.Buffer @@ -114,7 +103,7 @@ func TestCredentialStoreFailureIsSecretSafeAtProcessBoundary(t *testing.T) { Scope: statedir.Scope{Name: config.Service}, Cache: statedir.Cache{Tool: config.Tool}, Data: statedir.Data{Tool: config.Tool}, Now: func() time.Time { return time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) }, OpenStore: func(credentials.OpenRequest) (credentials.Store, error) { - return processFailStore{err: errors.New("backend echoed " + canary)}, nil + return &credstoretest.Store{SetErr: errors.New("backend echoed " + canary)}, nil }, }) cmd.SetArgs(args) diff --git a/internal/auth/authorizer_test.go b/internal/auth/authorizer_test.go index 9b02342..1651a09 100644 --- a/internal/auth/authorizer_test.go +++ b/internal/auth/authorizer_test.go @@ -31,26 +31,14 @@ func TestAuthorizeUsesDynamicLoopbackPKCEAndExactRedirect(t *testing.T) { })) defer server.Close() - var authURL *url.URL + browser := &loopbackBrowser{callback: func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "?code=auth-code&state=" + url.QueryEscape(authURL.Query().Get("state")) + }} authorizer := Authorizer{ - HTTPClient: server.Client(), - Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, - Timeout: 2 * time.Second, - OpenBrowser: func(raw string) error { - var err error - authURL, err = url.Parse(raw) - if err != nil { - return err - } - callback := authURL.Query().Get("redirect_uri") + "?code=auth-code&state=" + url.QueryEscape(authURL.Query().Get("state")) - go func() { - response, getErr := http.Get(callback) // #nosec G107 -- test callback URL is generated by the local authorizer. - if getErr == nil { - _ = response.Body.Close() - } - }() - return nil - }, + HTTPClient: server.Client(), + Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, + Timeout: 2 * time.Second, + OpenBrowser: browser.open, } var stderr bytes.Buffer got, err := authorizer.Authorize(context.Background(), Request{ @@ -65,6 +53,7 @@ func TestAuthorizeUsesDynamicLoopbackPKCEAndExactRedirect(t *testing.T) { if want := []string{ScopePlaylistModifyPrivate, ScopePlaylistModifyPublic, ScopePlaylistReadCollaborative, ScopePlaylistReadPrivate, ScopeUserLibraryModify, ScopeUserLibraryRead, ScopeUserReadPrivate}; !slices.Equal(got.Scopes, want) { t.Fatalf("scopes = %v, want %v", got.Scopes, want) } + authURL := browser.authURL if authURL == nil || authURL.Query().Get("code_challenge_method") != "S256" || authURL.Query().Get("scope") != "playlist-modify-private playlist-modify-public playlist-read-collaborative playlist-read-private user-library-modify user-library-read user-read-private" { t.Fatalf("authorization URL = %v", authURL) @@ -128,33 +117,24 @@ func TestAuthorizeAcceptsUppercaseSchemeAndRootLoopbackPath(t *testing.T) { _, _ = io.WriteString(writer, `{"access_token":"access","token_type":"Bearer","refresh_token":"refresh","expires_in":3600,"scope":"user-read-private"}`) })) defer server.Close() + browser := &loopbackBrowser{callback: func(authURL *url.URL) string { + callback, err := url.Parse(authURL.Query().Get("redirect_uri")) + if err != nil { + t.Fatal(err) + } + callback.Scheme = strings.ToLower(callback.Scheme) + callback.Path = "/" + query := callback.Query() + query.Set("code", "code") + query.Set("state", authURL.Query().Get("state")) + callback.RawQuery = query.Encode() + return callback.String() + }} authorizer := Authorizer{ - HTTPClient: server.Client(), - Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, - Timeout: 2 * time.Second, - OpenBrowser: func(raw string) error { - authURL, err := url.Parse(raw) - if err != nil { - return err - } - callback, err := url.Parse(authURL.Query().Get("redirect_uri")) - if err != nil { - return err - } - callback.Scheme = strings.ToLower(callback.Scheme) - callback.Path = "/" - query := callback.Query() - query.Set("code", "code") - query.Set("state", authURL.Query().Get("state")) - callback.RawQuery = query.Encode() - go func() { - response, getErr := http.Get(callback.String()) // #nosec G107 -- test callback URL is generated by the local authorizer. - if getErr == nil { - _ = response.Body.Close() - } - }() - return nil - }, + HTTPClient: server.Client(), + Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, + Timeout: 2 * time.Second, + OpenBrowser: browser.open, } if _, err := authorizer.Authorize(context.Background(), Request{ ClientID: "client-id", RedirectURI: "HTTP://127.0.0.1", ErrOut: io.Discard, @@ -164,25 +144,13 @@ func TestAuthorizeAcceptsUppercaseSchemeAndRootLoopbackPath(t *testing.T) { } func TestAuthorizeRejectsWrongStateWithoutExchangeOrLeak(t *testing.T) { - var expectedState string + browser := &loopbackBrowser{callback: func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "?code=auth-code&state=wrong-state-sentinel" + }} authorizer := Authorizer{ - Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: "https://accounts.example/token"}, - Timeout: 2 * time.Second, - OpenBrowser: func(raw string) error { - authURL, err := url.Parse(raw) - if err != nil { - return err - } - expectedState = authURL.Query().Get("state") - callback := authURL.Query().Get("redirect_uri") + "?code=auth-code&state=wrong-state-sentinel" - go func() { - response, getErr := http.Get(callback) // #nosec G107 -- test callback URL is generated by the local authorizer. - if getErr == nil { - _ = response.Body.Close() - } - }() - return nil - }, + Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: "https://accounts.example/token"}, + Timeout: 2 * time.Second, + OpenBrowser: browser.open, } _, err := authorizer.Authorize(context.Background(), Request{ ClientID: "client-id", RedirectURI: "http://127.0.0.1/callback", ErrOut: io.Discard, @@ -190,7 +158,7 @@ func TestAuthorizeRejectsWrongStateWithoutExchangeOrLeak(t *testing.T) { if !errors.Is(err, ErrStateMismatch) { t.Fatalf("error = %v, want ErrStateMismatch", err) } - if strings.Contains(err.Error(), expectedState) || strings.Contains(err.Error(), "wrong-state-sentinel") { + if strings.Contains(err.Error(), browser.authURL.Query().Get("state")) || strings.Contains(err.Error(), "wrong-state-sentinel") { t.Fatalf("state leaked in error: %v", err) } } @@ -202,8 +170,6 @@ func TestAuthorizeReadsCompleteRedirectURLFromStdinWithoutBrowser(t *testing.T) })) defer server.Close() - reader, writer := io.Pipe() - urls := make(chan string, 1) authorizer := Authorizer{ HTTPClient: server.Client(), Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, @@ -212,26 +178,12 @@ func TestAuthorizeReadsCompleteRedirectURLFromStdinWithoutBrowser(t *testing.T) return nil }, } - result := make(chan error, 1) - go func() { - _, err := authorizer.Authorize(context.Background(), Request{ - ClientID: "client-id", RedirectURI: "https://callback.example/spotify", AuthCodeStdin: true, - In: reader, ErrOut: &authURLWriter{urls: urls}, - }) - result <- err - }() - - raw := <-urls - authURL, err := url.Parse(raw) + _, err := authorizeViaStdin(t, authorizer, "https://callback.example/spotify", func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "?code=auth-code&state=" + url.QueryEscape(authURL.Query().Get("state")) + "\n" + }) if err != nil { t.Fatal(err) } - callback := authURL.Query().Get("redirect_uri") + "?code=auth-code&state=" + url.QueryEscape(authURL.Query().Get("state")) - _, _ = io.WriteString(writer, callback+"\n") - _ = writer.Close() - if err := <-result; err != nil { - t.Fatal(err) - } } func TestAuthorizeStdinTreatsEmptyAndSlashRootPathsEqually(t *testing.T) { @@ -240,30 +192,16 @@ func TestAuthorizeStdinTreatsEmptyAndSlashRootPathsEqually(t *testing.T) { _, _ = io.WriteString(writer, `{"access_token":"access","token_type":"Bearer","refresh_token":"refresh","expires_in":3600,"scope":"user-read-private"}`) })) defer server.Close() - reader, writer := io.Pipe() - urls := make(chan string, 1) - result := make(chan error, 1) authorizer := Authorizer{ HTTPClient: server.Client(), Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, } - go func() { - _, err := authorizer.Authorize(context.Background(), Request{ - ClientID: "client-id", RedirectURI: "https://callback.example", AuthCodeStdin: true, - In: reader, ErrOut: &authURLWriter{urls: urls}, - }) - result <- err - }() - authURL, err := url.Parse(<-urls) + _, err := authorizeViaStdin(t, authorizer, "https://callback.example", func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "/?code=code&state=" + url.QueryEscape(authURL.Query().Get("state")) + }) if err != nil { t.Fatal(err) } - callback := authURL.Query().Get("redirect_uri") + "/?code=code&state=" + url.QueryEscape(authURL.Query().Get("state")) - _, _ = io.WriteString(writer, callback) - _ = writer.Close() - if err := <-result; err != nil { - t.Fatal(err) - } } func TestAuthorizeRejectsRawCodeAndTimesOutWithoutBrowser(t *testing.T) { @@ -294,28 +232,17 @@ func TestAuthorizeRejectsRawCodeAndTimesOutWithoutBrowser(t *testing.T) { func TestAuthorizeReportsDenialWithoutLeakingCallbackValues(t *testing.T) { const denialSentinel = "denial-value-sentinel" responses := make(chan string, 1) - authorizer := Authorizer{ - Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: "https://accounts.example/token"}, - Timeout: 2 * time.Second, - OpenBrowser: func(raw string) error { - authURL, err := url.Parse(raw) - if err != nil { - return err - } - callback := authURL.Query().Get("redirect_uri") + "?error=" + denialSentinel + "&state=" + url.QueryEscape(authURL.Query().Get("state")) - go func() { - response, getErr := http.Get(callback) // #nosec G107 -- test callback URL is generated by the local authorizer. - if getErr != nil { - responses <- getErr.Error() - return - } - defer func() { _ = response.Body.Close() }() - body, _ := io.ReadAll(response.Body) - responses <- string(body) - }() - return nil + browser := &loopbackBrowser{ + responses: responses, + callback: func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "?error=" + denialSentinel + "&state=" + url.QueryEscape(authURL.Query().Get("state")) }, } + authorizer := Authorizer{ + Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: "https://accounts.example/token"}, + Timeout: 2 * time.Second, + OpenBrowser: browser.open, + } _, err := authorizer.Authorize(context.Background(), Request{ ClientID: "client-id", RedirectURI: "http://127.0.0.1/callback", ErrOut: io.Discard, }) @@ -334,25 +261,18 @@ func TestAuthorizeContinuesAfterBrowserOpenFailure(t *testing.T) { })) defer server.Close() - authorizer := Authorizer{ - HTTPClient: server.Client(), - Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, - Timeout: 2 * time.Second, - OpenBrowser: func(raw string) error { - authURL, err := url.Parse(raw) - if err != nil { - return err - } - callback := authURL.Query().Get("redirect_uri") + "?code=code&state=" + url.QueryEscape(authURL.Query().Get("state")) - go func() { - response, getErr := http.Get(callback) // #nosec G107 -- test callback URL is generated by the local authorizer. - if getErr == nil { - _ = response.Body.Close() - } - }() - return errors.New("browser-launch-detail-sentinel") + browser := &loopbackBrowser{ + err: errors.New("browser-launch-detail-sentinel"), + callback: func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "?code=code&state=" + url.QueryEscape(authURL.Query().Get("state")) }, } + authorizer := Authorizer{ + HTTPClient: server.Client(), + Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, + Timeout: 2 * time.Second, + OpenBrowser: browser.open, + } var stderr bytes.Buffer if _, err := authorizer.Authorize(context.Background(), Request{ ClientID: "client-id", RedirectURI: "http://127.0.0.1/callback", ErrOut: &stderr, @@ -378,33 +298,17 @@ func TestAuthorizeExchangeFailureDoesNotLeakCodeOrVerifier(t *testing.T) { })) defer server.Close() - reader, writer := io.Pipe() - urls := make(chan string, 1) - var stderr bytes.Buffer - result := make(chan error, 1) authorizer := Authorizer{ HTTPClient: server.Client(), Endpoints: Endpoints{AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL}, } - go func() { - _, err := authorizer.Authorize(context.Background(), Request{ - ClientID: "client-id", RedirectURI: "https://callback.example/spotify", AuthCodeStdin: true, - In: reader, ErrOut: io.MultiWriter(&stderr, &authURLWriter{urls: urls}), - }) - result <- err - }() - authURL, err := url.Parse(<-urls) - if err != nil { - t.Fatal(err) - } - callback := authURL.Query().Get("redirect_uri") + "?code=" + code + "&state=" + url.QueryEscape(authURL.Query().Get("state")) - _, _ = io.WriteString(writer, callback) - _ = writer.Close() - err = <-result + stderr, err := authorizeViaStdin(t, authorizer, "https://callback.example/spotify", func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "?code=" + code + "&state=" + url.QueryEscape(authURL.Query().Get("state")) + }) if !errors.Is(err, ErrExchange) { t.Fatalf("error = %v, want ErrExchange", err) } - combined := stderr.String() + err.Error() + combined := stderr + err.Error() if verifier == "" || strings.Contains(combined, code) || strings.Contains(combined, verifier) { t.Fatalf("exchange secret leaked: %q", combined) } @@ -425,27 +329,12 @@ func TestAuthorizeDistinguishesOmittedAndEmptyScope(t *testing.T) { _, _ = fmt.Fprintf(writer, `{"access_token":"access","token_type":"Bearer","refresh_token":"refresh","expires_in":3600%s}`, test.scope) })) defer server.Close() - reader, writer := io.Pipe() - urls := make(chan string, 1) - result := make(chan error, 1) authorizer := Authorizer{HTTPClient: server.Client(), Endpoints: Endpoints{ AuthorizeURL: "https://accounts.example/authorize", TokenURL: server.URL, }} - go func() { - _, err := authorizer.Authorize(context.Background(), Request{ - ClientID: "client-id", RedirectURI: "https://callback.example/spotify", AuthCodeStdin: true, - In: reader, ErrOut: &authURLWriter{urls: urls}, - }) - result <- err - }() - authURL, err := url.Parse(<-urls) - if err != nil { - t.Fatal(err) - } - callback := authURL.Query().Get("redirect_uri") + "?code=code&state=" + url.QueryEscape(authURL.Query().Get("state")) - _, _ = io.WriteString(writer, callback) - _ = writer.Close() - err = <-result + _, err := authorizeViaStdin(t, authorizer, "https://callback.example/spotify", func(authURL *url.URL) string { + return authURL.Query().Get("redirect_uri") + "?code=code&state=" + url.QueryEscape(authURL.Query().Get("state")) + }) if !errors.Is(err, test.wantErr) { t.Fatalf("error = %v, want %v", err, test.wantErr) } @@ -491,6 +380,61 @@ type authURLWriter struct { urls chan<- string } +type loopbackBrowser struct { + authURL *url.URL + callback func(*url.URL) string + responses chan string + err error +} + +func (browser *loopbackBrowser) open(raw string) error { + authURL, err := url.Parse(raw) + if err != nil { + return err + } + browser.authURL = authURL + callback := browser.callback(authURL) + go func() { + response, getErr := http.Get(callback) // #nosec G107 -- test callback URL is generated by the local authorizer. + if browser.responses == nil { + if getErr == nil { + _ = response.Body.Close() + } + return + } + if getErr != nil { + browser.responses <- getErr.Error() + return + } + defer func() { _ = response.Body.Close() }() + body, _ := io.ReadAll(response.Body) + browser.responses <- string(body) + }() + return browser.err +} + +func authorizeViaStdin(t *testing.T, authorizer Authorizer, redirectURI string, callback func(*url.URL) string) (string, error) { + t.Helper() + reader, writer := io.Pipe() + urls := make(chan string, 1) + var stderr bytes.Buffer + result := make(chan error, 1) + go func() { + _, err := authorizer.Authorize(context.Background(), Request{ + ClientID: "client-id", RedirectURI: redirectURI, AuthCodeStdin: true, + In: reader, ErrOut: io.MultiWriter(&stderr, &authURLWriter{urls: urls}), + }) + result <- err + }() + authURL, err := url.Parse(<-urls) + if err != nil { + t.Fatal(err) + } + _, _ = io.WriteString(writer, callback(authURL)) + _ = writer.Close() + return stderr.String(), <-result +} + func (writer *authURLWriter) Write(data []byte) (int, error) { writer.mu.Lock() defer writer.mu.Unlock() diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 2048e72..6dc411b 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -146,52 +146,28 @@ func TestCatalogGetValidatesInputAndResponse(t *testing.T) { } } -func TestCatalogGetInheritsBoundedTransportBehavior(t *testing.T) { +func TestCatalogEntryPointsInheritTransportBehavior(t *testing.T) { + const id = "0123456789ABCDEFGHIJKL" for _, test := range []struct { name string - body string - code int - want error + call func(Client) error }{ - {name: "unauthorized", code: http.StatusUnauthorized, want: ErrUnauthorized}, - {name: "forbidden", code: http.StatusForbidden, want: ErrForbidden}, - {name: "malformed", body: "not-json", code: http.StatusOK, want: ErrInvalidResponse}, - {name: "oversized", body: `{"id":"` + strings.Repeat("x", maxResponseBytes) + `"}`, code: http.StatusOK, want: ErrInvalidResponse}, + {name: "track", call: func(value Client) error { _, err := value.GetTrack(context.Background(), id); return err }}, + {name: "album", call: func(value Client) error { _, err := value.GetAlbum(context.Background(), id); return err }}, + {name: "artist", call: func(value Client) error { _, err := value.GetArtist(context.Background(), id); return err }}, + {name: "album tracks", call: func(value Client) error { _, err := value.ListAlbumTracks(context.Background(), id, 1, 0); return err }}, + {name: "artist albums", call: func(value Client) error { _, err := value.ListArtistAlbums(context.Background(), id, 1, 0); return err }}, } { t.Run(test.name, func(t *testing.T) { httpClient := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { - return response(test.code, test.body), nil + return response(http.StatusUnauthorized, "secret-canary"), nil })} - _, err := (Client{HTTPClient: httpClient}).GetArtist(context.Background(), "0123456789ABCDEFGHIJKL") - if !errors.Is(err, test.want) { - t.Fatalf("error=%v want=%v", err, test.want) + err := test.call(Client{HTTPClient: httpClient}) + if !errors.Is(err, ErrUnauthorized) || strings.Contains(err.Error(), "canary") { + t.Fatalf("error=%v", err) } }) } - - calls := 0 - spotify := Client{ - HTTPClient: &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { - calls++ - if calls == 1 { - return response(http.StatusServiceUnavailable, ""), nil - } - return response(http.StatusOK, `{"id":"0123456789ABCDEFGHIJKL"}`), nil - })}, - Wait: func(context.Context, time.Duration) error { return nil }, - } - if _, err := spotify.GetTrack(context.Background(), "0123456789ABCDEFGHIJKL"); err != nil || calls != 2 { - t.Fatalf("retry error=%v calls=%d", err, calls) - } - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - spotify.HTTPClient = &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { - return nil, request.Context().Err() - })} - if _, err := spotify.GetTrack(ctx, "0123456789ABCDEFGHIJKL"); !errors.Is(err, context.Canceled) { - t.Fatalf("cancellation error=%v", err) - } } func TestCatalogTraversalUsesExactPathsAndDecodesPages(t *testing.T) { @@ -272,49 +248,6 @@ func TestCatalogTraversalRejectsInvalidInputsAndPages(t *testing.T) { } } -func TestCatalogTraversalInheritsTransportBehavior(t *testing.T) { - const id = "0123456789ABCDEFGHIJKL" - for _, test := range []struct { - status int - want error - }{ - {http.StatusUnauthorized, ErrUnauthorized}, - {http.StatusForbidden, ErrForbidden}, - } { - httpClient := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { - return response(test.status, "secret-canary"), nil - })} - _, err := (Client{HTTPClient: httpClient}).ListArtistAlbums(context.Background(), id, 1, 0) - if !errors.Is(err, test.want) || strings.Contains(err.Error(), "canary") { - t.Fatalf("status=%d error=%v", test.status, err) - } - } - - calls := 0 - spotify := Client{ - HTTPClient: &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { - calls++ - if calls == 1 { - return response(http.StatusServiceUnavailable, ""), nil - } - return response(http.StatusOK, `{"items":[],"limit":1,"offset":0,"total":0,"next":null}`), nil - })}, - Wait: func(context.Context, time.Duration) error { return nil }, - } - if _, err := spotify.ListAlbumTracks(context.Background(), id, 1, 0); err != nil || calls != 2 { - t.Fatalf("retry error=%v calls=%d", err, calls) - } - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - spotify.HTTPClient = &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { - return nil, request.Context().Err() - })} - if _, err := spotify.ListAlbumTracks(ctx, id, 1, 0); !errors.Is(err, context.Canceled) { - t.Fatalf("cancellation error=%v", err) - } -} - func TestSearchTracksEncodesQueryAndDecodesBreadcrumbs(t *testing.T) { httpClient := &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { if request.Method != http.MethodGet || request.URL.Path != "/v1/search" { diff --git a/internal/client/library_test.go b/internal/client/library_test.go index f68fc9f..9e50873 100644 --- a/internal/client/library_test.go +++ b/internal/client/library_test.go @@ -45,38 +45,6 @@ func TestSavedTrackListUsesFixedPathAndValidatesPage(t *testing.T) { } } -func TestSavedTrackMutationsChunkAtForty(t *testing.T) { - for _, count := range []int{40, 41, 80, 81} { - for _, method := range []string{http.MethodPut, http.MethodDelete} { - t.Run(fmt.Sprintf("%s/%d", method, count), func(t *testing.T) { - var sizes []int - httpClient := &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { - if request.Method != method || request.URL.Path != "/v1/me/library" || len(request.URL.Query()) != 1 { - t.Fatalf("request=%s %s", request.Method, request.URL.String()) - } - sizes = append(sizes, len(strings.Split(request.URL.Query().Get("uris"), ","))) - return response(http.StatusNoContent, ""), nil - })} - spotify := Client{HTTPClient: httpClient} - var err error - if method == http.MethodPut { - err = spotify.SaveSavedItems(context.Background(), spotifyref.Track, libraryIDs(count)) - } else { - err = spotify.RemoveSavedItems(context.Background(), spotifyref.Track, libraryIDs(count)) - } - if err != nil || len(sizes) != (count+39)/40 { - t.Fatalf("sizes=%v error=%v", sizes, err) - } - for _, size := range sizes { - if size < 1 || size > 40 { - t.Fatalf("sizes=%v", sizes) - } - } - }) - } - } -} - func TestQueryOnlySavedItemMutationOutcomesAreTypedAndNeverRetried(t *testing.T) { const id = "0123456789ABCDEFGHIJKL" for _, operation := range []struct { @@ -155,48 +123,6 @@ func TestQueryOnlySavedItemMutationPreservesTypedAuthFailures(t *testing.T) { } } -func TestSavedTrackOperationsChunkAtForty(t *testing.T) { - for _, count := range []int{40, 41, 80, 81} { - t.Run(fmt.Sprint(count), func(t *testing.T) { - ids := libraryIDs(count) - var sizes []int - isSaved := func(uri string) bool { - index, _ := strconv.Atoi(strings.TrimPrefix(uri, "spotify:track:")) - return index%7 == 0 || index%11 == 3 - } - httpClient := &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { - if request.URL.Host != "api.spotify.invalid" || request.URL.Path != "/v1/me/library/contains" || - request.Method != http.MethodGet || len(request.URL.Query()) != 1 { - t.Fatalf("request=%s %s", request.Method, request.URL.String()) - } - chunk := strings.Split(request.URL.Query().Get("uris"), ",") - size := len(chunk) - sizes = append(sizes, size) - values := make([]string, size) - for index, uri := range chunk { - values[index] = strconv.FormatBool(isSaved(uri)) - } - return response(http.StatusOK, "["+strings.Join(values, ",")+"]"), nil - })} - got, err := (Client{HTTPClient: httpClient, BaseURL: "https://api.spotify.invalid/v1"}).CheckSavedItems(context.Background(), spotifyref.Track, ids) - wantCalls := (count + 39) / 40 - wantResults := make([]bool, count) - for index, id := range ids { - wantResults[index] = isSaved("spotify:track:" + id) - } - if err != nil || !slices.Equal(got, wantResults) || len(sizes) != wantCalls { - t.Fatalf("count=%d sizes=%v results=%v want=%v error=%v", count, sizes, got, wantResults, err) - } - for index, size := range sizes { - want := min(40, count-index*40) - if size != want { - t.Fatalf("count=%d sizes=%v", count, sizes) - } - } - }) - } -} - func TestSavedTrackCheckRejectsResponseLengthMismatch(t *testing.T) { httpClient := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { return response(http.StatusOK, `[true]`), nil @@ -315,33 +241,45 @@ func TestSavedAlbumListUsesFixedPathAndValidatesPage(t *testing.T) { } func TestSavedAlbumOperationsUseGenericLibraryChunks(t *testing.T) { - for _, count := range []int{40, 41, 80, 81} { - t.Run(fmt.Sprint(count), func(t *testing.T) { + const count = 41 + for _, test := range []struct { + name string + kind spotifyref.Kind + prefix string + }{ + {name: "album", kind: spotifyref.Album, prefix: "spotify:album:"}, + {name: "track", kind: spotifyref.Track, prefix: "spotify:track:"}, + } { + t.Run(test.name, func(t *testing.T) { ids := libraryIDs(count) isSaved := func(uri string) bool { - index, _ := strconv.Atoi(strings.TrimPrefix(uri, "spotify:album:")) + index, _ := strconv.Atoi(strings.TrimPrefix(uri, test.prefix)) return index%5 == 0 || index%13 == 2 } var checkSizes []int + var checkSeen []string checkClient := &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { if request.Method != http.MethodGet || request.URL.Path != "/v1/me/library/contains" { t.Fatalf("request=%s %s", request.Method, request.URL.String()) } chunk := strings.Split(request.URL.Query().Get("uris"), ",") checkSizes = append(checkSizes, len(chunk)) + checkSeen = append(checkSeen, chunk...) values := make([]string, len(chunk)) for index, uri := range chunk { values[index] = strconv.FormatBool(isSaved(uri)) } return response(http.StatusOK, "["+strings.Join(values, ",")+"]"), nil })} - got, err := (Client{HTTPClient: checkClient}).CheckSavedItems(context.Background(), spotifyref.Album, ids) + got, err := (Client{HTTPClient: checkClient}).CheckSavedItems(context.Background(), test.kind, ids) want := make([]bool, count) + wantURIs := make([]string, count) for index, id := range ids { - want[index] = isSaved("spotify:album:" + id) + wantURIs[index] = test.prefix + id + want[index] = isSaved(wantURIs[index]) } - if err != nil || !slices.Equal(got, want) || len(checkSizes) != (count+39)/40 { - t.Fatalf("check sizes=%v results=%v want=%v error=%v", checkSizes, got, want, err) + if err != nil || !slices.Equal(got, want) || !slices.Equal(checkSizes, []int{40, 1}) || !slices.Equal(checkSeen, wantURIs) { + t.Fatalf("check sizes=%v seen=%v results=%v want=%v error=%v", checkSizes, checkSeen, got, want, err) } for _, method := range []string{http.MethodPut, http.MethodDelete} { @@ -358,18 +296,13 @@ func TestSavedAlbumOperationsUseGenericLibraryChunks(t *testing.T) { })} spotify := Client{HTTPClient: httpClient} if method == http.MethodPut { - err = spotify.SaveSavedItems(context.Background(), spotifyref.Album, ids) + err = spotify.SaveSavedItems(context.Background(), test.kind, ids) } else { - err = spotify.RemoveSavedItems(context.Background(), spotifyref.Album, ids) + err = spotify.RemoveSavedItems(context.Background(), test.kind, ids) } - if err != nil || len(sizes) != (count+39)/40 || !slices.Equal(seen, albumLibraryURIs(count)) { + if err != nil || !slices.Equal(sizes, []int{40, 1}) || !slices.Equal(seen, wantURIs) { t.Fatalf("method=%s sizes=%v seen=%v error=%v", method, sizes, seen, err) } - for index, size := range sizes { - if size != min(40, count-index*40) { - t.Fatalf("method=%s sizes=%v", method, sizes) - } - } } }) } @@ -423,11 +356,3 @@ func libraryIDs(count int) []string { } return result } - -func albumLibraryURIs(count int) []string { - result := make([]string, count) - for index := range result { - result[index] = fmt.Sprintf("spotify:album:%022d", index) - } - return result -} diff --git a/internal/livesmoke/playlist_contract_test.go b/internal/client/live_playlist_contract_test.go similarity index 99% rename from internal/livesmoke/playlist_contract_test.go rename to internal/client/live_playlist_contract_test.go index b9d6d69..a1044c4 100644 --- a/internal/livesmoke/playlist_contract_test.go +++ b/internal/client/live_playlist_contract_test.go @@ -1,6 +1,6 @@ //go:build spotify_live -package livesmoke +package client_test import ( "context" diff --git a/internal/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index 06df529..14da9b9 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -17,6 +17,7 @@ import ( "github.com/open-cli-collective/spotify-cli/internal/client" "github.com/open-cli-collective/spotify-cli/internal/config" "github.com/open-cli-collective/spotify-cli/internal/credentials" + "github.com/open-cli-collective/spotify-cli/internal/credstoretest" "github.com/open-cli-collective/spotify-cli/internal/exitcode" "github.com/open-cli-collective/spotify-cli/internal/token" ) @@ -60,7 +61,7 @@ func TestInteractiveInitPromptsVerifiesThenCommits(t *testing.T) { if loaded.ClientID != "client-id" || loaded.Keyring.Backend != "file" { t.Fatalf("saved config = %+v", loaded) } - stored, err := token.Decode([]byte(harness.store.values["default/"+credentials.OAuthTokenKey]), harness.now) + stored, err := token.Decode([]byte(harness.store.Values["default/"+credentials.OAuthTokenKey]), harness.now) if err != nil { t.Fatal(err) } @@ -89,7 +90,7 @@ func TestNonInteractiveInitHasFlagParityAndNeverPrompts(t *testing.T) { ); err != nil { t.Fatal(err) } - if _, ok := harness.store.values["automation/"+credentials.OAuthTokenKey]; !ok { + if _, ok := harness.store.Values["automation/"+credentials.OAuthTokenKey]; !ok { t.Fatal("credential was not written to requested ref") } } @@ -128,7 +129,7 @@ func TestInteractiveInitBackendChangeReplacesDestinationCredential(t *testing.T) t.Fatal(err) } key := "default/" + credentials.OAuthTokenKey - harness.store.values[key] = "stale-destination-credential" + harness.store.Values[key] = "stale-destination-credential" harness.interactive = true harness.prompt = func(setup *Setup) error { setup.ClientID = "client-id" @@ -138,8 +139,8 @@ func TestInteractiveInitBackendChangeReplacesDestinationCredential(t *testing.T) if err := harness.execute("--no-verify"); err != nil { t.Fatal(err) } - if harness.store.values[key] == "stale-destination-credential" || harness.store.setCalls != 1 { - t.Fatalf("credential = %q, set calls = %d", harness.store.values[key], harness.store.setCalls) + if harness.store.Values[key] == "stale-destination-credential" || harness.store.SetCalls != 1 { + t.Fatalf("credential = %q, set calls = %d", harness.store.Values[key], harness.store.SetCalls) } } @@ -150,7 +151,7 @@ func TestNonInteractiveInitBackendChangeStillRequiresOverwrite(t *testing.T) { if err := config.Save(harness.scope, cfg); err != nil { t.Fatal(err) } - harness.store.values["default/"+credentials.OAuthTokenKey] = "old-secret" + harness.store.Values["default/"+credentials.OAuthTokenKey] = "old-secret" err := harness.execute("--backend", "keychain", "--non-interactive", "--client-id", "client-id", "--no-verify") if !errors.Is(err, credstore.ErrExists) { t.Fatalf("error = %v", err) @@ -180,7 +181,7 @@ func TestInitArgumentMessage(t *testing.T) { func TestInitRefusesExistingCredentialBeforeAuthorization(t *testing.T) { harness := newInitHarness(t) - harness.store.values["default/"+credentials.OAuthTokenKey] = "old-secret" + harness.store.Values["default/"+credentials.OAuthTokenKey] = "old-secret" called := false harness.authorize = func(context.Context, auth.Request) (token.Envelope, error) { called = true @@ -195,12 +196,12 @@ func TestInitRefusesExistingCredentialBeforeAuthorization(t *testing.T) { func TestInitOverwritesExistingCredentialWhenRequested(t *testing.T) { harness := newInitHarness(t) key := "default/" + credentials.OAuthTokenKey - harness.store.values[key] = "old-secret" + harness.store.Values[key] = "old-secret" if err := harness.execute("--client-id", "client-id", "--no-verify", "--overwrite"); err != nil { t.Fatal(err) } - if harness.store.values[key] == "old-secret" || harness.store.setCalls != 1 { - t.Fatalf("credential = %q, set calls = %d", harness.store.values[key], harness.store.setCalls) + if harness.store.Values[key] == "old-secret" || harness.store.SetCalls != 1 { + t.Fatalf("credential = %q, set calls = %d", harness.store.Values[key], harness.store.SetCalls) } } @@ -211,8 +212,8 @@ func TestInitVerificationFailureWritesNothing(t *testing.T) { return client.User{}, errors.New("verification failed") } err := harness.execute("--client-id", "client-id") - if exitcode.Code(err) != exitcode.Upstream || harness.store.setCalls != 0 || containsEvent(harness.events, "save") { - t.Fatalf("error=%v events=%v setCalls=%d", err, harness.events, harness.store.setCalls) + if exitcode.Code(err) != exitcode.Upstream || harness.store.SetCalls != 0 || containsEvent(harness.events, "save") { + t.Fatalf("error=%v events=%v setCalls=%d", err, harness.events, harness.store.SetCalls) } } @@ -225,8 +226,8 @@ func TestInitMissingConfigSaverWritesNothing(t *testing.T) { return harness.envelope(), nil } err := harness.execute("--client-id", "client-id", "--no-verify") - if exitcode.Code(err) != exitcode.Config || called || harness.store.setCalls != 0 { - t.Fatalf("error=%v authorize=%t setCalls=%d", err, called, harness.store.setCalls) + if exitcode.Code(err) != exitcode.Config || called || harness.store.SetCalls != 0 { + t.Fatalf("error=%v authorize=%t setCalls=%d", err, called, harness.store.SetCalls) } } @@ -248,8 +249,8 @@ func TestInitClassifiesAuthorizationFailures(t *testing.T) { return token.Envelope{}, test.err } err := harness.execute("--client-id", "client-id", "--no-verify") - if exitcode.Code(err) != test.code || harness.store.setCalls != 0 { - t.Fatalf("source %v: error=%v code=%d setCalls=%d", test.err, err, exitcode.Code(err), harness.store.setCalls) + if exitcode.Code(err) != test.code || harness.store.SetCalls != 0 { + t.Fatalf("source %v: error=%v code=%d setCalls=%d", test.err, err, exitcode.Code(err), harness.store.SetCalls) } } } @@ -265,13 +266,13 @@ func TestInitializerFailureSitesRetainExitCodes(t *testing.T) { {name: "store open", configure: func(h *initHarness) { h.openStore = func(credentials.OpenRequest) (CredentialStore, error) { return nil, errors.New("open failed") } }, code: exitcode.Config}, - {name: "exists", configure: func(h *initHarness) { h.store.existsErr = errors.New("exists failed") }, code: exitcode.Config}, + {name: "exists", configure: func(h *initHarness) { h.store.ExistsErr = errors.New("exists failed") }, code: exitcode.Config}, {name: "existing without overwrite", configure: func(h *initHarness) { - h.store.values["default/"+credentials.OAuthTokenKey] = "old-secret" + h.store.Values["default/"+credentials.OAuthTokenKey] = "old-secret" }, code: exitcode.Generic}, {name: "read previous", configure: func(h *initHarness) { - h.store.values["default/"+credentials.OAuthTokenKey] = "old-secret" - h.store.getErr = errors.New("get failed") + h.store.Values["default/"+credentials.OAuthTokenKey] = "old-secret" + h.store.GetErr = errors.New("get failed") }, args: []string{"--overwrite"}, code: exitcode.Config}, {name: "nil authorizer", configure: func(h *initHarness) { h.authorize = nil }, code: exitcode.Generic}, {name: "nil verifier", configure: func(h *initHarness) { h.verify = nil }, args: []string{}, code: exitcode.Generic}, @@ -314,7 +315,7 @@ func TestInitRollsBackCredentialWhenConfigSaveFails(t *testing.T) { harness := newInitHarness(t) key := "default/" + credentials.OAuthTokenKey if test.oldValue != "" { - harness.store.values[key] = test.oldValue + harness.store.Values[key] = test.oldValue } harness.saveConfig = func(config.Config) error { harness.events = append(harness.events, "save") @@ -328,7 +329,7 @@ func TestInitRollsBackCredentialWhenConfigSaveFails(t *testing.T) { if exitcode.Code(err) != exitcode.Config { t.Fatalf("error = %v code=%d", err, exitcode.Code(err)) } - if got := harness.store.values[key]; got != test.want { + if got := harness.store.Values[key]; got != test.want { t.Fatalf("credential after rollback = %q, want %q", got, test.want) } }) @@ -344,7 +345,7 @@ func TestInitCredentialAndRollbackFailuresAreSecretSafe(t *testing.T) { { name: "credential write", configure: func(harness *initHarness) { - harness.store.setErr = errors.New("backend echoed access-secret and refresh-secret") + harness.store.SetErr = errors.New("backend echoed access-secret and refresh-secret") }, args: []string{"--client-id", "client-id", "--no-verify"}, }, @@ -352,16 +353,16 @@ func TestInitCredentialAndRollbackFailuresAreSecretSafe(t *testing.T) { name: "new credential rollback", configure: func(harness *initHarness) { harness.saveConfig = func(config.Config) error { return errors.New("config echoed access-secret") } - harness.store.deleteErr = errors.New("delete echoed refresh-secret") + harness.store.DeleteErr = errors.New("delete echoed refresh-secret") }, args: []string{"--client-id", "client-id", "--no-verify"}, }, { name: "overwritten credential rollback", configure: func(harness *initHarness) { - harness.store.values["default/"+credentials.OAuthTokenKey] = "old-secret" + harness.store.Values["default/"+credentials.OAuthTokenKey] = "old-secret" harness.saveConfig = func(config.Config) error { return errors.New("config echoed old-secret") } - harness.store.setErrors = []error{nil, errors.New("restore echoed old-secret and access-secret")} + harness.store.SetErrs = []error{nil, errors.New("restore echoed old-secret and access-secret")} }, args: []string{"--client-id", "client-id", "--no-verify", "--overwrite"}, }, @@ -386,7 +387,7 @@ func TestInitCredentialAndRollbackFailuresAreSecretSafe(t *testing.T) { type initHarness struct { scope statedir.Scope - store *initStore + store *credstoretest.Store now time.Time interactive bool backend string @@ -406,7 +407,7 @@ func newInitHarness(t *testing.T) *initHarness { statedirtest.Hermetic(t) harness := &initHarness{ scope: statedir.Scope{Name: config.Service}, - store: &initStore{values: map[string]string{}}, + store: &credstoretest.Store{Values: map[string]string{}}, now: time.Now().UTC(), } harness.authorize = func(context.Context, auth.Request) (token.Envelope, error) { @@ -421,7 +422,7 @@ func newInitHarness(t *testing.T) *initHarness { harness.events = append(harness.events, "save") return config.Save(harness.scope, value) } - harness.store.onSet = func() { harness.events = append(harness.events, "set") } + harness.store.OnSet = func() { harness.events = append(harness.events, "set") } harness.openStore = func(request credentials.OpenRequest) (CredentialStore, error) { harness.requests = append(harness.requests, request) return harness.store, nil @@ -454,59 +455,6 @@ func (harness *initHarness) execute(args ...string) error { return command.ExecuteContext(context.Background()) } -type initStore struct { - values map[string]string - setCalls int - setErr error - setErrors []error - deleteErr error - existsErr error - getErr error - onSet func() -} - -func (*initStore) Close() error { return nil } -func (store *initStore) Get(profile, key string) (string, error) { - if store.getErr != nil { - return "", store.getErr - } - value, ok := store.values[profile+"/"+key] - if !ok { - return "", credstore.ErrNotFound - } - return value, nil -} -func (store *initStore) Set(profile, key, value string, _ ...credstore.SetOpt) error { - store.setCalls++ - if store.onSet != nil { - store.onSet() - } - err := store.setErr - if len(store.setErrors) > 0 { - err = store.setErrors[0] - store.setErrors = store.setErrors[1:] - } - if err != nil { - return err - } - store.values[profile+"/"+key] = value - return nil -} -func (store *initStore) Delete(profile, key string) error { - if store.deleteErr != nil { - return store.deleteErr - } - delete(store.values, profile+"/"+key) - return nil -} -func (store *initStore) Exists(profile, key string) (bool, error) { - if store.existsErr != nil { - return false, store.existsErr - } - _, ok := store.values[profile+"/"+key] - return ok, nil -} - func containsEvent(events []string, want string) bool { for _, event := range events { if event == want { diff --git a/internal/cmd/mecmd/me_test.go b/internal/cmd/mecmd/me_test.go index 16c4c8e..8e4f167 100644 --- a/internal/cmd/mecmd/me_test.go +++ b/internal/cmd/mecmd/me_test.go @@ -18,6 +18,7 @@ import ( "github.com/open-cli-collective/spotify-cli/internal/config" "github.com/open-cli-collective/spotify-cli/internal/credentials" + "github.com/open-cli-collective/spotify-cli/internal/credstoretest" "github.com/open-cli-collective/spotify-cli/internal/exitcode" "github.com/open-cli-collective/spotify-cli/internal/session" "github.com/open-cli-collective/spotify-cli/internal/token" @@ -114,7 +115,7 @@ func TestMeRefreshesAndPersistsSameCredential(t *testing.T) { if err != nil { t.Fatal(err) } - stored, err := token.Decode([]byte(harness.store.values["default/"+credentials.OAuthTokenKey]), now) + stored, err := token.Decode([]byte(harness.store.Values["default/"+credentials.OAuthTokenKey]), now) if err != nil { t.Fatal(err) } @@ -124,8 +125,8 @@ func TestMeRefreshesAndPersistsSameCredential(t *testing.T) { if !strings.Contains(stdout, "scopes\tplaylist-read-private,user-read-private\n") { t.Fatalf("stdout = %q", stdout) } - if harness.store.setCalls != 1 || !harness.store.overwrite { - t.Fatalf("set calls = %d overwrite = %t", harness.store.setCalls, harness.store.overwrite) + if harness.store.SetCalls != 1 || !harness.store.Overwrite { + t.Fatalf("set calls = %d overwrite = %t", harness.store.SetCalls, harness.store.Overwrite) } } @@ -162,7 +163,7 @@ func TestMeFailureClasses(t *testing.T) { }) t.Run("invalid stored credential", func(t *testing.T) { harness := newHarness(t, now, nil) - harness.store.values["default/"+credentials.OAuthTokenKey] = "invalid-token-secret-sentinel" + harness.store.Values["default/"+credentials.OAuthTokenKey] = "invalid-token-secret-sentinel" stdout, err := harness.execute() if exitcode.Code(err) != exitcode.Config || stdout != "" || strings.Contains(err.Error(), "invalid-token-secret-sentinel") { t.Fatalf("stdout=%q error=%v code=%d", stdout, err, exitcode.Code(err)) @@ -249,7 +250,7 @@ func TestMeReportsOutputWriterFailure(t *testing.T) { type meHarness struct { scope statedir.Scope - store *memoryStore + store *credstoretest.Store now time.Time httpClient *http.Client tokenURL string @@ -266,7 +267,7 @@ func newHarness(t *testing.T, now time.Time, server *httptest.Server) *meHarness if err := config.Save(scope, cfg); err != nil { t.Fatal(err) } - harness := &meHarness{scope: scope, store: &memoryStore{values: map[string]string{}}, now: now} + harness := &meHarness{scope: scope, store: &credstoretest.Store{Values: map[string]string{}}, now: now} if server != nil { harness.httpClient = server.Client() harness.apiBaseURL = server.URL + "/v1" @@ -280,7 +281,7 @@ func (harness *meHarness) storeEnvelope(t *testing.T, value token.Envelope) { if err != nil { t.Fatal(err) } - harness.store.values["default/"+credentials.OAuthTokenKey] = string(encoded) + harness.store.Values["default/"+credentials.OAuthTokenKey] = string(encoded) } func (harness *meHarness) execute(args ...string) (string, error) { @@ -315,27 +316,6 @@ type failingWriter struct{} func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("writer failed") } -type memoryStore struct { - values map[string]string - setCalls int - overwrite bool -} - -func (store *memoryStore) Close() error { return nil } -func (store *memoryStore) Get(profile, key string) (string, error) { - value, ok := store.values[profile+"/"+key] - if !ok { - return "", credstore.ErrNotFound - } - return value, nil -} -func (store *memoryStore) Set(profile, key, value string, opts ...credstore.SetOpt) error { - store.setCalls++ - store.overwrite = len(opts) > 0 - store.values[profile+"/"+key] = value - return nil -} - type failingTransport struct{} func (failingTransport) RoundTrip(*http.Request) (*http.Response, error) { diff --git a/internal/cmd/root/root_test.go b/internal/cmd/root/root_test.go index d388bcd..74b028a 100644 --- a/internal/cmd/root/root_test.go +++ b/internal/cmd/root/root_test.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "path/filepath" + "slices" "strings" "testing" "time" @@ -21,59 +22,20 @@ import ( "github.com/open-cli-collective/spotify-cli/internal/auth" "github.com/open-cli-collective/spotify-cli/internal/config" "github.com/open-cli-collective/spotify-cli/internal/credentials" + "github.com/open-cli-collective/spotify-cli/internal/credstoretest" "github.com/open-cli-collective/spotify-cli/internal/exitcode" "github.com/open-cli-collective/spotify-cli/internal/token" ) -type fakeStore struct { - values map[string]string - backend credstore.Backend - source credstore.Source - setErr error -} - type failingWriter struct{} func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("writer failed") } -func (s *fakeStore) Backend() (credstore.Backend, credstore.Source) { return s.backend, s.source } -func (s *fakeStore) Close() error { return nil } -func (s *fakeStore) Get(profile, key string) (string, error) { - value, ok := s.values[profile+"/"+key] - if !ok { - return "", credstore.ErrNotFound - } - return value, nil -} -func (s *fakeStore) Set(profile, key, value string, opts ...credstore.SetOpt) error { - if s.setErr != nil { - return s.setErr - } - item := profile + "/" + key - if _, ok := s.values[item]; ok && len(opts) == 0 { - return credstore.ErrExists - } - s.values[item] = value - return nil -} -func (s *fakeStore) Delete(profile, key string) error { - item := profile + "/" + key - if _, ok := s.values[item]; !ok { - return credstore.ErrNotFound - } - delete(s.values, item) - return nil -} -func (s *fakeStore) Exists(profile, key string) (bool, error) { - _, ok := s.values[profile+"/"+key] - return ok, nil -} - type harness struct { in *bytes.Buffer out *bytes.Buffer errOut *bytes.Buffer - store *fakeStore + store *credstoretest.Store requests []credentials.OpenRequest deps Dependencies } @@ -85,10 +47,10 @@ func newHarness(t *testing.T) *harness { in: &bytes.Buffer{}, out: &bytes.Buffer{}, errOut: &bytes.Buffer{}, - store: &fakeStore{ - values: map[string]string{}, - backend: credstore.BackendMemory, - source: credstore.SourceExplicit, + store: &credstoretest.Store{ + Values: map[string]string{}, + BackendValue: credstore.BackendMemory, + Source: credstore.SourceExplicit, }, } openStore := func(request credentials.OpenRequest) (credentials.Store, error) { @@ -142,165 +104,94 @@ func TestUnknownCommandsAreUsageErrors(t *testing.T) { } } -func TestSearchCommandsAreWiredToAuthenticatedSession(t *testing.T) { - for _, resource := range []string{"track", "album", "artist"} { - h := newHarness(t) - cfg := config.Default() - cfg.ClientID = "client-id" - if err := config.Save(h.deps.Scope, cfg); err != nil { - t.Fatal(err) - } - err := h.execute("search", resource, "query") - if exitcode.Code(err) != exitcode.Config || len(h.requests) != 1 { - t.Fatalf("resource=%s error=%v code=%d store opens=%d", resource, err, exitcode.Code(err), len(h.requests)) - } - } -} - -func TestSearchCommandsRejectJSONBeforeSession(t *testing.T) { - for _, resource := range []string{"track", "album", "artist"} { - h := newHarness(t) - err := h.execute("search", resource, "query", "--json") - if exitcode.Code(err) != exitcode.Usage || len(h.requests) != 0 { - t.Fatalf("resource=%s error=%v code=%d store opens=%d", resource, err, exitcode.Code(err), len(h.requests)) - } - } -} - -func TestLibraryCommandsAreWiredAndValidateBeforeSession(t *testing.T) { +func TestAuthenticatedCommandsAreWired(t *testing.T) { const id = "0123456789ABCDEFGHIJKL" for _, test := range []struct { - resource string - verb string - args []string + name string + args []string }{ - {resource: "tracks", verb: "list", args: []string{"library", "tracks", "list"}}, - {resource: "tracks", verb: "check", args: []string{"library", "tracks", "check", id}}, - {resource: "tracks", verb: "add", args: []string{"library", "tracks", "add", id}}, - {resource: "tracks", verb: "remove", args: []string{"library", "tracks", "remove", id}}, - {resource: "albums", verb: "list", args: []string{"library", "albums", "list"}}, - {resource: "albums", verb: "check", args: []string{"library", "albums", "check", id}}, - {resource: "albums", verb: "add", args: []string{"library", "albums", "add", id}}, - {resource: "albums", verb: "remove", args: []string{"library", "albums", "remove", id}}, + {name: "search", args: []string{"search", "track", "query"}}, + {name: "library", args: []string{"library", "tracks", "list"}}, + {name: "catalog get", args: []string{"tracks", "get", id}}, + {name: "catalog traversal", args: []string{"albums", "tracks", "list", id}}, + {name: "playlist", args: []string{"playlists", "list"}}, } { - h := newHarness(t) - if test.verb != "list" { - if err := h.execute("library", test.resource, test.verb, id, "bad"); exitcode.Code(err) != exitcode.Usage || len(h.requests) != 0 { - t.Fatalf("resource=%s verb=%s invalid error=%v code=%d store opens=%d", test.resource, test.verb, err, exitcode.Code(err), len(h.requests)) + t.Run(test.name, func(t *testing.T) { + h := newHarness(t) + saveClientID(t, h.deps.Scope) + err := h.execute(test.args...) + if exitcode.Code(err) != exitcode.Config || len(h.requests) != 1 { + t.Fatalf("error=%v code=%d store opens=%d", err, exitcode.Code(err), len(h.requests)) } - } - cfg := config.Default() - cfg.ClientID = "client-id" - if err := config.Save(h.deps.Scope, cfg); err != nil { - t.Fatal(err) - } - err := h.execute(test.args...) - if exitcode.Code(err) != exitcode.Config || len(h.requests) != 1 { - t.Fatalf("resource=%s verb=%s error=%v code=%d store opens=%d", test.resource, test.verb, err, exitcode.Code(err), len(h.requests)) - } + }) } } -func TestSavedAlbumArtistsFlowFromProviderJSONToCommandOutput(t *testing.T) { +func TestPlaylistAddUsesRealSessionWiring(t *testing.T) { const ( - albumID = "0123456789ABCDEFGHIJKL" - artistOne = "abcdefghijklmnopqrstuv" - artistTwo = "ABCDEFGHIJKLMNOPQRSTUV" + playlistID = "0123456789ABCDEFGHIJKL" + trackID = "abcdefghijklmnopqrstuv" ) h := newHarness(t) + saveClientID(t, h.deps.Scope) now := h.deps.Now() - cfg := config.Default() - cfg.ClientID = "client-id" - if err := config.Save(h.deps.Scope, cfg); err != nil { - t.Fatal(err) - } encoded, err := token.Encode(token.Envelope{ - AccessToken: "access", TokenType: "Bearer", RefreshToken: "refresh", - ExpiresAt: time.Now().UTC().Add(time.Hour), Scopes: []string{auth.ScopeUserLibraryRead}, + AccessToken: "access", TokenType: "Bearer", RefreshToken: "refresh", ExpiresAt: time.Now().UTC().Add(time.Hour), + Scopes: []string{ + auth.ScopePlaylistModifyPrivate, auth.ScopePlaylistModifyPublic, + auth.ScopePlaylistReadCollaborative, auth.ScopePlaylistReadPrivate, + }, }, now) if err != nil { t.Fatal(err) } - h.store.values["default/oauth_token"] = string(encoded) + h.store.Values["default/oauth_token"] = string(encoded) + var paths []string server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { - if request.URL.Path != "/v1/me/albums" || request.Header.Get("Authorization") != "Bearer access" { - t.Fatalf("request=%s authorization=%q", request.URL.String(), request.Header.Get("Authorization")) + paths = append(paths, request.URL.Path) + if request.Header.Get("Authorization") != "Bearer access" { + t.Fatalf("authorization=%q", request.Header.Get("Authorization")) } writer.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(writer, `{"items":[{"added_at":"2026-07-23T12:00:00Z","album":{"id":"`+albumID+`","name":"Duets","artists":[{"id":"`+artistOne+`","name":"First"},{"id":"`+artistTwo+`","name":"Second"}],"release_date":"2026","total_tracks":2}}],"limit":10,"offset":0,"total":1,"next":null}`) + switch { + case request.Method == http.MethodGet && request.URL.Path == "/v1/playlists/"+playlistID: + _, _ = io.WriteString(writer, `{"id":"`+playlistID+`","name":"Mix","owner":{"id":"owner-1","display_name":"Ada"},"items":{"total":3},"public":false,"collaborative":false,"snapshot_id":"before"}`) + case request.Method == http.MethodPost && request.URL.Path == "/v1/playlists/"+playlistID+"/items": + _, _ = io.WriteString(writer, `{"snapshot_id":"after-add"}`) + default: + http.NotFound(writer, request) + } })) t.Cleanup(server.Close) h.deps.HTTPClient = server.Client() h.deps.APIBaseURL = server.URL + "/v1" - if err := h.execute("library", "albums", "list"); err != nil { - t.Fatal(err) + if err := h.execute("playlists", "items", "add", playlistID, trackID, "--position", "1"); err != nil { + t.Fatalf("error=%v paths=%v", err, paths) } - want := "ADDED_AT | ID | ALBUM | ARTIST_IDS | ARTISTS | RELEASE_DATE | TOTAL_TRACKS\n" + - "2026-07-23T12:00:00Z | " + albumID + " | Duets | " + artistOne + "," + artistTwo + " | First,Second | 2026 | 2\n" - if h.out.String() != want || h.errOut.Len() != 0 { - t.Fatalf("stdout=%q stderr=%q", h.out.String(), h.errOut.String()) + if h.out.String() != "added\t"+playlistID+"\t1\t1\tafter-add\n" || h.errOut.Len() != 0 || + !slices.Equal(paths, []string{"/v1/playlists/" + playlistID, "/v1/playlists/" + playlistID + "/items"}) { + t.Fatalf("stdout=%q stderr=%q paths=%v", h.out.String(), h.errOut.String(), paths) } } -func TestCatalogGetCommandsAreWiredAndValidateBeforeSession(t *testing.T) { - const id = "0123456789ABCDEFGHIJKL" - for _, resource := range []string{"tracks", "albums", "artists"} { - h := newHarness(t) - if err := h.execute(resource, "get", "bad"); exitcode.Code(err) != exitcode.Usage || len(h.requests) != 0 { - t.Fatalf("resource=%s invalid error=%v code=%d store opens=%d", resource, err, exitcode.Code(err), len(h.requests)) - } - - h = newHarness(t) - cfg := config.Default() - cfg.ClientID = "client-id" - if err := config.Save(h.deps.Scope, cfg); err != nil { - t.Fatal(err) - } - err := h.execute(resource, "get", id) - if exitcode.Code(err) != exitcode.Config || len(h.requests) != 1 { - t.Fatalf("resource=%s error=%v code=%d store opens=%d", resource, err, exitcode.Code(err), len(h.requests)) - } - - h = newHarness(t) - err = h.execute(resource, "get", id, "--json") - if exitcode.Code(err) != exitcode.Usage || len(h.requests) != 0 { - t.Fatalf("resource=%s JSON error=%v code=%d store opens=%d", resource, err, exitcode.Code(err), len(h.requests)) - } - } -} - -func TestCatalogTraversalCommandsAreWiredAndValidateBeforeSession(t *testing.T) { - const id = "0123456789ABCDEFGHIJKL" - for _, args := range [][]string{ - {"albums", "tracks", "list", id, "--max", "51"}, - {"artists", "albums", "list", id, "--max", "11"}, - } { - h := newHarness(t) - if err := h.execute(args...); exitcode.Code(err) != exitcode.Usage || len(h.requests) != 0 { - t.Fatalf("args=%v error=%v code=%d store opens=%d", args, err, exitcode.Code(err), len(h.requests)) - } - } - for _, args := range [][]string{ - {"albums", "tracks", "list", id}, - {"artists", "albums", "list", id}, - } { - h := newHarness(t) - cfg := config.Default() - cfg.ClientID = "client-id" - if err := config.Save(h.deps.Scope, cfg); err != nil { - t.Fatal(err) - } - err := h.execute(args...) - if exitcode.Code(err) != exitcode.Config || len(h.requests) != 1 { - t.Fatalf("args=%v error=%v code=%d store opens=%d", args, err, exitcode.Code(err), len(h.requests)) - } +func saveClientID(t *testing.T, scope statedir.Scope) { + t.Helper() + cfg := config.Default() + cfg.ClientID = "client-id" + if err := config.Save(scope, cfg); err != nil { + t.Fatal(err) } } -func TestPlaylistCommandsUseRealSessionAndClientPath(t *testing.T) { - const id = "0123456789ABCDEFGHIJKL" +func TestSavedAlbumArtistsFlowFromProviderJSONToCommandOutput(t *testing.T) { + const ( + albumID = "0123456789ABCDEFGHIJKL" + artistOne = "abcdefghijklmnopqrstuv" + artistTwo = "ABCDEFGHIJKLMNOPQRSTUV" + ) h := newHarness(t) + now := h.deps.Now() cfg := config.Default() cfg.ClientID = "client-id" if err := config.Save(h.deps.Scope, cfg); err != nil { @@ -308,73 +199,30 @@ func TestPlaylistCommandsUseRealSessionAndClientPath(t *testing.T) { } encoded, err := token.Encode(token.Envelope{ AccessToken: "access", TokenType: "Bearer", RefreshToken: "refresh", - ExpiresAt: time.Now().UTC().Add(time.Hour), - Scopes: []string{ - auth.ScopePlaylistModifyPrivate, auth.ScopePlaylistModifyPublic, - auth.ScopePlaylistReadCollaborative, auth.ScopePlaylistReadPrivate, - }, - }, h.deps.Now()) + ExpiresAt: time.Now().UTC().Add(time.Hour), Scopes: []string{auth.ScopeUserLibraryRead}, + }, now) if err != nil { t.Fatal(err) } - h.store.values["default/oauth_token"] = string(encoded) - var paths []string + h.store.Values["default/oauth_token"] = string(encoded) server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { - paths = append(paths, request.URL.RequestURI()) - if request.Header.Get("Authorization") != "Bearer access" { - t.Fatalf("authorization=%q", request.Header.Get("Authorization")) + if request.URL.Path != "/v1/me/albums" || request.Header.Get("Authorization") != "Bearer access" { + t.Fatalf("request=%s authorization=%q", request.URL.String(), request.Header.Get("Authorization")) } writer.Header().Set("Content-Type", "application/json") - switch request.URL.Path { - case "/v1/me/playlists": - _, _ = io.WriteString(writer, `{"items":[{"id":"`+id+`","name":"Mix","owner":{"id":"owner-1","display_name":"Ada"},"items":{"total":3},"public":null,"collaborative":true}],"limit":10,"offset":0,"total":1,"next":null}`) - case "/v1/playlists/" + id: - _, _ = io.WriteString(writer, `{"id":"`+id+`","name":"Mix","owner":{"id":"owner-1","display_name":"Ada"},"items":{"total":3},"public":null,"collaborative":true}`) - case "/v1/playlists/" + id + "/items": - if request.Method == http.MethodPost { - _, _ = io.WriteString(writer, `{"snapshot_id":"after-add"}`) - } else { - _, _ = io.WriteString(writer, `{"items":[{"added_at":"2026-07-24T12:00:00Z","added_by":{"id":"owner-1"},"is_local":false,"item":{"type":"track","id":"abcdefghijklmnopqrstuv","name":"Song","artists":[{"id":"1111111111111111111111","name":"Artist"}],"album":{"id":"2222222222222222222222","name":"Album","images":[]},"duration_ms":61000}}],"limit":10,"offset":0,"total":1,"next":null}`) - } - default: - http.NotFound(writer, request) - } + _, _ = io.WriteString(writer, `{"items":[{"added_at":"2026-07-23T12:00:00Z","album":{"id":"`+albumID+`","name":"Duets","artists":[{"id":"`+artistOne+`","name":"First"},{"id":"`+artistTwo+`","name":"Second"}],"release_date":"2026","total_tracks":2}}],"limit":10,"offset":0,"total":1,"next":null}`) })) t.Cleanup(server.Close) h.deps.HTTPClient = server.Client() h.deps.APIBaseURL = server.URL + "/v1" - if err := h.execute("playlists", "list"); err != nil { - t.Fatal(err) - } - wantList := "ID | PLAYLIST | OWNER_ID | OWNER | ITEM_COUNT | PUBLIC | COLLABORATIVE\n" + id + " | Mix | owner-1 | Ada | 3 | - | true\n" - if h.out.String() != wantList || h.errOut.Len() != 0 { - t.Fatalf("list stdout=%q stderr=%q", h.out.String(), h.errOut.String()) - } - h.out.Reset() - if err := h.execute("playlists", "get", "spotify:playlist:"+id, "--fields", "playlist,owner_id"); err != nil { - t.Fatal(err) - } - if h.out.String() != id+" Mix\nOwner ID: owner-1\n" || h.errOut.Len() != 0 { - t.Fatalf("get stdout=%q stderr=%q", h.out.String(), h.errOut.String()) - } - h.out.Reset() - if err := h.execute("playlists", "items", "list", "https://open.spotify.com/playlist/"+id); err != nil { - t.Fatal(err) - } - wantItems := "Playlist ID: " + id + "\nPOSITION | TYPE | ID | ITEM | ARTIST_IDS | ARTISTS | ALBUM_ID | ALBUM | DURATION\n0 | track | abcdefghijklmnopqrstuv | Song | 1111111111111111111111 | Artist | 2222222222222222222222 | Album | 1:01\n" - if h.out.String() != wantItems || h.errOut.Len() != 0 { - t.Fatalf("items stdout=%q stderr=%q", h.out.String(), h.errOut.String()) - } - h.out.Reset() - if err := h.execute("playlists", "items", "add", id, "abcdefghijklmnopqrstuv", "--position", "1"); err != nil { + if err := h.execute("library", "albums", "list"); err != nil { t.Fatal(err) } - if h.out.String() != "added\t"+id+"\t1\t1\tafter-add\n" || h.errOut.Len() != 0 { - t.Fatalf("add stdout=%q stderr=%q", h.out.String(), h.errOut.String()) - } - if strings.Join(paths, ",") != "/v1/me/playlists?limit=10&offset=0,/v1/playlists/"+id+",/v1/playlists/"+id+"/items?additional_types=episode&limit=10&offset=0,/v1/playlists/"+id+",/v1/playlists/"+id+"/items" { - t.Fatalf("paths=%v", paths) + want := "ADDED_AT | ID | ALBUM | ARTIST_IDS | ARTISTS | RELEASE_DATE | TOTAL_TRACKS\n" + + "2026-07-23T12:00:00Z | " + albumID + " | Duets | " + artistOne + "," + artistTwo + " | First,Second | 2026 | 2\n" + if h.out.String() != want || h.errOut.Len() != 0 { + t.Fatalf("stdout=%q stderr=%q", h.out.String(), h.errOut.String()) } } @@ -637,7 +485,7 @@ func TestClearPartialFailureKeepsConfigExitWhenRenderingFails(t *testing.T) { func TestConfigShowJSONDoesNotLeakCredential(t *testing.T) { h := newHarness(t) canary := "stored-super-secret-canary" - h.store.values["default/oauth_token"] = canary + h.store.Values["default/oauth_token"] = canary if err := h.execute("config", "show", "--json"); err != nil { t.Fatal(err) @@ -692,7 +540,7 @@ func TestSetCredentialJSONCanonicalizesWithoutLeaking(t *testing.T) { if strings.Contains(h.out.String()+h.errOut.String(), canary) { t.Fatal("set-credential output leaked the credential") } - stored := h.store.values["work/oauth_token"] + stored := h.store.Values["work/oauth_token"] if !strings.Contains(stored, canary) || !strings.Contains(stored, `"token_type":"Bearer"`) { t.Fatalf("stored envelope was not canonicalized: %q", stored) } @@ -761,7 +609,7 @@ func TestSetCredentialStoreErrorsAreRedacted(t *testing.T) { h := newHarness(t) canary := "access-super-secret-canary" raw := `{"version":1,"access_token":"` + canary + `","token_type":"Bearer","expires_at":"2026-07-22T13:00:00Z","scopes":["user-read-private"]}` - h.store.setErr = errors.New("backend failed while handling " + canary) + h.store.SetErr = errors.New("backend failed while handling " + canary) args := []string{"set-credential", "--ref", "spotify-cli/default", "--key", "oauth_token"} if ingress == "stdin" { h.in.WriteString(raw) @@ -794,7 +642,7 @@ func TestSetCredentialWrappedSentinelsAreRedactedAndClassified(t *testing.T) { t.Run(tt.name, func(t *testing.T) { h := newHarness(t) canary := "wrapped-sentinel-secret-canary" - h.store.setErr = fmt.Errorf("backend echoed %s: %w", canary, tt.cause) + h.store.SetErr = fmt.Errorf("backend echoed %s: %w", canary, tt.cause) h.in.WriteString(`{"version":1,"access_token":"` + canary + `","token_type":"Bearer","expires_at":"2026-07-22T13:00:00Z","scopes":["user-read-private"]}`) err := h.execute("set-credential", "--ref", "spotify-cli/default", "--key", "oauth_token", "--stdin", "--json") if exitcode.Code(err) != tt.code || !errors.Is(err, tt.cause) { @@ -833,7 +681,7 @@ func TestConfigShowReportsFilePassphraseSource(t *testing.T) { } { t.Run(tt.name, func(t *testing.T) { h := newHarness(t) - h.store.backend = credstore.BackendFile + h.store.BackendValue = credstore.BackendFile t.Setenv("SPOTIFY_CLI_KEYRING_PASSPHRASE", tt.value) if err := h.execute("config", "show", "--json"); err != nil { t.Fatal(err) @@ -853,7 +701,7 @@ func TestSetCredentialFromEnvironment(t *testing.T) { if err := h.execute("set-credential", "--ref", "spotify-cli/default", "--key", "oauth_token", "--from-env", envName); err != nil { t.Fatal(err) } - if h.store.values["default/oauth_token"] == "" { + if h.store.Values["default/oauth_token"] == "" { t.Fatal("credential was not stored") } } @@ -901,16 +749,16 @@ func TestConfigClearDryRunReportsOnlyActiveRef(t *testing.T) { if err := config.Save(h.deps.Scope, cfg); err != nil { t.Fatal(err) } - h.store.values["work/oauth_token"] = "active" - h.store.values["other/oauth_token"] = "inactive" + h.store.Values["work/oauth_token"] = "active" + h.store.Values["other/oauth_token"] = "inactive" if err := h.execute("config", "clear", "--dry-run"); err != nil { t.Fatal(err) } if got := h.out.String(); got != "would_remove\tcredential\tspotify-cli/work/oauth_token\n" { t.Fatalf("stdout = %q", got) } - if h.store.values["work/oauth_token"] != "active" || h.store.values["other/oauth_token"] != "inactive" || len(h.requests) != 0 { - t.Fatalf("dry-run mutated/opened store: values=%v opens=%d", h.store.values, len(h.requests)) + if h.store.Values["work/oauth_token"] != "active" || h.store.Values["other/oauth_token"] != "inactive" || len(h.requests) != 0 { + t.Fatalf("dry-run mutated/opened store: values=%v opens=%d", h.store.Values, len(h.requests)) } } @@ -973,16 +821,16 @@ func TestConfigClearAllRecoversFromMalformedConfig(t *testing.T) { func TestConfigClearOnlyActiveCredential(t *testing.T) { h := newHarness(t) - h.store.values["default/oauth_token"] = "active-canary" - h.store.values["other/oauth_token"] = "other-canary" + h.store.Values["default/oauth_token"] = "active-canary" + h.store.Values["other/oauth_token"] = "other-canary" if err := h.execute("config", "clear"); err != nil { t.Fatal(err) } - if _, ok := h.store.values["default/oauth_token"]; ok { + if _, ok := h.store.Values["default/oauth_token"]; ok { t.Fatal("active credential remains") } - if h.store.values["other/oauth_token"] != "other-canary" { + if h.store.Values["other/oauth_token"] != "other-canary" { t.Fatal("inactive credential was touched") } if got := h.out.String(); got != "removed\tcredential\tspotify-cli/default/oauth_token\n" { @@ -1000,7 +848,7 @@ func TestConfigClearOnlyActiveCredential(t *testing.T) { func TestSetCredentialExistingRequiresOverwrite(t *testing.T) { h := newHarness(t) - h.store.values["default/oauth_token"] = "old" + h.store.Values["default/oauth_token"] = "old" h.in.WriteString(`{"version":1,"access_token":"new","token_type":"Bearer","expires_at":"2026-07-22T13:00:00Z","scopes":["user-read-private"]}`) err := h.execute("set-credential", "--ref", "spotify-cli/default", "--key", "oauth_token", "--stdin") if !errors.Is(err, credstore.ErrExists) || exitcode.Code(err) != exitcode.Generic { @@ -1010,13 +858,13 @@ func TestSetCredentialExistingRequiresOverwrite(t *testing.T) { func TestSetCredentialOverwriteSucceeds(t *testing.T) { h := newHarness(t) - h.store.values["default/oauth_token"] = "old" + h.store.Values["default/oauth_token"] = "old" h.in.WriteString(`{"version":1,"access_token":"new","token_type":"Bearer","expires_at":"2026-07-22T13:00:00Z","scopes":["user-read-private"]}`) if err := h.execute("set-credential", "--ref", "spotify-cli/default", "--key", "oauth_token", "--stdin", "--overwrite"); err != nil { t.Fatal(err) } - if !strings.Contains(h.store.values["default/oauth_token"], `"access_token":"new"`) { - t.Fatalf("stored credential = %q", h.store.values["default/oauth_token"]) + if !strings.Contains(h.store.Values["default/oauth_token"], `"access_token":"new"`) { + t.Fatalf("stored credential = %q", h.store.Values["default/oauth_token"]) } } diff --git a/internal/cmd/searchcmd/search_test.go b/internal/cmd/searchcmd/search_test.go index ff2b821..f226a62 100644 --- a/internal/cmd/searchcmd/search_test.go +++ b/internal/cmd/searchcmd/search_test.go @@ -139,7 +139,8 @@ func TestSearchFactoryDerivesNounSpecificText(t *testing.T) { } { subcommand, _, err := command.Find([]string{test.noun}) if err != nil || subcommand.Use != test.use || subcommand.Short != test.short || - subcommand.Flags().Lookup("id").Usage != test.idUsage || subcommand.Flags().Lookup("extended").Usage != test.extendedUsage { + subcommand.Flags().Lookup("id").Usage != test.idUsage || subcommand.Flags().Lookup("extended").Usage != test.extendedUsage || + subcommand.Flags().Lookup("json") != nil { t.Fatalf("noun=%s command=%+v error=%v", test.noun, subcommand, err) } _, _, opens, err := executeSearch("", test.noun, "q", "--fields", "nope") diff --git a/internal/livesmoke/expire_test.go b/internal/credentials/live_expire_test.go similarity index 67% rename from internal/livesmoke/expire_test.go rename to internal/credentials/live_expire_test.go index 8f08bb0..ed5ac87 100644 --- a/internal/livesmoke/expire_test.go +++ b/internal/credentials/live_expire_test.go @@ -1,10 +1,12 @@ //go:build spotify_live -package livesmoke +package credentials_test import ( "errors" "os" + "path/filepath" + "strings" "testing" "time" @@ -59,3 +61,21 @@ func TestExpireCredential(t *testing.T) { t.Fatal(err) } } + +func requireLiveOptIn(t *testing.T) { + t.Helper() + if os.Getenv("SPOTIFY_CLI_LIVE") != "1" || os.Getenv("SPOTIFY_CLI_LIVE_DEDICATED_ACCOUNT") != "1" { + t.Skip("live smoke opt-in is not enabled") + } + root := os.Getenv("SPOTIFY_CLI_LIVE_ROOT") + if root == "" { + t.Fatal("SPOTIFY_CLI_LIVE_ROOT is required") + } + for _, name := range []string{"HOME", "USERPROFILE", "AppData", "LocalAppData", "XDG_CONFIG_HOME", "XDG_CACHE_HOME", "XDG_DATA_HOME", "XDG_STATE_HOME"} { + value := os.Getenv(name) + relative, err := filepath.Rel(root, value) + if err != nil || value == "" || relative == "." || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + t.Fatalf("%s is not isolated under the live root", name) + } + } +} diff --git a/internal/credstoretest/store.go b/internal/credstoretest/store.go new file mode 100644 index 0000000..602d320 --- /dev/null +++ b/internal/credstoretest/store.go @@ -0,0 +1,92 @@ +// Package credstoretest provides a credential store fake for tests. +package credstoretest + +import "github.com/open-cli-collective/cli-common/credstore" + +// Store is an in-memory credential store with injectable failures. +type Store struct { + Values map[string]string + BackendValue credstore.Backend + Source credstore.Source + GetErr error + SetErr error + SetErrs []error + DeleteErr error + ExistsErr error + CloseErr error + OnSet func() + SetCalls int + Closed bool + Overwrite bool +} + +// Backend returns the configured backend metadata. +func (store *Store) Backend() (credstore.Backend, credstore.Source) { + return store.BackendValue, store.Source +} + +// Close records closure without disabling later inspection or use. +func (store *Store) Close() error { + store.Closed = true + return store.CloseErr +} + +// Get returns a stored value or credstore.ErrNotFound. +func (store *Store) Get(profile, key string) (string, error) { + if store.GetErr != nil { + return "", store.GetErr + } + value, ok := store.Values[profile+"/"+key] + if !ok { + return "", credstore.ErrNotFound + } + return value, nil +} + +// Set records the call before applying queued or persistent failures. +func (store *Store) Set(profile, key, value string, opts ...credstore.SetOpt) error { + store.SetCalls++ + store.Overwrite = len(opts) > 0 + if store.OnSet != nil { + store.OnSet() + } + err := store.SetErr + if len(store.SetErrs) > 0 { + err = store.SetErrs[0] + store.SetErrs = store.SetErrs[1:] + } + if err != nil { + return err + } + item := profile + "/" + key + if _, exists := store.Values[item]; exists && !store.Overwrite { + return credstore.ErrExists + } + if store.Values == nil { + store.Values = map[string]string{} + } + store.Values[item] = value + return nil +} + +// Delete removes a stored value or returns credstore.ErrNotFound. +func (store *Store) Delete(profile, key string) error { + if store.DeleteErr != nil { + return store.DeleteErr + } + item := profile + "/" + key + if _, exists := store.Values[item]; !exists { + return credstore.ErrNotFound + } + delete(store.Values, item) + return nil +} + +// Exists reports whether a value is stored. +func (store *Store) Exists(profile, key string) (bool, error) { + if store.ExistsErr != nil { + return false, store.ExistsErr + } + _, exists := store.Values[profile+"/"+key] + return exists, nil +} diff --git a/internal/livesmoke/doc.go b/internal/livesmoke/doc.go deleted file mode 100644 index 66acdd6..0000000 --- a/internal/livesmoke/doc.go +++ /dev/null @@ -1,2 +0,0 @@ -// Package livesmoke contains build-tagged helpers for the opt-in live harness. -package livesmoke diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 2ee3bbb..016df35 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -8,12 +8,12 @@ import ( "testing" "time" - "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/cli-common/statedir" "github.com/open-cli-collective/cli-common/statedirtest" "github.com/open-cli-collective/spotify-cli/internal/config" "github.com/open-cli-collective/spotify-cli/internal/credentials" + "github.com/open-cli-collective/spotify-cli/internal/credstoretest" "github.com/open-cli-collective/spotify-cli/internal/token" ) @@ -35,8 +35,8 @@ func TestOpenPassesBackendAndClosesPrivateStore(t *testing.T) { if request.Backend != "file" || !request.BackendSet || len(authenticated.Scopes()) != 1 { t.Fatalf("request=%+v scopes=%v", request, authenticated.Scopes()) } - if err := authenticated.Close(); err != nil || !store.closed { - t.Fatalf("close error=%v closed=%t", err, store.closed) + if err := authenticated.Close(); err != nil || !store.Closed { + t.Fatalf("close error=%v closed=%t", err, store.Closed) } } @@ -86,15 +86,15 @@ func TestOpenErrorsDoNotEchoStoredCredential(t *testing.T) { if err := config.Save(scope, cfg); err != nil { t.Fatal(err) } - store := &memoryStore{values: map[string]string{"default/oauth_token": "secret-canary-invalid"}} + store := &credstoretest.Store{Values: map[string]string{"default/oauth_token": "secret-canary-invalid"}} opener := Opener{Scope: scope, OpenStore: func(credentials.OpenRequest) (CredentialStore, error) { return store, nil }, Now: func() time.Time { return now }} _, err := opener.Open(context.Background(), "", false) - if err == nil || strings.Contains(err.Error(), "secret-canary") || !store.closed { - t.Fatalf("error=%v closed=%t", err, store.closed) + if err == nil || strings.Contains(err.Error(), "secret-canary") || !store.Closed { + t.Fatalf("error=%v closed=%t", err, store.Closed) } } -func configuredStore(t *testing.T, now time.Time, envelope token.Envelope) (statedir.Scope, *memoryStore) { +func configuredStore(t *testing.T, now time.Time, envelope token.Envelope) (statedir.Scope, *credstoretest.Store) { t.Helper() statedirtest.Hermetic(t) scope := statedir.Scope{Name: config.Service} @@ -107,25 +107,7 @@ func configuredStore(t *testing.T, now time.Time, envelope token.Envelope) (stat if err != nil { t.Fatal(err) } - return scope, &memoryStore{values: map[string]string{"default/oauth_token": string(encoded)}} -} - -type memoryStore struct { - values map[string]string - closed bool -} - -func (store *memoryStore) Close() error { store.closed = true; return nil } -func (store *memoryStore) Get(profile, key string) (string, error) { - value, ok := store.values[profile+"/"+key] - if !ok { - return "", credstore.ErrNotFound - } - return value, nil -} -func (store *memoryStore) Set(profile, key, value string, _ ...credstore.SetOpt) error { - store.values[profile+"/"+key] = value - return nil + return scope, &credstoretest.Store{Values: map[string]string{"default/oauth_token": string(encoded)}} } type roundTripFunc func(*http.Request) (*http.Response, error) diff --git a/scripts/live-smoke.sh b/scripts/live-smoke.sh index fe1beb2..8913cf2 100755 --- a/scripts/live-smoke.sh +++ b/scripts/live-smoke.sh @@ -270,7 +270,7 @@ fi playlist_items_ids=$("$SPTFY" --backend file playlists items list "$SPOTIFY_CLI_LIVE_PLAYLIST_ID" --id --max 3) [[ $playlist_items_ids == "$playlist_prefix_ids" ]] || { printf '%s\n' 'playlist item ID-only prefix did not match normal output' >&2; exit 1; } if [[ $live_dry != 1 ]]; then - go test -tags=keyring_nopassage,spotify_live ./internal/livesmoke -run '^TestPlaylistDuplicateURIRemovalContract$' -count=1 + go test -tags=keyring_nopassage,spotify_live ./internal/client -run '^TestPlaylistDuplicateURIRemovalContract$' -count=1 fi playlist_restore_needed=1 @@ -354,7 +354,7 @@ artist_albums_out=$("$SPTFY" --backend file artists albums list "https://open.sp "$SPTFY" --backend file init --non-interactive --client-id "$SPOTIFY_CLIENT_ID" --overwrite if [[ $live_dry != 1 ]]; then - go test -tags=keyring_nopassage,spotify_live ./internal/livesmoke -run '^TestExpireCredential$' -count=1 + go test -tags=keyring_nopassage,spotify_live ./internal/credentials -run '^TestExpireCredential$' -count=1 fi grep -q '^account_id' < <("$SPTFY" --backend file me)