From 0c2d70238f57689b25bbd22324a879d1bed282b9 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 1 Oct 2026 10:35:05 +0000 Subject: [PATCH 1/2] authentication/oidc: accept ordered list of claim names Accept an ordered list of claim names for username_claim and group_claim, trying each in order of preference and using the first match. A single string value still works (backward compatible). - Add StringOrSlice custom type with UnmarshalJSON and MarshalJSON that accepts both "string" and ["list"] JSON forms - Add mapstructure decode hook for StringOrSlice conversion - Update oidcConfig to use StringOrSlice for UsernameClaim/GroupClaim - Refactor checkAuth to iterate claim lists with first-match-wins - Add safety gate: at least one of username or group must match when both claim lists are configured - Update main.go legacy tenant struct for list support via YAML config - Add 7 new test scenarios for claim list behavior (first match, second match, no match, mixed lists) plus 4 StringOrSlice unit tests - All 10 existing test scenarios continue to pass (backward compat) Implements: RHOBS-1795 Co-Authored-By: Claude Opus 4.6 --- authentication/oidc.go | 181 ++++++++++++++++++++++------- authentication/oidc_test.go | 226 ++++++++++++++++++++++++++++++++++++ main.go | 16 +-- 3 files changed, 374 insertions(+), 49 deletions(-) diff --git a/authentication/oidc.go b/authentication/oidc.go index ba1baefd7..be42db010 100644 --- a/authentication/oidc.go +++ b/authentication/oidc.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "crypto/x509" + "encoding/json" "encoding/pem" "errors" "fmt" @@ -11,6 +12,7 @@ import ( "net/http" "os" "path" + "reflect" "strings" "time" @@ -31,6 +33,68 @@ import ( "github.com/observatorium/api/httperr" ) +// StringOrSlice is a []string that can be unmarshaled from either a single +// JSON string or a JSON array of strings. When a single string is provided +// it becomes a one-element slice, preserving backward compatibility with +// configs that specify a single claim name. +type StringOrSlice []string + +// UnmarshalJSON implements json.Unmarshaler. +// It accepts both `"claim"` and `["claim1","claim2"]`. +func (s *StringOrSlice) UnmarshalJSON(data []byte) error { + var str string + if err := json.Unmarshal(data, &str); err == nil { + *s = StringOrSlice{str} + + return nil + } + + var arr []string + if err := json.Unmarshal(data, &arr); err != nil { + return fmt.Errorf("StringOrSlice: expected string or []string: %w", err) + } + + *s = StringOrSlice(arr) + + return nil +} + +// MarshalJSON implements json.Marshaler. +func (s StringOrSlice) MarshalJSON() ([]byte, error) { + return json.Marshal([]string(s)) +} + +// stringOrSliceDecodeHook is a mapstructure DecodeHookFunc that converts +// a string or []interface{} to StringOrSlice when the target type matches. +func stringOrSliceDecodeHook() mapstructure.DecodeHookFunc { + return func(f reflect.Type, t reflect.Type, data interface{}) (interface{}, error) { + if t != reflect.TypeOf(StringOrSlice{}) { + return data, nil + } + + switch v := data.(type) { + case string: + return []string{v}, nil + case []interface{}: + result := make([]string, len(v)) + for i, elem := range v { + s, ok := elem.(string) + if !ok { + return nil, fmt.Errorf("StringOrSlice element %d is not a string", i) + } + + result[i] = s + } + + return result, nil + case []string: + return v, nil + default: + return data, nil + } + } +} + // OIDCAuthenticatorType represents the oidc authentication provider type. const OIDCAuthenticatorType = "oidc" @@ -40,15 +104,15 @@ func init() { // oidcConfig represents the oidc authenticator config. type oidcConfig struct { - ClientID string `json:"clientID"` - ClientSecret string `json:"clientSecret"` - GroupClaim string `json:"groupClaim"` - IssuerRawCA []byte `json:"issuerCA"` - IssuerCAPath string `json:"issuerCAPath"` + ClientID string `json:"clientID"` + ClientSecret string `json:"clientSecret"` + GroupClaim StringOrSlice `json:"groupClaim"` + IssuerRawCA []byte `json:"issuerCA"` + IssuerCAPath string `json:"issuerCAPath"` issuerCA *x509.Certificate - IssuerURL string `json:"issuerURL"` - RedirectURL string `json:"redirectURL"` - UsernameClaim string `json:"usernameClaim"` + IssuerURL string `json:"issuerURL"` + RedirectURL string `json:"redirectURL"` + UsernameClaim StringOrSlice `json:"usernameClaim"` } type oidcAuthenticator struct { @@ -77,8 +141,15 @@ func newOIDCAuthenticator(c map[string]interface{}, tenant string, ctx := context.Background() - err := mapstructure.Decode(c, &config) + decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: stringOrSliceDecodeHook(), + Result: &config, + }) if err != nil { + return nil, fmt.Errorf("create config decoder: %w", err) + } + + if err := decoder.Decode(c); err != nil { return nil, err } @@ -371,9 +442,14 @@ func (a oidcAuthenticator) checkAuth(ctx context.Context, token string) (context sub := idToken.Subject usernameExtracted := false + hasUsernameClaims := len(a.config.UsernameClaim) > 0 + hasGroupClaims := len(a.config.GroupClaim) > 0 + + // Extract claims once for both username and group lookups. + var claims map[string]interface{} - if a.config.UsernameClaim != "" { - claims := map[string]interface{}{} + if hasUsernameClaims || hasGroupClaims { + claims = map[string]interface{}{} if err := idToken.Claims(&claims); err != nil { const msg = "failed to read claims" @@ -381,59 +457,58 @@ func (a oidcAuthenticator) checkAuth(ctx context.Context, token string) (context return ctx, msg, http.StatusInternalServerError, codes.Internal } + } - rawUsername, ok := claims[a.config.UsernameClaim] - if !ok { - if a.config.GroupClaim == "" { - const msg = "username cannot be empty" - - level.Debug(a.logger).Log("msg", msg) - - return ctx, msg, http.StatusBadRequest, codes.PermissionDenied + // Try username claims in order of preference — first match wins. + if hasUsernameClaims { + for _, claimName := range a.config.UsernameClaim { + rawUsername, ok := claims[claimName] + if !ok { + continue } - level.Debug(a.logger).Log("msg", "username claim not found in token, proceeding with group claim") - } else { username, ok := rawUsername.(string) if !ok || username == "" { const msg = "invalid username claim value" - level.Debug(a.logger).Log("msg", msg) + level.Debug(a.logger).Log("msg", msg, "claim", claimName) return ctx, msg, http.StatusBadRequest, codes.PermissionDenied } sub = username usernameExtracted = true - } - } - ctx = context.WithValue(ctx, subjectKey, sub) + break + } - if a.config.GroupClaim != "" { - var groups []string + if !usernameExtracted && !hasGroupClaims { + const msg = "username cannot be empty" - claims := map[string]interface{}{} - if err := idToken.Claims(&claims); err != nil { - const msg = "failed to read claims" + level.Debug(a.logger).Log("msg", msg) - level.Warn(a.logger).Log("msg", msg, "err", err) + return ctx, msg, http.StatusBadRequest, codes.PermissionDenied + } - return ctx, msg, http.StatusInternalServerError, codes.Internal + if !usernameExtracted { + level.Debug(a.logger).Log("msg", "username claim not found in token, proceeding with group claim") } + } - rawGroup, ok := claims[a.config.GroupClaim] - if !ok { - if usernameExtracted { - level.Debug(a.logger).Log("msg", "group claim not found, proceeding with username") - } else { - const msg = "group cannot be empty" + ctx = context.WithValue(ctx, subjectKey, sub) - level.Debug(a.logger).Log("msg", msg) + // Try group claims in order of preference — first match wins. + groupExtracted := false - return ctx, msg, http.StatusBadRequest, codes.PermissionDenied + if hasGroupClaims { + for _, claimName := range a.config.GroupClaim { + rawGroup, ok := claims[claimName] + if !ok { + continue } - } else { + + var groups []string + switch v := rawGroup.(type) { case string: groups = append(groups, v) @@ -447,7 +522,31 @@ func (a oidcAuthenticator) checkAuth(ctx context.Context, token string) (context } ctx = context.WithValue(ctx, groupsKey, groups) + groupExtracted = true + + break } + + if !groupExtracted { + if usernameExtracted { + level.Debug(a.logger).Log("msg", "group claim not found, proceeding with username") + } else { + const msg = "group cannot be empty" + + level.Debug(a.logger).Log("msg", msg) + + return ctx, msg, http.StatusBadRequest, codes.PermissionDenied + } + } + } + + // Safety gate: when both claim lists are configured, at least one must match. + if hasUsernameClaims && hasGroupClaims && !usernameExtracted && !groupExtracted { + const msg = "no matching claims found" + + level.Debug(a.logger).Log("msg", msg) + + return ctx, msg, http.StatusBadRequest, codes.PermissionDenied } return ctx, "", http.StatusOK, codes.OK diff --git a/authentication/oidc_test.go b/authentication/oidc_test.go index f2971f082..ad0b5e8dc 100644 --- a/authentication/oidc_test.go +++ b/authentication/oidc_test.go @@ -68,9 +68,29 @@ func newTestOIDCEnv(t *testing.T) *testOIDCEnv { // newAuthenticator creates an oidcAuthenticator wired to the test OIDC server // with the given username/group claim configuration. +// Each claim parameter accepts a single string (backward compat) which is +// converted to a one-element StringOrSlice, or an empty string which stays +// as a nil/empty slice. func (env *testOIDCEnv) newAuthenticator(t *testing.T, usernameClaim, groupClaim string) *oidcAuthenticator { t.Helper() + var uc, gc StringOrSlice + if usernameClaim != "" { + uc = StringOrSlice{usernameClaim} + } + + if groupClaim != "" { + gc = StringOrSlice{groupClaim} + } + + return env.newAuthenticatorFromSlices(t, uc, gc) +} + +// newAuthenticatorFromSlices creates an oidcAuthenticator with full +// StringOrSlice claim lists (for testing ordered claim list behavior). +func (env *testOIDCEnv) newAuthenticatorFromSlices(t *testing.T, usernameClaim, groupClaim StringOrSlice) *oidcAuthenticator { + t.Helper() + ctx := oidc.ClientContext(context.Background(), env.server.Client()) provider, err := oidc.NewProvider(ctx, env.issuerURL) @@ -311,3 +331,209 @@ func TestCheckAuth(t *testing.T) { }) } } + +func TestCheckAuthClaimLists(t *testing.T) { + env := newTestOIDCEnv(t) + defer env.server.Close() + + tests := []struct { + name string + usernameClaim StringOrSlice + groupClaim StringOrSlice + extraClaims map[string]interface{} + wantCode int + wantSubject string + wantGroups []string + }{ + { + name: "username list first claim matches", + usernameClaim: StringOrSlice{"preferred_username", "email"}, + groupClaim: StringOrSlice{"groups"}, + extraClaims: map[string]interface{}{ + "preferred_username": "alice", + "email": "alice@example.com", + "groups": []interface{}{"devs"}, + }, + wantCode: http.StatusOK, + wantSubject: "alice", + wantGroups: []string{"devs"}, + }, + { + name: "username list second claim matches when first absent", + usernameClaim: StringOrSlice{"preferred_username", "email"}, + groupClaim: StringOrSlice{"groups"}, + extraClaims: map[string]interface{}{ + // preferred_username absent + "email": "bob@example.com", + "groups": []interface{}{"ops"}, + }, + wantCode: http.StatusOK, + wantSubject: "bob@example.com", + wantGroups: []string{"ops"}, + }, + { + name: "group list first claim matches", + usernameClaim: StringOrSlice{"preferred_username"}, + groupClaim: StringOrSlice{"org_id", "rh-org-id", "groups"}, + extraClaims: map[string]interface{}{ + "preferred_username": "carol", + "org_id": "org-123", + "groups": []interface{}{"team-a"}, + }, + wantCode: http.StatusOK, + wantSubject: "carol", + wantGroups: []string{"org-123"}, + }, + { + name: "group list second claim matches when first absent", + usernameClaim: StringOrSlice{"preferred_username"}, + groupClaim: StringOrSlice{"org_id", "rh-org-id", "groups"}, + extraClaims: map[string]interface{}{ + "preferred_username": "dave", + // org_id absent + "rh-org-id": "rh-456", + "groups": []interface{}{"team-b"}, + }, + wantCode: http.StatusOK, + wantSubject: "dave", + wantGroups: []string{"rh-456"}, + }, + { + name: "no claims match across both lists returns 400", + usernameClaim: StringOrSlice{"preferred_username", "email"}, + groupClaim: StringOrSlice{"org_id", "rh-org-id"}, + extraClaims: map[string]interface{}{ + // none of the configured claims are present + }, + wantCode: http.StatusBadRequest, + }, + { + name: "mixed lists only group matches proceeds OK", + usernameClaim: StringOrSlice{"preferred_username", "email"}, + groupClaim: StringOrSlice{"org_id", "groups"}, + extraClaims: map[string]interface{}{ + // no username claims present + "groups": []interface{}{"sre-team"}, + }, + wantCode: http.StatusOK, + wantSubject: "test-subject", // falls back to token subject + wantGroups: []string{"sre-team"}, + }, + { + name: "mixed lists only username matches proceeds OK", + usernameClaim: StringOrSlice{"email"}, + groupClaim: StringOrSlice{"org_id", "rh-org-id"}, + extraClaims: map[string]interface{}{ + "email": "eve@example.com", + // no group claims present + }, + wantCode: http.StatusOK, + wantSubject: "eve@example.com", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + auth := env.newAuthenticatorFromSlices(t, tt.usernameClaim, tt.groupClaim) + token := env.signToken(t, tt.extraClaims) + + ctx, _, httpCode, _ := auth.checkAuth(context.Background(), token) + + if httpCode != tt.wantCode { + t.Fatalf("HTTP status = %d, want %d", httpCode, tt.wantCode) + } + + if tt.wantCode != http.StatusOK { + return + } + + if tt.wantSubject != "" { + gotSubject, ok := GetSubject(ctx) + if !ok { + t.Fatal("expected subject in context, got none") + } + + if gotSubject != tt.wantSubject { + t.Errorf("subject = %q, want %q", gotSubject, tt.wantSubject) + } + } + + if tt.wantGroups != nil { + gotGroups, ok := GetGroups(ctx) + if !ok { + t.Fatal("expected groups in context, got none") + } + + if len(gotGroups) != len(tt.wantGroups) { + t.Fatalf("got %d groups %v, want %d groups %v", + len(gotGroups), gotGroups, len(tt.wantGroups), tt.wantGroups) + } + + for i, g := range gotGroups { + if g != tt.wantGroups[i] { + t.Errorf("groups[%d] = %q, want %q", i, g, tt.wantGroups[i]) + } + } + } + }) + } +} + +func TestStringOrSliceUnmarshalJSON(t *testing.T) { + tests := []struct { + name string + input string + want StringOrSlice + wantErr bool + }{ + { + name: "single string", + input: `"preferred_username"`, + want: StringOrSlice{"preferred_username"}, + }, + { + name: "array of strings", + input: `["preferred_username","email"]`, + want: StringOrSlice{"preferred_username", "email"}, + }, + { + name: "empty array", + input: `[]`, + want: StringOrSlice{}, + }, + { + name: "invalid type number", + input: `42`, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got StringOrSlice + err := got.UnmarshalJSON([]byte(tt.input)) + + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(got) != len(tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + + for i, v := range got { + if v != tt.want[i] { + t.Errorf("element %d = %q, want %q", i, v, tt.want[i]) + } + } + }) + } +} diff --git a/main.go b/main.go index 1351537c2..f3b972476 100644 --- a/main.go +++ b/main.go @@ -234,15 +234,15 @@ type tenant struct { Name string `json:"name"` ID string `json:"id"` OIDC *struct { - ClientID string `json:"clientID"` - ClientSecret string `json:"clientSecret"` - GroupClaim string `json:"groupClaim"` - IssuerRawCA []byte `json:"issuerCA"` - IssuerCAPath string `json:"issuerCAPath"` + ClientID string `json:"clientID"` + ClientSecret string `json:"clientSecret"` + GroupClaim authentication.StringOrSlice `json:"groupClaim"` + IssuerRawCA []byte `json:"issuerCA"` + IssuerCAPath string `json:"issuerCAPath"` issuerCA *x509.Certificate - IssuerURL string `json:"issuerURL"` - RedirectURL string `json:"redirectURL"` - UsernameClaim string `json:"usernameClaim"` + IssuerURL string `json:"issuerURL"` + RedirectURL string `json:"redirectURL"` + UsernameClaim authentication.StringOrSlice `json:"usernameClaim"` config map[string]interface{} } `json:"oidc"` OpenShift *struct { From d06225ba5e1ba149fcfe7903728a5c5199067ebb Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 1 Oct 2026 14:59:50 +0000 Subject: [PATCH 2/2] fix: use explicit StringOrSlice type conversions Co-Authored-By: Claude Opus 4.6 --- authentication/oidc.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authentication/oidc.go b/authentication/oidc.go index be42db010..3db182605 100644 --- a/authentication/oidc.go +++ b/authentication/oidc.go @@ -74,7 +74,7 @@ func stringOrSliceDecodeHook() mapstructure.DecodeHookFunc { switch v := data.(type) { case string: - return []string{v}, nil + return StringOrSlice{v}, nil case []interface{}: result := make([]string, len(v)) for i, elem := range v { @@ -86,7 +86,7 @@ func stringOrSliceDecodeHook() mapstructure.DecodeHookFunc { result[i] = s } - return result, nil + return StringOrSlice(result), nil case []string: return v, nil default: