polish(security): SUF-02/05 follow-ups from #333/#334 review

[devondragon]

Post-merge polish addressing valid review feedback from the merged SUF security PRs (#333, #334). All items verified against the code before implementing; none reopen the closed issues or are security-critical (the fail-closed core is unchanged). Nothing here is a released-API break — SUF-02/05 shipped in #334 but are still unreleased (5.0.2-SNAPSHOT).

Items (from the #333/#334 review triage)

1. StepUpService SPI Javadoc was misleading (real defect). It told integrators they could read the step-up proof from the request "body," but POST /user/setPassword binds @Valid @RequestBody SetPasswordDto, which drains the input stream before isStepUpSatisfied() runs — a consumer reading getInputStream()/getReader() gets an empty stream. Javadoc now says: read the proof from a header, request parameter, or the session (or install a content-caching filter).
2. Startup WARN when setPassword is disabled-by-default. When no StepUpService bean is present and allowInitialPasswordSetWithoutStepUp=false, the endpoint silently returns 403. A @PostConstruct now logs a startup warning (mirroring the SUF-01 fail-fast pattern) so operators learn about it at boot, not from a production 403.
3. Distinct response code for the two failure branches. Disabled-by-default was reusing code 6 (also the step-up-denied code). Disabled is now code 7 (HTTP 403); step-up-denied stays code 6 (HTTP 401), so clients can disambiguate.
4. Config metadata. Registered user.security.requireCanonicalAppUrl (SUF-01) and user.security.allowInitialPasswordSetWithoutStepUp (SUF-02) in additional-spring-configuration-metadata.json for IDE auto-completion.
5. Closed the real-context test gap. New full-context tests boot the real ObjectProvider<StepUpService> (empty → passwordless setPassword returns 403/code 7) and verify the WebInterceptorConfig @Value-wired reset-page headers survive the Spring Security filter chain — previously only exercised via standalone/mocked MockMvc.

Deliberately excluded (per the triage + prior decisions): extending SUF-05 headers to registrationConfirm (same "reset token already in the email URL" reasoning that killed the SUF-05 follow-up; verification token has an even smaller window), and cosmetic nits (hardcoded action literal, direct interceptor instantiation, raw assert, import order).

Testing

• New unit tests: startup-warning matrix (fires only when disabled-by-default) and the distinct disabled-vs-denied codes.
• New full-context tests: passwordless setPassword → 403 through the real provider; reset-page headers through the real interceptor wiring.
• ./gradlew check green (all tests + ArchUnit); the new @PostConstruct does not disturb any full-context boot.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Polish follow-up to the SUF-02/SUF-05 security work from #333/#334, focused on clarifying integrator guidance, improving operational visibility, and tightening client-facing response semantics without changing the underlying fail-closed behavior.

Changes:

• Clarifies StepUpService SPI guidance to avoid relying on re-reading the request body after @RequestBody binding.
• Adds a startup WARN when /user/setPassword is disabled-by-default, and introduces distinct JSON response codes for “disabled” vs “step-up denied”.
• Adds configuration metadata entries and full-context tests covering real ObjectProvider<StepUpService> wiring and reset-page security headers through the Spring Security filter chain.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java	Updates SUF-02 unit assertions for distinct codes and adds unit coverage for the new startup warning.
src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java	Adds full-context tests for disabled-by-default /setPassword behavior and reset-page headers surviving the security chain.
src/main/resources/META-INF/additional-spring-configuration-metadata.json	Registers SUF-01/SUF-02 properties for IDE metadata/autocomplete.
src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java	Updates SPI Javadoc to reflect request-body consumption realities for @RequestBody endpoints.
src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java	Adds startup WARN for disabled-by-default /setPassword and changes disabled branch response code to 7.
CHANGELOG.md	Documents the new distinct response codes and startup warning behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Review Summary

This is a tightly-scoped polish PR addressing review feedback from the merged SUF-02/05 security work (#333/#334). I verified the diff against the current code (checked out at the PR's merge ref) and traced through the relevant call sites — everything is consistent and correct.

What it does

1. Fixes misleading StepUpService Javadoc (the request body is already drained by @RequestBody binding before isStepUpSatisfied() runs — good catch, and the fix correctly points implementers at headers/params/session).
2. Adds a @PostConstruct startup warning in UserAPI when setPassword is left disabled-by-default (no StepUpService bean + allowInitialPasswordSetWithoutStepUp=false).
3. Splits the previously-shared code 6 into distinct code 6 (step-up denied, 401) vs code 7 (disabled-by-default, 403).
4. Registers the two SUF-01/02 properties in additional-spring-configuration-metadata.json for IDE autocomplete.
5. Adds full-context tests exercising the real ObjectProvider<StepUpService> and the real WebInterceptorConfig-wired security headers.

Code quality

• The @PostConstruct warning mirrors the existing SUF-01 pattern in UserSecurityBeansAutoConfiguration.appUrlResolver() almost exactly (same tone, same "warn vs. fail-fast" philosophy) — good consistency with established conventions in this codebase.
• warnIfInitialPasswordSetDisabled() is package-private, which is fine for a @PostConstruct callback and lets the unit test invoke it directly from the same package.
• The new full-context test (SetInitialPassword) correctly drives the endpoint through the real, empty ObjectProvider<StepUpService> rather than a mocked one, which is a genuine gap-closer — the unit tests only ever exercised a stubbed provider.
• The reset-page-headers test is a good addition: it confirms the @Value-resolved URIs in WebInterceptorConfig actually match the real changePasswordURI default and that the interceptor's headers survive the Security filter chain, not just a standalone MockMvc setup.

Minor / non-blocking observations

• Magic numbers for response codes: The file already has a named constant ERROR_CODE_REGISTRATION_DENIED = 6 for the registration-guard code, but the two setPassword codes (6 and 7) are still inline literals. Since this PR is specifically about making the two codes distinguishable, pulling them into named constants (e.g. ERROR_CODE_SET_PASSWORD_STEP_UP_DENIED, ERROR_CODE_SET_PASSWORD_DISABLED) would make the "distinct code" intent self-documenting at the call site and harder to accidentally collide with in the future. Not blocking — just a readability nit consistent with the pattern already in the file.
• Startup log noise in tests: Any test context that boots UserAPI without a StepUpService bean and without allowInitialPasswordSetWithoutStepUp=true will now emit a WARN on every run (e.g. most @IntegrationTest/@SecurityTest contexts). This is intentional and mirrors the accepted SUF-01 precedent, but worth knowing if CI log volume becomes a nuisance later.
• Response code values are only unique per-endpoint (e.g. setPassword's code 6/7 reuses the numeric space also used by other endpoints like the registration-guard's code 6). Pre-existing pattern, not introduced by this PR, but worth a mental note that "code" is not a global enum — flagging only because this PR is specifically working on code disambiguation.

Security

No security concerns — the fail-closed default behavior is unchanged (as stated in the PR description), and this is purely observability/docs/test polish on top of already-merged, already-unreleased (5.0.2-SNAPSHOT) changes.

Test coverage

Good. The new tests close a real gap (mocked-provider-only coverage → real-provider coverage) and directly assert the new code 7/code 6 values plus the startup-warning matrix (fires only when actually disabled-by-default, not when a StepUpService is present or the opt-in flag is set).

Verdict

Solid, low-risk, well-tested documentation/observability polish. No correctness or security issues found. The magic-number nit is the only suggestion I'd consider before merge, and even that's optional given the existing codebase convention isn't fully consistent either.

[claude]

Review: security/suf-polish (#336)

Reviewed the diff end-to-end (UserAPI.java, StepUpService.java, config metadata, and both test files) against CLAUDE.md conventions. This is a tight, well-scoped "polish" PR: small, each change traces to a specific triage item, and the new tests exercise real wiring rather than re-mocking what was already covered. No blocking issues found.

Code quality / correctness

• warnIfInitialPasswordSetDisabled() reads stepUpServiceProvider / allowInitialPasswordSetWithoutStepUp in @PostConstruct. Both are set before @PostConstruct runs (constructor injection for the ObjectProvider, field injection for @value -- both happen in the populate-bean phase, ahead of the initialize-bean phase), so there is no init-order hazard. Good mirror of the SUF-01 fail-fast/warn pattern.
• The new response code 7 for the disabled-by-default branch is distinct from code 6 (step-up-denied) within this endpoint. Worth flagging for future readers: code 6 is also reused as ERROR_CODE_REGISTRATION_DENIED elsewhere in UserAPI. Response codes are locally scoped per-endpoint rather than globally unique -- pre-existing, and not something this PR needs to fix, but a latent footgun if a client ever tries to interpret "code" without also keying on the endpoint/status.
• The StepUpService Javadoc fix is a legitimate, previously-misleading contract clarification, useful independent of the rest of the PR: a consumer implementation reading the body via getInputStream()/getReader() would silently get nothing back, since the body is already drained by the @RequestBody binding before isStepUpSatisfied runs.

Test coverage

• UserAPIUnitTest: the startup-warning matrix (fires only when disabled-by-default; silent when a StepUpService bean exists or the opt-in flag is true) attaches a ListAppender to the real UserAPI logger -- a reasonable way to assert log output rather than mocking Logger, and the appender is correctly attached/detached per test (@BeforeEach/@AfterEach) to avoid cross-test log-capture leakage.
• UserApiTest: the two new full-context tests close a real gap -- booting the real ObjectProvider (empty by default) proves the fail-closed behavior is not just an artifact of a mocked provider, and asserting the reset-page headers through the actual Spring Security filter chain (rather than a hand-registered interceptor in a standalone MockMvc setup) verifies WebInterceptorConfigs @Value-resolved URIs are wired correctly end-to-end.
• Verified hasPassword/removeUserPassword used by the new integration test exist on UserService, and that the message.set-password.* keys referenced by the endpoint already exist in dsspringusermessages.properties -- no missing-message-key regression.

Config metadata

• additional-spring-configuration-metadata.json is valid, well-formed JSON with no duplicate keys, and both new entries (requireCanonicalAppUrl, allowInitialPasswordSetWithoutStepUp) match the actual @value defaults (false) in code.

Security

• No security-relevant behavior changes here -- this is documentation, observability (startup warning), and response-code disambiguation only. The fail-closed default (403 when the endpoint is disabled) is unchanged, consistent with the PR description.
• No new attack surface introduced; the startup warning log message does not leak secrets or PII.

Minor / non-blocking

• The PR description explicitly lists what was deliberately excluded (extending SUF-05 headers to registrationConfirm, cosmetic nits) along with the reasoning -- good practice so reviewers know what was consciously scoped out rather than missed.
• Nothing else stood out; the diff is small enough that every changed line traces cleanly to its stated purpose in the PR description and commit messages.

Solid, well-tested follow-up PR. No blocking issues from this review (note: I do not have CI/build execution in this environment to independently confirm ./gradlew check, but static review of the diff found no compile-time or logical issues).