Skip to content

Commit bf6f89a

Browse files
committed
Move DCR restart test back into runner package
Drops the package authserver_test workaround introduced to break the runner -> authserver import cycle that blocked extending pkg/authserver/integration_test.go. The test's actual subject is the runner-side DCR-store wiring (it goes through runner.NewEmbeddedAuthServer and asserts on runner.EmbeddedAuthServer.DCRStore()), so the runner package is the natural home and matches the location of its sibling TestNewEmbeddedAuthServer_DCRBoot / _ClosesStorageOnError tests. The runner-package newMockAuthorizationServer helper replaces the duplicate newMockUpstreamAS the cross-package placement forced; the "DO NOT COPY THIS A THIRD TIME" tripwire on it is dropped now that the duplication is gone.
1 parent efc4428 commit bf6f89a

2 files changed

Lines changed: 99 additions & 211 deletions

File tree

pkg/authserver/integration_dcr_restart_test.go

Lines changed: 0 additions & 204 deletions
This file was deleted.

pkg/authserver/runner/embeddedauthserver_test.go

Lines changed: 99 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1478,13 +1478,6 @@ func TestRegisterHandlers(t *testing.T) {
14781478
// cache hits issue zero additional network I/O. The issuer advertised in
14791479
// metadata is the server's own URL (loopback), which satisfies the HTTPS
14801480
// redirect-URI policy in resolveUpstreamRedirectURI.
1481-
//
1482-
// DO NOT COPY THIS A THIRD TIME. There is one near-identical copy in
1483-
// pkg/authserver/integration_dcr_restart_test.go (newMockUpstreamAS) that
1484-
// the import-cycle into authserver_test forced. The next caller must
1485-
// extract this helper to a shared internal test-helpers package (e.g.
1486-
// pkg/authserver/internal/testhelpers) and rewrite both copies to call
1487-
// into it; two copies are tolerable, three is a bug factory.
14881481
func newMockAuthorizationServer(t *testing.T) (*httptest.Server, *int32) {
14891482
t.Helper()
14901483

@@ -1830,6 +1823,105 @@ func TestNewEmbeddedAuthServer_ClosesStorageOnError(t *testing.T) {
18301823
"a count of 0 indicates the deferred Close did not run, leaking the backend on the error path")
18311824
}
18321825

1826+
// TestEmbeddedAuthServer_DCRStorePersistsAcrossClose verifies that the DCR
1827+
// store reachable through EmbeddedAuthServer.DCRStore() holds the resolved
1828+
// RFC 7591 client registration after the constructor's full DCR resolver
1829+
// runs against a mock AS. The Get is issued BEFORE Close so the assertion
1830+
// does not depend on the (undocumented) MemoryStorage post-Close
1831+
// readability that an earlier version of this test silently relied on.
1832+
//
1833+
// What this test does cover:
1834+
//
1835+
// - NewEmbeddedAuthServer runs the full DCR resolver against a mock AS
1836+
// during construction, populating the storage-backed DCR store, and
1837+
// surfaces the same storage.DCRCredentialStore the authserver itself
1838+
// reads from via DCRStore(). The persisted credentials are readable
1839+
// by issuing a Get against the captured store while the server is
1840+
// still live.
1841+
//
1842+
// What this test does NOT cover (deferred follow-up):
1843+
//
1844+
// - The full "boot, close, boot again on the same backend, observe zero
1845+
// /register calls on the second boot" cross-restart scenario. Closing
1846+
// that gap requires either miniredis-Sentinel emulation or a
1847+
// Docker-based Redis Sentinel cluster in the test harness, since the
1848+
// production restart path lives on Redis (Memory cannot be shared
1849+
// across two NewEmbeddedAuthServer constructors). Tracked as a
1850+
// follow-up; this test deliberately scopes itself to what is
1851+
// exercisable today against the production constructor seam.
1852+
func TestEmbeddedAuthServer_DCRStorePersistsAcrossClose(t *testing.T) {
1853+
t.Parallel()
1854+
1855+
server, requestCount := newMockAuthorizationServer(t)
1856+
1857+
cfg := &authserver.RunConfig{
1858+
SchemaVersion: authserver.CurrentSchemaVersion,
1859+
Issuer: server.URL,
1860+
Upstreams: []authserver.UpstreamRunConfig{
1861+
{
1862+
Name: "dcr-upstream",
1863+
Type: authserver.UpstreamProviderTypeOAuth2,
1864+
OAuth2Config: &authserver.OAuth2UpstreamRunConfig{
1865+
ClientID: "",
1866+
AuthorizationEndpoint: server.URL + "/authorize",
1867+
TokenEndpoint: server.URL + "/token",
1868+
Scopes: []string{"openid", "profile"},
1869+
DCRConfig: &authserver.DCRUpstreamConfig{
1870+
DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server",
1871+
},
1872+
},
1873+
},
1874+
},
1875+
AllowedAudiences: []string{"https://mcp.example.com"},
1876+
}
1877+
1878+
embed, err := NewEmbeddedAuthServer(context.Background(), cfg)
1879+
require.NoError(t, err)
1880+
require.NotNil(t, embed)
1881+
t.Cleanup(func() { _ = embed.Close() })
1882+
1883+
firstBootRequests := atomic.LoadInt32(requestCount)
1884+
require.Greater(t, firstBootRequests, int32(0),
1885+
"first boot must have issued network I/O to the mock AS during DCR")
1886+
1887+
// Capture the storage instance the constructor wired into the DCR
1888+
// store. This is the same backend the authserver itself was using; in
1889+
// production it is shared across authserver state, so DCR survives
1890+
// restart on the same backend.
1891+
persistentStore := embed.DCRStore()
1892+
require.NotNil(t, persistentStore,
1893+
"NewEmbeddedAuthServer must surface a storage-level DCRCredentialStore")
1894+
1895+
// Verify the persisted DCR row by issuing a Get against the captured
1896+
// store BEFORE closing the server. Doing the Get pre-Close avoids
1897+
// silently depending on whichever storage backend the test happens to
1898+
// use staying readable after Close (a contract MemoryStorage honors
1899+
// today but RedisStorage's closed connection pool does not). The
1900+
// assertion proves the persistence boundary the production cross-
1901+
// replica and cross-restart reuse paths depend on: that the
1902+
// resolution lives in storage, not in process-local cache state.
1903+
redirectURI := server.URL + "/oauth/callback"
1904+
key := DCRKey{
1905+
Issuer: server.URL,
1906+
RedirectURI: redirectURI,
1907+
ScopesHash: storage.ScopesHash([]string{"openid", "profile"}),
1908+
}
1909+
creds, err := persistentStore.GetDCRCredentials(context.Background(), key)
1910+
require.NoError(t, err,
1911+
"DCR credentials must be readable from the captured store — "+
1912+
"this is the persistence boundary cross-replica reuse depends on")
1913+
require.NotNil(t, creds)
1914+
assert.Equal(t, "dcr-client-id", creds.ClientID,
1915+
"persisted ClientID must match the first boot's DCR resolution")
1916+
assert.Equal(t, "dcr-client-secret", creds.ClientSecret,
1917+
"persisted ClientSecret must match the first boot's DCR resolution")
1918+
1919+
// Mock-AS request count is unchanged after the survival check — the
1920+
// Get is a pure store read with no upstream traffic.
1921+
assert.Equal(t, firstBootRequests, atomic.LoadInt32(requestCount),
1922+
"GetDCRCredentials must not issue any HTTP requests to the mock AS")
1923+
}
1924+
18331925
// urlErrorOnCloseStorage wraps an authserver storage and returns a fixed
18341926
// URL-bearing error from Close. It exists so
18351927
// TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog can verify that the

0 commit comments

Comments
 (0)