Type user.security.* as @ConfigurationProperties + secret-free ${userSecurity} template attribute - #356
Merged
Merged
Conversation
Design for converting the user.security.* namespace from ~40 scattered @value injections into a cohesive family of typed @ConfigurationProperties (UserSecurityConfigProperties, PasswordPolicyConfigProperties, RememberMeConfigProperties), with full internal migration, generated config metadata, and a secret-free template view object exposed as ${userSecurity}. Additive: zero config-key changes for consumers. Motivated by DemoApp#82 (Boot 4.1.0 / Thymeleaf 3.1.5 restricted expressions).
…SecurityConfigProperties
Update tests that construct PasswordPolicyService/UserService directly (constructor args, @mock fields) for the new constructor parameter.
…consumers Update tests that construct LoginAttemptService/TokenHasher/UserEmailService directly (constructor args, @mock fields) and UserApiTest's lockout-threshold read for the new constructor parameter.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the library’s user.security.* configuration from scattered @Value injections into typed @ConfigurationProperties beans, migrates internal consumers to those beans, replaces hand-maintained security metadata with generated metadata + coverage tests, and introduces a secret-free ${userSecurity} model attribute for template access.
Changes:
- Added typed config properties classes for
user.security.*,user.security.password.*, anduser.security.remember-me.*, and migrated internal wiring to constructor-injected beans. - Replaced hand-maintained
user.security.*metadata with generated metadata and added tests to guard legacy-key coverage + default parity. - Added
UserSecurityUriControllerAdvice+UserSecurityUriViewto expose secret-free URI data to MVC templates via${userSecurity.*}(opt-out supported).
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityConfigProperties.java | New typed properties for flat user.security.* keys + URI list filtering. |
| src/main/java/com/digitalsanctuary/spring/user/security/PasswordPolicyConfigProperties.java | New typed properties for password policy keys. |
| src/main/java/com/digitalsanctuary/spring/user/security/RememberMeConfigProperties.java | New typed properties for remember-me keys. |
| src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java | Enables the new properties beans; migrates encoder/appUrlResolver wiring to bean-backed values. |
| src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java | Migrates security filter-chain config from @Value fields to injected properties beans. |
| src/main/java/com/digitalsanctuary/spring/user/security/HtmxAwareAuthenticationEntryPointConfiguration.java | Migrates login page URI resolution to injected properties bean. |
| src/main/java/com/digitalsanctuary/spring/user/service/PasswordPolicyService.java | Migrates password policy reads to PasswordPolicyConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/service/UserService.java | Migrates password history retention to PasswordPolicyConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/service/LoginAttemptService.java | Migrates lockout thresholds/durations to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java | Migrates login redirect + always-use-default behavior to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/service/LogoutSuccessService.java | Migrates logout redirect to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java | Migrates reset token validity to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/service/TokenHasher.java | Migrates token secret source to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/util/PasswordHashTimeTester.java | Migrates startup hash-timing flag to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java | Migrates MVC redirects to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java | Migrates API redirect URIs + opt-in flag to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/web/WebInterceptorConfig.java | Migrates interceptor path patterns to UserSecurityConfigProperties. |
| src/main/java/com/digitalsanctuary/spring/user/web/UserSecurityUriView.java | Adds secret-free, template-facing URI view record. |
| src/main/java/com/digitalsanctuary/spring/user/web/UserSecurityUriControllerAdvice.java | Adds ${userSecurity} model attribute (opt-out via property). |
| src/main/resources/META-INF/additional-spring-configuration-metadata.json | Removes hand-maintained user.security.* entries; adds metadata for expose-uris-to-model. |
| src/test/resources/metadata/legacy-user-security-keys.json | Snapshot of retired metadata keys for coverage assertions. |
| src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityMetadataCoverageTest.java | Asserts generated metadata still covers all retired keys. |
| src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityDefaultsParityTest.java | Guards initializer defaults parity vs shipped properties file. |
| src/test/java/com/digitalsanctuary/spring/user/security/UriPlaceholderParityTest.java | Guards mapping-placeholder keys vs bean getter parity. |
| src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityConfigPropertiesTest.java | Binding/defaults tests for UserSecurityConfigProperties. |
| src/test/java/com/digitalsanctuary/spring/user/security/PasswordPolicyConfigPropertiesTest.java | Binding/defaults tests for PasswordPolicyConfigProperties. |
| src/test/java/com/digitalsanctuary/spring/user/security/RememberMeConfigPropertiesTest.java | Binding/defaults tests for RememberMeConfigProperties. |
| src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java | Updates auto-config tests for new constructor wiring. |
| src/test/java/com/digitalsanctuary/spring/user/security/HtmxAwareAuthenticationEntryPointConfigurationTest.java | Updates entry-point config test to provide properties bean. |
| src/test/java/com/digitalsanctuary/spring/user/security/CoreBeanOverrideTest.java | Updates reflection assertions for changed bean method signature. |
| src/test/java/com/digitalsanctuary/spring/user/web/UserSecurityUriControllerAdviceTest.java | Tests ${userSecurity} attribute + verifies secrets aren’t exposed. |
| src/test/java/com/digitalsanctuary/spring/user/service/PasswordPolicyServiceTest.java | Updates unit tests to mutate PasswordPolicyConfigProperties instead of reflection. |
| src/test/java/com/digitalsanctuary/spring/user/service/LoginAttemptServiceTest.java | Updates unit tests to use UserSecurityConfigProperties wiring. |
| src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessServiceTest.java | Updates unit tests to stub UserSecurityConfigProperties. |
| src/test/java/com/digitalsanctuary/spring/user/service/LogoutSuccessServiceTest.java | Updates unit tests to stub UserSecurityConfigProperties. |
| src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java | Updates unit tests to pass real properties bean and assert lifetimes. |
| src/test/java/com/digitalsanctuary/spring/user/service/UserVerificationServiceTest.java | Updates unit tests to construct TokenHasher with properties bean. |
| src/test/java/com/digitalsanctuary/spring/user/service/TokenHasherTest.java | Updates unit tests to construct TokenHasher via properties bean. |
| src/test/java/com/digitalsanctuary/spring/user/service/TokenHashingSecurityTest.java | Updates security tests for token hashing + lifetime via properties. |
| src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java | Updates service tests to include PasswordPolicyConfigProperties dependency. |
| src/test/java/com/digitalsanctuary/spring/user/service/UserServiceRegistrationGuardTest.java | Updates service tests to include PasswordPolicyConfigProperties dependency. |
| src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java | Updates controller tests to pass real properties bean. |
| src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java | Updates API unit tests to pass real properties bean and mutate flags directly. |
| src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java | Updates API guard tests to pass real properties bean. |
| src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java | Updates integration test wiring to read lockout threshold from properties bean. |
| CONFIG.md | Documents canonical camelCase keys + ${userSecurity} template attribute. |
| MIGRATION.md | Adds migration note (no config key change; template access guidance). |
| CHANGELOG.md | Adds unreleased notes for the refactor + ${userSecurity} feature. |
| docs/design/2026-08-13-user-security-config-properties-design.md | Adds design doc for the refactor. |
| docs/plans/2026-08-13-user-security-config-properties-plan.md | Adds implementation plan (long-form). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+56
to
+60
| * @param userSecurityConfig the user security configuration properties, whose {@code tokenHashSecret} | ||
| * may be {@code null} or blank, in which case plain SHA-256 is used | ||
| */ | ||
| public TokenHasher(@Value("${user.security.tokenHashSecret:#{null}}") final String tokenHashSecret) { | ||
| this.tokenHashSecret = tokenHashSecret; | ||
| public TokenHasher(final UserSecurityConfigProperties userSecurityConfig) { | ||
| this.tokenHashSecret = userSecurityConfig.getTokenHashSecret(); |
Owner
Author
There was a problem hiding this comment.
Declining the deprecated TokenHasher(String) overload. TokenHasher is a Spring-wired @Component; the only direct construction is in this repo's own tests, and this project's conventions avoid carrying deprecated compatibility shims for framework-internal wiring. The precedent is 5.2.0, which shipped bean-constructor changes (WebSecurityConfig, SessionInvalidationService) in a minor with a MIGRATION.md note — this change is documented the same way (MIGRATION.md "Breaking for direct instantiation/subclassing only" plus the changelog's Breaking Changes section, d36ccbd).
Comment on lines
+7
to
+8
| ### Refactoring | ||
| - Internal refactor of `user.security.*` to typed `@ConfigurationProperties`: `UserSecurityConfigProperties` (page/action URIs, URI lists, security scalars), `PasswordPolicyConfigProperties`, and `RememberMeConfigProperties`. Config keys are **unchanged** — no consumer action required. `WebSecurityConfig`'s previously `@Data`-generated public URI getters (e.g. `getLoginPageURI()`) are removed; they had no callers outside the framework. |
Owner
Author
There was a problem hiding this comment.
Fixed in d36ccbd: the Unreleased changelog now has a dedicated Breaking Changes section (matching the 5.2.0 format) that spells out the removed WebSecurityConfig getters and the constructor signature changes, scoped to direct instantiation/subclassing. The PR description now says "config-compatible — no config key changed" instead of "purely additive", with the constructor changes called out. MIGRATION.md already documents the migration path.
Comment on lines
48
to
52
| private static final String DEFAULT_ACTION_DENY = "deny"; | ||
| private static final String DEFAULT_ACTION_ALLOW = "allow"; | ||
|
|
||
| @Value("${user.security.defaultAction}") | ||
| private String defaultAction; | ||
|
|
||
| @Value("${user.security.protectedURIs}") | ||
| private String protectedURIsProperty; | ||
|
|
||
| @Value("${user.security.unprotectedURIs}") | ||
| private String unprotectedURIsProperty; | ||
|
|
||
| @Value("${user.security.disableCSRFURIs}") | ||
| private String disableCSRFURIsProperty; | ||
|
|
||
| @Value("${user.security.loginPageURI}") | ||
| private String loginPageURI; | ||
|
|
||
| @Value("${user.security.loginActionURI}") | ||
| private String loginActionURI; | ||
|
|
||
| @Value("${user.security.loginSuccessURI}") | ||
| private String loginSuccessURI; | ||
|
|
||
| @Value("${user.security.logoutActionURI}") | ||
| private String logoutActionURI; | ||
|
|
||
| @Value("${user.security.logoutSuccessURI}") | ||
| private String logoutSuccessURI; | ||
|
|
||
| @Value("${user.security.registrationURI}") | ||
| private String registrationURI; | ||
|
|
||
| @Value("${user.security.registrationPendingURI}") | ||
| private String registrationPendingURI; | ||
|
|
||
| @Value("${user.security.registrationSuccessURI}") | ||
| private String registrationSuccessURI; | ||
|
|
||
| @Value("${user.security.forgotPasswordURI}") | ||
| private String forgotPasswordURI; | ||
|
|
||
| @Value("${user.security.forgotPasswordPendingURI}") | ||
| private String forgotPasswordPendingURI; | ||
|
|
||
| @Value("${user.security.forgotPasswordChangeURI}") | ||
| private String forgotPasswordChangeURI; | ||
|
|
||
| @Value("${user.security.registrationNewVerificationURI}") | ||
| private String registrationNewVerificationURI; | ||
|
|
||
| @Value("${spring.security.oauth2.enabled:false}") | ||
| private boolean oauth2Enabled; |
Owner
Author
There was a problem hiding this comment.
Declining the delegating getters, keeping the removal — now explicitly documented as a breaking change rather than implied additive. The removed methods were @Data byproducts of the deleted @Value fields that returned raw property strings (e.g. getUnprotectedURIsProperty()); a review pass over the codebase found no callers outside the framework and no plausible external use for them. The typed UserSecurityConfigProperties bean is the supported way to read these values. The removal and migration path are spelled out in MIGRATION.md and the changelog's new Breaking Changes section (d36ccbd).
Review findings on #356, code portion: - The persistent-token repository condition now uses the canonical kebab key (user.security.remember-me.use-persistent-tokens), which relaxed-matches every spelling; the previous camelCase name only exact-matched the literal camelCase key, so kebab config bound the bean but silently downgraded remember-me to hash-based tokens. WebSecurityConfig now also warns when usePersistentTokens is set without a repository bean, and when remember-me is enabled without a signing key (previously skipped silently). - New UriPlaceholderParityValidator fails startup, naming the keys, when a user.security URI diverges between the bound bean and the exact camelCase Environment key that @GetMapping placeholders resolve (the kebab-only split-brain scenario). Test coverage now spans all 13 placeholder keys and reflectively pins the annotation defaults to the field initializers. - @EnableConfigurationProperties for the three user.security classes moved from UserSecurityBeansAutoConfiguration to UserConfiguration, so excluding the beans auto-config no longer removes the properties beans that ~14 component-scanned consumers inject. - user.security.expose-uris-to-model is now a typed field (exposeUrisToModel) with generated metadata, and the advice's conditional registration (default on, opt-out in both spellings) is tested. - Bean Validation constraints on the properties classes (bcrypt strength 4-31, password-policy min<=max, similarity 0-100, non-empty specialChars when required): startup failures with the property named when a validator is on the classpath, inert otherwise. - getTrustedHosts() gets the same trim/blank-filter treatment as the other URI lists; all list getters now return immutable copies. appUrlResolver drops its duplicated normalization. - JavaDoc corrections that feed generated metadata: remember-me key (no ephemeral-key fallback exists — required when enabled), appUrl/trustedHosts/requireCanonicalAppUrl (email-link Host-header defense, not "redirect validation"), lockout sentinels, step-up fallback, tokenHashSecret scope and SHA-256 fallback. - TokenHasher logs its no-secret fallback at INFO instead of DEBUG. - Defaults-parity test now compares whole bound objects against fresh instances for all three classes; advice test asserts all 18 view components against per-field sentinels and checks secret values, not just component names.
- CONFIG.md: kebab-only URI spellings now fail startup instead of silently splitting config from controllers; note startup-time Bean Validation of ranges. - MIGRATION.md: heading no longer claims "no action required"; lists the changed constructors (TokenHasher, LoginSuccessService, and the Lombok-generated ones), the new startup checks, and the bcryptStrength code-fallback alignment (10 -> 12, effective behavior unchanged). - CHANGELOG.md: entries for the persistent-token spelling fix, the parity/validation startup checks, and the new warnings.
Copilot review on #356 flagged that the removed WebSecurityConfig getters and changed constructors were documented but framed under Refactoring while the PR claimed to be purely additive. Restructure the Unreleased changelog to match the 5.2.0 format: a dedicated Breaking Changes section scoped to direct instantiation/subclassing, with the getter removal and constructor list spelled out.
|
Review: PR #356 — user.security. to typed @ConfigurationProperties* Reviewed the full diff (~5,100 lines across 50 files), prioritizing WebSecurityConfig.java, UserSecurityConfigProperties.java, UriPlaceholderParityValidator.java, PasswordPolicyConfigProperties.java/PasswordPolicyService.java, RememberMeConfigProperties.java, UserSecurityUriControllerAdvice.java/UserSecurityUriView.java, UserSecurityBeansAutoConfiguration.java, and the associated tests, cross-checked against config/dsspringuserconfig.properties, MIGRATION.md, and CHANGELOG.md. Overall: this is a strong, well-tested PR. No blockers or major issues found. Correctness
Security
Breaking changes
Test coverage
Minor / nit
Nice work on this one — the parity/coverage test suite in particular (defaults parity, metadata coverage, URI placeholder parity) is exactly the kind of guardrail that makes a mechanical-but-risky refactor like this trustworthy. Generated with Claude Code |
…AccountLockoutDuration The @Value-to-@ConfigurationProperties migration dropped LoginAttemptService's public @DaTa getters (getMaxFailedLoginAttempts/getAccountLockoutDuration). A local integration test against the demo app surfaced that consumers read these directly. Restore them as public accessors delegating to UserSecurityConfigProperties, preserving backward compatibility.
ReviewReviewed the diff and the merged working tree (design/plan docs, all Overall impressionThis is a genuinely careful piece of engineering. The migration from ~40 scattered Strengths worth calling out
Minor observations (non-blocking)
Test coverageCoverage is thorough: per-class binding tests (default + relaxed/legacy-spelling binding), a defaults-parity test comparing shipped-file values against field initializers in both directions, a metadata-coverage test asserting every retired hand-maintained key still resolves in generated metadata, and full request-mapping-reflection-based parity tests. I didn't find gaps worth flagging — the mid-PR commits already closed the ones a first pass would typically catch (persistent-token condition spelling, missing back-compat getters). SecurityNo concerns. The Nice work — this is a good example of a large mechanical refactor done with enough test coverage and self-review that I'd be comfortable merging it. |
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.
Closes #355.
Converts the
user.security.*configuration namespace from ~40 scattered@Valueinjections into a cohesive family of typed@ConfigurationProperties, migrates all internal consumers, replaces hand-maintained config metadata with generated metadata, and adds a secret-free${userSecurity}template model attribute. Config-compatible — no config key changed for consumers. Constructor signatures of the migrated internal components did change (breaking only for direct instantiation or subclassing; documented in MIGRATION.md and the CHANGELOG's Breaking Changes section).Design doc:
docs/design/2026-08-13-user-security-config-properties-design.md· Plan:docs/plans/2026-08-13-user-security-config-properties-plan.mdWhat changed
com.digitalsanctuary.spring.user.security):UserSecurityConfigProperties(flat URIs/lists/scalars),PasswordPolicyConfigProperties(user.security.password.*),RememberMeConfigProperties(user.security.remember-me.*), registered via@EnableConfigurationPropertiesonUserSecurityBeansAutoConfiguration. This makesuser.security.*consistent with every other config area (MFA, WebAuthn, Captcha, GDPR, …).user.security.*@Valueonto the beans — services,WebSecurityConfig, the auto-config, controllers, and the web interceptor.@GetMapping/@ConditionalOnPropertyannotation placeholders are intentionally left as Environment placeholders.WebSecurityConfig's old@DataURI getters were removed (no external callers);splitAndFilterPropertyfolded into the list getters.user.security.*entries inadditional-spring-configuration-metadata.jsonare deleted and now generated from the typed fields + JavaDoc. A coverage test guards that every retired key is still described.${userSecurity}template attribute:UserSecurityUriControllerAdviceexposes a narrow, immutableUserSecurityUriView(the page/action URIs +copyrightFirstYear) — never the config bean, sotokenHashSecretcan't leak. Opt out withuser.security.expose-uris-to-model=false. This lets consuming apps replace${@environment.getProperty('user.security.*')}(which Thymeleaf 3.1.5 forbids in layout-decorated templates under Spring Boot 4.1.0) with${userSecurity.*}.Backward compatibility
config/dsspringuserconfig.propertiesis byte-identical tomain(it stays Environment-visible for@GetMappingplaceholder resolution). Field initializers mirror the file's effective values (incl.bcryptStrength=12,password.history-count=3,appUrl=""), guarded by a defaults-parity test.tokenHashSecretand remember-mekeycarry@ToString.Exclude.Testing
./gradlew clean build— BUILD SUCCESSFUL, 1152 tests, 0 failures. Added: per-class binding tests, a defaults-parity regression test (scalars + URI lists), a metadata-coverage test, the controller-advice test (asserts the secret is unreachable), and a placeholder/bean parity guard. Filter-chain behavior verified equivalent tomain. Migrated test wiring uses real config instances or explicitly-stubbed values (no Mockito zero/false/null standing in for real config).Notes / follow-ups
${userSecurity.*}, closing Adopt Spring Boot 4.1.0: migrate ${@environment.getProperty(...)} template pattern for Thymeleaf 3.1.5 restricted expressions SpringUserFrameworkDemoApp#82.UriPlaceholderParityValidatorfails startup (naming the keys) when auser.securityURI diverges between the bound bean and the camelCase placeholder key, and the parity test covers all 13 placeholder keys reflectively. A multi-agent review pass also fixed theusePersistentTokensconditional (camelCase-only exact match → relaxed), moved@EnableConfigurationPropertiestoUserConfiguration, typedexposeUrisToModel, added Bean Validation on the properties classes, and corrected the metadata-feeding JavaDoc.