From 0f8300db3cff90e8d0aad04b95f5730876e2cde6 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 15 Oct 2026 12:07:52 +0200 Subject: [PATCH 1/2] fix: `flagsmith init` fails with `invalid_grant` on an expired session --- internal/auth/oauth.go | 28 +++++++-- internal/auth/oauth_test.go | 26 ++++++++- internal/cmd/cmd_test.go | 103 ++++++++++++++++++++++++++++++++++ internal/cmd/sentinel_test.go | 1 + 4 files changed, 151 insertions(+), 7 deletions(-) diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index e52c7b4..bd5237e 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -191,10 +191,7 @@ func postToken(ctx context.Context, httpClient *http.Client, endpoint string, fo Description string `json:"error_description"` } if json.Unmarshal(body, &oauthErr) == nil && oauthErr.Error != "" { - if oauthErr.Description != "" { - return nil, bug.Mark(fmt.Errorf("%s: %s", oauthErr.Error, oauthErr.Description)) - } - return nil, bug.Mark(errors.New(oauthErr.Error)) + return nil, bug.Mark(&tokenError{Code: oauthErr.Error, Description: oauthErr.Description}) } return nil, bug.Mark(fmt.Errorf("token endpoint returned %s", resp.Status)) } @@ -205,6 +202,20 @@ func postToken(ctx context.Context, httpClient *http.Client, endpoint string, fo return &tok, nil } +// tokenError is an OAuth error response from the token endpoint (RFC 6749 +// §5.2). +type tokenError struct { + Code string + Description string +} + +func (e *tokenError) Error() string { + if e.Description != "" { + return e.Code + ": " + e.Description + } + return e.Code +} + func randomURLSafe(n int) string { b := make([]byte, n) if _, err := rand.Read(b); err != nil { @@ -331,6 +342,11 @@ func credentialsFromToken(apiURL string, tok *tokenResponse) *Credentials { // the stored session is unusable. var ErrRefreshFailed = errors.New("refreshing session failed") +// ErrSessionExpired means the server rejected the stored session's refresh +// token — it has expired or been revoked. The instance is then as good as +// logged out, so this matches ErrNotLoggedIn. +var ErrSessionExpired = fmt.Errorf("session expired: %w", ErrNotLoggedIn) + // EnsureFresh refreshes the access token if it is expired or about to expire. // The server rotates refresh tokens (120s grace), so refreshed credentials // must be saved by the caller. @@ -350,6 +366,10 @@ func EnsureFresh(ctx context.Context, httpClient *http.Client, c *Credentials) ( "refresh_token": {c.RefreshToken}, "client_id": {ClientID}, }) + var tokErr *tokenError + if errors.As(err, &tokErr) && tokErr.Code == "invalid_grant" { + return nil, false, ErrSessionExpired + } if err != nil { return nil, false, fmt.Errorf("%w: %w", ErrRefreshFailed, err) } diff --git a/internal/auth/oauth_test.go b/internal/auth/oauth_test.go index 0d566fe..b0a28de 100644 --- a/internal/auth/oauth_test.go +++ b/internal/auth/oauth_test.go @@ -660,7 +660,7 @@ func TestEnsureFresh(t *testing.T) { } }) - t.Run("revoked refresh token is a recognisable refresh failure", func(t *testing.T) { + t.Run("rejected refresh token means logged out", func(t *testing.T) { // Given f := newFakeAuthServer(t) f.tokenHandler = func(url.Values) (int, any) { @@ -672,8 +672,28 @@ func TestEnsureFresh(t *testing.T) { _, _, err := EnsureFresh(context.Background(), http.DefaultClient, c) // Then - if !errors.Is(err, ErrRefreshFailed) { - t.Errorf("err = %v, want ErrRefreshFailed", err) + if !errors.Is(err, ErrSessionExpired) || !errors.Is(err, ErrNotLoggedIn) { + t.Errorf("err = %v, want ErrSessionExpired matching ErrNotLoggedIn", err) + } + if errors.Is(err, bug.ErrUnexpected) { + t.Errorf("err = %v, want an expired session not to be reported as a bug", err) + } + }) + + t.Run("failing token endpoint is a refresh failure", func(t *testing.T) { + // Given + f := newFakeAuthServer(t) + f.tokenHandler = func(url.Values) (int, any) { + return http.StatusInternalServerError, map[string]string{"error": "server_error"} + } + c := &Credentials{APIURL: f.srv.URL, RefreshToken: "refresh-1", ExpiresAt: time.Now().Add(-time.Minute)} + + // When + _, _, err := EnsureFresh(context.Background(), http.DefaultClient, c) + + // Then + if !errors.Is(err, ErrRefreshFailed) || errors.Is(err, ErrNotLoggedIn) { + t.Errorf("err = %v, want ErrRefreshFailed, not ErrNotLoggedIn", err) } }) } diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 1777c36..32471de 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -170,6 +170,7 @@ type fakeInstance struct { projGetCalls int // count of GET /projects/{id}/ (retrieve) orgListCalls int // count of GET /organisations/ list calls tokenPosts int // count of POST /o/token/ (refresh) calls + refreshRevoked bool // refresh-token grants answer invalid_grant updateCalls int // count of update-flag calls lastUpdate map[string]any // last update-flag request body lastUpdateVerb string // method of the last update-flag call @@ -338,7 +339,13 @@ func newFakeInstance(t *testing.T) *fakeInstance { mux.HandleFunc("POST /o/token/", func(w http.ResponseWriter, r *http.Request) { f.mu.Lock() f.tokenPosts++ + revoked := f.refreshRevoked f.mu.Unlock() + if revoked && r.FormValue("grant_type") == "refresh_token" { + w.WriteHeader(http.StatusBadRequest) + json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"}) + return + } json.NewEncoder(w).Encode(map[string]any{ "access_token": oauthAccess, "refresh_token": "cmd-refresh", @@ -2713,6 +2720,102 @@ func TestInitNoCredentials(t *testing.T) { } } +// saveExpiredSession stores a session for url whose access token has expired, +// so the next credential lookup must refresh it. +func saveExpiredSession(t *testing.T, url string) { + t.Helper() + if err := auth.Save(&auth.Credentials{ + APIURL: url, + AccessToken: "stale", + RefreshToken: "cmd-refresh", + ExpiresAt: time.Now().Add(-time.Hour), + }); err != nil { + t.Fatal(err) + } + resetCredentialCache() +} + +func TestInitLogsInWhenSessionExpired(t *testing.T) { + // Given + isolateStorage(t) + f := newFakeInstance(t) + f.refreshRevoked = true + saveExpiredSession(t, f.srv.URL) + root := tempRepo(t) + fakeTTY(t) + + // When + resetFlags() + noBrowser = true // never open a real browser from a test + out := &syncBuffer{} + rootCmd.SetOut(out) + rootCmd.SetErr(out) + rootCmd.SetIn(strings.NewReader("1\n1\n")) + rootCmd.SetArgs([]string{"init", "--api-url", f.srv.URL}) + done := make(chan error, 1) + go func() { done <- rootCmd.Execute() }() + + authURLPattern := regexp.MustCompile(`https?://\S+/oauth/authorize/\?\S+`) + var q url.Values + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) && q == nil { + if m := authURLPattern.FindString(out.String()); m != "" { + u, err := url.Parse(m) + if err != nil { + t.Fatal(err) + } + q = u.Query() + break + } + time.Sleep(10 * time.Millisecond) + } + if q == nil { + t.Fatalf("init never offered a login; output: %q", out.String()) + } + if _, err := http.Get(q.Get("redirect_uri") + "?code=c&state=" + url.QueryEscape(q.Get("state"))); err != nil { + t.Fatal(err) + } + + // Then + select { + case err := <-done: + if err != nil { + t.Fatalf("init: %v\noutput: %s", err, out.String()) + } + case <-time.After(10 * time.Second): + t.Fatal("init did not return") + } + if written := loadWritten(t, root); refID(written.Project) != 101 { + t.Errorf("written = %+v, want project 101", written) + } + creds, err := auth.Load(f.srv.URL) + if err != nil { + t.Fatal(err) + } + if creds.AccessToken != oauthAccess { + t.Errorf("stored access token = %q, want the new session", creds.AccessToken) + } +} + +func TestExpiredSessionHintsLogin(t *testing.T) { + // Given + isolateStorage(t) + f := newFakeInstance(t) + f.refreshRevoked = true + saveExpiredSession(t, f.srv.URL) + + // When + _, err := run("", "auth", "status", "--api-url", f.srv.URL) + + // Then + if !errors.Is(err, auth.ErrSessionExpired) { + t.Fatalf("err = %v, want ErrSessionExpired", err) + } + if hint := hintFor(err); !strings.Contains(hint, "flagsmith login") { + t.Errorf("hint = %q, want it to point at `flagsmith login`", hint) + } +} + func TestInitRefusesOverwriteWithoutYes(t *testing.T) { // Given isolateStorage(t) diff --git a/internal/cmd/sentinel_test.go b/internal/cmd/sentinel_test.go index b169482..e9d4242 100644 --- a/internal/cmd/sentinel_test.go +++ b/internal/cmd/sentinel_test.go @@ -31,6 +31,7 @@ func TestEverySentinelHasAHintDecision(t *testing.T) { "auth.ErrNoDiscovery": auth.ErrNoDiscovery, "auth.ErrKeychainUnavailable": auth.ErrKeychainUnavailable, "auth.ErrRefreshFailed": auth.ErrRefreshFailed, + "auth.ErrSessionExpired": auth.ErrSessionExpired, "auth.ErrServerSideKey": auth.ErrServerSideKey, "auth.ErrLegacyAuthtoken": auth.ErrLegacyAuthtoken, "auth.ErrNotMasterKey": auth.ErrNotMasterKey, From 1d44c1a19bf7afeeb08cfe124262217a16c048e1 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 15 Oct 2026 12:11:31 +0200 Subject: [PATCH 2/2] test: Drop init-level tests in favour of the auth ones --- internal/cmd/cmd_test.go | 103 --------------------------------------- 1 file changed, 103 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 32471de..1777c36 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -170,7 +170,6 @@ type fakeInstance struct { projGetCalls int // count of GET /projects/{id}/ (retrieve) orgListCalls int // count of GET /organisations/ list calls tokenPosts int // count of POST /o/token/ (refresh) calls - refreshRevoked bool // refresh-token grants answer invalid_grant updateCalls int // count of update-flag calls lastUpdate map[string]any // last update-flag request body lastUpdateVerb string // method of the last update-flag call @@ -339,13 +338,7 @@ func newFakeInstance(t *testing.T) *fakeInstance { mux.HandleFunc("POST /o/token/", func(w http.ResponseWriter, r *http.Request) { f.mu.Lock() f.tokenPosts++ - revoked := f.refreshRevoked f.mu.Unlock() - if revoked && r.FormValue("grant_type") == "refresh_token" { - w.WriteHeader(http.StatusBadRequest) - json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"}) - return - } json.NewEncoder(w).Encode(map[string]any{ "access_token": oauthAccess, "refresh_token": "cmd-refresh", @@ -2720,102 +2713,6 @@ func TestInitNoCredentials(t *testing.T) { } } -// saveExpiredSession stores a session for url whose access token has expired, -// so the next credential lookup must refresh it. -func saveExpiredSession(t *testing.T, url string) { - t.Helper() - if err := auth.Save(&auth.Credentials{ - APIURL: url, - AccessToken: "stale", - RefreshToken: "cmd-refresh", - ExpiresAt: time.Now().Add(-time.Hour), - }); err != nil { - t.Fatal(err) - } - resetCredentialCache() -} - -func TestInitLogsInWhenSessionExpired(t *testing.T) { - // Given - isolateStorage(t) - f := newFakeInstance(t) - f.refreshRevoked = true - saveExpiredSession(t, f.srv.URL) - root := tempRepo(t) - fakeTTY(t) - - // When - resetFlags() - noBrowser = true // never open a real browser from a test - out := &syncBuffer{} - rootCmd.SetOut(out) - rootCmd.SetErr(out) - rootCmd.SetIn(strings.NewReader("1\n1\n")) - rootCmd.SetArgs([]string{"init", "--api-url", f.srv.URL}) - done := make(chan error, 1) - go func() { done <- rootCmd.Execute() }() - - authURLPattern := regexp.MustCompile(`https?://\S+/oauth/authorize/\?\S+`) - var q url.Values - deadline := time.Now().Add(5 * time.Second) - for time.Now().Before(deadline) && q == nil { - if m := authURLPattern.FindString(out.String()); m != "" { - u, err := url.Parse(m) - if err != nil { - t.Fatal(err) - } - q = u.Query() - break - } - time.Sleep(10 * time.Millisecond) - } - if q == nil { - t.Fatalf("init never offered a login; output: %q", out.String()) - } - if _, err := http.Get(q.Get("redirect_uri") + "?code=c&state=" + url.QueryEscape(q.Get("state"))); err != nil { - t.Fatal(err) - } - - // Then - select { - case err := <-done: - if err != nil { - t.Fatalf("init: %v\noutput: %s", err, out.String()) - } - case <-time.After(10 * time.Second): - t.Fatal("init did not return") - } - if written := loadWritten(t, root); refID(written.Project) != 101 { - t.Errorf("written = %+v, want project 101", written) - } - creds, err := auth.Load(f.srv.URL) - if err != nil { - t.Fatal(err) - } - if creds.AccessToken != oauthAccess { - t.Errorf("stored access token = %q, want the new session", creds.AccessToken) - } -} - -func TestExpiredSessionHintsLogin(t *testing.T) { - // Given - isolateStorage(t) - f := newFakeInstance(t) - f.refreshRevoked = true - saveExpiredSession(t, f.srv.URL) - - // When - _, err := run("", "auth", "status", "--api-url", f.srv.URL) - - // Then - if !errors.Is(err, auth.ErrSessionExpired) { - t.Fatalf("err = %v, want ErrSessionExpired", err) - } - if hint := hintFor(err); !strings.Contains(hint, "flagsmith login") { - t.Errorf("hint = %q, want it to point at `flagsmith login`", hint) - } -} - func TestInitRefusesOverwriteWithoutYes(t *testing.T) { // Given isolateStorage(t)