fix(authserver): replace custom JdbcRegisteredClientRepository with Spring stock#130
Merged
dfcoffin merged 1 commit intoMay 26, 2026
Merged
Conversation
…pring stock Closes #127. The hand-written JdbcRegisteredClientRepository (335 lines) reimplemented Spring Authorization Server's stock repository and carried multiple defects: the TokenSettings/ClientSettings serialization bug worked around in #128, auto-encoding of client secrets on save, and a findAll() that returned empty under the autocommit defect fixed in #125. This replaces it with Spring's stock org.springframework.security.oauth2.server.authorization.client.JdbcRegisteredClientRepository, which is maintained upstream and uses the correct Jackson modules out of the box. Changes: - AuthorizationServerConfig: construct the stock JdbcRegisteredClientRepository (JdbcOperations-only constructor); initializeDefaultClients() now takes the RegisteredClientRepository interface and no longer calls the custom findAll(). - OAuth2ClientManagementConfig: PasswordEncoder bean is now PasswordEncoderFactories.createDelegatingPasswordEncoder() instead of a bare BCryptPasswordEncoder(12). The stock repo stores client secrets verbatim (prefix included) rather than re-encoding on save, so authentication needs a prefix-aware encoder: {bcrypt}... for production, {noop}... for dev seeds. The old bare bcrypt encoder ignored the prefix and is why {noop}secret failed with invalid_client until this change. - Default seed clients use distinct secrets ({noop}dc-secret, {noop}tp-secret, {noop}tpadmin-secret). The stock repo enforces client-secret uniqueness (a security check the custom repo lacked); three identical {noop}secret values were rejected with "duplicate client secret". - New RegisteredClientAdminDao: small JdbcTemplate-backed component exposing the two operations not on the RegisteredClientRepository interface that the admin UI needs - findAllClientIds() and deleteById(). OAuthAdminController now resolves each id through RegisteredClientRepository.findByClientId(), so listing goes through Spring's tested deserialization path. - OAuthAdminController: dropped the `instanceof JdbcRegisteredClientRepository` branches (and their dead fallback paths) in favor of the admin DAO. - Deleted the custom JdbcRegisteredClientRepository and its 559-line test. Updated AuthorizationServerConfigTest (stock class + 1-arg bean signature), OAuthAdminControllerTest (admin DAO mocks; removed 2 tests for the now-gone instanceof fallback), and the MySQL/PostgreSQL TestContainers tests (findAll/deleteById -> RegisteredClientAdminDao). Verified end-to-end against a fresh MySQL container (dev-mysql): POST /oauth2/token (data_custodian_admin:dc-secret, client_credentials) -> 200, 128-char opaque token (0 dots, REFERENCE format) POST /oauth2/introspect -> 200, RFC 7662 response (active, sub, aud, scope, iss, exp, iat, jti, client_id, token_type) POST /oauth2/token with wrong secret -> 401 (delegating encoder enforces auth) Net: +112 / -1045 lines. Pre-existing test debt (NOT introduced here): AuthorizationServerConfigTest's mock-HttpSecurity unit tests and OAuthAdminControllerTest's standaloneSetup security tests fail because of how they're written, and the authserver module is excluded from CI entirely. Both are documented in #129; this change leaves them no worse (OAuthAdminControllerTest 5 -> 4 failures). Refs: #122 #128 #129 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #127. Replaces the hand-written 335-line
JdbcRegisteredClientRepositorywith Spring Authorization Server's stock implementation. Net +188 / −1045 lines.The custom repo was the source of three defects surfaced during Phase 2.0 bring-up:
TokenSettings/ClientSettingsJackson serialization bug worked around in #128 (that workaround is removed here along with the custom class — stock uses the right Jackson modules upstream){bcrypt}secretconfusion)findAll()returning empty under the autocommit defect fixed in #125Changes
AuthorizationServerConfigJdbcRegisteredClientRepository(JdbcOperations);initializeDefaultClients()takes theRegisteredClientRepositoryinterfaceOAuth2ClientManagementConfigPasswordEncoder→PasswordEncoderFactories.createDelegatingPasswordEncoder()(was bareBCryptPasswordEncoder(12))AuthorizationServerConfig(seeds){noop}dc-secret,{noop}tp-secret,{noop}tpadmin-secretRegisteredClientAdminDao(NEW)JdbcTemplateDAO for the two non-interface ops the admin UI needs:findAllClientIds(),deleteById()OAuthAdminControllerinstanceof JdbcRegisteredClientRepositorybranchesJdbcRegisteredClientRepository+ testTwo corrections the swap forced (both improvements)
Delegating password encoder. The stock repo stores secrets verbatim (with their
{id}prefix) instead of re-encoding on save, so authentication needs a prefix-aware encoder. The previous bareBCryptPasswordEncoder(12)ignored the prefix — which is why{noop}secretfailed withinvalid_clientuntil this change. The delegating encoder honors{bcrypt}(prod) and{noop}(dev seeds) and bcrypts unprefixed values. This is the Spring Security-recommended default.Distinct client secrets. Spring's stock repo enforces client-secret uniqueness (a security check the custom repo lacked) — three identical
{noop}secretvalues were rejected with "duplicate client secret." Each seed client now has a distinct secret. The stock repo flagging this is a feature, not a regression.Verification (local, fresh MySQL container, dev-mysql)
Also:
Default ESPI Clients seeded: 3 of 3on boot — all three persist correctly through the stock repo.Test plan
mvn -pl openespi-authserver compile/test-compilesucceedPre-existing test debt (NOT introduced by this PR)
AuthorizationServerConfigTest(mock-HttpSecurityunit tests) andOAuthAdminControllerTest's 4standaloneSetupsecurity tests fail because of how they're written, and the authserver module is excluded from CI entirely (.github/workflows/ci.ymlcomments out the authserver test step and omits it from the integration/Sonar-pllists). Both problems — broken tests and their CI exclusion — are documented in #129. This PR leaves them no worse:OAuthAdminControllerTestwent 5 → 4 failures (removed one pre-existing failure; dropped 2 obsoleteinstanceof-fallback tests).Related
🤖 Generated with Claude Code