From 3c107c5958244eb26653aeccbbd8fd74ab7b68db Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 4 May 2026 13:39:59 +0300 Subject: [PATCH 1/3] auth/oidc: test slice-typed extra claims comparison Adds coverage for the value-type combinations claimMatches handles: scalar/array (both directions), array/array overlap, []string-typed claims, and missing-claim. Refactors shared config into testOIDCConfig to keep new test cases concise. Follow-up to #1238, addresses #988. --- internal/api/handlers/v0/auth/oidc_test.go | 146 +++++++++++++++------ 1 file changed, 104 insertions(+), 42 deletions(-) diff --git a/internal/api/handlers/v0/auth/oidc_test.go b/internal/api/handlers/v0/auth/oidc_test.go index d4e9500b..d3eb76ea 100644 --- a/internal/api/handlers/v0/auth/oidc_test.go +++ b/internal/api/handlers/v0/auth/oidc_test.go @@ -11,6 +11,12 @@ import ( "github.com/stretchr/testify/require" ) +const ( + testJWTPrivateKey = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef" // 32-byte hex test key + testOIDCIssuer = "https://accounts.google.com" + testOIDCClientID = "test-client-id" +) + // MockGenericOIDCValidator for testing type MockGenericOIDCValidator struct { validateFunc func(ctx context.Context, token string) (*auth.OIDCClaims, error) @@ -23,6 +29,25 @@ func (m *MockGenericOIDCValidator) ValidateToken(ctx context.Context, token stri return nil, fmt.Errorf("not implemented") } +func testOIDCConfig(extraClaims string) *config.Config { + return &config.Config{ + OIDCEnabled: true, + OIDCIssuer: testOIDCIssuer, + OIDCClientID: testOIDCClientID, + OIDCExtraClaims: extraClaims, + OIDCPublishPerms: "*", + JWTPrivateKey: testJWTPrivateKey, + } +} + +func mockValidator(claims map[string]any) *MockGenericOIDCValidator { + return &MockGenericOIDCValidator{ + validateFunc: func(_ context.Context, _ string) (*auth.OIDCClaims, error) { + return &auth.OIDCClaims{ExtraClaims: claims}, nil + }, + } +} + func TestOIDCHandler_ExchangeToken(t *testing.T) { tests := []struct { name string @@ -32,55 +57,92 @@ func TestOIDCHandler_ExchangeToken(t *testing.T) { expectedError bool }{ { //nolint:gosec // G101: test data, not real credentials - name: "successful token exchange with publish permissions", - config: &config.Config{ - OIDCEnabled: true, - OIDCIssuer: "https://accounts.google.com", - OIDCClientID: "test-client-id", - OIDCExtraClaims: `[{"hd":"modelcontextprotocol.io"}]`, - OIDCPublishPerms: "*", - JWTPrivateKey: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", // 32 byte hex - }, - mockValidator: &MockGenericOIDCValidator{ - validateFunc: func(_ context.Context, _ string) (*auth.OIDCClaims, error) { - return &auth.OIDCClaims{ - ExtraClaims: map[string]any{ - "email": "admin@modelcontextprotocol.io", - "email_verified": true, - "hd": "modelcontextprotocol.io", - "preferred_username": "admin", - }, - }, nil - }, - }, + name: "successful token exchange with publish permissions", + config: testOIDCConfig(`[{"hd":"modelcontextprotocol.io"}]`), + mockValidator: mockValidator(map[string]any{ + "email": "admin@modelcontextprotocol.io", + "email_verified": true, + "hd": "modelcontextprotocol.io", + "preferred_username": "admin", //nolint:goconst // role label repeated across test fixtures, not a constant + }), token: "valid-oidc-token", expectedError: false, }, { - name: "failed validation with invalid hosted domain", - config: &config.Config{ - OIDCEnabled: true, - OIDCIssuer: "https://accounts.google.com", - OIDCClientID: "test-client-id", - OIDCExtraClaims: `[{"hd":"modelcontextprotocol.io"}]`, - OIDCPublishPerms: "*", - JWTPrivateKey: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", - }, - mockValidator: &MockGenericOIDCValidator{ - validateFunc: func(_ context.Context, _ string) (*auth.OIDCClaims, error) { - return &auth.OIDCClaims{ - ExtraClaims: map[string]any{ - "email": "user@example.com", - "email_verified": true, - "hd": "example.com", // Wrong domain - "preferred_username": "user", - }, - }, nil - }, - }, + name: "failed validation with invalid hosted domain", + config: testOIDCConfig(`[{"hd":"modelcontextprotocol.io"}]`), + mockValidator: mockValidator(map[string]any{ + "email": "user@example.com", + "email_verified": true, + "hd": "example.com", + "preferred_username": "user", + }), token: "invalid-domain-token", expectedError: true, }, + { + name: "scalar expected matches array claim", + config: testOIDCConfig(`[{"groups":"admin"}]`), + mockValidator: mockValidator(map[string]any{ + "groups": []any{"admin", "users", "developers"}, //nolint:goconst // claim key repeated across fixtures, not a constant + }), + token: "scalar-expected-array-actual-match", + expectedError: false, + }, + { + name: "scalar expected not present in array claim", + config: testOIDCConfig(`[{"groups":"super-admin"}]`), + mockValidator: mockValidator(map[string]any{ + "groups": []any{"admin", "users", "developers"}, + }), + token: "scalar-expected-array-actual-no-match", + expectedError: true, + }, + { + name: "array expected overlaps array claim", + config: testOIDCConfig(`[{"roles":["admin","moderator"]}]`), + mockValidator: mockValidator(map[string]any{ + "roles": []any{"admin", "users"}, + }), + token: "array-array-overlap", + expectedError: false, + }, + { //nolint:gosec // G101: test data, not real credentials + name: "array expected disjoint from array claim", + config: testOIDCConfig(`[{"roles":["super-admin","owner"]}]`), + mockValidator: mockValidator(map[string]any{ + "roles": []any{"admin", "users"}, + }), + token: "array-array-no-overlap", + expectedError: true, + }, + { //nolint:gosec // G101: test data, not real credentials + name: "array expected matches scalar claim", + config: testOIDCConfig(`[{"role":["admin","moderator"]}]`), + mockValidator: mockValidator(map[string]any{ + "role": "admin", + }), + token: "scalar-actual-array-expected-match", + expectedError: false, + }, + { //nolint:gosec // G101: test data, not real credentials + name: "[]string typed claim matches scalar expected", + config: testOIDCConfig(`[{"groups":"admin"}]`), + mockValidator: mockValidator(map[string]any{ + "groups": []string{"admin", "users"}, + }), + token: "string-slice-claim", + expectedError: false, + }, + { + name: "required claim missing", + config: testOIDCConfig(`[{"required_claim":"expected_value"}]`), + mockValidator: mockValidator(map[string]any{ + "other_claim": "some_value", + }), + token: "missing-claim", + expectedError: true, + }, } for _, tt := range tests { From f9d6b7b4b4e9ab2b773985cfe8939f9e7c0b34ab Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 4 May 2026 13:45:10 +0300 Subject: [PATCH 2/3] auth/oidc: drop unused goconst nolint directives The CI lint version (golangci-lint v2.11.4) doesn't trigger goconst on these lines, so the //nolint:goconst comments were flagged as unused by nolintlint. --- internal/api/handlers/v0/auth/oidc_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/handlers/v0/auth/oidc_test.go b/internal/api/handlers/v0/auth/oidc_test.go index d3eb76ea..85412e31 100644 --- a/internal/api/handlers/v0/auth/oidc_test.go +++ b/internal/api/handlers/v0/auth/oidc_test.go @@ -63,7 +63,7 @@ func TestOIDCHandler_ExchangeToken(t *testing.T) { "email": "admin@modelcontextprotocol.io", "email_verified": true, "hd": "modelcontextprotocol.io", - "preferred_username": "admin", //nolint:goconst // role label repeated across test fixtures, not a constant + "preferred_username": "admin", }), token: "valid-oidc-token", expectedError: false, @@ -84,7 +84,7 @@ func TestOIDCHandler_ExchangeToken(t *testing.T) { name: "scalar expected matches array claim", config: testOIDCConfig(`[{"groups":"admin"}]`), mockValidator: mockValidator(map[string]any{ - "groups": []any{"admin", "users", "developers"}, //nolint:goconst // claim key repeated across fixtures, not a constant + "groups": []any{"admin", "users", "developers"}, }), token: "scalar-expected-array-actual-match", expectedError: false, From a1dee0b56998339175ef5dfcf22f21bea81447a7 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 4 May 2026 13:48:24 +0300 Subject: [PATCH 3/3] auth/oidc: replace integration-style tests with focused unit tests Drops the heavyweight ExchangeToken-based test cases added in earlier commits on this branch. The actual change in #1238 is the claimMatches helper, which is much simpler to cover with direct unit tests. Reverts the existing oidc_test.go to its main-branch state and adds a new oidc_internal_test.go (package auth) with table-driven cases for scalar/array/[]string combinations. --- .../handlers/v0/auth/oidc_internal_test.go | 41 +++++ internal/api/handlers/v0/auth/oidc_test.go | 146 +++++------------- 2 files changed, 83 insertions(+), 104 deletions(-) create mode 100644 internal/api/handlers/v0/auth/oidc_internal_test.go diff --git a/internal/api/handlers/v0/auth/oidc_internal_test.go b/internal/api/handlers/v0/auth/oidc_internal_test.go new file mode 100644 index 00000000..cc3e472b --- /dev/null +++ b/internal/api/handlers/v0/auth/oidc_internal_test.go @@ -0,0 +1,41 @@ +package auth + +import "testing" + +func TestClaimMatches(t *testing.T) { + tests := []struct { + name string + actual any + expected any + want bool + }{ + {"scalar/scalar match", "x", "x", true}, + {"scalar/scalar mismatch", "x", "y", false}, + + {"scalar actual in array expected", "x", []any{"x", "y"}, true}, + {"scalar actual not in array expected", "z", []any{"x", "y"}, false}, + + {"array actual contains scalar expected", []any{"x", "y"}, "x", true}, + {"array actual missing scalar expected", []any{"y", "z"}, "x", false}, + + // Pre-#1238 array/array comparison panicked with "comparing uncomparable type []interface {}". + {"array/array overlap", []any{"a", "b"}, []any{"b", "c"}, true}, + {"array/array disjoint", []any{"a", "b"}, []any{"c", "d"}, false}, + + // Exercises the []string branch of toAnySlice (concrete typed slice, not []any). + {"[]string actual matches scalar", []string{"p", "q"}, "p", true}, + {"scalar matches []string expected", "p", []string{"p", "q"}, true}, + + {"non-string scalar match", 42, 42, true}, + {"bool scalar match", true, true, true}, + {"empty actual array", []any{}, "x", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := claimMatches(tt.actual, tt.expected); got != tt.want { + t.Errorf("claimMatches(%v, %v) = %v, want %v", tt.actual, tt.expected, got, tt.want) + } + }) + } +} diff --git a/internal/api/handlers/v0/auth/oidc_test.go b/internal/api/handlers/v0/auth/oidc_test.go index 85412e31..d4e9500b 100644 --- a/internal/api/handlers/v0/auth/oidc_test.go +++ b/internal/api/handlers/v0/auth/oidc_test.go @@ -11,12 +11,6 @@ import ( "github.com/stretchr/testify/require" ) -const ( - testJWTPrivateKey = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef" // 32-byte hex test key - testOIDCIssuer = "https://accounts.google.com" - testOIDCClientID = "test-client-id" -) - // MockGenericOIDCValidator for testing type MockGenericOIDCValidator struct { validateFunc func(ctx context.Context, token string) (*auth.OIDCClaims, error) @@ -29,25 +23,6 @@ func (m *MockGenericOIDCValidator) ValidateToken(ctx context.Context, token stri return nil, fmt.Errorf("not implemented") } -func testOIDCConfig(extraClaims string) *config.Config { - return &config.Config{ - OIDCEnabled: true, - OIDCIssuer: testOIDCIssuer, - OIDCClientID: testOIDCClientID, - OIDCExtraClaims: extraClaims, - OIDCPublishPerms: "*", - JWTPrivateKey: testJWTPrivateKey, - } -} - -func mockValidator(claims map[string]any) *MockGenericOIDCValidator { - return &MockGenericOIDCValidator{ - validateFunc: func(_ context.Context, _ string) (*auth.OIDCClaims, error) { - return &auth.OIDCClaims{ExtraClaims: claims}, nil - }, - } -} - func TestOIDCHandler_ExchangeToken(t *testing.T) { tests := []struct { name string @@ -57,92 +32,55 @@ func TestOIDCHandler_ExchangeToken(t *testing.T) { expectedError bool }{ { //nolint:gosec // G101: test data, not real credentials - name: "successful token exchange with publish permissions", - config: testOIDCConfig(`[{"hd":"modelcontextprotocol.io"}]`), - mockValidator: mockValidator(map[string]any{ - "email": "admin@modelcontextprotocol.io", - "email_verified": true, - "hd": "modelcontextprotocol.io", - "preferred_username": "admin", - }), + name: "successful token exchange with publish permissions", + config: &config.Config{ + OIDCEnabled: true, + OIDCIssuer: "https://accounts.google.com", + OIDCClientID: "test-client-id", + OIDCExtraClaims: `[{"hd":"modelcontextprotocol.io"}]`, + OIDCPublishPerms: "*", + JWTPrivateKey: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", // 32 byte hex + }, + mockValidator: &MockGenericOIDCValidator{ + validateFunc: func(_ context.Context, _ string) (*auth.OIDCClaims, error) { + return &auth.OIDCClaims{ + ExtraClaims: map[string]any{ + "email": "admin@modelcontextprotocol.io", + "email_verified": true, + "hd": "modelcontextprotocol.io", + "preferred_username": "admin", + }, + }, nil + }, + }, token: "valid-oidc-token", expectedError: false, }, { - name: "failed validation with invalid hosted domain", - config: testOIDCConfig(`[{"hd":"modelcontextprotocol.io"}]`), - mockValidator: mockValidator(map[string]any{ - "email": "user@example.com", - "email_verified": true, - "hd": "example.com", - "preferred_username": "user", - }), + name: "failed validation with invalid hosted domain", + config: &config.Config{ + OIDCEnabled: true, + OIDCIssuer: "https://accounts.google.com", + OIDCClientID: "test-client-id", + OIDCExtraClaims: `[{"hd":"modelcontextprotocol.io"}]`, + OIDCPublishPerms: "*", + JWTPrivateKey: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", + }, + mockValidator: &MockGenericOIDCValidator{ + validateFunc: func(_ context.Context, _ string) (*auth.OIDCClaims, error) { + return &auth.OIDCClaims{ + ExtraClaims: map[string]any{ + "email": "user@example.com", + "email_verified": true, + "hd": "example.com", // Wrong domain + "preferred_username": "user", + }, + }, nil + }, + }, token: "invalid-domain-token", expectedError: true, }, - { - name: "scalar expected matches array claim", - config: testOIDCConfig(`[{"groups":"admin"}]`), - mockValidator: mockValidator(map[string]any{ - "groups": []any{"admin", "users", "developers"}, - }), - token: "scalar-expected-array-actual-match", - expectedError: false, - }, - { - name: "scalar expected not present in array claim", - config: testOIDCConfig(`[{"groups":"super-admin"}]`), - mockValidator: mockValidator(map[string]any{ - "groups": []any{"admin", "users", "developers"}, - }), - token: "scalar-expected-array-actual-no-match", - expectedError: true, - }, - { - name: "array expected overlaps array claim", - config: testOIDCConfig(`[{"roles":["admin","moderator"]}]`), - mockValidator: mockValidator(map[string]any{ - "roles": []any{"admin", "users"}, - }), - token: "array-array-overlap", - expectedError: false, - }, - { //nolint:gosec // G101: test data, not real credentials - name: "array expected disjoint from array claim", - config: testOIDCConfig(`[{"roles":["super-admin","owner"]}]`), - mockValidator: mockValidator(map[string]any{ - "roles": []any{"admin", "users"}, - }), - token: "array-array-no-overlap", - expectedError: true, - }, - { //nolint:gosec // G101: test data, not real credentials - name: "array expected matches scalar claim", - config: testOIDCConfig(`[{"role":["admin","moderator"]}]`), - mockValidator: mockValidator(map[string]any{ - "role": "admin", - }), - token: "scalar-actual-array-expected-match", - expectedError: false, - }, - { //nolint:gosec // G101: test data, not real credentials - name: "[]string typed claim matches scalar expected", - config: testOIDCConfig(`[{"groups":"admin"}]`), - mockValidator: mockValidator(map[string]any{ - "groups": []string{"admin", "users"}, - }), - token: "string-slice-claim", - expectedError: false, - }, - { - name: "required claim missing", - config: testOIDCConfig(`[{"required_claim":"expected_value"}]`), - mockValidator: mockValidator(map[string]any{ - "other_claim": "some_value", - }), - token: "missing-claim", - expectedError: true, - }, } for _, tt := range tests {