Fix OIDC service inconsistencies with OAuth2 service (#278)

* fix(oidc): align DSOidcUserService behavior with DSOAuth2UserService

Fixes four inconsistencies identified in issue #276:

- Add .toLowerCase() to email lookup in handleOidcLoginSuccess() to
  normalize email case before findByEmail(), matching OAuth2 behavior
- Add @transactional at class level and on registerNewOidcUser() to
  ensure database operations are transactional, matching OAuth2 behavior
- Publish AuditEvent on new OIDC user registration (\"OIDC Registration
  Success\"), matching the OAuth2 \"OAuth2 Registration Success\" event
- Call loginHelperService.userLoginHelper() in loadUser() to update
  lastActivityDate and run lockout checks; set OIDC-specific tokens
  (idToken, userInfo) via new @Setter fields on DSUserDetails

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

Update DSOidcUserServiceTest and DSOidcUserServiceRegistrationGuardTest
to inject @mock LoginHelperService and ApplicationEventPublisher.

Closes #276

* fix: address PR review feedback from RegistrationGuard SPI (#271)

- Include denial reason in OAuth2Error description field so
  AuthenticationFailureHandlers can access it via getError().getDescription()
  in both DSOAuth2UserService and DSOidcUserService
- Replace String.trim().isEmpty() with String.isBlank() in
  RegistrationDecision.deny() (Java 17+ idiomatic)
- Extract DEFAULT_DENIAL_REASON constant in RegistrationDecision
- Add @FunctionalInterface to RegistrationGuard interface
- Add JavaDoc clarifying RegistrationDecision reason is only meaningful
  when allowed is false
- Add RegistrationContextTest covering null source validation

* fix: address PR #278 review feedback

- Add LoginHelperService.userLoginHelper(User, OidcUserInfo, OidcIdToken) overload
  so DSOidcUserService can build an immutable DSUserDetails with OIDC tokens in
  one step, eliminating the need for @Setter on DSUserDetails fields
- Remove @Setter from DSUserDetails.oidcUserInfo / oidcIdToken to restore immutability
- Remove @transactional from private registerNewOidcUser/registerNewOAuthUser methods
  (silently ignored by Spring AOP proxy; class-level @transactional covers them)
- Swap audit event publication to after save() in both services to prevent false-positive
  audit records when save throws (e.g. unique-constraint violation)
- Use trim() + toLowerCase(Locale.ROOT) for email normalization to handle locale edge
  cases and leading/trailing whitespace in both extraction and lookup
- Fix import ordering in DSOidcUserService.java to alphabetical per CLAUDE.md
- Add JavaDoc on loginHelperService field in DSOidcUserService
- Update DSOidcUserServiceTest: use 3-arg userLoginHelper mock, assert non-empty
  authorities, assert eventPublisher.publishEvent() called on new registration,
  update shouldPreserveOidcTokensInDSUserDetails to use the new constructor path

* chore: minor cleanups in DSOidcUserService

- Extract USER_ROLE_NAME constant to match DSOAuth2UserService
- Fix stale log message: "OAuth2 provider" → "OIDC provider"

Author: devondragon

src/main/java/com/digitalsanctuary/spring/user/registration/RegistrationDecision.java

@@ -4,11 +4,19 @@
  * The result of a {@link RegistrationGuard} evaluation indicating whether
  * a registration attempt should be allowed or denied.
  *
+ * <p>Use the static factory methods {@link #allow()} and {@link #deny(String)}
+ * rather than the canonical constructor. The {@code reason} parameter is only
+ * meaningful when {@code allowed} is {@code false}.</p>
+ *
  * @param allowed whether the registration is permitted
- * @param reason  a human-readable denial reason (may be {@code null} when allowed)
+ * @param reason  a human-readable denial reason; only meaningful when {@code allowed}
+ *                is {@code false}, may be {@code null} when allowed
  */
 public record RegistrationDecision(boolean allowed, String reason) {
 
+    /** Default reason used when {@link #deny(String)} is called with a blank or null reason. */
+    private static final String DEFAULT_DENIAL_REASON = "Registration denied.";
+
     /**
      * Creates a decision that allows the registration to proceed.
      *
@@ -25,8 +33,8 @@ public static RegistrationDecision allow() {
      * @return a denying decision with the given reason
      */
     public static RegistrationDecision deny(String reason) {
-        if (reason == null || reason.trim().isEmpty()) {
-            reason = "Registration denied.";
+        if (reason == null || reason.isBlank()) {
+            reason = DEFAULT_DENIAL_REASON;
         }
         return new RegistrationDecision(false, reason);
     }

src/main/java/com/digitalsanctuary/spring/user/registration/RegistrationGuard.java

@@ -40,6 +40,7 @@
  * @see RegistrationDecision
  * @see DefaultRegistrationGuard
  */
+@FunctionalInterface
 public interface RegistrationGuard {
 
     /**

src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java

@@ -110,7 +110,7 @@ public User handleOAuthLoginSuccess(String registrationId, OAuth2User oAuth2User
                 log.info("Registration denied for email: {} source: OAUTH2 provider: {} reason: {}",
                         user.getEmail(), registrationId, decision.reason());
                 throw new OAuth2AuthenticationException(
-                        new OAuth2Error("registration_denied"), decision.reason());
+                        new OAuth2Error("registration_denied", decision.reason(), null), decision.reason());
             }
             user = registerNewOAuthUser(registrationId, user);
             return user;
@@ -126,18 +126,17 @@ public User handleOAuthLoginSuccess(String registrationId, OAuth2User oAuth2User
      * @param user The User object representing the authenticated user.
      * @return A User object representing the newly registered user.
      */
-    @Transactional
     private User registerNewOAuthUser(String registrationId, User user) {
         User.Provider provider = User.Provider.valueOf(registrationId.toUpperCase());
         user.setProvider(provider);
         user.setRoles(Arrays.asList(roleRepository.findByName(USER_ROLE_NAME)));
         // We will trust OAuth2 providers to provide us with a verified email address.
         user.setEnabled(true);
-        AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(user).action("OAuth2 Registration Success").actionStatus("Success")
+        User savedUser = userRepository.save(user);
+        AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(savedUser).action("OAuth2 Registration Success").actionStatus("Success")
                 .message("Registration Confirmed. User logged in.").build();
-
         eventPublisher.publishEvent(registrationAuditEvent);
-        return userRepository.save(user);
+        return savedUser;
     }
 
     /**

src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java

@@ -1,15 +1,8 @@
 package com.digitalsanctuary.spring.user.service;
 
 import java.util.Arrays;
-import com.digitalsanctuary.spring.user.persistence.model.User;
-import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
-import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
-import com.digitalsanctuary.spring.user.registration.RegistrationContext;
-import com.digitalsanctuary.spring.user.registration.RegistrationDecision;
-import com.digitalsanctuary.spring.user.registration.RegistrationGuard;
-import com.digitalsanctuary.spring.user.registration.RegistrationSource;
-import lombok.RequiredArgsConstructor;
-import lombok.extern.slf4j.Slf4j;
+import java.util.Locale;
+import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserRequest;
 import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserService;
 import org.springframework.security.oauth2.client.userinfo.OAuth2UserRequest;
@@ -19,6 +12,17 @@
 import org.springframework.security.oauth2.core.oidc.user.OidcUser;
 import org.springframework.security.oauth2.core.user.OAuth2User;
 import org.springframework.stereotype.Service;
+import org.springframework.transaction.annotation.Transactional;
+import com.digitalsanctuary.spring.user.audit.AuditEvent;
+import com.digitalsanctuary.spring.user.persistence.model.User;
+import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
+import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
+import com.digitalsanctuary.spring.user.registration.RegistrationContext;
+import com.digitalsanctuary.spring.user.registration.RegistrationDecision;
+import com.digitalsanctuary.spring.user.registration.RegistrationGuard;
+import com.digitalsanctuary.spring.user.registration.RegistrationSource;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
 
 /**
  * OIDC user service implementation for handling OpenID Connect authentication (Keycloak).
@@ -36,17 +40,27 @@
  */
 @Slf4j
 @Service
+@Transactional
 @RequiredArgsConstructor
 public class DSOidcUserService implements OAuth2UserService<OidcUserRequest, OidcUser> {
 
     /** The user repository. */
     private final UserRepository userRepository;
-    
+
     /** The role repository. */
     private final RoleRepository roleRepository;
 
+    /** The login helper service. */
+    private final LoginHelperService loginHelperService;
+
     private final RegistrationGuard registrationGuard;
 
+    /** The Event Publisher. */
+    private final ApplicationEventPublisher eventPublisher;
+
+    /** The user role name. */
+    private static final String USER_ROLE_NAME = "ROLE_USER";
+
     OidcUserService defaultOidcUserService = new OidcUserService();
 
     /**
@@ -78,8 +92,11 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
             throw new OAuth2AuthenticationException(new OAuth2Error("Missing Email"),
                     "Unable to retrieve email address from " + registrationId + ". Please ensure you have granted email permissions.");
         }
-        log.debug("handleOidcLoginSuccess: looking up user with email: {}", user.getEmail());
-        User existingUser = userRepository.findByEmail(user.getEmail());
+        // Normalize email for consistent lookup — getUserFromKeycloakOidc2User already lowercases,
+        // but we normalize again here defensively in case additional sources are added.
+        String normalizedEmail = user.getEmail().trim().toLowerCase(Locale.ROOT);
+        log.debug("handleOidcLoginSuccess: looking up user with email: {}", normalizedEmail);
+        User existingUser = userRepository.findByEmail(normalizedEmail);
         log.debug("handleOidcLoginSuccess: existingUser: {}", existingUser);
         if (existingUser != null && registrationId != null) {
             log.debug("handleOidcLoginSuccess: existingUser.getProvider(): {}", existingUser.getProvider());
@@ -93,14 +110,14 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
             existingUser = updateExistingUser(existingUser, user);
             return userRepository.save(existingUser);
         } else {
-            log.debug("handleOidcLoginSuccess: registering new user with email: {}", user.getEmail());
+            log.debug("handleOidcLoginSuccess: registering new user with email: {}", normalizedEmail);
             RegistrationDecision decision = registrationGuard.evaluate(
-                    new RegistrationContext(user.getEmail(), RegistrationSource.OIDC, registrationId));
+                    new RegistrationContext(normalizedEmail, RegistrationSource.OIDC, registrationId));
             if (!decision.allowed()) {
                 log.info("Registration denied for email: {} source: OIDC provider: {} reason: {}",
-                        user.getEmail(), registrationId, decision.reason());
+                        normalizedEmail, registrationId, decision.reason());
                 throw new OAuth2AuthenticationException(
-                        new OAuth2Error("registration_denied"), decision.reason());
+                        new OAuth2Error("registration_denied", decision.reason(), null), decision.reason());
             }
             user = registerNewOidcUser(registrationId, user);
             return user;
@@ -119,10 +136,14 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
     private User registerNewOidcUser(String registrationId, User user) {
         User.Provider provider = User.Provider.valueOf(registrationId.toUpperCase());
         user.setProvider(provider);
-        user.setRoles(Arrays.asList(roleRepository.findByName("ROLE_USER")));
-        // We will trust OAuth2 providers to provide us with a verified email address.
+        user.setRoles(Arrays.asList(roleRepository.findByName(USER_ROLE_NAME)));
+        // We will trust OIDC providers to provide us with a verified email address.
         user.setEnabled(true);
-        return userRepository.save(user);
+        User savedUser = userRepository.save(user);
+        AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(savedUser).action("OIDC Registration Success").actionStatus("Success")
+                .message("Registration Confirmed. User logged in.").build();
+        eventPublisher.publishEvent(registrationAuditEvent);
+        return savedUser;
     }
 
     /**
@@ -153,11 +174,8 @@ public User getUserFromKeycloakOidc2User(OidcUser principal) {
         }
         log.debug("Principal attributes: {}", principal.getAttributes());
         User user = new User();
-/*        user.setEmail(principal.getAttribute("email"));
-        user.setFirstName(principal.getAttribute("given_name"));
-        user.setLastName(principal.getAttribute("family_name"));*/
         String email = principal.getEmail();
-        user.setEmail(email != null ? email.toLowerCase() : null);
+        user.setEmail(email != null ? email.trim().toLowerCase(Locale.ROOT) : null);
         user.setFirstName(principal.getGivenName());
         user.setLastName(principal.getFamilyName());
         user.setProvider(User.Provider.KEYCLOAK);
@@ -175,17 +193,11 @@ public User getUserFromKeycloakOidc2User(OidcUser principal) {
      */
     @Override
     public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2AuthenticationException {
-        log.debug("Loading user from OAuth2 provider with userRequest: {}", userRequest);
+        log.debug("Loading user from OIDC provider with userRequest: {}", userRequest);
         OidcUser user = defaultOidcUserService.loadUser(userRequest);
         String registrationId = userRequest.getClientRegistration().getRegistrationId();
         log.debug("registrationId: {}", registrationId);
         User dbUser = handleOidcLoginSuccess(registrationId, user);
-        DSUserDetails dsUserDetails = DSUserDetails.builder()
-                .user(dbUser)
-                .oidcUserInfo(user.getUserInfo())
-                .oidcIdToken(user.getIdToken())
-                .grantedAuthorities(user.getAuthorities())
-                .build();
-        return dsUserDetails;
+        return loginHelperService.userLoginHelper(dbUser, user.getUserInfo(), user.getIdToken());
     }
 }

src/main/java/com/digitalsanctuary/spring/user/service/LoginHelperService.java

@@ -3,6 +3,8 @@
 import java.util.Collection;
 import java.util.Date;
 import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.oauth2.core.oidc.OidcIdToken;
+import org.springframework.security.oauth2.core.oidc.OidcUserInfo;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
 import com.digitalsanctuary.spring.user.persistence.model.User;
@@ -49,4 +51,24 @@ public DSUserDetails userLoginHelper(User dbUser) {
         DSUserDetails userDetails = new DSUserDetails(dbUser, authorities);
         return userDetails;
     }
+
+    /**
+     * Helper method to authenticate an OIDC user after login, attaching the OIDC-specific tokens
+     * and claims to the principal while keeping {@link DSUserDetails} immutable.
+     *
+     * @param dbUser       The user to authenticate.
+     * @param oidcUserInfo The OIDC user info claims.
+     * @param oidcIdToken  The OIDC ID token.
+     * @return The user details object with OIDC tokens set.
+     */
+    public DSUserDetails userLoginHelper(User dbUser, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken) {
+        // Updating lastActivity date for this login
+        dbUser.setLastActivityDate(new Date());
+
+        // Check if the user account is locked, but should be unlocked now, and unlock it
+        dbUser = loginAttemptService.checkIfUserShouldBeUnlocked(dbUser);
+
+        Collection<? extends GrantedAuthority> authorities = authorityService.getAuthoritiesFromUser(dbUser);
+        return new DSUserDetails(dbUser, oidcUserInfo, oidcIdToken, authorities);
+    }
 }

src/test/java/com/digitalsanctuary/spring/user/registration/RegistrationContextTest.java

@@ -0,0 +1,39 @@
+package com.digitalsanctuary.spring.user.registration;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@DisplayName("RegistrationContext Tests")
+class RegistrationContextTest {
+
+    @Test
+    @DisplayName("Should reject null source in RegistrationContext")
+    void shouldRejectNullSource() {
+        assertThatThrownBy(() -> new RegistrationContext("user@example.com", null, null))
+                .isInstanceOf(NullPointerException.class)
+                .hasMessageContaining("source must not be null");
+    }
+
+    @Test
+    @DisplayName("Should accept valid context with all fields")
+    void shouldAcceptValidContext() {
+        RegistrationContext context = new RegistrationContext("user@example.com", RegistrationSource.OAUTH2, "google");
+
+        assertThat(context.email()).isEqualTo("user@example.com");
+        assertThat(context.source()).isEqualTo(RegistrationSource.OAUTH2);
+        assertThat(context.providerName()).isEqualTo("google");
+    }
+
+    @Test
+    @DisplayName("Should accept null email and null providerName")
+    void shouldAcceptNullEmailAndProvider() {
+        RegistrationContext context = new RegistrationContext(null, RegistrationSource.FORM, null);
+
+        assertThat(context.email()).isNull();
+        assertThat(context.source()).isEqualTo(RegistrationSource.FORM);
+        assertThat(context.providerName()).isNull();
+    }
+}

src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceRegistrationGuardTest.java

@@ -15,6 +15,7 @@
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.security.oauth2.core.OAuth2AuthenticationException;
 import org.springframework.security.oauth2.core.oidc.user.OidcUser;
 
@@ -37,9 +38,15 @@ class DSOidcUserServiceRegistrationGuardTest {
     @Mock
     private RoleRepository roleRepository;
 
+    @Mock
+    private LoginHelperService loginHelperService;
+
     @Mock
     private RegistrationGuard registrationGuard;
 
+    @Mock
+    private ApplicationEventPublisher eventPublisher;
+
     @InjectMocks
     private DSOidcUserService service;