Skip to content

Commit 233b73a

Browse files
amirejazclaude
andcommitted
Address PR review feedback on CIMD wiring and add missing tests
- Fix CacheFallbackTTL comment to say it is a fixed TTL (not fallback); matches the fix already applied in PR #5343 - Add TODO(cimd) comment above CIMDRunConfig noting the CRD exposure gap - Add discovery handler tests: CIMDEnabled=true advertises the flag, CIMDEnabled=false omits it, for both AS metadata and OIDC endpoints - Add config defaults tests: CIMDEnabled=true fills in cache size/TTL defaults; CIMDEnabled=false leaves zero fields unchanged - Add resolveCIMDConfig nil-guard test: nil input returns zero values, non-nil passes all fields through Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent a48ad9a commit 233b73a

4 files changed

Lines changed: 133 additions & 2 deletions

File tree

pkg/authserver/config.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ func (c *RunConfig) validateBaselineClientScopes() error {
124124
}
125125

126126
// CIMDRunConfig controls client_id metadata document (CIMD) support.
127+
//
128+
// TODO(cimd): expose these fields in the MCPExternalAuthConfig CRD so Kubernetes
129+
// operators can configure CIMD through the normal CRD workflow instead of
130+
// writing RunConfig YAML directly.
127131
type CIMDRunConfig struct {
128132
// Enabled activates CIMD client lookup when true.
129133
Enabled bool `json:"enabled" yaml:"enabled"`
@@ -132,8 +136,8 @@ type CIMDRunConfig struct {
132136
// Defaults to 256 when Enabled is true and this field is zero.
133137
CacheMaxSize int `json:"cache_max_size,omitempty" yaml:"cache_max_size,omitempty"`
134138

135-
// CacheFallbackTTL is how long a cached CIMD document is considered valid when
136-
// the fetched document carries no Cache-Control header.
139+
// CacheFallbackTTL is the fixed TTL applied to every cached CIMD document.
140+
// Cache-Control header parsing is not yet implemented; all entries use this value.
137141
// Defaults to 5 minutes when Enabled is true and this field is zero.
138142
//nolint:lll // field tags require full JSON+YAML names
139143
CacheFallbackTTL time.Duration `json:"cache_fallback_ttl,omitempty" yaml:"cache_fallback_ttl,omitempty" swaggertype:"string" example:"5m"`

pkg/authserver/config_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -589,3 +589,49 @@ func TestConfigApplyDefaults_BaselineClientScopes(t *testing.T) {
589589
})
590590
}
591591
}
592+
593+
func TestConfigApplyDefaults_CIMD(t *testing.T) {
594+
t.Parallel()
595+
596+
tests := []struct {
597+
name string
598+
cfg Config
599+
wantMaxSize int
600+
wantFallbackTTL time.Duration
601+
}{
602+
{
603+
name: "CIMD enabled with zero fields applies defaults",
604+
cfg: Config{Issuer: "https://example.com", CIMDEnabled: true},
605+
wantMaxSize: 256,
606+
wantFallbackTTL: 5 * time.Minute,
607+
},
608+
{
609+
name: "CIMD enabled preserves non-zero values",
610+
cfg: Config{
611+
Issuer: "https://example.com",
612+
CIMDEnabled: true,
613+
CIMDCacheMaxSize: 128,
614+
CIMDCacheFallbackTTL: 10 * time.Minute,
615+
},
616+
wantMaxSize: 128,
617+
wantFallbackTTL: 10 * time.Minute,
618+
},
619+
{
620+
name: "CIMD disabled leaves zero fields unchanged",
621+
cfg: Config{Issuer: "https://example.com", CIMDEnabled: false},
622+
wantMaxSize: 0,
623+
wantFallbackTTL: 0,
624+
},
625+
}
626+
627+
for _, tt := range tests {
628+
t.Run(tt.name, func(t *testing.T) {
629+
t.Parallel()
630+
cfg := tt.cfg
631+
err := cfg.applyDefaults()
632+
require.NoError(t, err)
633+
require.Equal(t, tt.wantMaxSize, cfg.CIMDCacheMaxSize)
634+
require.Equal(t, tt.wantFallbackTTL, cfg.CIMDCacheFallbackTTL)
635+
})
636+
}
637+
}

pkg/authserver/runner/embeddedauthserver_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2035,3 +2035,28 @@ func TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog(t *testing.T) {
20352035
assert.Contains(t, logged, "redis.example.com",
20362036
"closeErr host must remain in the Warn record after sanitisation")
20372037
}
2038+
2039+
func TestResolveCIMDConfig(t *testing.T) {
2040+
t.Parallel()
2041+
2042+
t.Run("nil input returns zero values", func(t *testing.T) {
2043+
t.Parallel()
2044+
enabled, size, ttl := resolveCIMDConfig(nil)
2045+
assert.False(t, enabled)
2046+
assert.Zero(t, size)
2047+
assert.Zero(t, ttl)
2048+
})
2049+
2050+
t.Run("non-nil input passes values through", func(t *testing.T) {
2051+
t.Parallel()
2052+
cfg := &authserver.CIMDRunConfig{
2053+
Enabled: true,
2054+
CacheMaxSize: 128,
2055+
CacheFallbackTTL: 10 * time.Minute,
2056+
}
2057+
enabled, size, ttl := resolveCIMDConfig(cfg)
2058+
assert.True(t, enabled)
2059+
assert.Equal(t, 128, size)
2060+
assert.Equal(t, 10*time.Minute, ttl)
2061+
})
2062+
}

pkg/authserver/server/handlers/handlers_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import (
3838
// testSetupOptions allows customizing the test handler setup.
3939
type testSetupOptions struct {
4040
AuthorizationEndpointBaseURL string
41+
CIMDEnabled bool
4142
}
4243

4344
// testSetup creates a Handler with all dependencies for testing.
@@ -66,6 +67,7 @@ func testSetupWithOptions(t *testing.T, opts testSetupOptions) *Handler {
6667
cfg := &server.AuthorizationServerParams{
6768
Issuer: "https://auth.example.com",
6869
AuthorizationEndpointBaseURL: opts.AuthorizationEndpointBaseURL,
70+
CIMDEnabled: opts.CIMDEnabled,
6971
AccessTokenLifespan: time.Hour,
7072
RefreshTokenLifespan: time.Hour * 24,
7173
AuthCodeLifespan: time.Minute * 10,
@@ -340,3 +342,57 @@ func TestWellKnownRoutes(t *testing.T) {
340342
}
341343

342344
// TODO: Add TestOAuthRoutes once OAuth handlers are implemented
345+
346+
func TestDiscoveryHandlers_CIMDEnabled_AdvertisesSupport(t *testing.T) {
347+
t.Parallel()
348+
349+
handler := testSetupWithOptions(t, testSetupOptions{CIMDEnabled: true})
350+
351+
for _, tc := range []struct {
352+
name string
353+
fn func(http.ResponseWriter, *http.Request)
354+
}{
355+
{"OAuth AS metadata", handler.OAuthDiscoveryHandler},
356+
{"OIDC discovery", handler.OIDCDiscoveryHandler},
357+
} {
358+
t.Run(tc.name, func(t *testing.T) {
359+
t.Parallel()
360+
req := httptest.NewRequest(http.MethodGet, "/", nil)
361+
rec := httptest.NewRecorder()
362+
tc.fn(rec, req)
363+
require.Equal(t, http.StatusOK, rec.Code)
364+
365+
var meta sharedobauth.AuthorizationServerMetadata
366+
require.NoError(t, json.NewDecoder(rec.Body).Decode(&meta))
367+
assert.True(t, meta.ClientIDMetadataDocumentSupported,
368+
"client_id_metadata_document_supported must be true when CIMD is enabled")
369+
})
370+
}
371+
}
372+
373+
func TestDiscoveryHandlers_CIMDDisabled_OmitsFlag(t *testing.T) {
374+
t.Parallel()
375+
376+
handler := testSetupWithOptions(t, testSetupOptions{CIMDEnabled: false})
377+
378+
for _, tc := range []struct {
379+
name string
380+
fn func(http.ResponseWriter, *http.Request)
381+
}{
382+
{"OAuth AS metadata", handler.OAuthDiscoveryHandler},
383+
{"OIDC discovery", handler.OIDCDiscoveryHandler},
384+
} {
385+
t.Run(tc.name, func(t *testing.T) {
386+
t.Parallel()
387+
req := httptest.NewRequest(http.MethodGet, "/", nil)
388+
rec := httptest.NewRecorder()
389+
tc.fn(rec, req)
390+
require.Equal(t, http.StatusOK, rec.Code)
391+
392+
var meta sharedobauth.AuthorizationServerMetadata
393+
require.NoError(t, json.NewDecoder(rec.Body).Decode(&meta))
394+
assert.False(t, meta.ClientIDMetadataDocumentSupported,
395+
"client_id_metadata_document_supported must be absent when CIMD is disabled")
396+
})
397+
}
398+
}

0 commit comments

Comments
 (0)