Skip to content

Commit 7c372cf

Browse files
devondragondevondragon
committed
fix: address PR #285 review feedback
- DSUserDetails.getAttributes(): return Collections.unmodifiableMap so callers cannot mutate the internal map, preventing hard-to-debug security issues - buildFallbackAttributes(): build 'name' claim without calling getFullName() to avoid "Test null" / "null User" values when only one name part is set; now safely skips missing parts and only adds the claim when non-empty - DSUserDetailsTest: add @ExtendWith(MockitoExtension.class) per project conventions; add tests for first-name-only, last-name-only, and unmodifiable-map scenarios - LoginHelperServiceTest: replace raw (Collection) cast with typed field declaration (Collection<? extends GrantedAuthority>) and doReturn() stubs to eliminate unchecked warnings - DSOAuth2UserServiceTest / DSOidcUserServiceTest: replace raw any(Map.class) matchers with typed ArgumentMatchers.<Map<String,Object>>any()
1 parent bc4be55 commit 7c372cf

5 files changed

Lines changed: 79 additions & 23 deletions

File tree

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.util.ArrayList;
44
import java.util.Collection;
5+
import java.util.Collections;
56
import java.util.HashMap;
67
import java.util.Map;
78
import org.springframework.security.core.GrantedAuthority;
@@ -160,8 +161,18 @@ private static Map<String, Object> buildFallbackAttributes(User user) {
160161
if (user.getLastName() != null) {
161162
attrs.put("family_name", user.getLastName());
162163
}
163-
if (user.getFirstName() != null || user.getLastName() != null) {
164-
attrs.put("name", user.getFullName());
164+
StringBuilder name = new StringBuilder();
165+
if (user.getFirstName() != null && !user.getFirstName().trim().isEmpty()) {
166+
name.append(user.getFirstName().trim());
167+
}
168+
if (user.getLastName() != null && !user.getLastName().trim().isEmpty()) {
169+
if (name.length() > 0) {
170+
name.append(' ');
171+
}
172+
name.append(user.getLastName().trim());
173+
}
174+
if (name.length() > 0) {
175+
attrs.put("name", name.toString());
165176
}
166177
return attrs;
167178
}
@@ -247,7 +258,7 @@ public User getUser() {
247258

248259
@Override
249260
public Map<String, Object> getAttributes() {
250-
return attributes;
261+
return Collections.unmodifiableMap(attributes);
251262
}
252263

253264
@Override

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
import static org.mockito.ArgumentMatchers.any;
66
import static org.mockito.ArgumentMatchers.anyString;
77
import static org.mockito.ArgumentMatchers.eq;
8+
import org.mockito.ArgumentMatchers;
9+
810
import static org.mockito.Mockito.*;
911

1012
import java.util.Arrays;
@@ -416,7 +418,7 @@ void shouldLoadUserThroughOAuth2RequestFlow() {
416418
when(spyService.defaultOAuth2UserService.loadUser(userRequest)).thenReturn(googleUser);
417419

418420
DSUserDetails mockUserDetails = mock(DSUserDetails.class);
419-
when(loginHelperService.userLoginHelper(any(User.class), any(Map.class))).thenReturn(mockUserDetails);
421+
when(loginHelperService.userLoginHelper(any(User.class), ArgumentMatchers.<Map<String, Object>>any())).thenReturn(mockUserDetails);
420422
when(userRepository.findByEmail("loadtest@gmail.com")).thenReturn(null);
421423
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
422424

@@ -425,7 +427,7 @@ void shouldLoadUserThroughOAuth2RequestFlow() {
425427

426428
// Then
427429
assertThat(result).isEqualTo(mockUserDetails);
428-
verify(loginHelperService).userLoginHelper(any(User.class), any(Map.class));
430+
verify(loginHelperService).userLoginHelper(any(User.class), ArgumentMatchers.<Map<String, Object>>any());
429431
verify(userRepository).save(any(User.class));
430432
}
431433
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import static org.assertj.core.api.Assertions.assertThatThrownBy;
55
import static org.mockito.ArgumentMatchers.any;
66
import static org.mockito.Mockito.*;
7+
import org.mockito.ArgumentMatchers;
78

89
import java.util.Arrays;
910
import java.util.List;
@@ -373,7 +374,7 @@ void shouldLoadUserThroughOidcRequestFlow() {
373374
user.setId(999L);
374375
return user;
375376
});
376-
when(loginHelperService.userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class), any(Map.class)))
377+
when(loginHelperService.userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class), ArgumentMatchers.<Map<String, Object>>any()))
377378
.thenAnswer(invocation -> {
378379
User user = invocation.getArgument(0);
379380
OidcUserInfo oidcUserInfo = invocation.getArgument(1);
@@ -396,7 +397,7 @@ void shouldLoadUserThroughOidcRequestFlow() {
396397
assertThat(dsUserDetails.getAttributes()).isNotEmpty();
397398

398399
verify(userRepository).save(any(User.class));
399-
verify(loginHelperService).userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class), any(Map.class));
400+
verify(loginHelperService).userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class), ArgumentMatchers.<Map<String, Object>>any());
400401
}
401402

402403
@Test

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
import org.junit.jupiter.api.DisplayName;
1212
import org.junit.jupiter.api.Nested;
1313
import org.junit.jupiter.api.Test;
14+
import org.junit.jupiter.api.extension.ExtendWith;
15+
import org.mockito.junit.jupiter.MockitoExtension;
1416
import org.springframework.security.core.authority.SimpleGrantedAuthority;
1517
import org.springframework.security.oauth2.core.oidc.OidcIdToken;
1618
import org.springframework.security.oauth2.core.oidc.OidcUserInfo;
@@ -23,6 +25,7 @@
2325
* <p>Verifies that {@code getAttributes()} satisfies the {@code OAuth2User} contract across all
2426
* constructor paths: OAuth2, OIDC, and local/password login.</p>
2527
*/
28+
@ExtendWith(MockitoExtension.class)
2629
@DisplayName("DSUserDetails Tests")
2730
class DSUserDetailsTest {
2831

@@ -83,6 +86,44 @@ void shouldReturnEmptyMapForMinimalUser() {
8386
assertThat(details.getAttributes()).isEmpty();
8487
}
8588

89+
@Test
90+
@DisplayName("Should build name from first name only without 'null' suffix")
91+
void shouldBuildNameFromFirstNameOnly() {
92+
User user = new User();
93+
user.setFirstName("Test");
94+
95+
DSUserDetails details = new DSUserDetails(user);
96+
97+
assertThat(details.getAttributes()).containsEntry("given_name", "Test");
98+
assertThat(details.getAttributes()).containsEntry("name", "Test");
99+
assertThat((String) details.getAttributes().get("name")).doesNotContain("null");
100+
}
101+
102+
@Test
103+
@DisplayName("Should build name from last name only without 'null' prefix")
104+
void shouldBuildNameFromLastNameOnly() {
105+
User user = new User();
106+
user.setLastName("User");
107+
108+
DSUserDetails details = new DSUserDetails(user);
109+
110+
assertThat(details.getAttributes()).containsEntry("family_name", "User");
111+
assertThat(details.getAttributes()).containsEntry("name", "User");
112+
assertThat((String) details.getAttributes().get("name")).doesNotContain("null");
113+
}
114+
115+
@Test
116+
@DisplayName("Should return an unmodifiable map from getAttributes()")
117+
void shouldReturnUnmodifiableAttributes() {
118+
Map<String, Object> providerAttrs = new HashMap<>();
119+
providerAttrs.put("email", "test@example.com");
120+
121+
DSUserDetails details = new DSUserDetails(testUser,
122+
List.of(new SimpleGrantedAuthority("ROLE_USER")), providerAttrs);
123+
124+
assertThat(details.getAttributes()).isUnmodifiable();
125+
}
126+
86127
@Test
87128
@DisplayName("getAttribute('email') should return email for OAuth2 user")
88129
void shouldSupportGetAttributePattern() {

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

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.mockito.ArgumentMatchers.any;
5+
import static org.mockito.Mockito.doReturn;
56
import static org.mockito.Mockito.verify;
67
import static org.mockito.Mockito.when;
78
import java.time.Instant;
@@ -96,7 +97,7 @@ void shouldUpdateLastActivityDate() {
9697
// Given
9798
Date beforeLogin = testUser.getLastActivityDate();
9899
when(loginAttemptService.checkIfUserShouldBeUnlocked(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
99-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
100+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
100101

101102
// When
102103
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -122,7 +123,7 @@ void shouldCheckUserUnlockStatus() {
122123
unlockedUser.setLockedDate(null);
123124

124125
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(unlockedUser);
125-
when(authorityService.getAuthoritiesFromUser(unlockedUser)).thenReturn((Collection) testAuthorities);
126+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(unlockedUser);
126127

127128
// When
128129
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -138,7 +139,7 @@ void shouldCheckUserUnlockStatus() {
138139
void shouldCreateUserDetailsWithAuthorities() {
139140
// Given
140141
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
141-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
142+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
142143

143144
// When
144145
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -178,7 +179,7 @@ void shouldHandleLockedUserThatRemainsLocked() {
178179
testUser.setLockedDate(lockedDate);
179180

180181
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); // User remains locked
181-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
182+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
182183

183184
// When
184185
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -196,7 +197,7 @@ void shouldHandleDisabledUser() {
196197
// Given
197198
testUser.setEnabled(false);
198199
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
199-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
200+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
200201

201202
// When
202203
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -217,7 +218,7 @@ void shouldPreserveUserStateDuringProcess() {
217218
// Note: imageUrl and usingMfa fields don't exist in User class
218219

219220
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
220-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
221+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
221222

222223
// When
223224
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -253,7 +254,7 @@ void shouldAutomaticallyUnlockUserAfterDuration() {
253254
unlockedUser.setEnabled(true);
254255

255256
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(unlockedUser);
256-
when(authorityService.getAuthoritiesFromUser(unlockedUser)).thenReturn((Collection) testAuthorities);
257+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(unlockedUser);
257258

258259
// When
259260
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -271,7 +272,7 @@ void shouldTrackTimingOfLastActivityUpdate() {
271272
// Given
272273
Date testStartTime = new Date();
273274
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
274-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
275+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
275276

276277
// When
277278
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -359,7 +360,7 @@ void shouldCreateCompleteUserDetails() {
359360
testUser.setLastName("User");
360361

361362
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
362-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
363+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
363364

364365
// When
365366
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -383,7 +384,7 @@ void shouldHandleOAuth2User() {
383384
testUser.setPassword(null); // OAuth2 users don't have passwords
384385

385386
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
386-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
387+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
387388

388389
// When
389390
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -407,7 +408,7 @@ void shouldHandleNullLastActivityDate() {
407408
testUser.setLastActivityDate(null);
408409

409410
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
410-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
411+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
411412

412413
// When
413414
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -422,7 +423,7 @@ void shouldHandleNullLastActivityDate() {
422423
void shouldHandleRapidSuccessiveLogins() {
423424
// Given
424425
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
425-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
426+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
426427

427428
// When - Simulate rapid successive logins
428429
DSUserDetails result1 = loginHelperService.userLoginHelper(testUser);
@@ -459,7 +460,7 @@ void shouldPassOAuth2AttributesToDSUserDetails() {
459460
providerAttrs.put("picture", "https://example.com/photo.jpg");
460461

461462
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
462-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
463+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
463464

464465
// When
465466
DSUserDetails result = loginHelperService.userLoginHelper(testUser, providerAttrs);
@@ -477,7 +478,7 @@ void shouldPassOAuth2AttributesToDSUserDetails() {
477478
void shouldFallBackWhenOAuth2AttributesNull() {
478479
// Given
479480
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
480-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
481+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
481482

482483
// When
483484
DSUserDetails result = loginHelperService.userLoginHelper(testUser);
@@ -503,7 +504,7 @@ void shouldPassOidcAttributesToDSUserDetails() {
503504
providerAttrs.put("extra_claim", "extra_value");
504505

505506
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
506-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
507+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
507508

508509
// When
509510
DSUserDetails result = loginHelperService.userLoginHelper(testUser, userInfo, idToken, providerAttrs);
@@ -528,7 +529,7 @@ void shouldFallBackToIdTokenClaimsWhenOidcAttributesNull() {
528529
OidcUserInfo userInfo = new OidcUserInfo(Map.of("sub", "oidc-sub-123"));
529530

530531
when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser);
531-
when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
532+
doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser);
532533

533534
// When
534535
DSUserDetails result = loginHelperService.userLoginHelper(testUser, userInfo, idToken);

0 commit comments

Comments
 (0)