From 51c192173cef370644aac30cb6aaec5949888692 Mon Sep 17 00:00:00 2001 From: michaelryanmcneill Date: Fri, 24 Jul 2026 16:52:24 -0400 Subject: [PATCH] OCPBUGS-99757: Include extra scopes in OIDC token cache key The token cache key only included issuer URL and client ID, causing cached tokens to be returned even when different --extra-scopes were requested. This resulted in ID tokens missing scope-dependent claims (e.g., email, profile) after the initial authentication. Add ExtraScopes to the tokencache.Key struct so that different scope sets produce distinct cache entries. Scopes are sorted before key construction to ensure deterministic cache filenames regardless of flag ordering. Signed-off-by: michaelryanmcneill --- pkg/cli/gettoken/gettoken.go | 5 +- pkg/cli/gettoken/tokencache/tokencache.go | 10 ++ .../gettoken/tokencache/tokencache_test.go | 140 ++++++++++++++++++ 3 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 pkg/cli/gettoken/tokencache/tokencache_test.go diff --git a/pkg/cli/gettoken/gettoken.go b/pkg/cli/gettoken/gettoken.go index 55022a5bb4..c60555bc41 100644 --- a/pkg/cli/gettoken/gettoken.go +++ b/pkg/cli/gettoken/gettoken.go @@ -144,8 +144,9 @@ func (o *GetTokenOptions) Validate() error { // external OIDC issuer. If not, it forces user to log in. func (o *GetTokenOptions) Run() error { tokenCacheKey := tokencache.Key{ - IssuerURL: o.IssuerURL, - ClientID: o.ClientID, + IssuerURL: o.IssuerURL, + ClientID: o.ClientID, + ExtraScopes: o.ExtraScopes, } // Ignoring the error because if there is any error occurred diff --git a/pkg/cli/gettoken/tokencache/tokencache.go b/pkg/cli/gettoken/tokencache/tokencache.go index 39045e8bfb..6f73262a6d 100644 --- a/pkg/cli/gettoken/tokencache/tokencache.go +++ b/pkg/cli/gettoken/tokencache/tokencache.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path/filepath" + "sort" ) // Set stores the id token and the refresh token @@ -23,6 +24,8 @@ type Set struct { type Key struct { IssuerURL string ClientID string + // ExtraScopes are sorted internally by computeFilename to ensure deterministic cache keys. + ExtraScopes []string } // tokenCacheEntity is a internal structure for the in-memory token @@ -87,6 +90,13 @@ func (r *Repository) Save(dir string, key Key, tokenSet Set) error { } func computeFilename(key Key) (string, error) { + if key.ExtraScopes != nil { + sorted := make([]string, len(key.ExtraScopes)) + copy(sorted, key.ExtraScopes) + sort.Strings(sorted) + key.ExtraScopes = sorted + } + s := sha256.New() e := gob.NewEncoder(s) if err := e.Encode(&key); err != nil { diff --git a/pkg/cli/gettoken/tokencache/tokencache_test.go b/pkg/cli/gettoken/tokencache/tokencache_test.go new file mode 100644 index 0000000000..b2d3a1d69f --- /dev/null +++ b/pkg/cli/gettoken/tokencache/tokencache_test.go @@ -0,0 +1,140 @@ +package tokencache + +import ( + "os" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestComputeFilename(t *testing.T) { + tests := []struct { + name string + k1 Key + k2 Key + wantEqual bool + }{ + { + name: "same keys without scopes produce same filename", + k1: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client"}, + k2: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client"}, + wantEqual: true, + }, + { + name: "identical sorted scopes produce same filename", + k1: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client", ExtraScopes: []string{"email", "profile"}}, + k2: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client", ExtraScopes: []string{"email", "profile"}}, + wantEqual: true, + }, + { + name: "presence of scopes changes filename", + k1: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client"}, + k2: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client", ExtraScopes: []string{"email", "profile"}}, + wantEqual: false, + }, + { + name: "differently ordered scopes produce same filename", + k1: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client", ExtraScopes: []string{"email", "profile"}}, + k2: Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client", ExtraScopes: []string{"profile", "email"}}, + wantEqual: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f1, err := computeFilename(tt.k1) + if err != nil { + t.Fatalf("computeFilename(k1) error: %v", err) + } + f2, err := computeFilename(tt.k2) + if err != nil { + t.Fatalf("computeFilename(k2) error: %v", err) + } + + if tt.wantEqual && f1 != f2 { + t.Errorf("expected same filename, got %s and %s", f1, f2) + } + if !tt.wantEqual && f1 == f2 { + t.Errorf("expected different filenames, got %s for both", f1) + } + }) + } +} + +func TestSaveAndFindByKey(t *testing.T) { + dir := t.TempDir() + repo := &Repository{} + + key := Key{ + IssuerURL: "https://issuer.example.com", + ClientID: "my-client", + ExtraScopes: []string{"email", "profile"}, + } + want := Set{ + IDToken: "test-id-token", + RefreshToken: "test-refresh-token", + } + + if err := repo.Save(dir, key, want); err != nil { + t.Fatalf("Save() error: %v", err) + } + + got, err := repo.FindByKey(dir, key) + if err != nil { + t.Fatalf("FindByKey() error: %v", err) + } + + if diff := cmp.Diff(&want, got); diff != "" { + t.Errorf("FindByKey() mismatch (-want +got):\n%s", diff) + } +} + +func TestFindByKey_DifferentScopesMiss(t *testing.T) { + dir := t.TempDir() + repo := &Repository{} + + key := Key{ + IssuerURL: "https://issuer.example.com", + ClientID: "my-client", + ExtraScopes: []string{"email"}, + } + if err := repo.Save(dir, key, Set{IDToken: "token1", RefreshToken: "refresh1"}); err != nil { + t.Fatalf("Save() error: %v", err) + } + + differentKey := Key{ + IssuerURL: "https://issuer.example.com", + ClientID: "my-client", + ExtraScopes: []string{"email", "profile"}, + } + _, err := repo.FindByKey(dir, differentKey) + if err == nil { + t.Fatal("expected FindByKey to fail for a key with different scopes, but it succeeded") + } + if !os.IsNotExist(err) { + t.Errorf("expected file-not-found error, got: %v", err) + } +} + +func TestSave_FilePermissions(t *testing.T) { + dir := t.TempDir() + repo := &Repository{} + + key := Key{IssuerURL: "https://issuer.example.com", ClientID: "my-client"} + if err := repo.Save(dir, key, Set{IDToken: "tok"}); err != nil { + t.Fatalf("Save() error: %v", err) + } + + filename, err := computeFilename(key) + if err != nil { + t.Fatalf("computeFilename() error: %v", err) + } + info, err := os.Stat(filepath.Join(dir, filename)) + if err != nil { + t.Fatalf("Stat() error: %v", err) + } + if perm := info.Mode().Perm(); perm != 0600 { + t.Errorf("expected file permission 0600, got %o", perm) + } +}