Fix OIDC service inconsistencies with OAuth2 service

[devondragon]

Summary

Aligns DSOidcUserService behavior with DSOAuth2UserService by fixing four inconsistencies:

• Email normalization: Add .toLowerCase() to email lookup before findByEmail(), preventing case-sensitive mismatches
• Audit event: Publish "OIDC Registration Success" audit event on new user registration, matching OAuth2's "OAuth2 Registration Success" event
• @Transactional: Add @Transactional at class and method level for database operations
• loginHelperService call: Route through loginHelperService.userLoginHelper() in loadUser() so lastActivityDate is updated and lockout checks run

Also adds @Setter to DSUserDetails.oidcUserInfo and oidcIdToken to support setting OIDC tokens after loginHelperService creates the instance.

Closes #276

Test plan

• All 614 existing tests pass (0 failures)
• Updated DSOidcUserServiceTest and DSOidcUserServiceRegistrationGuardTest to inject new mock dependencies
• loadUser integration test verifies loginHelperService.userLoginHelper() is called

[Copilot]

Pull request overview

This PR aligns the OIDC authentication flow (DSOidcUserService) with the existing OAuth2 flow (DSOAuth2UserService) to ensure consistent user lookup, auditing, transactional behavior, and post-login side effects (e.g., activity updates / lockout checks).

Changes:

• Normalize OIDC email lookup to lowercase and add an OIDC registration success audit event publication.
• Route OIDC loadUser() through LoginHelperService.userLoginHelper() and add transactional boundaries to match OAuth2 behavior.
• Add setters on DSUserDetails for OIDC token/userInfo so they can be attached after LoginHelperService constructs the principal, and update tests to inject the new dependencies.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

File	Description
src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java	Aligns OIDC behavior with OAuth2 (email normalization, audit event publish, transactional annotation, and login helper usage).
src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetails.java	Allows OIDC token/userInfo to be set post-construction to support the login helper flow.
src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java	Updates tests to mock new collaborators and verifies the LoginHelperService path is used.
src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceRegistrationGuardTest.java	Updates test wiring to include new mocks required by the service constructor.

---

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

[Copilot]

This test stubs loginHelperService.userLoginHelper(...) to return a DSUserDetails with an empty authorities list, and it no longer asserts anything about authorities. Since the production path derives authorities from the DB user roles, consider returning a non-empty authorities collection here and asserting that the resulting DSUserDetails contains the expected authorities so the test continues validating authorization behavior after routing through LoginHelperService.

[Copilot]

Exposing public setters on the authentication principal for oidcUserInfo / oidcIdToken makes DSUserDetails mutable after creation, which can lead to partially-initialized principals or accidental overwrites later in the request lifecycle. Consider keeping DSUserDetails immutable by extending LoginHelperService.userLoginHelper(...) (or adding an overload/factory) to accept the OIDC token/userInfo and construct the principal in one step.

[Copilot]

Email normalization here uses toLowerCase() without trimming or specifying a locale. Using the default locale can produce incorrect results in some environments (e.g., Turkish locale), and leading/trailing whitespace would still cause lookup misses. Consider normalizing with trim() and toLowerCase(Locale.ROOT) once into a local variable and using that for both lookup and guard evaluation/logging.

[Copilot]

Adding @Transactional at the class level means the transaction starts before defaultOidcUserService.loadUser(userRequest) runs, so the DB transaction/connection can be held open while making a network call to the OIDC provider. To reduce contention and improve reliability, consider narrowing the transaction scope to only the DB interaction portion (e.g., using a separate transactional component/method invoked via proxy, or TransactionTemplate around handleOidcLoginSuccess + login helper persistence work).

[Copilot]

This new audit event publication is an observable behavior change, but the unit tests don’t currently assert that eventPublisher.publishEvent(...) is called (or that the action/message are correct) during a new OIDC registration. Adding an assertion in the new-user registration tests would prevent regressions in audit logging behavior.

[claude]

PR Review: Fix OIDC service inconsistencies with OAuth2 service

Overall, this is a well-scoped and well-motivated PR. The four changes are clearly described and align DSOIdcUserService correctly with DSOAuth2UserService. The approach is sound. I have a few observations ranging from minor mits to one correctness issue worth addressing.

Correctness / Potential Bugs

1. @transactional on a private method has no effect (DSOidcUserService line 128)

Spring's proxy-based AOP cannot intercept private methods, so this annotation is silently ignored. The same pattern already exists in DSOAuth2UserService line 130, so this is a pre-existing issue that was copied rather than introduced here. Since this PR is explicitly aligning the two classes, it is worth noting. The class-level @transactional already covers this method when called from the public handleOidcLoginSuccess, so the per-method annotation is redundant and misleading. Consider removing it here and in DSOAuth2UserService.

2. Audit event published before userRepository.save() (DSOidcUserService lines 135-139)

The audit event fires before the user is actually persisted. If userRepository.save() throws (e.g. a constraint violation on a race condition), the audit log will record a successful registration that never actually happened. DSOAuth2UserService has the same ordering. Since this PR is a deliberate port, it would be cleaner to fix the ordering here (publish after save) rather than faithfully copying a latent bug.

Code Quality / Design

3. Hard-coded "ROLE_USER" string (DSOidcUserService line 132)

DSOAuth2UserService uses the constant USER_ROLE_NAME = "ROLE_USER". The OIDC service should do the same to avoid diverging again in the future.

4. Commented-out dead code (DSOidcUserService lines 170-172)

The commented block in getUserFromKeycloakOidc2User is pre-existing but worth cleaning up since this method was touched. Dead code in an actively modified method raises questions for future reviewers.

Test Coverage

5. assertThat(dsUserDetails.getAuthorities()).isNotEmpty() was removed

The mock loginHelperService.userLoginHelper() returns new DSUserDetails(user, new ArrayList<>()) so authorities are always empty, making the assertion impossible without changing the mock. Consider making the mock return a non-empty authorities list and re-adding the assertion to maintain that coverage.

6. No state assertion for lastActivityDate update

The PR description calls out updating lastActivityDate as a key behavioral fix. The test verifies loginHelperService.userLoginHelper() is called (good), but there is no assertion that lastActivityDate was actually set on the user passed to it. A verify with an ArgumentCaptor would give stronger confidence.

Minor / Style

7. Stale log message (DSOidcUserService line 192)

The log.debug message says "Loading user from OAuth2 provider" but this is the OIDC service. Should read "Loading user from OIDC provider...".

Summary

Category	Item	Severity
Correctness	@transactional on private method is no-op (pre-existing)	Low
Correctness	Audit event fires before DB save (pre-existing copy)	Medium
Consistency	Hard-coded "ROLE_USER" vs. constant used in OAuth2 service	Low
Tests	Authority assertion removed without replacement	Medium
Tests	No state assertion for lastActivityDate	Low
Style	Stale log message says "OAuth2" in OIDC service	Trivial
Cleanup	Commented-out dead code in getUserFromKeycloakOidc2User	Trivial

The core fixes (email normalization, audit event, @transactional, loginHelperService routing) are all correct and necessary. The most actionable items before merge are the removed authority assertion (item 5) and the audit-event-before-save ordering (item 2).

[claude]

PR Review: Fix OIDC service inconsistencies with OAuth2 service

This is a solid, well-scoped PR. The four stated fixes (email normalization, audit event, @Transactional, loginHelperService routing) are all correct and necessary. A few items worth flagging:

---

Bug: attributes field uninitialized in OIDC DSUserDetails constructor

In DSUserDetails.java, the new 4-arg OIDC constructor (added in this PR) does not initialize the attributes field:

// 2-arg constructor — attributes initialized ✓
public DSUserDetails(User user, Collection<? extends GrantedAuthority> grantedAuthorities) {
    this.user = user;
    this.grantedAuthorities = ...;
    this.attributes = new HashMap<>();  // ← initialized
}

// 4-arg OIDC constructor — attributes NOT initialized ✗
public DSUserDetails(User user, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken,
                     Collection<? extends GrantedAuthority> grantedAuthorities) {
    this.user = user;
    this.oidcUserInfo = oidcUserInfo;
    this.oidcIdToken = oidcIdToken;
    this.grantedAuthorities = ...;
    // this.attributes is null!
}

getAttributes() just returns attributes directly, so OIDC users get null instead of an empty map. The OAuth2User contract doesn't explicitly forbid null, but Spring Security's internal code (e.g., attribute-based access rules, OIDC session tracking) may call getAttributes() and NPE if it returns null. The 3-arg constructor delegates to the 4-arg constructor, so it also has this issue.

Fix: Initialize attributes in the 4-arg constructor: this.attributes = new HashMap<>();

---

Double @Builder on two constructors in DSUserDetails

Both the 4-arg and 3-arg constructors carry @Builder. Lombok generates a builder() factory method for each; having two in the same class produces a method name conflict and typically generates a compile-time warning (or error depending on Lombok version). Since the 3-arg constructor is now only a delegating wrapper and no call sites use its builder, removing @Builder from the 3-arg constructor (or removing the 3-arg constructor entirely, since it just delegates to the 4-arg) would clean this up.

---

Email normalization inconsistency: DSOAuth2UserService still uses .toLowerCase() without Locale.ROOT

The OIDC service now correctly uses .trim().toLowerCase(Locale.ROOT) for defensive normalization, but the OAuth2 service still uses .toLowerCase() at line 92:

User existingUser = userRepository.findByEmail(user.getEmail().toLowerCase());

This is a pre-existing issue (not introduced here), but since the goal of the PR is to align the two services, it's a natural candidate for a follow-up fix or a minor amendment.

---

Positives worth calling out

• Save-before-audit-event ordering in both registerNewOAuthUser and registerNewOidcUser is now correct — the savedUser (with its persisted ID) is used in the audit event. This is a real correctness improvement over the previous approach.
• loginHelperService.userLoginHelper() routing is the right way to ensure lastActivityDate and lockout checks apply uniformly to OIDC logins, not just form and OAuth2 logins.
• @FunctionalInterface on RegistrationGuard is a nice, low-risk improvement that clarifies intent and enables lambda-based implementations.
• OAuth2Error now includes a description (new OAuth2Error("registration_denied", decision.reason(), null)) in both services — this makes errors surfaced to OAuth2 clients much more useful.
• Removing @Transactional from private methods (which Spring AOP cannot intercept) is the correct cleanup; the class-level annotation covers those callers.
• isBlank() over trim().isEmpty() is a clean Java 11+ idiom, and the extracted DEFAULT_DENIAL_REASON constant avoids magic strings.
• Test coverage is good: the loadUser integration path now verifies loginHelperService.userLoginHelper() is called and produces the correct DSUserDetails shape.

---

Summary

Category	Item	Severity
Bug	attributes is null for OIDC DSUserDetails instances	Medium
Code quality	Double @Builder on two constructors	Low
Consistency	OAuth2 service still uses .toLowerCase() without Locale.ROOT	Low (follow-up)

The attributes initialization gap is the most actionable item before this code is exercised in an environment that calls getAttributes() on the OIDC principal (e.g., attribute-based authorization rules).