From 1df9b98aeb17fd283dda98f49345ae067633dd50 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 16:06:43 +0100 Subject: [PATCH] R60: accept single-string groups/roles; fix mem over-evict --- custom_claims_test.go | 6 ++-- main_test.go | 12 ++++++- review_r60_claims_mem_test.go | 65 +++++++++++++++++++++++++++++++++++ token_manager.go | 52 +++++++++++++++++++--------- universal_cache.go | 22 ++++++++++-- 5 files changed, 136 insertions(+), 21 deletions(-) create mode 100644 review_r60_claims_mem_test.go diff --git a/custom_claims_test.go b/custom_claims_test.go index 4aeafec..ef12e71 100644 --- a/custom_claims_test.go +++ b/custom_claims_test.go @@ -151,9 +151,11 @@ func TestCustomClaimNames_MalformedRoleClaim(t *testing.T) { // Configure custom claim names ts.tOidc.roleClaimName = "custom_roles" - // Create token with malformed role claim (not an array) + // Create token with malformed role claim (non-string, non-array scalar). + // A single-string role claim is now accepted as a 1-element list, so + // "malformed" here means a scalar that is neither. claims := map[string]interface{}{ - "custom_roles": "this-should-be-an-array", + "custom_roles": 12345, } token, err := createTestJWT(ts.rsaPrivateKey, "RS256", "test-key-id", claims) diff --git a/main_test.go b/main_test.go index 0f7141e..aeb2b5f 100644 --- a/main_test.go +++ b/main_test.go @@ -2299,10 +2299,20 @@ func TestExtractGroupsAndRoles(t *testing.T) { expectRoles: []string{}, expectError: false, }, + { + name: "Single string groups and roles", + claims: map[string]interface{}{ + "groups": "domain_users", // single string (not array) + "roles": []interface{}{"role1"}, + }, + expectGroups: []string{"domain_users"}, + expectRoles: []string{"role1"}, + expectError: false, + }, { name: "Invalid groups format", claims: map[string]interface{}{ - "groups": "not-an-array", + "groups": 12345, // non-string, non-array scalar "roles": []interface{}{"role1"}, }, expectError: true, diff --git a/review_r60_claims_mem_test.go b/review_r60_claims_mem_test.go new file mode 100644 index 0000000..bc54ee3 --- /dev/null +++ b/review_r60_claims_mem_test.go @@ -0,0 +1,65 @@ +package traefikoidc + +import ( + "strings" + "testing" + "time" +) + +// TestExtractGroupsRolesSingleString guards against reject-on-single-string for +// groups/roles claims. Some providers emit these as a bare string rather than +// a JSON array; the previous strict []interface{} branch returned an error, +// which dropped the identity (no allowedRolesAndGroups) or yielded a hard 403 +// (allowedRolesAndGroups configured) for a valid user. +func TestExtractGroupsRolesSingleString(t *testing.T) { + oidc := &TraefikOidc{ + groupClaimName: "groups", + roleClaimName: "roles", + logger: NewLogger(DefaultLogLevel), + } + + groups, roles, err := oidc.extractGroupsAndRolesFromClaims(map[string]interface{}{ + "groups": "domain_users", // single string + "roles": []interface{}{"admin"}, // array form still works + }) + if err != nil { + t.Fatalf("single-string groups claim should be accepted, got: %v", err) + } + if len(groups) != 1 || groups[0] != "domain_users" { + t.Fatalf("groups = %v, want [domain_users]", groups) + } + if len(roles) != 1 || roles[0] != "admin" { + t.Fatalf("roles = %v, want [admin]", roles) + } +} + +// TestUniversalCacheMemoryUpdateDoesNotOverEvict guards the memory-limit eviction +// loop: updating an existing key whose NET growth fits the cap must not evict +// live (older, still-valid) entries. The old loop tested the FULL new size +// against currentMemory while the old size was still accounted, so a +// net-fitting update evicted otherwise-fit live entries. +func TestUniversalCacheMemoryUpdateDoesNotOverEvict(t *testing.T) { + cache := NewUniversalCache(UniversalCacheConfig{ + Type: CacheTypeMetadata, + DefaultTTL: time.Hour, + MaxSize: 1000, + MaxMemoryBytes: 100, + Logger: NewLogger(DefaultLogLevel), + }) + defer cache.Close() + + live := strings.Repeat("L", 49) // size 49 + xold := strings.Repeat("x", 50) // size 50 -> total 99 + xnew := strings.Repeat("X", 51) // size 51, net growth +1 -> total 100 (fits) + + cache.Set("live", live, time.Hour) // oldest + cache.Set("x", xold, time.Hour) + cache.Set("x", xnew, time.Hour) + + if _, ok := cache.Get("live"); !ok { + t.Fatalf("live entry was evicted by a net-fitting update (over-eviction on growth)") + } + if v, ok := cache.Get("x"); !ok || v != xnew { + t.Fatalf("updated entry should be present with the new value") + } +} diff --git a/token_manager.go b/token_manager.go index 07eceef..3d3eb01 100644 --- a/token_manager.go +++ b/token_manager.go @@ -946,6 +946,34 @@ func (t *TraefikOidc) extractGroupsAndRoles(idToken string) ([]string, []string, return t.extractGroupsAndRolesFromClaims(claims) } +// stringListFromClaim coerces a claims value into a []string, accepting: +// - []interface{} of strings (common JSON path) +// - []string (some decoders emit this directly) +// - a bare string (single group/role), treated as a 1-element list. +// +// Providers sometimes emit groups/roles as a single string rather than a +// JSON array; rejecting that (as the previous strict []interface{} only +// branch did) meant a valid user holding an allowed group was denied (or +// rendered with an empty identity). +func stringListFromClaim(value interface{}) ([]string, bool) { + switch v := value.(type) { + case string: + return []string{v}, true + case []string: + return v, true + case []interface{}: + out := make([]string, 0, len(v)) + for _, item := range v { + if s, ok := item.(string); ok { + out = append(out, s) + } + } + return out, true + default: + return nil, false + } +} + // extractGroupsAndRolesFromClaims extracts group and role information from // already-parsed claims. Hot path: callers that have a cached claims map (such // as SessionData.GetIDTokenClaims) should use this to skip a redundant @@ -955,32 +983,24 @@ func (t *TraefikOidc) extractGroupsAndRolesFromClaims(claims map[string]interfac var roles []string if groupsClaim, exists := claims[t.groupClaimName]; exists { - groupsSlice, ok := groupsClaim.([]interface{}) + groupsSlice, ok := stringListFromClaim(groupsClaim) if !ok { return nil, nil, fmt.Errorf("%s claim is not an array", t.groupClaimName) } - for _, group := range groupsSlice { - if groupStr, ok := group.(string); ok { - t.logger.Debugf("Found group from %s claim: %s", t.groupClaimName, groupStr) - groups = append(groups, groupStr) - } else { - t.logger.Errorf("Non-string value found in %s claim array: %v", t.groupClaimName, group) - } + for _, groupStr := range groupsSlice { + t.logger.Debugf("Found group from %s claim: %s", t.groupClaimName, groupStr) + groups = append(groups, groupStr) } } if rolesClaim, exists := claims[t.roleClaimName]; exists { - rolesSlice, ok := rolesClaim.([]interface{}) + rolesSlice, ok := stringListFromClaim(rolesClaim) if !ok { return nil, nil, fmt.Errorf("%s claim is not an array", t.roleClaimName) } - for _, role := range rolesSlice { - if roleStr, ok := role.(string); ok { - t.logger.Debugf("Found role from %s claim: %s", t.roleClaimName, roleStr) - roles = append(roles, roleStr) - } else { - t.logger.Errorf("Non-string value found in %s claim array: %v", t.roleClaimName, role) - } + for _, roleStr := range rolesSlice { + t.logger.Debugf("Found role from %s claim: %s", t.roleClaimName, roleStr) + roles = append(roles, roleStr) } } diff --git a/universal_cache.go b/universal_cache.go index 02b7c43..624d2ba 100644 --- a/universal_cache.go +++ b/universal_cache.go @@ -276,10 +276,20 @@ func (c *UniversalCache) setLocal(key string, value interface{}, ttl time.Durati c.mu.Lock() defer c.mu.Unlock() + // For an existing key the replace below frees the OLD value's size, so the + // memory cap should only be checked against the NET growth (new - old), not + // the full new size while currentMemory still holds the old size. Using the + // full size here over-triggers eviction and drops live entries that would + // have fit once the old value was released. + growth := size + if existing, exists := c.items[key]; exists { + growth = size - existing.Size + } + // Check memory limits if c.config.MaxMemoryBytes > 0 { // Evict items if necessary to make room - for c.currentMemory+size > c.config.MaxMemoryBytes && c.lruList.Len() > 0 { + for c.currentMemory+growth > c.config.MaxMemoryBytes && c.lruList.Len() > 0 { c.evictOldest() } } @@ -950,9 +960,17 @@ func (c *UniversalCache) updateLocalCache(key string, value interface{}, ttl tim c.mu.Lock() defer c.mu.Unlock() + // Evict against NET growth for an existing key (the replace below frees the + // old size), so the memory cap isn't over-triggered and live entries aren't + // needlessly evicted. See setLocal. + growth := size + if existing, exists := c.items[key]; exists { + growth = size - existing.Size + } + // Check memory limits if c.config.MaxMemoryBytes > 0 { - for c.currentMemory+size > c.config.MaxMemoryBytes && c.lruList.Len() > 0 { + for c.currentMemory+growth > c.config.MaxMemoryBytes && c.lruList.Len() > 0 { c.evictOldest() } }