R60: accept single-string groups/roles; fix mem over-evict

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)

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,

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")
+	}
+}

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)
 		}
 	}
 

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()
 		}
 	}