diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java index ce0ca20..6831f36 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java @@ -219,7 +219,7 @@ public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2Authentic String registrationId = userRequest.getClientRegistration().getRegistrationId(); log.debug("registrationId: {}", registrationId); User dbUser = handleOAuthLoginSuccess(registrationId, user); - DSUserDetails userDetails = loginHelperService.userLoginHelper(dbUser); + DSUserDetails userDetails = loginHelperService.userLoginHelper(dbUser, user.getAttributes()); return userDetails; } diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java index ad1bd12..2c7b0d6 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java @@ -198,6 +198,6 @@ public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2Authenticatio String registrationId = userRequest.getClientRegistration().getRegistrationId(); log.debug("registrationId: {}", registrationId); User dbUser = handleOidcLoginSuccess(registrationId, user); - return loginHelperService.userLoginHelper(dbUser, user.getUserInfo(), user.getIdToken()); + return loginHelperService.userLoginHelper(dbUser, user.getUserInfo(), user.getIdToken(), user.getAttributes()); } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetails.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetails.java index 222d9e3..eb3efec 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetails.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetails.java @@ -2,6 +2,7 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import org.springframework.security.core.GrantedAuthority; @@ -60,15 +61,27 @@ public class DSUserDetails implements UserDetails, OidcUser { private OidcIdToken oidcIdToken; /** - * Instantiates a new DS user details. + * Instantiates a new DS user details with OAuth2 provider attributes. * * @param user the user * @param grantedAuthorities the granted authorities (optional, default = empty list) + * @param attributes the OAuth2 provider attributes (optional, falls back to User entity fields) */ - public DSUserDetails(User user, Collection grantedAuthorities) { + public DSUserDetails(User user, Collection grantedAuthorities, Map attributes) { this.user = user; this.grantedAuthorities = grantedAuthorities != null ? grantedAuthorities : new ArrayList<>(); - this.attributes = new HashMap<>(); + this.attributes = attributes != null ? new HashMap<>(attributes) : buildFallbackAttributes(user); + } + + /** + * Instantiates a new DS user details without provider attributes. Attributes will be populated + * from the {@link User} entity fields as a fallback. + * + * @param user the user + * @param grantedAuthorities the granted authorities (optional, default = empty list) + */ + public DSUserDetails(User user, Collection grantedAuthorities) { + this(user, grantedAuthorities, (Map) null); } /** @@ -77,35 +90,91 @@ public DSUserDetails(User user, Collection grantedAu * @param user the user */ public DSUserDetails(User user) { - this(user, null); + this(user, null, (Map) null); } /** - * Instantiates a new DS user details. + * Instantiates a new DS user details with OIDC tokens and provider attributes. * * @param user the user * @param oidcUserInfo containing claims about the user * @param oidcIdToken containing claims about the user * @param grantedAuthorities the granted authorities (optional, default = empty list) + * @param attributes the OAuth2/OIDC provider attributes (optional, falls back to idToken claims or User entity) */ @Builder - public DSUserDetails(User user, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken, Collection grantedAuthorities) { + public DSUserDetails(User user, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken, + Collection grantedAuthorities, Map attributes) { this.user = user; this.oidcUserInfo = oidcUserInfo; this.oidcIdToken = oidcIdToken; this.grantedAuthorities = grantedAuthorities != null ? grantedAuthorities : new ArrayList<>(); + if (attributes != null) { + this.attributes = new HashMap<>(attributes); + } else if (oidcIdToken != null) { + this.attributes = new HashMap<>(oidcIdToken.getClaims()); + } else { + this.attributes = buildFallbackAttributes(user); + } } /** - * Instantiates a new DS user details. + * Instantiates a new DS user details with OIDC tokens. Attributes will be populated from the + * OIDC ID token claims or {@link User} entity as a fallback. + * + * @param user the user + * @param oidcUserInfo containing claims about the user + * @param oidcIdToken containing claims about the user + * @param grantedAuthorities the granted authorities (optional, default = empty list) + */ + public DSUserDetails(User user, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken, + Collection grantedAuthorities) { + this(user, oidcUserInfo, oidcIdToken, grantedAuthorities, null); + } + + /** + * Instantiates a new DS user details with OIDC tokens and no authorities. * * @param user the user * @param oidcUserInfo containing claims about the user * @param oidcIdToken containing claims about the user */ - @Builder public DSUserDetails(User user, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken) { - this(user, oidcUserInfo, oidcIdToken, null); + this(user, oidcUserInfo, oidcIdToken, null, null); + } + + /** + * Builds a fallback attributes map from the {@link User} entity fields. Used when no provider + * attributes are available (e.g., local/password login). + * + * @param user the user entity + * @return a map containing available user fields using standard OAuth2/OIDC claim names + */ + private static Map buildFallbackAttributes(User user) { + Map attrs = new HashMap<>(); + if (user.getEmail() != null) { + attrs.put("email", user.getEmail()); + } + if (user.getFirstName() != null) { + attrs.put("given_name", user.getFirstName()); + } + if (user.getLastName() != null) { + attrs.put("family_name", user.getLastName()); + } + StringBuilder name = new StringBuilder(); + if (user.getFirstName() != null && !user.getFirstName().trim().isEmpty()) { + name.append(user.getFirstName().trim()); + } + if (user.getLastName() != null && !user.getLastName().trim().isEmpty()) { + if (name.length() > 0) { + name.append(' '); + } + name.append(user.getLastName().trim()); + } + if (name.length() > 0) { + attrs.put("name", name.toString()); + } + return attrs; } /** @@ -189,7 +258,7 @@ public User getUser() { @Override public Map getAttributes() { - return attributes; + return Collections.unmodifiableMap(attributes); } @Override diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/LoginHelperService.java b/src/main/java/com/digitalsanctuary/spring/user/service/LoginHelperService.java index 89420f0..c85c809 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/LoginHelperService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/LoginHelperService.java @@ -2,6 +2,7 @@ import java.util.Collection; import java.util.Date; +import java.util.Map; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.oauth2.core.oidc.OidcIdToken; import org.springframework.security.oauth2.core.oidc.OidcUserInfo; @@ -34,13 +35,25 @@ public class LoginHelperService { private final AuthorityService authorityService; /** - * Helper method to authenticate a user after login. This method is called from the DSUserDetailsService and DSOAuth2UserService classes after a - * user has been successfully authenticated. + * Helper method to authenticate a user after login. This method is called from the DSUserDetailsService after a user has been successfully + * authenticated via local/password login. Attributes are populated from the {@link User} entity as a fallback. * * @param dbUser The user to authenticate. * @return The user details object. */ public DSUserDetails userLoginHelper(User dbUser) { + return userLoginHelper(dbUser, (Map) null); + } + + /** + * Helper method to authenticate a user after OAuth2 login, preserving the original provider attributes so that + * {@link DSUserDetails#getAttributes()} returns the full set of attributes from the OAuth2 provider. + * + * @param dbUser The user to authenticate. + * @param attributes The OAuth2 provider attributes (may be null for local login fallback). + * @return The user details object with provider attributes set. + */ + public DSUserDetails userLoginHelper(User dbUser, Map attributes) { // Updating lastActivity date for this login dbUser.setLastActivityDate(new Date()); @@ -48,13 +61,13 @@ public DSUserDetails userLoginHelper(User dbUser) { dbUser = loginAttemptService.checkIfUserShouldBeUnlocked(dbUser); Collection authorities = authorityService.getAuthoritiesFromUser(dbUser); - DSUserDetails userDetails = new DSUserDetails(dbUser, authorities); - return userDetails; + return new DSUserDetails(dbUser, authorities, attributes); } /** * Helper method to authenticate an OIDC user after login, attaching the OIDC-specific tokens - * and claims to the principal while keeping {@link DSUserDetails} immutable. + * and claims to the principal while keeping {@link DSUserDetails} immutable. Attributes are + * populated from the OIDC ID token claims as a fallback. * * @param dbUser The user to authenticate. * @param oidcUserInfo The OIDC user info claims. @@ -62,6 +75,21 @@ public DSUserDetails userLoginHelper(User dbUser) { * @return The user details object with OIDC tokens set. */ public DSUserDetails userLoginHelper(User dbUser, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken) { + return userLoginHelper(dbUser, oidcUserInfo, oidcIdToken, null); + } + + /** + * Helper method to authenticate an OIDC user after login, preserving the original provider attributes and + * attaching the OIDC-specific tokens and claims to the principal. + * + * @param dbUser The user to authenticate. + * @param oidcUserInfo The OIDC user info claims. + * @param oidcIdToken The OIDC ID token. + * @param attributes The OIDC provider attributes (may be null to fall back to idToken claims). + * @return The user details object with OIDC tokens and provider attributes set. + */ + public DSUserDetails userLoginHelper(User dbUser, OidcUserInfo oidcUserInfo, OidcIdToken oidcIdToken, + Map attributes) { // Updating lastActivity date for this login dbUser.setLastActivityDate(new Date()); @@ -69,6 +97,6 @@ public DSUserDetails userLoginHelper(User dbUser, OidcUserInfo oidcUserInfo, Oid dbUser = loginAttemptService.checkIfUserShouldBeUnlocked(dbUser); Collection authorities = authorityService.getAuthoritiesFromUser(dbUser); - return new DSUserDetails(dbUser, oidcUserInfo, oidcIdToken, authorities); + return new DSUserDetails(dbUser, oidcUserInfo, oidcIdToken, authorities, attributes); } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java index 1c839eb..85dcf51 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java @@ -5,6 +5,8 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import org.mockito.ArgumentMatchers; + import static org.mockito.Mockito.*; import java.util.Arrays; @@ -416,7 +418,7 @@ void shouldLoadUserThroughOAuth2RequestFlow() { when(spyService.defaultOAuth2UserService.loadUser(userRequest)).thenReturn(googleUser); DSUserDetails mockUserDetails = mock(DSUserDetails.class); - when(loginHelperService.userLoginHelper(any(User.class))).thenReturn(mockUserDetails); + when(loginHelperService.userLoginHelper(any(User.class), ArgumentMatchers.>any())).thenReturn(mockUserDetails); when(userRepository.findByEmail("loadtest@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); @@ -425,7 +427,7 @@ void shouldLoadUserThroughOAuth2RequestFlow() { // Then assertThat(result).isEqualTo(mockUserDetails); - verify(loginHelperService).userLoginHelper(any(User.class)); + verify(loginHelperService).userLoginHelper(any(User.class), ArgumentMatchers.>any()); verify(userRepository).save(any(User.class)); } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java index 1b9ee2b..65f90f5 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java @@ -4,9 +4,11 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; +import org.mockito.ArgumentMatchers; import java.util.Arrays; import java.util.List; +import java.util.Map; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; @@ -372,13 +374,14 @@ void shouldLoadUserThroughOidcRequestFlow() { user.setId(999L); return user; }); - when(loginHelperService.userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class))) + when(loginHelperService.userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class), ArgumentMatchers.>any())) .thenAnswer(invocation -> { User user = invocation.getArgument(0); OidcUserInfo oidcUserInfo = invocation.getArgument(1); OidcIdToken oidcIdToken = invocation.getArgument(2); + Map attributes = invocation.getArgument(3); return new DSUserDetails(user, oidcUserInfo, oidcIdToken, - List.of(new SimpleGrantedAuthority("ROLE_USER"))); + List.of(new SimpleGrantedAuthority("ROLE_USER")), attributes); }); // When @@ -391,9 +394,10 @@ void shouldLoadUserThroughOidcRequestFlow() { assertThat(dsUserDetails.getIdToken()).isNotNull(); assertThat(dsUserDetails.getUserInfo()).isNotNull(); assertThat(dsUserDetails.getAuthorities()).isNotEmpty(); + assertThat(dsUserDetails.getAttributes()).isNotEmpty(); verify(userRepository).save(any(User.class)); - verify(loginHelperService).userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class)); + verify(loginHelperService).userLoginHelper(any(User.class), any(OidcUserInfo.class), any(OidcIdToken.class), ArgumentMatchers.>any()); } @Test diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsTest.java new file mode 100644 index 0000000..df125df --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsTest.java @@ -0,0 +1,285 @@ +package com.digitalsanctuary.spring.user.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.time.Instant; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.oauth2.core.oidc.OidcIdToken; +import org.springframework.security.oauth2.core.oidc.OidcUserInfo; + +import com.digitalsanctuary.spring.user.persistence.model.User; + +/** + * Unit tests for {@link DSUserDetails} attribute handling. + * + *

Verifies that {@code getAttributes()} satisfies the {@code OAuth2User} contract across all + * constructor paths: OAuth2, OIDC, and local/password login.

+ */ +@ExtendWith(MockitoExtension.class) +@DisplayName("DSUserDetails Tests") +class DSUserDetailsTest { + + private User testUser; + + @BeforeEach + void setUp() { + testUser = new User(); + testUser.setEmail("test@example.com"); + testUser.setFirstName("Test"); + testUser.setLastName("User"); + } + + @Nested + @DisplayName("Simple Constructor (OAuth2 / Local Login)") + class SimpleConstructorTests { + + @Test + @DisplayName("Should return provider attributes when provided") + void shouldReturnProviderAttributesWhenProvided() { + Map providerAttrs = new HashMap<>(); + providerAttrs.put("email", "test@example.com"); + providerAttrs.put("given_name", "Test"); + providerAttrs.put("family_name", "User"); + providerAttrs.put("picture", "https://example.com/photo.jpg"); + providerAttrs.put("sub", "123456789"); + + DSUserDetails details = new DSUserDetails(testUser, + List.of(new SimpleGrantedAuthority("ROLE_USER")), providerAttrs); + + assertThat(details.getAttributes()).isNotNull(); + assertThat(details.getAttributes()).containsEntry("email", "test@example.com"); + assertThat(details.getAttributes()).containsEntry("picture", "https://example.com/photo.jpg"); + assertThat(details.getAttributes()).containsEntry("sub", "123456789"); + } + + @Test + @DisplayName("Should build fallback attributes from User entity when attributes are null") + void shouldBuildFallbackAttributesWhenNull() { + DSUserDetails details = new DSUserDetails(testUser, + List.of(new SimpleGrantedAuthority("ROLE_USER"))); + + assertThat(details.getAttributes()).isNotNull(); + assertThat(details.getAttributes()).containsEntry("email", "test@example.com"); + assertThat(details.getAttributes()).containsEntry("given_name", "Test"); + assertThat(details.getAttributes()).containsEntry("family_name", "User"); + assertThat(details.getAttributes()).containsEntry("name", "Test User"); + } + + @Test + @DisplayName("Should return empty map for User with no fields set") + void shouldReturnEmptyMapForMinimalUser() { + User minimalUser = new User(); + + DSUserDetails details = new DSUserDetails(minimalUser); + + assertThat(details.getAttributes()).isNotNull(); + assertThat(details.getAttributes()).isEmpty(); + } + + @Test + @DisplayName("Should build name from first name only without 'null' suffix") + void shouldBuildNameFromFirstNameOnly() { + User user = new User(); + user.setFirstName("Test"); + + DSUserDetails details = new DSUserDetails(user); + + assertThat(details.getAttributes()).containsEntry("given_name", "Test"); + assertThat(details.getAttributes()).containsEntry("name", "Test"); + assertThat((String) details.getAttributes().get("name")).doesNotContain("null"); + } + + @Test + @DisplayName("Should build name from last name only without 'null' prefix") + void shouldBuildNameFromLastNameOnly() { + User user = new User(); + user.setLastName("User"); + + DSUserDetails details = new DSUserDetails(user); + + assertThat(details.getAttributes()).containsEntry("family_name", "User"); + assertThat(details.getAttributes()).containsEntry("name", "User"); + assertThat((String) details.getAttributes().get("name")).doesNotContain("null"); + } + + @Test + @DisplayName("Should return an unmodifiable map from getAttributes()") + void shouldReturnUnmodifiableAttributes() { + Map providerAttrs = new HashMap<>(); + providerAttrs.put("email", "test@example.com"); + + DSUserDetails details = new DSUserDetails(testUser, + List.of(new SimpleGrantedAuthority("ROLE_USER")), providerAttrs); + + assertThat(details.getAttributes()).isUnmodifiable(); + } + + @Test + @DisplayName("getAttribute('email') should return email for OAuth2 user") + void shouldSupportGetAttributePattern() { + Map providerAttrs = Map.of( + "email", "oauth@example.com", + "sub", "abc123" + ); + + DSUserDetails details = new DSUserDetails(testUser, + List.of(new SimpleGrantedAuthority("ROLE_USER")), providerAttrs); + + // This is the standard OAuth2User pattern that was broken before the fix + assertThat((String) details.getAttribute("email")).isEqualTo("oauth@example.com"); + } + + @Test + @DisplayName("Should defensively copy provided attributes") + void shouldDefensivelyCopyAttributes() { + Map providerAttrs = new HashMap<>(); + providerAttrs.put("email", "test@example.com"); + + DSUserDetails details = new DSUserDetails(testUser, null, providerAttrs); + + // Modifying the original map should not affect DSUserDetails + providerAttrs.put("injected", "value"); + assertThat(details.getAttributes()).doesNotContainKey("injected"); + } + } + + @Nested + @DisplayName("OIDC Constructor") + class OidcConstructorTests { + + private OidcIdToken idToken; + private OidcUserInfo userInfo; + + @BeforeEach + void setUp() { + Map tokenClaims = new HashMap<>(); + tokenClaims.put("sub", "f:12345678:testuser"); + tokenClaims.put("email", "oidc@example.com"); + tokenClaims.put("email_verified", true); + tokenClaims.put("given_name", "Test"); + tokenClaims.put("family_name", "User"); + tokenClaims.put("iss", "https://idp.example.com"); + + idToken = new OidcIdToken("token-value", Instant.now(), Instant.now().plusSeconds(3600), tokenClaims); + + Map infoClaims = new HashMap<>(); + infoClaims.put("sub", "f:12345678:testuser"); + infoClaims.put("email", "oidc@example.com"); + userInfo = new OidcUserInfo(infoClaims); + } + + @Test + @DisplayName("Should return provider attributes when provided to OIDC constructor") + void shouldReturnProviderAttributesWhenProvided() { + Map providerAttrs = new HashMap<>(); + providerAttrs.put("email", "oidc@example.com"); + providerAttrs.put("custom_claim", "custom_value"); + + DSUserDetails details = new DSUserDetails(testUser, userInfo, idToken, + List.of(new SimpleGrantedAuthority("ROLE_USER")), providerAttrs); + + assertThat(details.getAttributes()).isNotNull(); + assertThat(details.getAttributes()).containsEntry("email", "oidc@example.com"); + assertThat(details.getAttributes()).containsEntry("custom_claim", "custom_value"); + } + + @Test + @DisplayName("Should fall back to idToken claims when attributes are null") + void shouldFallBackToIdTokenClaimsWhenAttributesNull() { + DSUserDetails details = new DSUserDetails(testUser, userInfo, idToken, + List.of(new SimpleGrantedAuthority("ROLE_USER"))); + + assertThat(details.getAttributes()).isNotNull(); + assertThat(details.getAttributes()).containsEntry("sub", "f:12345678:testuser"); + assertThat(details.getAttributes()).containsEntry("email", "oidc@example.com"); + assertThat(details.getAttributes()).containsEntry("email_verified", true); + assertThat(details.getAttributes()).containsEntry("iss", "https://idp.example.com"); + } + + @Test + @DisplayName("Should fall back to User entity when both attributes and idToken are null") + void shouldFallBackToUserEntityWhenBothNull() { + DSUserDetails details = new DSUserDetails(testUser, userInfo, null, + List.of(new SimpleGrantedAuthority("ROLE_USER"))); + + assertThat(details.getAttributes()).isNotNull(); + assertThat(details.getAttributes()).containsEntry("email", "test@example.com"); + assertThat(details.getAttributes()).containsEntry("given_name", "Test"); + } + + @Test + @DisplayName("Should never return null from getAttributes() for OIDC path") + void shouldNeverReturnNullForOidcPath() { + // All-null OIDC constructor — was previously returning null (NPE risk) + DSUserDetails details = new DSUserDetails(testUser, (OidcUserInfo) null, (OidcIdToken) null); + + assertThat(details.getAttributes()).isNotNull(); + } + + @Test + @DisplayName("getAttribute('email') should work for OIDC user") + void shouldSupportGetAttributePatternForOidc() { + Map providerAttrs = Map.of("email", "oidc@example.com", "sub", "test-sub"); + + DSUserDetails details = new DSUserDetails(testUser, userInfo, idToken, + List.of(new SimpleGrantedAuthority("ROLE_USER")), providerAttrs); + + assertThat((String) details.getAttribute("email")).isEqualTo("oidc@example.com"); + } + + @Test + @DisplayName("Should preserve OIDC tokens alongside attributes") + void shouldPreserveOidcTokensAlongsideAttributes() { + Map providerAttrs = Map.of("email", "oidc@example.com"); + + DSUserDetails details = new DSUserDetails(testUser, userInfo, idToken, + List.of(new SimpleGrantedAuthority("ROLE_USER")), providerAttrs); + + assertThat(details.getIdToken()).isEqualTo(idToken); + assertThat(details.getUserInfo()).isEqualTo(userInfo); + assertThat(details.getAttributes()).containsEntry("email", "oidc@example.com"); + } + } + + @Nested + @DisplayName("Builder") + class BuilderTests { + + @Test + @DisplayName("Builder should accept attributes parameter") + void shouldAcceptAttributesViaBuilder() { + Map providerAttrs = Map.of("email", "builder@example.com", "sub", "builder-sub"); + + DSUserDetails details = DSUserDetails.builder() + .user(testUser) + .attributes(providerAttrs) + .grantedAuthorities(List.of(new SimpleGrantedAuthority("ROLE_USER"))) + .build(); + + assertThat(details.getAttributes()).containsEntry("email", "builder@example.com"); + assertThat(details.getAttributes()).containsEntry("sub", "builder-sub"); + } + + @Test + @DisplayName("Builder without attributes should use fallback") + void shouldUseFallbackWhenBuilderOmitsAttributes() { + DSUserDetails details = DSUserDetails.builder() + .user(testUser) + .grantedAuthorities(List.of(new SimpleGrantedAuthority("ROLE_USER"))) + .build(); + + assertThat(details.getAttributes()).isNotNull(); + assertThat(details.getAttributes()).containsEntry("email", "test@example.com"); + } + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/LoginHelperServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/LoginHelperServiceTest.java index 919f454..73de706 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/LoginHelperServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/LoginHelperServiceTest.java @@ -2,12 +2,16 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.time.Instant; import java.util.Collection; import java.util.Collections; import java.util.Date; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; @@ -19,6 +23,8 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.oauth2.core.oidc.OidcIdToken; +import org.springframework.security.oauth2.core.oidc.OidcUserInfo; import com.digitalsanctuary.spring.user.persistence.model.Privilege; import com.digitalsanctuary.spring.user.persistence.model.Role; import com.digitalsanctuary.spring.user.persistence.model.User; @@ -91,7 +97,7 @@ void shouldUpdateLastActivityDate() { // Given Date beforeLogin = testUser.getLastActivityDate(); when(loginAttemptService.checkIfUserShouldBeUnlocked(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -117,7 +123,7 @@ void shouldCheckUserUnlockStatus() { unlockedUser.setLockedDate(null); when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(unlockedUser); - when(authorityService.getAuthoritiesFromUser(unlockedUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(unlockedUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -133,7 +139,7 @@ void shouldCheckUserUnlockStatus() { void shouldCreateUserDetailsWithAuthorities() { // Given when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -173,7 +179,7 @@ void shouldHandleLockedUserThatRemainsLocked() { testUser.setLockedDate(lockedDate); when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); // User remains locked - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -191,7 +197,7 @@ void shouldHandleDisabledUser() { // Given testUser.setEnabled(false); when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -212,7 +218,7 @@ void shouldPreserveUserStateDuringProcess() { // Note: imageUrl and usingMfa fields don't exist in User class when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -248,7 +254,7 @@ void shouldAutomaticallyUnlockUserAfterDuration() { unlockedUser.setEnabled(true); when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(unlockedUser); - when(authorityService.getAuthoritiesFromUser(unlockedUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(unlockedUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -266,7 +272,7 @@ void shouldTrackTimingOfLastActivityUpdate() { // Given Date testStartTime = new Date(); when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -354,7 +360,7 @@ void shouldCreateCompleteUserDetails() { testUser.setLastName("User"); when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -378,7 +384,7 @@ void shouldHandleOAuth2User() { testUser.setPassword(null); // OAuth2 users don't have passwords when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -402,7 +408,7 @@ void shouldHandleNullLastActivityDate() { testUser.setLastActivityDate(null); when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When DSUserDetails result = loginHelperService.userLoginHelper(testUser); @@ -417,7 +423,7 @@ void shouldHandleNullLastActivityDate() { void shouldHandleRapidSuccessiveLogins() { // Given when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); // When - Simulate rapid successive logins DSUserDetails result1 = loginHelperService.userLoginHelper(testUser); @@ -439,4 +445,99 @@ void shouldHandleRapidSuccessiveLogins() { assertThat(secondLoginTime).isAfter(firstLoginTime); } } + + @Nested + @DisplayName("Attribute Passthrough Tests") + class AttributePassthroughTests { + + @Test + @DisplayName("Should pass OAuth2 provider attributes to DSUserDetails") + void shouldPassOAuth2AttributesToDSUserDetails() { + // Given + Map providerAttrs = new HashMap<>(); + providerAttrs.put("email", "test@example.com"); + providerAttrs.put("sub", "123456789"); + providerAttrs.put("picture", "https://example.com/photo.jpg"); + + when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); + + // When + DSUserDetails result = loginHelperService.userLoginHelper(testUser, providerAttrs); + + // Then + assertThat(result.getAttributes()).isNotNull(); + assertThat(result.getAttributes()).containsEntry("email", "test@example.com"); + assertThat(result.getAttributes()).containsEntry("sub", "123456789"); + assertThat(result.getAttributes()).containsEntry("picture", "https://example.com/photo.jpg"); + assertThat((String) result.getAttribute("email")).isEqualTo("test@example.com"); + } + + @Test + @DisplayName("Should fall back to User entity attributes when OAuth2 attributes are null") + void shouldFallBackWhenOAuth2AttributesNull() { + // Given + when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); + + // When + DSUserDetails result = loginHelperService.userLoginHelper(testUser); + + // Then + assertThat(result.getAttributes()).isNotNull(); + assertThat(result.getAttributes()).containsEntry("email", "test@example.com"); + } + + @Test + @DisplayName("Should pass OIDC provider attributes to DSUserDetails") + void shouldPassOidcAttributesToDSUserDetails() { + // Given + Map tokenClaims = new HashMap<>(); + tokenClaims.put("sub", "oidc-sub-123"); + tokenClaims.put("email", "oidc@example.com"); + tokenClaims.put("iss", "https://idp.example.com"); + + OidcIdToken idToken = new OidcIdToken("token", Instant.now(), Instant.now().plusSeconds(3600), tokenClaims); + OidcUserInfo userInfo = new OidcUserInfo(Map.of("sub", "oidc-sub-123", "email", "oidc@example.com")); + + Map providerAttrs = new HashMap<>(tokenClaims); + providerAttrs.put("extra_claim", "extra_value"); + + when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); + + // When + DSUserDetails result = loginHelperService.userLoginHelper(testUser, userInfo, idToken, providerAttrs); + + // Then + assertThat(result.getAttributes()).isNotNull(); + assertThat(result.getAttributes()).containsEntry("email", "oidc@example.com"); + assertThat(result.getAttributes()).containsEntry("extra_claim", "extra_value"); + assertThat(result.getIdToken()).isEqualTo(idToken); + assertThat(result.getUserInfo()).isEqualTo(userInfo); + } + + @Test + @DisplayName("Should fall back to idToken claims when OIDC attributes are null") + void shouldFallBackToIdTokenClaimsWhenOidcAttributesNull() { + // Given + Map tokenClaims = new HashMap<>(); + tokenClaims.put("sub", "oidc-sub-123"); + tokenClaims.put("email", "oidc@example.com"); + + OidcIdToken idToken = new OidcIdToken("token", Instant.now(), Instant.now().plusSeconds(3600), tokenClaims); + OidcUserInfo userInfo = new OidcUserInfo(Map.of("sub", "oidc-sub-123")); + + when(loginAttemptService.checkIfUserShouldBeUnlocked(testUser)).thenReturn(testUser); + doReturn(testAuthorities).when(authorityService).getAuthoritiesFromUser(testUser); + + // When + DSUserDetails result = loginHelperService.userLoginHelper(testUser, userInfo, idToken); + + // Then + assertThat(result.getAttributes()).isNotNull(); + assertThat(result.getAttributes()).containsEntry("sub", "oidc-sub-123"); + assertThat(result.getAttributes()).containsEntry("email", "oidc@example.com"); + } + } }