fix(oauth2): populate DSUserDetails.getAttributes() with provider attributes

[devondragon]

Summary

Fixes #284. DSUserDetails.getAttributes() now returns the actual OAuth2/OIDC provider attributes instead of an empty map (OAuth2 path) or null (OIDC path).

• DSUserDetails: All constructors now properly initialize attributes. New buildFallbackAttributes() maps User entity fields to standard OAuth2 claim names when no provider attributes are available (local login). OIDC constructor falls back to idToken claims when explicit attributes aren't provided. getAttributes() is never null.
• LoginHelperService: New overloads accept Map<String, Object> attributes for both OAuth2 and OIDC paths. Original overloads preserved for backward compatibility (local login).
• DSOAuth2UserService: loadUser() passes original OAuth2User.getAttributes() through to LoginHelperService.
• DSOidcUserService: loadUser() passes original OidcUser.getAttributes() through to LoginHelperService.

Test plan

• New DSUserDetailsTest (13 tests) covers all constructor paths, fallback behavior, defensive copying, builder, and the getAttribute("email") pattern
• LoginHelperServiceTest — 4 new tests for attribute passthrough in OAuth2 and OIDC paths
• Updated DSOAuth2UserServiceTest and DSOidcUserServiceTest mock stubs for new method signatures
• All 630+ existing tests pass (./gradlew test)
• ./gradlew check clean

[claude]

PR Review: fix(oauth2): populate DSUserDetails.getAttributes() with provider attributes

This is a well-structured fix for a real bug. The layered fallback strategy (provider attrs → idToken claims → User entity fields) is sound, backward compatibility is preserved throughout, and the test coverage is solid. A few things worth considering before merge:

---

Issues

1. buildFallbackAttributes() may produce a malformed name claim

When only firstName or lastName is set (not both), user.getFullName() is still called and could produce values like "Test null" or " User" depending on the User.getFullName() implementation:

if (user.getFirstName() != null || user.getLastName() != null) {
    attrs.put("name", user.getFullName()); // ← getFullName() may concatenate a null half
}

Recommend checking what getFullName() returns when one name is null, or building it manually with a guard:

String fullName = Stream.of(user.getFirstName(), user.getLastName())
    .filter(Objects::nonNull)
    .collect(Collectors.joining(" "));
if (!fullName.isEmpty()) {
    attrs.put("name", fullName);
}

There's currently no test covering the "first name only" or "last name only" scenario.

---

2. getAttributes() returns a mutable internal map

The constructor defensively copies the input (new HashMap<>(attributes)) — good. But if getAttributes() returns the internal HashMap directly, callers can mutate it:

details.getAttributes().put("injected", "malicious");  // modifies internal state

The OAuth2User interface doesn't require immutability, but returning Collections.unmodifiableMap(this.attributes) would be safer given these attributes may influence security decisions downstream.

---

3. Raw type warning in test matchers

when(loginHelperService.userLoginHelper(any(User.class), any(Map.class)))  // raw Map.class
verify(loginHelperService).userLoginHelper(any(User.class), any(Map.class))

Prefer the type-safe form to suppress the unchecked warning:

any(User.class), ArgumentMatchers.<Map<String, Object>>any()

Same applies in DSOidcUserServiceTest.

---

4. DSUserDetailsTest skips the project's custom test annotations

Per CLAUDE.md conventions, service-layer unit tests should use @ServiceTest (from com.digitalsanctuary.spring.user.test.annotations). DSUserDetailsTest works fine without Spring context — but for consistency and to pick up BaseTestConfiguration (fixed Clock, etc.), consider adding @ServiceTest or at minimum @ExtendWith(MockitoExtension.class).

---

Minor notes

• The OIDC fallback to oidcIdToken.getClaims() inside DSUserDetails is a useful safety net for direct construction, but in practice it will never be reached when called from DSOidcUserService (since OidcUser.getAttributes() is never null in Spring Security). Worth a brief comment noting this is intentional for direct-construction callers.
• The unchecked (Collection) cast in LoginHelperServiceTest ((Collection) testAuthorities) is pre-existing, but since you're touching this file, it's an easy cleanup.

---

What's well done

• Layered fallback strategy (provider → idToken → entity) is intuitive and handles all construction paths cleanly.
• Backward compatibility via constructor delegation is the right approach — no callers break.
• Defensive copy on input prevents external mutation of the stored attributes.
• Test for defensive copying (shouldDefensivelyCopyAttributes) directly validates the security property.
• Builder backward compatibility: existing builder calls without attributes correctly trigger the entity fallback.
• Updated mock stubs in DSOAuth2UserServiceTest / DSOidcUserServiceTest properly validate the new method signatures.

Overall a clean, targeted fix. Resolving items 1 and 2 above before merge would be my main recommendation.

[Copilot]

Pull request overview

This PR addresses issue #284 by ensuring DSUserDetails.getAttributes() reflects the original OAuth2/OIDC provider attributes (instead of being empty or null), improving Spring Security interoperability with the standard getAttribute(...) pattern.

Changes:

• Propagate provider attribute maps from DSOAuth2UserService / DSOidcUserService into LoginHelperService, and into DSUserDetails.
• Update DSUserDetails constructors to always initialize attributes, with fallbacks to ID token claims (OIDC) or User entity fields (local login).
• Add/adjust unit tests to cover attribute passthrough and fallback behavior.

Reviewed changes

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

Show a summary per file

File	Description
src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetails.java	Adds attribute-aware constructors and fallback attribute building.
src/main/java/com/digitalsanctuary/spring/user/service/LoginHelperService.java	Adds overloads to accept provider attributes and pass them into DSUserDetails.
src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java	Passes OAuth2User.getAttributes() through to LoginHelperService.
src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java	Passes OidcUser.getAttributes() through to LoginHelperService.
src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsTest.java	New unit tests covering constructor paths, fallbacks, and defensive copying.
src/test/java/com/digitalsanctuary/spring/user/service/LoginHelperServiceTest.java	Adds tests validating attribute passthrough in OAuth2 and OIDC helper flows.
src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java	Updates mocks to match new LoginHelperService OAuth2 overload.
src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java	Updates mocks to match new LoginHelperService OIDC overload and asserts attributes present.

---

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

[Copilot]

These stubbings use raw casts ((Collection) testAuthorities) which will produce unchecked warnings and can hide type mismatches. Prefer declaring testAuthorities with the proper generic type (e.g., Collection<? extends GrantedAuthority>) or using a typed thenReturn without raw casts.

Suggested change

	when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) testAuthorities);
	when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn(testAuthorities);

[Copilot]

attributes is stored as a mutable HashMap and getAttributes() returns the backing map. That makes the authentication principal externally mutable (callers can do getAttributes().put(...)), which contradicts the “immutable” intent and can lead to hard-to-debug security/context issues. Consider storing an unmodifiable map (e.g., Map.copyOf(...)) and/or returning an unmodifiable/copy from getAttributes().

[Copilot]

buildFallbackAttributes() uses user.getFullName() for the standard name claim, but User#getFullName() currently concatenates firstName + " " + lastName and will yield values like "Test null" or "null Smith" when one part is missing. Consider constructing name defensively here (only include non-null parts and trim) so the fallback claim doesn’t contain the literal string "null".

Suggested change

	if (user.getFirstName() != null || user.getLastName() != null) {
	attrs.put("name", user.getFullName());
	}
	String firstName = user.getFirstName();
	String lastName = user.getLastName();
	StringBuilder fullNameBuilder = new StringBuilder();
	if (firstName != null && !firstName.trim().isEmpty()) {
	fullNameBuilder.append(firstName.trim());
	}
	if (lastName != null && !lastName.trim().isEmpty()) {
	if (fullNameBuilder.length() > 0) {
	fullNameBuilder.append(' ');
	}
	fullNameBuilder.append(lastName.trim());
	}
	if (fullNameBuilder.length() > 0) {
	attrs.put("name", fullNameBuilder.toString());
	}

[claude]

PR Review: fix(oauth2): populate DSUserDetails.getAttributes() with provider attributes

This is a clean, well-structured fix for a real bug. The changes are backward compatible, well-tested, and the overall approach is sound. A few items worth flagging:

---

Issues

1. DSUserDetailsTest should use @ServiceTest instead of @ExtendWith(MockitoExtension.class)

Per CLAUDE.md, the project has custom test annotations that should be used instead of raw Spring/Mockito annotations:

@ServiceTest — Unit testing services with mocks — None (Mockito only)

DSUserDetailsTest uses @ExtendWith(MockitoExtension.class) directly and doesn't actually use any Mockito mocks at all — it creates plain User and DSUserDetails instances. It should either use @ServiceTest (which wraps MockitoExtension) or simply drop the annotation entirely since no mocking is involved.

// Current
@ExtendWith(MockitoExtension.class)
@DisplayName("DSUserDetails Tests")
class DSUserDetailsTest {

// Should be (per project conventions)
@ServiceTest
@DisplayName("DSUserDetails Tests")
class DSUserDetailsTest {

---

Minor Observations

2. name.length() > 0 in buildFallbackAttributes

In DSUserDetails.buildFallbackAttributes() (line ~155), the idiom name.length() > 0 is slightly dated — !name.isEmpty() is idiomatic Java and more readable. Since trim() is already applied on append, !name.isEmpty() is the right check here. Minor, but consistent with clean code.

3. @Builder moved from 3-arg to 5-arg OIDC constructor

Existing code using DSUserDetails.builder() without .attributes(...) will still work correctly since the new attributes parameter defaults to null and the constructor handles that gracefully with the idToken fallback. Just worth confirming any callers external to this repo are aware the Lombok-generated builder now exposes an attributes field.

---

What's Done Well

• Defensive copying of the attributes map in all constructors prevents external mutation — good security hygiene.
• Collections.unmodifiableMap(attributes) in getAttributes() enforces the contract that the returned map is read-only.
• Fallback priority (explicit attributes → idToken claims → User entity fields) is logical and well-documented in Javadoc.
• Backward compatibility is fully preserved — all existing constructor signatures are kept as delegating overloads.
• Test coverage is comprehensive: 13 new unit tests covering all constructor paths, fallback behavior, defensive copying, builder usage, and the getAttribute("email") pattern that was broken.
• The doReturn(...) refactoring in LoginHelperServiceTest (replacing the unchecked (Collection) cast pattern with type-safe doReturn) is a nice cleanup.

---

Summary

The bug fix itself is correct and the implementation is solid. The main actionable item is aligning DSUserDetailsTest with the project's custom test annotation conventions (@ServiceTest). Everything else is minor polish.