Skip to content

Commit d22359d

Browse files
devondragondevondragon
committed
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
1 parent ac98b57 commit d22359d

5 files changed

Lines changed: 72 additions & 51 deletions

File tree

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,18 +126,17 @@ public User handleOAuthLoginSuccess(String registrationId, OAuth2User oAuth2User
126126
* @param user The User object representing the authenticated user.
127127
* @return A User object representing the newly registered user.
128128
*/
129-
@Transactional
130129
private User registerNewOAuthUser(String registrationId, User user) {
131130
User.Provider provider = User.Provider.valueOf(registrationId.toUpperCase());
132131
user.setProvider(provider);
133132
user.setRoles(Arrays.asList(roleRepository.findByName(USER_ROLE_NAME)));
134133
// We will trust OAuth2 providers to provide us with a verified email address.
135134
user.setEnabled(true);
136-
AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(user).action("OAuth2 Registration Success").actionStatus("Success")
135+
User savedUser = userRepository.save(user);
136+
AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(savedUser).action("OAuth2 Registration Success").actionStatus("Success")
137137
.message("Registration Confirmed. User logged in.").build();
138-
139138
eventPublisher.publishEvent(registrationAuditEvent);
140-
return userRepository.save(user);
139+
return savedUser;
141140
}
142141

143142
/**

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

Lines changed: 25 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,7 @@
11
package com.digitalsanctuary.spring.user.service;
22

33
import java.util.Arrays;
4-
import com.digitalsanctuary.spring.user.audit.AuditEvent;
5-
import com.digitalsanctuary.spring.user.persistence.model.User;
6-
import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
7-
import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
8-
import com.digitalsanctuary.spring.user.registration.RegistrationContext;
9-
import com.digitalsanctuary.spring.user.registration.RegistrationDecision;
10-
import com.digitalsanctuary.spring.user.registration.RegistrationGuard;
11-
import com.digitalsanctuary.spring.user.registration.RegistrationSource;
12-
import lombok.RequiredArgsConstructor;
13-
import lombok.extern.slf4j.Slf4j;
4+
import java.util.Locale;
145
import org.springframework.context.ApplicationEventPublisher;
156
import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserRequest;
167
import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserService;
@@ -22,6 +13,16 @@
2213
import org.springframework.security.oauth2.core.user.OAuth2User;
2314
import org.springframework.stereotype.Service;
2415
import org.springframework.transaction.annotation.Transactional;
16+
import com.digitalsanctuary.spring.user.audit.AuditEvent;
17+
import com.digitalsanctuary.spring.user.persistence.model.User;
18+
import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
19+
import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
20+
import com.digitalsanctuary.spring.user.registration.RegistrationContext;
21+
import com.digitalsanctuary.spring.user.registration.RegistrationDecision;
22+
import com.digitalsanctuary.spring.user.registration.RegistrationGuard;
23+
import com.digitalsanctuary.spring.user.registration.RegistrationSource;
24+
import lombok.RequiredArgsConstructor;
25+
import lombok.extern.slf4j.Slf4j;
2526

2627
/**
2728
* OIDC user service implementation for handling OpenID Connect authentication (Keycloak).
@@ -49,6 +50,7 @@ public class DSOidcUserService implements OAuth2UserService<OidcUserRequest, Oid
4950
/** The role repository. */
5051
private final RoleRepository roleRepository;
5152

53+
/** The login helper service. */
5254
private final LoginHelperService loginHelperService;
5355

5456
private final RegistrationGuard registrationGuard;
@@ -87,8 +89,11 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
8789
throw new OAuth2AuthenticationException(new OAuth2Error("Missing Email"),
8890
"Unable to retrieve email address from " + registrationId + ". Please ensure you have granted email permissions.");
8991
}
90-
log.debug("handleOidcLoginSuccess: looking up user with email: {}", user.getEmail());
91-
User existingUser = userRepository.findByEmail(user.getEmail().toLowerCase());
92+
// Normalize email for consistent lookup — getUserFromKeycloakOidc2User already lowercases,
93+
// but we normalize again here defensively in case additional sources are added.
94+
String normalizedEmail = user.getEmail().trim().toLowerCase(Locale.ROOT);
95+
log.debug("handleOidcLoginSuccess: looking up user with email: {}", normalizedEmail);
96+
User existingUser = userRepository.findByEmail(normalizedEmail);
9297
log.debug("handleOidcLoginSuccess: existingUser: {}", existingUser);
9398
if (existingUser != null && registrationId != null) {
9499
log.debug("handleOidcLoginSuccess: existingUser.getProvider(): {}", existingUser.getProvider());
@@ -102,12 +107,12 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
102107
existingUser = updateExistingUser(existingUser, user);
103108
return userRepository.save(existingUser);
104109
} else {
105-
log.debug("handleOidcLoginSuccess: registering new user with email: {}", user.getEmail());
110+
log.debug("handleOidcLoginSuccess: registering new user with email: {}", normalizedEmail);
106111
RegistrationDecision decision = registrationGuard.evaluate(
107-
new RegistrationContext(user.getEmail(), RegistrationSource.OIDC, registrationId));
112+
new RegistrationContext(normalizedEmail, RegistrationSource.OIDC, registrationId));
108113
if (!decision.allowed()) {
109114
log.info("Registration denied for email: {} source: OIDC provider: {} reason: {}",
110-
user.getEmail(), registrationId, decision.reason());
115+
normalizedEmail, registrationId, decision.reason());
111116
throw new OAuth2AuthenticationException(
112117
new OAuth2Error("registration_denied", decision.reason(), null), decision.reason());
113118
}
@@ -125,18 +130,17 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
125130
* @param user The User object representing the authenticated user.
126131
* @return A User object representing the newly registered user.
127132
*/
128-
@Transactional
129133
private User registerNewOidcUser(String registrationId, User user) {
130134
User.Provider provider = User.Provider.valueOf(registrationId.toUpperCase());
131135
user.setProvider(provider);
132136
user.setRoles(Arrays.asList(roleRepository.findByName("ROLE_USER")));
133137
// We will trust OIDC providers to provide us with a verified email address.
134138
user.setEnabled(true);
135-
AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(user).action("OIDC Registration Success").actionStatus("Success")
139+
User savedUser = userRepository.save(user);
140+
AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(savedUser).action("OIDC Registration Success").actionStatus("Success")
136141
.message("Registration Confirmed. User logged in.").build();
137-
138142
eventPublisher.publishEvent(registrationAuditEvent);
139-
return userRepository.save(user);
143+
return savedUser;
140144
}
141145

142146
/**
@@ -167,11 +171,8 @@ public User getUserFromKeycloakOidc2User(OidcUser principal) {
167171
}
168172
log.debug("Principal attributes: {}", principal.getAttributes());
169173
User user = new User();
170-
/* user.setEmail(principal.getAttribute("email"));
171-
user.setFirstName(principal.getAttribute("given_name"));
172-
user.setLastName(principal.getAttribute("family_name"));*/
173174
String email = principal.getEmail();
174-
user.setEmail(email != null ? email.toLowerCase() : null);
175+
user.setEmail(email != null ? email.trim().toLowerCase(Locale.ROOT) : null);
175176
user.setFirstName(principal.getGivenName());
176177
user.setLastName(principal.getFamilyName());
177178
user.setProvider(User.Provider.KEYCLOAK);
@@ -194,9 +195,6 @@ public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2Authenticatio
194195
String registrationId = userRequest.getClientRegistration().getRegistrationId();
195196
log.debug("registrationId: {}", registrationId);
196197
User dbUser = handleOidcLoginSuccess(registrationId, user);
197-
DSUserDetails dsUserDetails = loginHelperService.userLoginHelper(dbUser);
198-
dsUserDetails.setOidcUserInfo(user.getUserInfo());
199-
dsUserDetails.setOidcIdToken(user.getIdToken());
200-
return dsUserDetails;
198+
return loginHelperService.userLoginHelper(dbUser, user.getUserInfo(), user.getIdToken());
201199
}
202200
}

‎src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetails.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import org.springframework.security.oauth2.core.oidc.user.OidcUser;
1212
import com.digitalsanctuary.spring.user.persistence.model.User;
1313
import lombok.Builder;
14-
import lombok.Setter;
1514
import lombok.ToString;
1615

1716
/**
@@ -55,11 +54,9 @@ public class DSUserDetails implements UserDetails, OidcUser {
5554
private Map<String, Object> attributes;
5655

5756
/** The Oidc user properties. */
58-
@Setter
5957
private OidcUserInfo oidcUserInfo;
6058

6159
/** The Oidc user token. */
62-
@Setter
6360
private OidcIdToken oidcIdToken;
6461

6562
/**

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
import java.util.Collection;
44
import java.util.Date;
55
import org.springframework.security.core.GrantedAuthority;
6+
import org.springframework.security.oauth2.core.oidc.OidcIdToken;
7+
import org.springframework.security.oauth2.core.oidc.OidcUserInfo;
68
import org.springframework.stereotype.Service;
79
import org.springframework.transaction.annotation.Transactional;
810
import com.digitalsanctuary.spring.user.persistence.model.User;
@@ -49,4 +51,24 @@ public DSUserDetails userLoginHelper(User dbUser) {
4951
DSUserDetails userDetails = new DSUserDetails(dbUser, authorities);
5052
return userDetails;
5153
}
54+
55+
/**
56+
* Helper method to authenticate an OIDC user after login, attaching the OIDC-specific tokens
57+
* and claims to the principal while keeping {@link DSUserDetails} immutable.
58+
*
59+
* @param dbUser The user to authenticate.
60+
* @param oidcUserInfo The OIDC user info claims.
61+
* @param oidcIdToken The OIDC ID token.
62+
* @return The user details object with OIDC tokens set.
63+
*/
64+
public DSUserDetails userLoginHelper(User dbUser, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken) {
65+
// Updating lastActivity date for this login
66+
dbUser.setLastActivityDate(new Date());
67+
68+
// Check if the user account is locked, but should be unlocked now, and unlock it
69+
dbUser = loginAttemptService.checkIfUserShouldBeUnlocked(dbUser);
70+
71+
Collection<? extends GrantedAuthority> authorities = authorityService.getAuthoritiesFromUser(dbUser);
72+
return new DSUserDetails(dbUser, oidcUserInfo, oidcIdToken, authorities);
73+
}
5274
}

‎src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,10 @@
33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.assertj.core.api.Assertions.assertThatThrownBy;
55
import static org.mockito.ArgumentMatchers.any;
6-
import static org.mockito.ArgumentMatchers.anyString;
76
import static org.mockito.Mockito.*;
87

9-
import java.util.ArrayList;
108
import java.util.Arrays;
9+
import java.util.List;
1110
import org.junit.jupiter.api.BeforeEach;
1211
import org.junit.jupiter.api.DisplayName;
1312
import org.junit.jupiter.api.Nested;
@@ -16,15 +15,19 @@
1615
import org.mockito.InjectMocks;
1716
import org.mockito.Mock;
1817
import org.mockito.junit.jupiter.MockitoExtension;
18+
import org.springframework.context.ApplicationEventPublisher;
19+
import org.springframework.security.core.authority.SimpleGrantedAuthority;
1920
import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserRequest;
2021
import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserService;
2122
import org.springframework.security.oauth2.client.registration.ClientRegistration;
2223
import org.springframework.security.oauth2.core.AuthorizationGrantType;
2324
import org.springframework.security.oauth2.core.OAuth2AccessToken;
2425
import org.springframework.security.oauth2.core.OAuth2AuthenticationException;
26+
import org.springframework.security.oauth2.core.oidc.OidcIdToken;
27+
import org.springframework.security.oauth2.core.oidc.OidcUserInfo;
2528
import org.springframework.security.oauth2.core.oidc.user.OidcUser;
29+
import com.digitalsanctuary.spring.user.audit.AuditEvent;
2630
import com.digitalsanctuary.spring.user.fixtures.OidcUserTestDataBuilder;
27-
import org.springframework.context.ApplicationEventPublisher;
2831
import com.digitalsanctuary.spring.user.persistence.model.Role;
2932
import com.digitalsanctuary.spring.user.persistence.model.User;
3033
import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
@@ -104,6 +107,7 @@ void shouldCreateNewUserFromKeycloakOidc() {
104107
assertThat(result.getRoles()).containsExactly(userRole);
105108

106109
verify(userRepository).save(any(User.class));
110+
verify(eventPublisher).publishEvent(any(AuditEvent.class));
107111
}
108112

109113
@Test
@@ -182,6 +186,7 @@ void shouldHandleKeycloakUserWithMinimalClaims() {
182186
assertThat(result.getFirstName()).isNull();
183187
assertThat(result.getLastName()).isNull();
184188
assertThat(result.getProvider()).isEqualTo(User.Provider.KEYCLOAK);
189+
verify(eventPublisher).publishEvent(any(AuditEvent.class));
185190
}
186191

187192
@Test
@@ -367,10 +372,14 @@ void shouldLoadUserThroughOidcRequestFlow() {
367372
user.setId(999L);
368373
return user;
369374
});
370-
when(loginHelperService.userLoginHelper(any(User.class))).thenAnswer(invocation -> {
371-
User user = invocation.getArgument(0);
372-
return new DSUserDetails(user, new ArrayList<>());
373-
});
375+
when(loginHelperService.userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class)))
376+
.thenAnswer(invocation -> {
377+
User user = invocation.getArgument(0);
378+
OidcUserInfo oidcUserInfo = invocation.getArgument(1);
379+
OidcIdToken oidcIdToken = invocation.getArgument(2);
380+
return new DSUserDetails(user, oidcUserInfo, oidcIdToken,
381+
List.of(new SimpleGrantedAuthority("ROLE_USER")));
382+
});
374383

375384
// When
376385
OidcUser result = spyService.loadUser(userRequest);
@@ -381,9 +390,10 @@ void shouldLoadUserThroughOidcRequestFlow() {
381390
DSUserDetails dsUserDetails = (DSUserDetails) result;
382391
assertThat(dsUserDetails.getIdToken()).isNotNull();
383392
assertThat(dsUserDetails.getUserInfo()).isNotNull();
393+
assertThat(dsUserDetails.getAuthorities()).isNotEmpty();
384394

385395
verify(userRepository).save(any(User.class));
386-
verify(loginHelperService).userLoginHelper(any(User.class));
396+
verify(loginHelperService).userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class));
387397
}
388398

389399
@Test
@@ -399,16 +409,11 @@ void shouldPreserveOidcTokensInDSUserDetails() {
399409
dbUser.setEmail("tokens@keycloak.com");
400410
dbUser.setProvider(User.Provider.KEYCLOAK);
401411

402-
// When
412+
// When - Simulate what LoginHelperService.userLoginHelper() does for OIDC path
403413
User extractedUser = service.getUserFromKeycloakOidc2User(keycloakUser);
404-
405-
// Simulate what happens in loadUser
406-
DSUserDetails dsUserDetails = DSUserDetails.builder()
407-
.user(dbUser)
408-
.oidcUserInfo(keycloakUser.getUserInfo())
409-
.oidcIdToken(keycloakUser.getIdToken())
410-
.grantedAuthorities(keycloakUser.getAuthorities())
411-
.build();
414+
415+
DSUserDetails dsUserDetails = new DSUserDetails(dbUser, keycloakUser.getUserInfo(),
416+
keycloakUser.getIdToken(), keycloakUser.getAuthorities());
412417

413418
// Then
414419
assertThat(dsUserDetails.getIdToken()).isEqualTo(keycloakUser.getIdToken());

0 commit comments

Comments
 (0)