From 2adf21c0e36b9f1b51652f33bf97cd1e83457e09 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sun, 31 May 2026 12:45:59 +0800 Subject: [PATCH 1/6] fix: round-trip OIDC principal through OAuth2 authorization persistence The OAuth2 Authorization Server stores the authenticated principal in the authorization attributes as JSON. After an external-OIDC login that principal is a DhisOidcUser wrapping a UserDetailsImpl, neither of which is on Spring Security's SecurityJackson2Modules deserialization allowlist, so any read of such an authorization (e.g. the /oauth2/token exchange) failed with HTTP 500: "... is not in the allowlist". Serialization succeeded, which is why the failure only showed up on read. Register Dhis2OAuth2PrincipalJackson2Module on the authorization service's ObjectMapper to allowlist and (de)serialize both types, and make UserDetailsImpl round-trip cleanly through its Lombok builder (@Jacksonized) instead of the previous hand-written, never-exercised @JsonCreator factory. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../org/hisp/dhis/user/UserDetailsImpl.java | 91 ++++---------- .../Dhis2OAuth2AuthorizationServiceImpl.java | 4 + .../Dhis2OAuth2PrincipalJackson2Module.java | 118 ++++++++++++++++++ ...h2AuthorizationServiceIntegrationTest.java | 69 ++++++++++ 4 files changed, 217 insertions(+), 65 deletions(-) create mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java index 02a337a243f3..ea0550cb4e1c 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java @@ -29,7 +29,7 @@ */ package org.hisp.dhis.user; -import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonAutoDetect; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonTypeInfo; @@ -40,6 +40,7 @@ import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.Setter; +import lombok.extern.jackson.Jacksonized; import lombok.extern.slf4j.Slf4j; import org.hisp.dhis.security.Authorities; import org.hisp.dhis.security.twofa.TwoFactorType; @@ -47,76 +48,24 @@ @Getter @Builder +@Jacksonized @EqualsAndHashCode(onlyExplicitlyIncluded = true) @Slf4j @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS) +// Serialize by field name (not getters) so the JSON property names match the Lombok @Builder +// setters that @Jacksonized wires up for deserialization; boolean getters such as isSuper() would +// otherwise emit "super" while the builder method is named "isSuper". @Jacksonized makes Jackson +// rebuild instances through the builder, which avoids the ambiguity of the hand-written factory and +// the Lombok all-args constructor competing as creators. This is the shape the OAuth2 Authorization +// Server persistence layer round-trips when an OIDC principal (DhisOidcUser) wraps a +// UserDetailsImpl. +@JsonAutoDetect( + fieldVisibility = JsonAutoDetect.Visibility.ANY, + getterVisibility = JsonAutoDetect.Visibility.NONE, + isGetterVisibility = JsonAutoDetect.Visibility.NONE) @JsonIgnoreProperties(ignoreUnknown = true) public class UserDetailsImpl implements UserDetails { - @JsonCreator(mode = JsonCreator.Mode.PROPERTIES) - public static UserDetailsImpl userDetailsMixin( - @JsonProperty("id") String uid, - @JsonProperty("code") String code, - @JsonProperty("username") String username, - @JsonProperty("firstName") String firstName, - @JsonProperty("surname") String surname, - @JsonProperty("password") String password, - @JsonProperty("externalAuth") boolean externalAuth, - @JsonProperty("isTwoFactorEnabled") boolean isTwoFactorEnabled, - @JsonProperty("twoFactorType") TwoFactorType twoFactorType, - @JsonProperty("secret") String secret, - @JsonProperty("email") String email, - @JsonProperty("isEmailVerified") boolean isEmailVerified, - @JsonProperty("enabled") boolean enabled, - @JsonProperty("accountNonExpired") boolean accountNonExpired, - @JsonProperty("accountNonLocked") boolean accountNonLocked, - @JsonProperty("credentialsNonExpired") boolean credentialsNonExpired, - @JsonProperty("dataViewMaxOrganisationUnitLevel") int dataViewMaxOrganisationUnitLevel, - @JsonProperty("authorities") Collection authorities, - @JsonProperty("allAuthorities") Set allAuthorities, - @JsonProperty("allRestrictions") Set allRestrictions, - @JsonProperty("userGroupIds") Set userGroupIds, - @JsonProperty("userOrgUnitIds") Set userOrgUnitIds, - @JsonProperty("userDataOrgUnitIds") Set userDataOrgUnitIds, - @JsonProperty("userSearchOrgUnitIds") Set userSearchOrgUnitIds, - @JsonProperty("userEffectiveSearchOrgUnitIds") Set userEffectiveSearchOrgUnitIds, - @JsonProperty("isSuper") boolean isSuper, - @JsonProperty("userRoleIds") Set userRoleIds, - @JsonProperty("managedGroupLongIds") Set managedGroupLongIds, - @JsonProperty("userRoleLongIds") Set userRoleLongIds) { - return UserDetailsImpl.builder() - .uid(uid) - .code(code) - .username(username) - .firstName(firstName) - .surname(surname) - .password(password) - .externalAuth(externalAuth) - .isTwoFactorEnabled(isTwoFactorEnabled) - .twoFactorType(twoFactorType) - .secret(secret) - .email(email) - .isEmailVerified(isEmailVerified) - .enabled(enabled) - .accountNonExpired(accountNonExpired) - .accountNonLocked(accountNonLocked) - .credentialsNonExpired(credentialsNonExpired) - .dataViewMaxOrganisationUnitLevel(dataViewMaxOrganisationUnitLevel) - .authorities(authorities) - .allAuthorities(allAuthorities) - .allRestrictions(allRestrictions) - .userGroupIds(userGroupIds) - .userOrgUnitIds(userOrgUnitIds) - .userDataOrgUnitIds(userDataOrgUnitIds) - .userSearchOrgUnitIds(userSearchOrgUnitIds) - .userEffectiveSearchOrgUnitIds(userEffectiveSearchOrgUnitIds) - .isSuper(isSuper) - .userRoleIds(userRoleIds) - .managedGroupLongIds(managedGroupLongIds) - .userRoleLongIds(userRoleLongIds) - .build(); - } - private final String uid; @Setter private Long id; private final String code; @@ -148,6 +97,18 @@ public static UserDetailsImpl userDetailsMixin( @Nonnull private final Set managedGroupLongIds; @Nonnull private final Set userRoleLongIds; + /** + * Serialize the UID under its own name. The inherited {@link + * org.hisp.dhis.common.UidObject#getUid()} maps it to {@code "id"} for the public metadata API, + * but this value object is only (de)serialized by the OAuth2 Authorization Server persistence + * layer, where {@code "id"} would collide with the numeric {@link #id} field. + */ + @Override + @JsonProperty("uid") + public String getUid() { + return uid; + } + @Override public boolean canModifyUser(User other) { if (other == null) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java index 95dd03358341..a1f2392e60ae 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java @@ -119,6 +119,10 @@ public Dhis2OAuth2AuthorizationServiceImpl( SecurityJackson2Modules.getModules(classLoader); this.objectMapper.registerModules(securityModules); this.objectMapper.registerModule(new OAuth2AuthorizationServerJackson2Module()); + // Allowlist + (de)serialize the DHIS2-custom principal types (DhisOidcUser / UserDetailsImpl) + // that an OIDC login persists into the authorization attributes; without this the read side + // (e.g. /oauth2/token) fails the SecurityJackson2Modules allowlist. + this.objectMapper.registerModule(new Dhis2OAuth2PrincipalJackson2Module()); this.objectMapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java new file mode 100644 index 000000000000..4a7d20a62b03 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2004-2025, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.security.oauth2.authorization; + +import com.fasterxml.jackson.annotation.JsonAutoDetect; +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import com.fasterxml.jackson.databind.module.SimpleModule; +import java.util.Map; +import org.hisp.dhis.security.oidc.DhisOidcUser; +import org.hisp.dhis.user.UserDetails; +import org.hisp.dhis.user.UserDetailsImpl; +import org.springframework.security.jackson2.SecurityJackson2Modules; +import org.springframework.security.oauth2.core.oidc.OidcIdToken; + +/** + * Jackson module that teaches the OAuth2 Authorization Server's persistence {@link + * com.fasterxml.jackson.databind.ObjectMapper} (configured in {@link + * Dhis2OAuth2AuthorizationServiceImpl}) how to (de)serialize the DHIS2-custom principal types that + * can end up in an {@link + * org.springframework.security.oauth2.server.authorization.OAuth2Authorization}'s {@code + * attributes} map. + * + *

When a user authenticates through an external OIDC provider, the Spring principal carried in + * the {@code authorization_code} flow is an {@code OAuth2AuthenticationToken} whose principal is a + * {@link DhisOidcUser} wrapping a {@link UserDetailsImpl}. {@link SecurityJackson2Modules} enables + * Jackson default typing guarded by an allowlist: a concrete type is only deserialized if + * it is one of Spring's trusted classes or has an explicitly registered mixin. Neither {@code + * DhisOidcUser} nor {@code UserDetailsImpl} is trusted, so without the mixins registered here a + * read of such an authorization (e.g. the {@code /oauth2/token} exchange) fails with {@code "... is + * not in the allowlist"}. Serialization succeeds either way, which is why the failure only surfaces + * on read. + * + *

{@link DhisOidcUserMixin} both allowlists {@code DhisOidcUser} and declares how to rebuild it. + * {@code UserDetailsImpl} already declares its own JSON shape (a {@code @JsonCreator} factory plus + * field visibility) on the class; {@link UserDetailsImplMixin} is registered purely to add it to + * the allowlist. + * + * @author Morten Svanæs + */ +public class Dhis2OAuth2PrincipalJackson2Module extends SimpleModule { + + public Dhis2OAuth2PrincipalJackson2Module() { + super(Dhis2OAuth2PrincipalJackson2Module.class.getName()); + } + + @Override + public void setupModule(SetupContext context) { + context.setMixInAnnotations(DhisOidcUser.class, DhisOidcUserMixin.class); + context.setMixInAnnotations(UserDetailsImpl.class, UserDetailsImplMixin.class); + } + + /** + * Mixin for {@link DhisOidcUser}. The wrapped {@link UserDetails}, the OIDC {@code attributes} + * (claims), the name-attribute key and the ID token are sufficient to reconstruct the principal. + * The {@code authorities} are derived from the wrapped user by the {@code DhisOidcUser} + * constructor, so they are not a creator argument: they are still serialized (field visibility is + * {@code ANY}, matching Spring's own {@code DefaultOAuth2UserMixin}) but ignored on read. + * + *

{@code "id"} is ignored because {@code DhisOidcUser} inherits {@link + * org.hisp.dhis.common.UidObject#getUid()}, which is annotated {@code @JsonProperty("id")}; left + * in, that string UID would be written as {@code "id"} and then fail on read against {@code + * setId(Long)}. The user's identity is preserved in the nested {@code user} object. + */ + @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS) + @JsonAutoDetect( + fieldVisibility = JsonAutoDetect.Visibility.ANY, + getterVisibility = JsonAutoDetect.Visibility.NONE, + isGetterVisibility = JsonAutoDetect.Visibility.NONE) + @JsonIgnoreProperties( + value = {"id"}, + ignoreUnknown = true) + abstract static class DhisOidcUserMixin { + @JsonCreator + DhisOidcUserMixin( + @JsonProperty("user") UserDetails user, + @JsonProperty("attributes") Map attributes, + @JsonProperty("nameAttributeKey") String nameAttributeKey, + @JsonProperty("oidcIdToken") OidcIdToken oidcIdToken) {} + } + + /** + * Allowlist marker for {@link UserDetailsImpl}. The (de)serialization shape lives on the class + * itself; registering any mixin is what adds the type to {@link SecurityJackson2Modules}' + * deserialization allowlist (an empty mixin contributes no annotations, leaving the class's own + * configuration in effect). + */ + abstract static class UserDetailsImplMixin {} +} diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java index 4174242f8a7e..fdf79823b706 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java @@ -30,24 +30,34 @@ package org.hisp.dhis.security.oauth2.authorization; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import java.security.Principal; import java.time.Instant; import java.util.Map; import java.util.Set; import java.util.UUID; import org.hisp.dhis.common.CodeGenerator; import org.hisp.dhis.security.oauth2.client.Dhis2OAuth2ClientStore; +import org.hisp.dhis.security.oidc.DhisOidcUser; import org.hisp.dhis.test.integration.PostgresIntegrationTestBase; +import org.hisp.dhis.user.CurrentUserUtil; +import org.hisp.dhis.user.UserDetails; +import org.hisp.dhis.user.UserDetailsImpl; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.OAuth2AccessToken; import org.springframework.security.oauth2.core.OAuth2RefreshToken; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; +import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; +import org.springframework.security.oauth2.core.oidc.OidcIdToken; import org.springframework.security.oauth2.server.authorization.OAuth2Authorization; import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationCode; import org.springframework.security.oauth2.server.authorization.OAuth2TokenType; @@ -331,4 +341,63 @@ void testRemoveAuthorization() { assertNotNull(foundBeforeRemove); assertNull(foundAfterRemove); } + + @Test + void testFindByAuthorizationCodeWithOidcUserPrincipal() { + // Regression test for the /oauth2/token 500: an external-OIDC login persists an + // OAuth2AuthenticationToken whose principal is a DhisOidcUser (wrapping a UserDetailsImpl) in + // the authorization attributes. Reading the authorization back (the token exchange) must not + // trip Spring Security's Jackson deserialization allowlist. + UserDetails currentUser = CurrentUserUtil.getCurrentUserDetails(); + Instant now = Instant.now(); + Instant expiresAt = now.plusSeconds(300); + + Map claims = + Map.of(IdTokenClaimNames.SUB, currentUser.getUsername(), "email", "oidc@example.com"); + OidcIdToken idToken = + OidcIdToken.withTokenValue("oidc-id-token-value") + .issuedAt(now) + .expiresAt(expiresAt) + .subject(currentUser.getUsername()) + .claims(c -> c.putAll(claims)) + .build(); + DhisOidcUser oidcPrincipal = + new DhisOidcUser(currentUser, claims, IdTokenClaimNames.SUB, idToken); + OAuth2AuthenticationToken authentication = + new OAuth2AuthenticationToken(oidcPrincipal, oidcPrincipal.getAuthorities(), "google"); + + OAuth2AuthorizationCode authorizationCode = + new OAuth2AuthorizationCode("oidc-code-value", now, expiresAt); + OAuth2Authorization authorization = + OAuth2Authorization.withRegisteredClient(registeredClient) + .principalName(currentUser.getUsername()) + .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) + .token(authorizationCode) + .attribute(Principal.class.getName(), authentication) + .id(CodeGenerator.generateUid()) + .build(); + + // When + authorizationService.save(authorization); + OAuth2Authorization found = + authorizationService.findByToken( + "oidc-code-value", new OAuth2TokenType(OAuth2ParameterNames.CODE)); + + // Then: the principal graph round-trips intact. + assertNotNull(found); + Object principalAttribute = found.getAttribute(Principal.class.getName()); + assertInstanceOf(OAuth2AuthenticationToken.class, principalAttribute); + OAuth2AuthenticationToken roundTripped = (OAuth2AuthenticationToken) principalAttribute; + assertEquals("google", roundTripped.getAuthorizedClientRegistrationId()); + + assertInstanceOf(DhisOidcUser.class, roundTripped.getPrincipal()); + DhisOidcUser roundTrippedUser = (DhisOidcUser) roundTripped.getPrincipal(); + assertEquals(currentUser.getUsername(), roundTrippedUser.getUsername()); + assertEquals(currentUser.getUid(), roundTrippedUser.getUid()); + assertEquals(currentUser.getAllAuthorities(), roundTrippedUser.getAllAuthorities()); + // Guards the boolean field-name round-trip (isSuper would otherwise be lost): admin is super. + assertTrue(roundTrippedUser.isSuper()); + assertInstanceOf(UserDetailsImpl.class, roundTrippedUser.getUser()); + assertEquals("oidc-id-token-value", roundTrippedUser.getIdToken().getTokenValue()); + } } From 35b2b4b1f8ec58bc8a7ab19f443f5d4fd31060ff Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sun, 31 May 2026 20:15:24 +0800 Subject: [PATCH 2/6] chore: declare spring-security-oauth2-client as a test dependency The new OIDC-principal regression test uses OAuth2AuthenticationToken from spring-security-oauth2-client, which dhis-test-integration only pulled in transitively. Declare it explicitly at test scope so the dependency:analyze check (run by the unit-test and sonarqube jobs) passes. Co-Authored-By: Claude Opus 4.8 (1M context) --- dhis-2/dhis-test-integration/pom.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dhis-2/dhis-test-integration/pom.xml b/dhis-2/dhis-test-integration/pom.xml index 5cea0a0faf64..74ca1d86d3ce 100644 --- a/dhis-2/dhis-test-integration/pom.xml +++ b/dhis-2/dhis-test-integration/pom.xml @@ -347,6 +347,11 @@ quick test + + org.springframework.security + spring-security-oauth2-client + test + org.springframework.security spring-security-oauth2-core From 6c4fdcfe465f8eecaa5cf334e89964e7aeeec121 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sun, 31 May 2026 20:34:16 +0800 Subject: [PATCH 3/6] chore: address SonarCloud findings on the OAuth2 principal Jackson module - Drop @JsonTypeInfo from the DhisOidcUser mix-in (S4544): the principal's static type is the OAuth2User interface, so the allowlist default typing already writes the type id; round-trip re-verified. - Make the UserDetailsImpl allowlist marker an interface (S1610, S2094). - Suppress S1610 on the DhisOidcUser mix-in, which must stay a class to carry a @JsonCreator constructor. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Dhis2OAuth2PrincipalJackson2Module.java | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java index 4a7d20a62b03..057c4e66d116 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java @@ -33,7 +33,6 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.annotation.JsonTypeInfo; import com.fasterxml.jackson.databind.module.SimpleModule; import java.util.Map; import org.hisp.dhis.security.oidc.DhisOidcUser; @@ -61,9 +60,9 @@ * on read. * *

{@link DhisOidcUserMixin} both allowlists {@code DhisOidcUser} and declares how to rebuild it. - * {@code UserDetailsImpl} already declares its own JSON shape (a {@code @JsonCreator} factory plus - * field visibility) on the class; {@link UserDetailsImplMixin} is registered purely to add it to - * the allowlist. + * {@code UserDetailsImpl} already declares its own JSON shape (Lombok builder plus field + * visibility) on the class; {@link UserDetailsImplMixin} is registered purely to add it to the + * allowlist. * * @author Morten Svanæs */ @@ -90,8 +89,13 @@ public void setupModule(SetupContext context) { * org.hisp.dhis.common.UidObject#getUid()}, which is annotated {@code @JsonProperty("id")}; left * in, that string UID would be written as {@code "id"} and then fail on read against {@code * setId(Long)}. The user's identity is preserved in the nested {@code user} object. + * + *

No {@code @JsonTypeInfo} is needed: the principal's static type is the {@code OAuth2User} + * interface, so the type id is already written by the mapper's allowlist default typing. */ - @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS) + // Must be a class, not an interface (S1610): a Jackson mix-in that maps a @JsonCreator onto the + // target's constructor has to declare a matching constructor, which interfaces cannot. + @SuppressWarnings("java:S1610") @JsonAutoDetect( fieldVisibility = JsonAutoDetect.Visibility.ANY, getterVisibility = JsonAutoDetect.Visibility.NONE, @@ -109,10 +113,11 @@ abstract static class DhisOidcUserMixin { } /** - * Allowlist marker for {@link UserDetailsImpl}. The (de)serialization shape lives on the class - * itself; registering any mixin is what adds the type to {@link SecurityJackson2Modules}' - * deserialization allowlist (an empty mixin contributes no annotations, leaving the class's own - * configuration in effect). + * Allowlist marker for {@link UserDetailsImpl}, whose own (de)serialization shape is declared on + * the class. Registering any mix-in is what adds the type to {@link SecurityJackson2Modules}' + * deserialization allowlist; the {@code ignoreUnknown} setting just mirrors the class so a stray + * property never breaks the read. */ - abstract static class UserDetailsImplMixin {} + @JsonIgnoreProperties(ignoreUnknown = true) + interface UserDetailsImplMixin {} } From b54fc2fbd3ad29423d890b42317b925e678d5fa5 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Mon, 1 Jun 2026 00:46:17 +0800 Subject: [PATCH 4/6] refactor: persist a lean principal for OAuth2 authorizations Supersedes the custom Jackson-mixin approach. The authorization persistence layer now swaps a heavyweight DHIS2 principal (DhisOidcUser from a federated OIDC login, or UserDetailsImpl from a form login) for a lean Spring-native UsernamePasswordAuthenticationToken carrying just the DHIS2 username and authorities, at the toEntity serialization boundary. The persisted attributes then contain no DHIS2-custom types and round-trip without any custom mixins, so the /oauth2/token read no longer trips Spring Security's SecurityJackson2Modules allowlist. Spring AS builds the token-context principal from the persisted principal attribute, so the token customizer sees the lean principal: getName() is the DHIS2 username, which becomes the JWT username claim the resource server resolves the user by. This makes the federated OIDC flow work end to end (the username/email claims previously carried the IdP sub). - Delete Dhis2OAuth2PrincipalJackson2Module and the speculative Jackson annotations on UserDetailsImpl (no longer serialized into the authorization). - Add a leanPrincipal unit test, update the service integration test to assert the lean principal, and add a federated-OIDC web e2e (authorization_code -> /oauth2/token -> /api/me). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../org/hisp/dhis/user/UserDetailsImpl.java | 31 --- .../Dhis2OAuth2AuthorizationServiceImpl.java | 44 +++- .../Dhis2OAuth2PrincipalJackson2Module.java | 123 ---------- .../authorization/LeanPrincipalTest.java | 142 ++++++++++++ ...h2AuthorizationServiceIntegrationTest.java | 43 ++-- .../FederatedOidcTokenControllerTest.java | 211 ++++++++++++++++++ 6 files changed, 416 insertions(+), 178 deletions(-) delete mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/security/oauth2/authorization/LeanPrincipalTest.java create mode 100644 dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java index ea0550cb4e1c..8ccaa01e3118 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java @@ -29,10 +29,6 @@ */ package org.hisp.dhis.user; -import com.fasterxml.jackson.annotation.JsonAutoDetect; -import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.annotation.JsonTypeInfo; import java.util.Collection; import java.util.Set; import javax.annotation.Nonnull; @@ -40,7 +36,6 @@ import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.Setter; -import lombok.extern.jackson.Jacksonized; import lombok.extern.slf4j.Slf4j; import org.hisp.dhis.security.Authorities; import org.hisp.dhis.security.twofa.TwoFactorType; @@ -48,22 +43,8 @@ @Getter @Builder -@Jacksonized @EqualsAndHashCode(onlyExplicitlyIncluded = true) @Slf4j -@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS) -// Serialize by field name (not getters) so the JSON property names match the Lombok @Builder -// setters that @Jacksonized wires up for deserialization; boolean getters such as isSuper() would -// otherwise emit "super" while the builder method is named "isSuper". @Jacksonized makes Jackson -// rebuild instances through the builder, which avoids the ambiguity of the hand-written factory and -// the Lombok all-args constructor competing as creators. This is the shape the OAuth2 Authorization -// Server persistence layer round-trips when an OIDC principal (DhisOidcUser) wraps a -// UserDetailsImpl. -@JsonAutoDetect( - fieldVisibility = JsonAutoDetect.Visibility.ANY, - getterVisibility = JsonAutoDetect.Visibility.NONE, - isGetterVisibility = JsonAutoDetect.Visibility.NONE) -@JsonIgnoreProperties(ignoreUnknown = true) public class UserDetailsImpl implements UserDetails { private final String uid; @@ -97,18 +78,6 @@ public class UserDetailsImpl implements UserDetails { @Nonnull private final Set managedGroupLongIds; @Nonnull private final Set userRoleLongIds; - /** - * Serialize the UID under its own name. The inherited {@link - * org.hisp.dhis.common.UidObject#getUid()} maps it to {@code "id"} for the public metadata API, - * but this value object is only (de)serialized by the OAuth2 Authorization Server persistence - * layer, where {@code "id"} would collide with the numeric {@link #id} field. - */ - @Override - @JsonProperty("uid") - public String getUid() { - return uid; - } - @Override public boolean canModifyUser(User other) { if (other == null) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java index a1f2392e60ae..3281713d543c 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceImpl.java @@ -32,7 +32,9 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; +import java.security.Principal; import java.util.Date; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.function.Consumer; @@ -48,6 +50,7 @@ import org.hisp.dhis.user.UserDetails; import org.hisp.dhis.user.UserService; import org.springframework.dao.DataRetrievalFailureException; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.jackson2.SecurityJackson2Modules; import org.springframework.security.oauth2.core.OAuth2AccessToken; @@ -119,10 +122,6 @@ public Dhis2OAuth2AuthorizationServiceImpl( SecurityJackson2Modules.getModules(classLoader); this.objectMapper.registerModules(securityModules); this.objectMapper.registerModule(new OAuth2AuthorizationServerJackson2Module()); - // Allowlist + (de)serialize the DHIS2-custom principal types (DhisOidcUser / UserDetailsImpl) - // that an OIDC login persists into the authorization attributes; without this the read side - // (e.g. /oauth2/token) fails the SecurityJackson2Modules allowlist. - this.objectMapper.registerModule(new Dhis2OAuth2PrincipalJackson2Module()); this.objectMapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); } @@ -385,7 +384,7 @@ private Dhis2OAuth2Authorization toEntity(OAuth2Authorization authorization) { entity.setAuthorizationGrantType(authorization.getAuthorizationGrantType().getValue()); entity.setAuthorizedScopes( StringUtils.collectionToCommaDelimitedString(authorization.getAuthorizedScopes())); - entity.setAttributes(writeMap(authorization.getAttributes())); + entity.setAttributes(writeMap(leanPrincipal(authorization.getAttributes()))); entity.setState(authorization.getAttribute(OAuth2ParameterNames.STATE)); OAuth2Authorization.Token authorizationCode = @@ -487,6 +486,41 @@ private Map parseMap(String data) { } } + /** + * Replaces a heavyweight DHIS2 principal in the authorization attributes with a lean, + * Spring-native one before persistence. + * + *

After a federated OIDC login the {@code java.security.Principal} attribute is an {@link + * org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken} whose + * principal is a {@link org.hisp.dhis.security.oidc.DhisOidcUser} wrapping a {@link + * org.hisp.dhis.user.UserDetailsImpl}; a form login carries a {@code UserDetailsImpl} directly. + * Spring Authorization Server only ever reads the principal's {@link Authentication#getName() + * name} (the JWT/token customizer) and re-loads the user from the database on token validation, + * so the heavyweight object graph is dead weight in the row and the reason those DHIS2-custom + * types tripped {@link SecurityJackson2Modules}' deserialization allowlist. Swapping it for a + * {@link UsernamePasswordAuthenticationToken} carrying just the DHIS2 username and authorities + * keeps the persisted attributes entirely Spring-native, so they round-trip without any custom + * Jackson mixins. {@code principalName} is left untouched (the consent store keys on it). The + * token's {@code authorizedClientRegistrationId} (e.g. {@code "google"}) and the authentication + * {@code details} are intentionally not retained — nothing in the token-issuance or validation + * path reads them once the user is identified by name. + * + *

Package-private (rather than {@code private}) so the branch logic can be unit-tested + * directly. + */ + static Map leanPrincipal(Map attributes) { + if (attributes.get(Principal.class.getName()) instanceof Authentication authentication + && authentication.getPrincipal() instanceof UserDetails userDetails) { + Map lean = new LinkedHashMap<>(attributes); + lean.put( + Principal.class.getName(), + new UsernamePasswordAuthenticationToken( + userDetails.getUsername(), null, userDetails.getAuthorities())); + return lean; + } + return attributes; + } + /** * Converts a Map to a JSON string. * diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java deleted file mode 100644 index 057c4e66d116..000000000000 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2PrincipalJackson2Module.java +++ /dev/null @@ -1,123 +0,0 @@ -/* - * Copyright (c) 2004-2025, University of Oslo - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * 1. Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * 2. Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * - * 3. Neither the name of the copyright holder nor the names of its contributors - * may be used to endorse or promote products derived from this software without - * specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED - * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR - * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON - * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS - * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package org.hisp.dhis.security.oauth2.authorization; - -import com.fasterxml.jackson.annotation.JsonAutoDetect; -import com.fasterxml.jackson.annotation.JsonCreator; -import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.databind.module.SimpleModule; -import java.util.Map; -import org.hisp.dhis.security.oidc.DhisOidcUser; -import org.hisp.dhis.user.UserDetails; -import org.hisp.dhis.user.UserDetailsImpl; -import org.springframework.security.jackson2.SecurityJackson2Modules; -import org.springframework.security.oauth2.core.oidc.OidcIdToken; - -/** - * Jackson module that teaches the OAuth2 Authorization Server's persistence {@link - * com.fasterxml.jackson.databind.ObjectMapper} (configured in {@link - * Dhis2OAuth2AuthorizationServiceImpl}) how to (de)serialize the DHIS2-custom principal types that - * can end up in an {@link - * org.springframework.security.oauth2.server.authorization.OAuth2Authorization}'s {@code - * attributes} map. - * - *

When a user authenticates through an external OIDC provider, the Spring principal carried in - * the {@code authorization_code} flow is an {@code OAuth2AuthenticationToken} whose principal is a - * {@link DhisOidcUser} wrapping a {@link UserDetailsImpl}. {@link SecurityJackson2Modules} enables - * Jackson default typing guarded by an allowlist: a concrete type is only deserialized if - * it is one of Spring's trusted classes or has an explicitly registered mixin. Neither {@code - * DhisOidcUser} nor {@code UserDetailsImpl} is trusted, so without the mixins registered here a - * read of such an authorization (e.g. the {@code /oauth2/token} exchange) fails with {@code "... is - * not in the allowlist"}. Serialization succeeds either way, which is why the failure only surfaces - * on read. - * - *

{@link DhisOidcUserMixin} both allowlists {@code DhisOidcUser} and declares how to rebuild it. - * {@code UserDetailsImpl} already declares its own JSON shape (Lombok builder plus field - * visibility) on the class; {@link UserDetailsImplMixin} is registered purely to add it to the - * allowlist. - * - * @author Morten Svanæs - */ -public class Dhis2OAuth2PrincipalJackson2Module extends SimpleModule { - - public Dhis2OAuth2PrincipalJackson2Module() { - super(Dhis2OAuth2PrincipalJackson2Module.class.getName()); - } - - @Override - public void setupModule(SetupContext context) { - context.setMixInAnnotations(DhisOidcUser.class, DhisOidcUserMixin.class); - context.setMixInAnnotations(UserDetailsImpl.class, UserDetailsImplMixin.class); - } - - /** - * Mixin for {@link DhisOidcUser}. The wrapped {@link UserDetails}, the OIDC {@code attributes} - * (claims), the name-attribute key and the ID token are sufficient to reconstruct the principal. - * The {@code authorities} are derived from the wrapped user by the {@code DhisOidcUser} - * constructor, so they are not a creator argument: they are still serialized (field visibility is - * {@code ANY}, matching Spring's own {@code DefaultOAuth2UserMixin}) but ignored on read. - * - *

{@code "id"} is ignored because {@code DhisOidcUser} inherits {@link - * org.hisp.dhis.common.UidObject#getUid()}, which is annotated {@code @JsonProperty("id")}; left - * in, that string UID would be written as {@code "id"} and then fail on read against {@code - * setId(Long)}. The user's identity is preserved in the nested {@code user} object. - * - *

No {@code @JsonTypeInfo} is needed: the principal's static type is the {@code OAuth2User} - * interface, so the type id is already written by the mapper's allowlist default typing. - */ - // Must be a class, not an interface (S1610): a Jackson mix-in that maps a @JsonCreator onto the - // target's constructor has to declare a matching constructor, which interfaces cannot. - @SuppressWarnings("java:S1610") - @JsonAutoDetect( - fieldVisibility = JsonAutoDetect.Visibility.ANY, - getterVisibility = JsonAutoDetect.Visibility.NONE, - isGetterVisibility = JsonAutoDetect.Visibility.NONE) - @JsonIgnoreProperties( - value = {"id"}, - ignoreUnknown = true) - abstract static class DhisOidcUserMixin { - @JsonCreator - DhisOidcUserMixin( - @JsonProperty("user") UserDetails user, - @JsonProperty("attributes") Map attributes, - @JsonProperty("nameAttributeKey") String nameAttributeKey, - @JsonProperty("oidcIdToken") OidcIdToken oidcIdToken) {} - } - - /** - * Allowlist marker for {@link UserDetailsImpl}, whose own (de)serialization shape is declared on - * the class. Registering any mix-in is what adds the type to {@link SecurityJackson2Modules}' - * deserialization allowlist; the {@code ignoreUnknown} setting just mirrors the class so a stray - * property never breaks the read. - */ - @JsonIgnoreProperties(ignoreUnknown = true) - interface UserDetailsImplMixin {} -} diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/security/oauth2/authorization/LeanPrincipalTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/security/oauth2/authorization/LeanPrincipalTest.java new file mode 100644 index 000000000000..84f4c254cde2 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/security/oauth2/authorization/LeanPrincipalTest.java @@ -0,0 +1,142 @@ +/* + * Copyright (c) 2004-2025, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.security.oauth2.authorization; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.security.Principal; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import org.hisp.dhis.security.oidc.DhisOidcUser; +import org.hisp.dhis.user.UserDetailsImpl; +import org.junit.jupiter.api.Test; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken; +import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; +import org.springframework.security.oauth2.core.oidc.OidcIdToken; + +/** + * Unit tests for {@link Dhis2OAuth2AuthorizationServiceImpl#leanPrincipal} — the swap that keeps + * the persisted OAuth2 authorization principal Spring-native (so it never trips the Jackson + * allowlist). + */ +class LeanPrincipalTest { + + @Test + void noPrincipalAttribute_isUnchanged() { + Map attributes = new LinkedHashMap<>(); + attributes.put("state", "xyz"); + assertSame(attributes, Dhis2OAuth2AuthorizationServiceImpl.leanPrincipal(attributes)); + } + + @Test + void nonUserDetailsPrincipal_isUnchanged() { + // client_credentials-style: an Authentication whose principal is the client id (a String). + Map attributes = new LinkedHashMap<>(); + attributes.put( + Principal.class.getName(), + new UsernamePasswordAuthenticationToken("client-id", null, List.of())); + assertSame(attributes, Dhis2OAuth2AuthorizationServiceImpl.leanPrincipal(attributes)); + } + + @Test + void formLoginUserDetailsImplPrincipal_isLeaned() { + UserDetailsImpl userDetails = userDetails("formuser"); + Map attributes = new LinkedHashMap<>(); + attributes.put( + Principal.class.getName(), + new UsernamePasswordAuthenticationToken( + userDetails, "secret", userDetails.getAuthorities())); + + assertLean(Dhis2OAuth2AuthorizationServiceImpl.leanPrincipal(attributes), "formuser"); + } + + @Test + void federatedDhisOidcUserPrincipal_isLeaned() { + UserDetailsImpl userDetails = userDetails("oidcuser"); + Map oidcClaims = Map.of(IdTokenClaimNames.SUB, "google-sub-1"); + OidcIdToken idToken = + OidcIdToken.withTokenValue("id-token") + .subject("google-sub-1") + .claims(c -> c.putAll(oidcClaims)) + .build(); + DhisOidcUser oidcPrincipal = + new DhisOidcUser(userDetails, oidcClaims, IdTokenClaimNames.SUB, idToken); + Map attributes = new LinkedHashMap<>(); + attributes.put( + Principal.class.getName(), + new OAuth2AuthenticationToken(oidcPrincipal, oidcPrincipal.getAuthorities(), "google")); + + // The DhisOidcUser's getName() is the IdP sub; the leaned principal must carry the DHIS2 + // username. + assertLean(Dhis2OAuth2AuthorizationServiceImpl.leanPrincipal(attributes), "oidcuser"); + } + + private static void assertLean(Map result, String expectedUsername) { + Object principal = result.get(Principal.class.getName()); + assertInstanceOf(UsernamePasswordAuthenticationToken.class, principal); + UsernamePasswordAuthenticationToken token = (UsernamePasswordAuthenticationToken) principal; + assertEquals(expectedUsername, token.getName()); + assertTrue(token.isAuthenticated()); + assertEquals( + Set.of("ALL"), + token.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toSet())); + } + + private static UserDetailsImpl userDetails(String username) { + return UserDetailsImpl.builder() + .uid("uid-" + username) + .username(username) + .authorities(new ArrayList<>(List.of(new SimpleGrantedAuthority("ALL")))) + .allAuthorities(new HashSet<>(Set.of("ALL"))) + .allRestrictions(new HashSet<>()) + .userGroupIds(new HashSet<>()) + .userOrgUnitIds(new HashSet<>()) + .userDataOrgUnitIds(new HashSet<>()) + .userSearchOrgUnitIds(new HashSet<>()) + .userEffectiveSearchOrgUnitIds(new HashSet<>()) + .userRoleIds(new HashSet<>()) + .managedGroupLongIds(new HashSet<>()) + .userRoleLongIds(new HashSet<>()) + .build(); + } +} diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java index fdf79823b706..86cb04c77a81 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/security/oauth2/authorization/Dhis2OAuth2AuthorizationServiceIntegrationTest.java @@ -40,16 +40,18 @@ import java.util.Map; import java.util.Set; import java.util.UUID; +import java.util.stream.Collectors; import org.hisp.dhis.common.CodeGenerator; import org.hisp.dhis.security.oauth2.client.Dhis2OAuth2ClientStore; import org.hisp.dhis.security.oidc.DhisOidcUser; import org.hisp.dhis.test.integration.PostgresIntegrationTestBase; import org.hisp.dhis.user.CurrentUserUtil; import org.hisp.dhis.user.UserDetails; -import org.hisp.dhis.user.UserDetailsImpl; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; @@ -344,10 +346,10 @@ void testRemoveAuthorization() { @Test void testFindByAuthorizationCodeWithOidcUserPrincipal() { - // Regression test for the /oauth2/token 500: an external-OIDC login persists an - // OAuth2AuthenticationToken whose principal is a DhisOidcUser (wrapping a UserDetailsImpl) in - // the authorization attributes. Reading the authorization back (the token exchange) must not - // trip Spring Security's Jackson deserialization allowlist. + // Regression test for the /oauth2/token 500: an external-OIDC login presents an + // OAuth2AuthenticationToken whose principal is a DhisOidcUser (wrapping a UserDetailsImpl). The + // persistence layer must store a lean, Spring-native principal so the read side (the token + // exchange) never trips Spring Security's Jackson deserialization allowlist. UserDetails currentUser = CurrentUserUtil.getCurrentUserDetails(); Instant now = Instant.now(); Instant expiresAt = now.plusSeconds(300); @@ -383,21 +385,24 @@ void testFindByAuthorizationCodeWithOidcUserPrincipal() { authorizationService.findByToken( "oidc-code-value", new OAuth2TokenType(OAuth2ParameterNames.CODE)); - // Then: the principal graph round-trips intact. + // Then: the heavyweight OIDC principal was persisted as a lean, Spring-native + // UsernamePasswordAuthenticationToken (no DHIS2-custom types in the row), carrying the DHIS2 + // username and authorities. getName() is the DHIS2 username — what the token customizer emits + // as + // the username claim and what the resource server resolves the user by. assertNotNull(found); Object principalAttribute = found.getAttribute(Principal.class.getName()); - assertInstanceOf(OAuth2AuthenticationToken.class, principalAttribute); - OAuth2AuthenticationToken roundTripped = (OAuth2AuthenticationToken) principalAttribute; - assertEquals("google", roundTripped.getAuthorizedClientRegistrationId()); - - assertInstanceOf(DhisOidcUser.class, roundTripped.getPrincipal()); - DhisOidcUser roundTrippedUser = (DhisOidcUser) roundTripped.getPrincipal(); - assertEquals(currentUser.getUsername(), roundTrippedUser.getUsername()); - assertEquals(currentUser.getUid(), roundTrippedUser.getUid()); - assertEquals(currentUser.getAllAuthorities(), roundTrippedUser.getAllAuthorities()); - // Guards the boolean field-name round-trip (isSuper would otherwise be lost): admin is super. - assertTrue(roundTrippedUser.isSuper()); - assertInstanceOf(UserDetailsImpl.class, roundTrippedUser.getUser()); - assertEquals("oidc-id-token-value", roundTrippedUser.getIdToken().getTokenValue()); + assertInstanceOf(UsernamePasswordAuthenticationToken.class, principalAttribute); + UsernamePasswordAuthenticationToken roundTripped = + (UsernamePasswordAuthenticationToken) principalAttribute; + assertEquals(currentUser.getUsername(), roundTripped.getName()); + assertTrue(roundTripped.isAuthenticated()); + assertEquals( + currentUser.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toSet()), + roundTripped.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toSet())); } } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java new file mode 100644 index 000000000000..c471579962cf --- /dev/null +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java @@ -0,0 +1,211 @@ +/* + * Copyright (c) 2004-2025, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.webapi.controller.security; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import java.nio.charset.StandardCharsets; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.Base64; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import org.hisp.dhis.common.CodeGenerator; +import org.hisp.dhis.jsontree.JsonValue; +import org.hisp.dhis.security.jwt.Dhis2JwtAuthenticationManagerResolver; +import org.hisp.dhis.security.oauth2.authorization.Dhis2OAuth2AuthorizationService; +import org.hisp.dhis.security.oauth2.client.Dhis2OAuth2ClientService; +import org.hisp.dhis.security.oidc.DhisOidcUser; +import org.hisp.dhis.test.webapi.ControllerWithJwtTokenAuthTestBase; +import org.hisp.dhis.user.User; +import org.hisp.dhis.user.UserDetails; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.HttpHeaders; +import org.springframework.http.MediaType; +import org.springframework.security.crypto.password.PasswordEncoder; +import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken; +import org.springframework.security.oauth2.core.AuthorizationGrantType; +import org.springframework.security.oauth2.core.ClientAuthenticationMethod; +import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; +import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; +import org.springframework.security.oauth2.core.oidc.OidcIdToken; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.security.oauth2.jwt.JwtDecoder; +import org.springframework.security.oauth2.server.authorization.OAuth2Authorization; +import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationCode; +import org.springframework.security.oauth2.server.authorization.client.RegisteredClient; +import org.springframework.security.oauth2.server.authorization.settings.AuthorizationServerSettings; +import org.springframework.test.context.ActiveProfiles; + +/** + * End-to-end test for FEDERATED OIDC login through the OAuth2 Authorization Server. + * + *

Simulates the "log in with Google" path where the user is authenticated as a {@link + * DhisOidcUser} (whose {@code getName()} is the IdP {@code sub}), then exchanges the resulting + * authorization code at {@code /oauth2/token} and calls the API with the issued DHIS2 JWT. Verifies + * that the lean-principal persistence makes the issued token carry the DHIS2 username (not + * the IdP {@code sub}) and that the resource server resolves it to the correct DHIS2 user. + * + * @author Morten Svanæs + */ +@ActiveProfiles("oauth2-authorization-server-test") +class FederatedOidcTokenControllerTest extends ControllerWithJwtTokenAuthTestBase { + + @Autowired private Dhis2OAuth2ClientService oAuth2ClientService; + @Autowired private Dhis2OAuth2AuthorizationService authorizationService; + @Autowired private AuthorizationServerSettings authorizationServerSettings; + @Autowired private Dhis2JwtAuthenticationManagerResolver jwtAuthenticationManagerResolver; + @Autowired private JwtDecoder jwtDecoder; + @Autowired private PasswordEncoder passwordEncoder; + + @BeforeEach + void setUp() { + // Make the resource-server resolver validate tokens with the AS's own signing keys. + jwtAuthenticationManagerResolver.setJwtDecoder(jwtDecoder); + } + + @Test + @DisplayName("Federated OIDC login: token carries the DHIS2 username and resolves the user") + void federatedOidcLoginIssuesTokenThatResolvesToDhis2User() throws Exception { + // Given: a local DHIS2 user matched by an external OIDC login (external auth). + User user = createUserWithAuth("feduser", "ALL"); + UserDetails userDetails = userService.createUserDetails(user); + + // And: the Spring principal that the OIDC login produces — a DhisOidcUser whose getName() is + // the + // IdP 'sub', not the DHIS2 username. + Instant now = Instant.now(); + Map claims = + Map.of(IdTokenClaimNames.SUB, "google-sub-0001", "email", "feduser@dhis2.org"); + OidcIdToken idToken = + OidcIdToken.withTokenValue("id-token") + .issuedAt(now) + .expiresAt(now.plus(5, ChronoUnit.MINUTES)) + .subject("google-sub-0001") + .claims(c -> c.putAll(claims)) + .build(); + DhisOidcUser oidcPrincipal = + new DhisOidcUser(userDetails, claims, IdTokenClaimNames.SUB, idToken); + OAuth2AuthenticationToken oidcAuthentication = + new OAuth2AuthenticationToken(oidcPrincipal, oidcPrincipal.getAuthorities(), "google"); + + // And: a confidential authorization_code client (the native app's server-side equivalent). + String clientId = "federated-client-" + UUID.randomUUID(); + String clientSecret = "federated-secret"; + String redirectUri = "https://app.example.org/callback"; + RegisteredClient client = + RegisteredClient.withId(CodeGenerator.generateUid()) + .clientId(clientId) + .clientSecret(passwordEncoder.encode(clientSecret)) + .clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC) + .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) + .redirectUri(redirectUri) + .scope("openid") + .scope("profile") + .scope("username") + .scope("email") + .build(); + oAuth2ClientService.save(client); + + // And: the authorization the /oauth2/authorize step would have stored after the OIDC login. + String issuer = authorizationServerSettings.getIssuer(); + String codeValue = "fed-code-" + UUID.randomUUID(); + OAuth2AuthorizationCode authorizationCode = + new OAuth2AuthorizationCode(codeValue, now, now.plus(5, ChronoUnit.MINUTES)); + OAuth2AuthorizationRequest authorizationRequest = + OAuth2AuthorizationRequest.authorizationCode() + .authorizationUri(issuer + "oauth2/authorize") + .clientId(clientId) + .redirectUri(redirectUri) + .scopes(Set.of("openid", "profile", "username", "email")) + .state("state-0001") + .build(); + OAuth2Authorization authorization = + OAuth2Authorization.withRegisteredClient(client) + .id(CodeGenerator.generateUid()) + .principalName(oidcPrincipal.getName()) // the IdP sub, as Spring AS would set it + .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) + .authorizedScopes(Set.of("openid", "profile", "username", "email")) + .token(authorizationCode) + .attribute(java.security.Principal.class.getName(), oidcAuthentication) + .attribute(OAuth2AuthorizationRequest.class.getName(), authorizationRequest) + .build(); + authorizationService.save(authorization); // <-- lean-principal swap happens here + + // When: the client exchanges the code for a token. + String basicAuth = + Base64.getEncoder() + .encodeToString((clientId + ":" + clientSecret).getBytes(StandardCharsets.UTF_8)); + String tokenResponse = + mvc.perform( + post("/oauth2/token") + .header(HttpHeaders.AUTHORIZATION, "Basic " + basicAuth) + .contentType(MediaType.APPLICATION_FORM_URLENCODED) + .param("grant_type", "authorization_code") + .param("code", codeValue) + .param("redirect_uri", redirectUri)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.access_token").exists()) + .andReturn() + .getResponse() + .getContentAsString(); + String accessToken = JsonValue.of(tokenResponse).asObject().getString("access_token").string(); + assertNotNull(accessToken); + + // Then: the issued JWT carries the DHIS2 username (NOT the IdP sub) as the username claim. The + // sub and the DHIS2 username deliberately differ, so this is a real regression guard: if the + // lean-principal swap were removed, getName() would be the sub and these would fail (or + // /oauth2/token would 500 on the Jackson allowlist). + assertNotEquals( + oidcPrincipal.getName(), + user.getUsername(), + "the IdP sub must differ from the DHIS2 username, else this test proves nothing"); + Jwt decoded = jwtDecoder.decode(accessToken); + assertEquals(user.getUsername(), decoded.getClaimAsString("username")); + assertNotEquals("google-sub-0001", decoded.getClaimAsString("username")); + // And the email-scope branch of the token customizer also resolves off the lean username. + assertEquals(user.getEmail(), decoded.getClaimAsString("email")); + + // And: an API call with that token resolves to the correct DHIS2 user. + mvc.perform(get("/api/me").header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.username").value(user.getUsername())); + } +} From c4763360b8b94c99d4eacfb57cce91502ccf5e83 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Mon, 1 Jun 2026 01:03:41 +0800 Subject: [PATCH 5/6] chore: declare spring-security-crypto as a test dependency FederatedOidcTokenControllerTest uses PasswordEncoder (spring-security-crypto) to encode the client secret; dhis-test-web-api only pulled it in transitively. Declare it at test scope so the dependency:analyze check (run by the unit-test and sonarqube jobs) passes. Co-Authored-By: Claude Opus 4.8 (1M context) --- dhis-2/dhis-test-web-api/pom.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dhis-2/dhis-test-web-api/pom.xml b/dhis-2/dhis-test-web-api/pom.xml index 53c163d9b3cc..0bf94d12bf0c 100644 --- a/dhis-2/dhis-test-web-api/pom.xml +++ b/dhis-2/dhis-test-web-api/pom.xml @@ -260,6 +260,11 @@ spring-security-core test + + org.springframework.security + spring-security-crypto + test + org.springframework.security spring-security-config From 053a8505b7ca2ef5e421826d649c35c489b580b6 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Mon, 1 Jun 2026 01:14:43 +0800 Subject: [PATCH 6/6] test: use fixed values instead of random UUIDs in the federated OIDC test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud S5977 — tests should not depend on randomly generated values. The test runs in isolation, so a fixed client id and authorization code are safe and deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../security/FederatedOidcTokenControllerTest.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java index c471579962cf..bd5c3296254f 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/security/FederatedOidcTokenControllerTest.java @@ -43,7 +43,6 @@ import java.util.Base64; import java.util.Map; import java.util.Set; -import java.util.UUID; import org.hisp.dhis.common.CodeGenerator; import org.hisp.dhis.jsontree.JsonValue; import org.hisp.dhis.security.jwt.Dhis2JwtAuthenticationManagerResolver; @@ -127,7 +126,7 @@ void federatedOidcLoginIssuesTokenThatResolvesToDhis2User() throws Exception { new OAuth2AuthenticationToken(oidcPrincipal, oidcPrincipal.getAuthorities(), "google"); // And: a confidential authorization_code client (the native app's server-side equivalent). - String clientId = "federated-client-" + UUID.randomUUID(); + String clientId = "federated-test-client"; String clientSecret = "federated-secret"; String redirectUri = "https://app.example.org/callback"; RegisteredClient client = @@ -146,7 +145,7 @@ void federatedOidcLoginIssuesTokenThatResolvesToDhis2User() throws Exception { // And: the authorization the /oauth2/authorize step would have stored after the OIDC login. String issuer = authorizationServerSettings.getIssuer(); - String codeValue = "fed-code-" + UUID.randomUUID(); + String codeValue = "fed-code-value"; OAuth2AuthorizationCode authorizationCode = new OAuth2AuthorizationCode(codeValue, now, now.plus(5, ChronoUnit.MINUTES)); OAuth2AuthorizationRequest authorizationRequest =