From 3e23f0adb8056400ecef0aa6728b33dae0dce768 Mon Sep 17 00:00:00 2001 From: Akram Date: Tue, 5 May 2026 17:27:00 +0200 Subject: [PATCH 1/2] fix: Update audience scope mapper when included.custom.audience changes When an audience scope mapper already exists (409 Conflict), fetch the existing mapper and compare its included.custom.audience value. If it differs from the desired value (e.g. short-form "ns/wl" vs SPIFFE URI), update it via PUT. Previously, 409 was treated as success without checking the audience value, causing stale mappers when clients transitioned from short-form IDs to SPIFFE URIs. Fixes #330 Assisted-By: Claude (Anthropic AI) Signed-off-by: Akram --- .../internal/keycloak/audience.go | 73 ++++++++- .../internal/keycloak/audience_test.go | 149 ++++++++++++++++++ 2 files changed, 221 insertions(+), 1 deletion(-) diff --git a/kagenti-operator/internal/keycloak/audience.go b/kagenti-operator/internal/keycloak/audience.go index e9ba50a5..01b987c8 100644 --- a/kagenti-operator/internal/keycloak/audience.go +++ b/kagenti-operator/internal/keycloak/audience.go @@ -39,6 +39,7 @@ type clientScopeCreateRep struct { } type protocolMapperRep struct { + ID string `json:"id,omitempty"` Name string `json:"name"` Protocol string `json:"protocol"` ProtocolMapper string `json:"protocolMapper"` @@ -202,9 +203,13 @@ func (a *Admin) ensureAudienceMapper(ctx context.Context, token, realm, scopeID, return err } defer func() { _ = resp.Body.Close() }() - if resp.StatusCode == http.StatusCreated || resp.StatusCode == http.StatusConflict { + if resp.StatusCode == http.StatusCreated { return nil } + if resp.StatusCode == http.StatusConflict { + // Mapper already exists — check if its audience needs updating. + return a.updateAudienceMapperIfNeeded(ctx, token, realm, scopeID, scopeName, audience) + } body, _ := io.ReadAll(resp.Body) // Mapper may already exist — treat other errors as non-fatal (Python logs and continues). if resp.StatusCode >= 400 { @@ -213,6 +218,72 @@ func (a *Admin) ensureAudienceMapper(ctx context.Context, token, realm, scopeID, return nil } +// updateAudienceMapperIfNeeded fetches the existing mapper for the scope and updates +// its included.custom.audience if it differs from the desired value. +func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, scopeID, scopeName, audience string) error { + base := trimBaseURL(a.BaseURL) + endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/client-scopes/" + url.PathEscape(scopeID) + "/protocol-mappers/models" + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + if err != nil { + return err + } + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := a.httpc().Do(req) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() + body, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("keycloak list mappers: status %d: %s", resp.StatusCode, truncate(body, 256)) + } + + var mappers []protocolMapperRep + if err := json.Unmarshal(body, &mappers); err != nil { + return fmt.Errorf("keycloak list mappers decode: %w", err) + } + + for i := range mappers { + if mappers[i].Name != scopeName || mappers[i].ProtocolMapper != "oidc-audience-mapper" { + continue + } + if mappers[i].Config["included.custom.audience"] == audience { + return nil // already correct + } + // Update the mapper with the correct audience. + mappers[i].Config["included.custom.audience"] = audience + return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i]) + } + return nil +} + +func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID string, mapper protocolMapperRep) error { + payload, err := json.Marshal(mapper) + if err != nil { + return err + } + base := trimBaseURL(a.BaseURL) + endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/client-scopes/" + url.PathEscape(scopeID) + "/protocol-mappers/models/" + url.PathEscape(mapper.ID) + req, err := http.NewRequestWithContext(ctx, http.MethodPut, endpoint, bytes.NewReader(payload)) + if err != nil { + return err + } + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + + resp, err := a.httpc().Do(req) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNoContent || (resp.StatusCode >= 200 && resp.StatusCode < 300) { + return nil + } + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf("keycloak update audience mapper: status %d: %s", resp.StatusCode, truncate(body, 256)) +} + func (a *Admin) putRealmDefaultDefaultClientScope(ctx context.Context, token, realm, scopeID string) error { base := trimBaseURL(a.BaseURL) endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/default-default-client-scopes/" + url.PathEscape(scopeID) diff --git a/kagenti-operator/internal/keycloak/audience_test.go b/kagenti-operator/internal/keycloak/audience_test.go index ce8ba87d..0a6f4389 100644 --- a/kagenti-operator/internal/keycloak/audience_test.go +++ b/kagenti-operator/internal/keycloak/audience_test.go @@ -3,6 +3,7 @@ package keycloak import ( "context" "encoding/json" + "io" "net/http" "net/http/httptest" "strings" @@ -72,6 +73,154 @@ func TestEnsureAudienceScope(t *testing.T) { } } +// TestEnsureAudienceScope_UpdatesStaleMapper verifies that when an audience scope mapper +// already exists with a different audience (e.g. short-form "ns/wl" instead of SPIFFE URI), +// ensureAudienceMapper detects the mismatch and updates it via PUT. +func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) { + var getMapperCalls, putMapperCalls int + var putMapperBody protocolMapperRep + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + // Scope already exists + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + // POST mapper returns 409 (already exists) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + w.WriteHeader(http.StatusConflict) + + // GET mappers — returns mapper with stale audience + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + getMapperCalls++ + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "mapper-456", + Name: "agent-ns-wl-aud", + Protocol: "openid-connect", + ProtocolMapper: "oidc-audience-mapper", + Config: map[string]string{ + "included.custom.audience": "ns/wl", // stale short-form + "id.token.claim": "false", + "access.token.claim": "true", + "userinfo.token.claim": "false", + }, + }}) + + // PUT mapper — update with correct audience + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models/mapper-456") && r.Method == http.MethodPut: + putMapperCalls++ + body, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(body, &putMapperBody) + w.WriteHeader(http.StatusNoContent) + + // Realm default scope + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: spiffeURI, + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatal(err) + } + if getMapperCalls != 1 { + t.Fatalf("expected 1 GET mapper call, got %d", getMapperCalls) + } + if putMapperCalls != 1 { + t.Fatalf("expected 1 PUT mapper call, got %d", putMapperCalls) + } + if putMapperBody.Config["included.custom.audience"] != spiffeURI { + t.Fatalf("expected audience %q, got %q", spiffeURI, putMapperBody.Config["included.custom.audience"]) + } +} + +// TestEnsureAudienceScope_SkipsUpdateWhenCorrect verifies that when the existing mapper +// already has the correct audience, no PUT is issued. +func TestEnsureAudienceScope_SkipsUpdateWhenCorrect(t *testing.T) { + var putMapperCalls int + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + w.WriteHeader(http.StatusConflict) + + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "mapper-456", + Name: "agent-ns-wl-aud", + Protocol: "openid-connect", + ProtocolMapper: "oidc-audience-mapper", + Config: map[string]string{ + "included.custom.audience": spiffeURI, // already correct + "id.token.claim": "false", + "access.token.claim": "true", + "userinfo.token.claim": "false", + }, + }}) + + case strings.Contains(path, "/protocol-mappers/models/mapper-456") && r.Method == http.MethodPut: + putMapperCalls++ + w.WriteHeader(http.StatusNoContent) + + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: "spiffe://example.org/ns/ns/sa/wl", + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatal(err) + } + if putMapperCalls != 0 { + t.Fatalf("expected 0 PUT mapper calls (audience already correct), got %d", putMapperCalls) + } +} + func TestEnsureAudienceScope_Disabled(t *testing.T) { a := Admin{} err := a.EnsureAudienceScope(context.Background(), "t", AudienceParams{AudienceScopeEnabled: false}) From 082e5410ed5a8e736bd4185ad3e25efbd90df48d Mon Sep 17 00:00:00 2001 From: Akram Date: Thu, 7 May 2026 19:00:17 +0200 Subject: [PATCH 2/2] fix: address review feedback on audience mapper update - Drop omitempty from protocolMapperRep.ID to fail explicitly on empty ID - Add nil guard for Config map to prevent potential panic - Add slog.Debug log when no matching audience mapper is found Assisted-By: Claude (Anthropic AI) Signed-off-by: Akram --- kagenti-operator/internal/keycloak/audience.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/kagenti-operator/internal/keycloak/audience.go b/kagenti-operator/internal/keycloak/audience.go index 01b987c8..6989ec74 100644 --- a/kagenti-operator/internal/keycloak/audience.go +++ b/kagenti-operator/internal/keycloak/audience.go @@ -13,6 +13,7 @@ import ( "encoding/json" "fmt" "io" + "log/slog" "net/http" "net/url" "strings" @@ -39,7 +40,7 @@ type clientScopeCreateRep struct { } type protocolMapperRep struct { - ID string `json:"id,omitempty"` + ID string `json:"id"` Name string `json:"name"` Protocol string `json:"protocol"` ProtocolMapper string `json:"protocolMapper"` @@ -248,6 +249,9 @@ func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, if mappers[i].Name != scopeName || mappers[i].ProtocolMapper != "oidc-audience-mapper" { continue } + if mappers[i].Config == nil { + continue + } if mappers[i].Config["included.custom.audience"] == audience { return nil // already correct } @@ -255,6 +259,7 @@ func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, mappers[i].Config["included.custom.audience"] = audience return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i]) } + slog.Debug("no matching audience mapper found for scope", "scope", scopeName, "scopeID", scopeID) return nil }