Skip to content

Commit cf2f5e7

Browse files
toothbrushclaude
andcommitted
auth: surface refresh-slot read errors in LoadTokens
contextTokenStore.LoadTokens swallowed every error from the refresh-slot Get, so a transient keyring/file-store failure silently became an empty refresh token — discarding a valid token and forcing a re-login on what was really a recoverable storage hiccup. Mirror the access-token handling: ErrNotFound means no refresh, any other error surfaces. Adds tokenstore.UseFailingGetBackendForTesting (read-side fault seam) and a test asserting the error propagates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 80e5c48 commit cf2f5e7

3 files changed

Lines changed: 57 additions & 5 deletions

File tree

cmd/entire/cli/auth/refresh.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,14 @@ func (s contextTokenStore) LoadTokens(string) (tokens.TokenSet, error) {
5252
return tokens.TokenSet{}, fmt.Errorf("read access token: %w", err)
5353
}
5454
access, expiresAt := tokenstore.DecodeTokenWithExpiration(enc)
55-
refresh, _ := tokenstore.Get(tokenstore.RefreshService(s.service), s.handle) //nolint:errcheck // an absent refresh token is fine — treated as no-refresh
55+
// A missing refresh slot is fine (login predating offline_access) — treat
56+
// it as no-refresh. Any other store error must surface, not be swallowed:
57+
// dropping it would silently discard a valid refresh token and force a
58+
// re-login on what was really a transient keyring/file-store failure.
59+
refresh, err := tokenstore.Get(tokenstore.RefreshService(s.service), s.handle)
60+
if err != nil && !errors.Is(err, tokenstore.ErrNotFound) {
61+
return tokens.TokenSet{}, fmt.Errorf("read refresh token: %w", err)
62+
}
5663
return tokens.TokenSet{
5764
AccessToken: access,
5865
RefreshToken: refresh,

cmd/entire/cli/auth/refresh_test.go

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,15 @@ import (
1818
"github.com/entireio/cli/internal/entireclient/tokenstore"
1919
)
2020

21+
// testCoreService is the keychain access-token service used across the
22+
// contextTokenStore tests (paired refresh slot is RefreshService(it)).
23+
const testCoreService = "entire-core:https://core.example"
24+
2125
func TestContextTokenStore_RoundTrip(t *testing.T) {
2226
restore := tokenstore.UseFileBackendForTesting(filepath.Join(t.TempDir(), "tokens.json"))
2327
t.Cleanup(restore)
2428

25-
st := contextTokenStore{service: "entire-core:https://core.example", handle: "alice"}
29+
st := contextTokenStore{service: testCoreService, handle: "alice"}
2630

2731
// Missing → ErrNotFound.
2832
if _, err := st.LoadTokens(""); !errors.Is(err, authtokenstore.ErrNotFound) {
@@ -60,6 +64,32 @@ func TestContextTokenStore_RoundTrip(t *testing.T) {
6064
}
6165
}
6266

67+
// A non-NotFound failure reading the refresh slot must surface, not be
68+
// swallowed — swallowing would discard a valid refresh token and force a
69+
// re-login on a transient keyring/file-store hiccup.
70+
func TestContextTokenStore_LoadTokens_RefreshReadErrorSurfaces(t *testing.T) {
71+
svc := testCoreService
72+
73+
// Seed a valid access token through a clean backend.
74+
path := filepath.Join(t.TempDir(), "tokens.json")
75+
seedRestore := tokenstore.UseFileBackendForTesting(path)
76+
access := makeJWT(t, fmt.Sprintf(`{"iss":"https://core.example","handle":"alice","exp":%d}`, time.Now().Add(time.Hour).Unix()))
77+
if err := tokenstore.Set(svc, "alice", tokenstore.EncodeTokenWithExpiration(access, 3600)); err != nil {
78+
t.Fatalf("seed access: %v", err)
79+
}
80+
seedRestore()
81+
82+
// Fail only the refresh-slot read; the access read still succeeds.
83+
failRefreshGet := func(service, _ string) bool { return service == tokenstore.RefreshService(svc) }
84+
restore := tokenstore.UseFailingGetBackendForTesting(path, failRefreshGet)
85+
t.Cleanup(restore)
86+
87+
st := contextTokenStore{service: svc, handle: "alice"}
88+
if _, err := st.LoadTokens(""); err == nil {
89+
t.Fatal("LoadTokens: want error when the refresh-slot read fails, got nil")
90+
}
91+
}
92+
6393
func TestNewRefreshingLoginProvider_Validation(t *testing.T) {
6494
if _, err := NewRefreshingLoginProvider(nil, nil, false); err == nil {
6595
t.Error("nil context: want error")
@@ -189,7 +219,7 @@ func TestNewRefreshingLoginProvider_RefreshesAndRotates(t *testing.T) {
189219
// re-login. The store persists refresh-first to invert both failure modes.
190220
func TestContextTokenStore_SaveTokens_RefreshFirstOrdering(t *testing.T) {
191221
t.Run("refresh write fails: access slot untouched", func(t *testing.T) {
192-
svc := "entire-core:https://core.example"
222+
svc := testCoreService
193223
path := filepath.Join(t.TempDir(), "tokens.json")
194224

195225
// Seed an existing good pair through a clean backend first — the fault
@@ -224,7 +254,7 @@ func TestContextTokenStore_SaveTokens_RefreshFirstOrdering(t *testing.T) {
224254
})
225255

226256
t.Run("access write fails: refresh slot already advanced (self-heals)", func(t *testing.T) {
227-
svc := "entire-core:https://core.example"
257+
svc := testCoreService
228258
failAccess := func(service, _ string) bool { return service == svc }
229259
restore := tokenstore.UseFailingBackendForTesting(filepath.Join(t.TempDir(), "tokens.json"), failAccess)
230260
t.Cleanup(restore)

internal/entireclient/tokenstore/testing.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,21 @@ func UseFileBackendForTesting(path string) func() {
3333
// paths (e.g. the refresh-then-access ordering in contextTokenStore) without
3434
// exposing the unexported store interface. Returns a cleanup function.
3535
func UseFailingBackendForTesting(path string, failSet func(service, user string) bool) func() {
36+
return installFaultStore(faultStore{inner: &fileStore{path: path}, failSet: failSet})
37+
}
38+
39+
// UseFailingGetBackendForTesting is the read-side analogue: Get returns an
40+
// error for any (service, user) pair where failGet reports true. Used to test
41+
// that callers surface a real store failure rather than swallowing it.
42+
func UseFailingGetBackendForTesting(path string, failGet func(service, user string) bool) func() {
43+
return installFaultStore(faultStore{inner: &fileStore{path: path}, failGet: failGet})
44+
}
45+
46+
func installFaultStore(fs faultStore) func() {
3647
backendMu.Lock()
3748
prevBackend := backend
3849
prevResolved := resolved
39-
backend = faultStore{inner: &fileStore{path: path}, failSet: failSet}
50+
backend = fs
4051
resolved = true
4152
backendMu.Unlock()
4253

@@ -51,9 +62,13 @@ func UseFailingBackendForTesting(path string, failSet func(service, user string)
5162
type faultStore struct {
5263
inner store
5364
failSet func(service, user string) bool
65+
failGet func(service, user string) bool
5466
}
5567

5668
func (f faultStore) Get(service, user string) (string, error) {
69+
if f.failGet != nil && f.failGet(service, user) {
70+
return "", fmt.Errorf("injected Get failure for %s/%s", service, user)
71+
}
5772
//nolint:wrapcheck // thin test wrapper; callers handle errors
5873
return f.inner.Get(service, user)
5974
}

0 commit comments

Comments
 (0)