Merge pull request #367 from natalie-o-perret/fix/migrate-oauth-token-cache-key

fix(auth): migrate legacy token cache keys on first use after upgrade

Author: danielgtaylor

internal/cli/api_auth.go

@@ -794,6 +794,9 @@ func (c *CLI) operationSetForAPI(ctx context.Context, apiName string, apiCfg *co
 	}
 	if err != nil || s == nil {
 		if !spec.HasLocalSpecFiles(apiCfg.SpecFiles) {
+			if err == nil && !forceRefresh && (apiCfg.BaseURL != "" || apiCfg.SpecURL != "" || len(apiCfg.SpecFiles) > 0) {
+				c.hintf("spec cache for API %q is empty; run \"restish api sync %s\" to populate it", apiName, apiName)
+			}
 			return spec.OperationSet{}, false, err
 		}
 		s, err = spec.Discover(ctx, spec.DiscoverConfig{

internal/cli/auth.go

@@ -299,11 +299,29 @@ func (c *CLI) cachedOAuthTokenEntry(authType string, cacheKey, apiName, profileN
 	if cacheKey == ":" || cacheKey == "" {
 		return nil
 	}
-	cached, err := auth.NewTokenCache(c.tokenCachePath()).Get(cacheKey)
+	tc := auth.NewTokenCache(c.tokenCachePath())
+	cached, err := tc.Get(cacheKey)
 	if err != nil {
 		return nil
 	}
-	return cached
+	if cached != nil {
+		return cached
+	}
+	// The cache key format changed from "apiName:profileName" to "oauth:HASH" at
+	// some point, silently invalidating existing tokens on upgrade. Fall back to
+	// the legacy key here; if found, migrate it to the new key and drop the old one.
+	legacyKey := c.apiCacheNamespace(apiName, profileName)
+	if legacyKey == cacheKey || legacyKey == ":" || legacyKey == "" {
+		return nil
+	}
+	legacy, err := tc.Get(legacyKey)
+	if err != nil || legacy == nil {
+		return nil
+	}
+	if setErr := tc.Set(cacheKey, *legacy); setErr == nil {
+		_ = tc.Delete(legacyKey)
+	}
+	return legacy
 }
 
 func (c *CLI) cachedOAuthAuthCodeUsable(authType, cacheKey, apiName, profileName string) bool {

internal/cli/auth_internal_test.go

@@ -197,6 +197,49 @@ func TestInlineAuthCacheKeyDeduplicatesAbsoluteAuthCodeEndpoints(t *testing.T) {
 	}
 }
 
+func TestCachedOAuthTokenEntryMigratesLegacyInlineAuthCodeCacheKey(t *testing.T) {
+	cacheFile := filepath.Join(t.TempDir(), "tokens.cbor")
+	c := New()
+	c.Hooks().TokenCachePath = cacheFile
+
+	ac := &config.AuthConfig{
+		Type: "oauth-authorization-code",
+		Params: map[string]string{
+			"client_id":     "restish",
+			"authorize_url": "https://dex.example.com/auth",
+			"token_url":     "https://dex.example.com/token",
+		},
+	}
+	cacheKey := inlineAuthCacheKey("demo:default", ac, "https://api.example.com")
+	if cacheKey == "" || cacheKey == "demo:default" {
+		t.Fatalf("inline auth code cache key = %q, want hashed oauth key", cacheKey)
+	}
+
+	tc := auth.NewTokenCache(cacheFile)
+	if err := tc.Set("demo:default", auth.CachedToken{AccessToken: "legacy-token"}); err != nil {
+		t.Fatalf("seed legacy token: %v", err)
+	}
+
+	got := c.cachedOAuthTokenEntry("oauth-authorization-code", cacheKey, "demo", "default")
+	if got == nil || got.AccessToken != "legacy-token" {
+		t.Fatalf("migrated token = %+v, want legacy-token", got)
+	}
+	migrated, err := tc.Get(cacheKey)
+	if err != nil {
+		t.Fatalf("read migrated token: %v", err)
+	}
+	if migrated == nil || migrated.AccessToken != "legacy-token" {
+		t.Fatalf("migrated cache entry = %+v, want legacy-token", migrated)
+	}
+	legacy, err := tc.Get("demo:default")
+	if err != nil {
+		t.Fatalf("read legacy token: %v", err)
+	}
+	if legacy != nil {
+		t.Fatalf("legacy cache entry still present: %+v", legacy)
+	}
+}
+
 func TestInlineAuthCacheKeyDeduplicatesIssuerAuthCodeEndpoints(t *testing.T) {
 	ac := &config.AuthConfig{
 		Type: "oauth-authorization-code",
@@ -363,3 +406,50 @@ func TestAuthHandlerForOAuthUsesThemeCallbackColors(t *testing.T) {
 		t.Fatalf("failure color = %q, want #ff0000", oauthHandler.CallbackFailureColor)
 	}
 }
+
+func TestOperationSetForAPIHintsAPISync_WhenCacheEmpty(t *testing.T) {
+	var stderr bytes.Buffer
+	c := New()
+	c.Stderr = &stderr
+	c.Hooks().SpecCachePath = t.TempDir() // empty cache dir
+
+	apiCfg := &config.APIConfig{BaseURL: "https://api.example.com"}
+	set, ok, err := c.operationSetForAPI(context.Background(), "example", apiCfg, "default", false)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if ok || len(set.Operations) > 0 {
+		t.Fatal("expected empty operation set from cold cache")
+	}
+	if !strings.Contains(stderr.String(), "api sync example") {
+		t.Fatalf("expected api sync hint on stderr, got: %q", stderr.String())
+	}
+}
+
+func TestOperationSetForAPINoHintWhenForceRefresh(t *testing.T) {
+	// forceRefresh=true means an explicit sync is already in progress; no hint.
+	var stderr bytes.Buffer
+	c := New()
+	c.Stderr = &stderr
+	c.Hooks().SpecCachePath = t.TempDir()
+
+	apiCfg := &config.APIConfig{BaseURL: "https://203.0.113.1"}
+	_, _, _ = c.operationSetForAPI(context.Background(), "example", apiCfg, "default", true)
+	if strings.Contains(stderr.String(), "api sync") {
+		t.Fatalf("expected no api sync hint during forceRefresh, got: %q", stderr.String())
+	}
+}
+
+func TestOperationSetForAPINoHintWhenNoSpecSource(t *testing.T) {
+	// An API with no BaseURL, no SpecURL, no SpecFiles configured has nothing to sync.
+	var stderr bytes.Buffer
+	c := New()
+	c.Stderr = &stderr
+	c.Hooks().SpecCachePath = t.TempDir()
+
+	apiCfg := &config.APIConfig{}
+	_, _, _ = c.operationSetForAPI(context.Background(), "bare", apiCfg, "default", false)
+	if strings.Contains(stderr.String(), "api sync") {
+		t.Fatalf("expected no api sync hint for API with no spec source, got: %q", stderr.String())
+	}
+}

internal/cli/hooks.go

@@ -82,6 +82,9 @@ func (c *CLI) resolveTLSSigner(opts request.Options) (request.Options, error) {
 			return opts, nil
 		}
 	}
+	if opts.TLSSignerName == "pkcs11" {
+		return opts, fmt.Errorf("tls signer plugin %q not found; install the restish-pkcs11 binary and make sure it is on your PATH (see https://github.com/rest-sh/restish)", opts.TLSSignerName)
+	}
 	return opts, fmt.Errorf("tls signer plugin %q not found", opts.TLSSignerName)
 }
 

internal/config/migrate.go

@@ -228,6 +228,7 @@ func convertLegacyAPIConfig(name string, legacy *legacyAPIConfig) (*APIConfig, [
 		}
 	}
 
+	var migratedPKCS11 bool
 	if legacy.TLS != nil && legacy.TLS.PKCS11 != nil {
 		if api.Profiles == nil {
 			api.Profiles = map[string]*ProfileConfig{}
@@ -249,6 +250,7 @@ func convertLegacyAPIConfig(name string, legacy *legacyAPIConfig) (*APIConfig, [
 		if legacy.TLS.PKCS11.Label != "" && prof.TLSSignerParams["label"] == "" {
 			prof.TLSSignerParams["label"] = legacy.TLS.PKCS11.Label
 		}
+		migratedPKCS11 = true
 	}
 
 	if legacy.TLS != nil && (legacy.TLS.Cert != "" || legacy.TLS.Key != "") {
@@ -272,6 +274,9 @@ func convertLegacyAPIConfig(name string, legacy *legacyAPIConfig) (*APIConfig, [
 	if warning != "" {
 		warnings = append(warnings, warning)
 	}
+	if migratedPKCS11 {
+		warnings = append(warnings, fmt.Sprintf("api %q: migrated PKCS#11 TLS config; install the restish-pkcs11 plugin to continue using it (see https://github.com/rest-sh/restish)", name))
+	}
 	return api, warnings
 }