From 7a3b0f4ec55cfc6c1d09e8aaa3712875bda7bf19 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sun, 17 May 2026 04:03:14 +0800 Subject: [PATCH 1/8] fix: expose twoFactorType on /api/users endpoint [DHIS2-20097] v42 dropped twoFactorEnabled and userCredentials.twoFA from the User JSON schema, leaving no admin-readable surface for a user's 2FA state on /api/users. This blocks incident response, periodic 2FA-coverage audits and onboarding hardening. Re-expose the field as a read-only TwoFactorType on /api/users and /api/users/{uid}, mirroring the existing User.getName() READ_ONLY pattern (Jackson + schema). Read access stays gated by the existing User resource ACL; write access stays at the /api/2fa/* endpoints only (READ_ONLY blocks PATCH/PUT side-channels). AI Assisted. --- .../main/java/org/hisp/dhis/user/User.java | 4 +- .../test/webapi/json/domain/JsonUser.java | 4 + .../webapi/controller/UserControllerTest.java | 76 +++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java index 92957f13d7a6..b2488391d13a 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java @@ -459,7 +459,9 @@ public void setSecret(String secret) { this.secret = secret; } - @JsonIgnore + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + @JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0) + @Property(access = Property.Access.READ_ONLY) public TwoFactorType getTwoFactorType() { return this.twoFactorType == null ? TwoFactorType.NOT_ENABLED : this.twoFactorType; } diff --git a/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java b/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java index 2cb1b83debc8..110f85908d61 100644 --- a/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java +++ b/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java @@ -74,4 +74,8 @@ default LocalDateTime getLastLogin() { default LocalDateTime getAccountExpiry() { return get("accountExpiry", JsonDate.class).date(); } + + default String getTwoFactorType() { + return getString("twoFactorType").string(); + } } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java index 04eeae212ea6..d86f371140d7 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java @@ -1547,4 +1547,80 @@ void testGetUsersFilterByOrgUnitMembershipWithChildren() { assertFalse( uids.contains(viewer.getUid()), "viewer (in orgA, not orgB) should not be returned"); } + + @Test + @DisplayName("GET /users?fields=twoFactorType exposes the per-user 2FA enrolment state") + void testGetUsers_includesTwoFactorTypeWhenRequested() { + User totpUser = userService.getUser(peter.getUid()); + totpUser.setTwoFactorType(org.hisp.dhis.security.twofa.TwoFactorType.TOTP_ENABLED); + userService.updateUser(totpUser); + + JsonList users = + GET("/users?fields=id,username,twoFactorType&filter=id:eq:" + peter.getUid()) + .content(OK) + .getList("users", JsonUser.class); + + assertEquals(1, users.size()); + assertEquals("TOTP_ENABLED", users.get(0).getTwoFactorType()); + } + + @Test + @DisplayName("GET /users/{uid}?fields=:all includes twoFactorType") + void testGetUser_byUidWithFieldsAllIncludesTwoFactorType() { + User emailUser = userService.getUser(peter.getUid()); + emailUser.setTwoFactorType(org.hisp.dhis.security.twofa.TwoFactorType.EMAIL_ENABLED); + userService.updateUser(emailUser); + + JsonUser user = GET("/users/{id}?fields=:all", peter.getUid()).content(OK).as(JsonUser.class); + + assertEquals("EMAIL_ENABLED", user.getTwoFactorType()); + } + + @Test + @DisplayName("twoFactorType defaults to NOT_ENABLED for users without 2FA") + void testGetUser_twoFactorTypeDefaultsToNotEnabled() { + JsonUser user = + GET("/users/{id}?fields=id,username,twoFactorType", peter.getUid()) + .content(OK) + .as(JsonUser.class); + + assertEquals("NOT_ENABLED", user.getTwoFactorType()); + } + + @Test + @DisplayName("PUT /users/{uid} does not allow setting twoFactorType (read-only)") + void testPutUser_doesNotAllowSettingTwoFactorType() { + User target = userService.getUser(peter.getUid()); + target.setTwoFactorType(org.hisp.dhis.security.twofa.TwoFactorType.NOT_ENABLED); + userService.updateUser(target); + + assertStatus( + HttpStatus.OK, + PATCH( + "/users/{id}?importReportMode=ERRORS", + peter.getUid(), + Body("[{'op': 'replace', 'path': '/twoFactorType', 'value': 'TOTP_ENABLED'}]"))); + + User reloaded = userService.getUser(peter.getUid()); + assertEquals( + org.hisp.dhis.security.twofa.TwoFactorType.NOT_ENABLED, reloaded.getTwoFactorType()); + } + + @Test + @DisplayName("GET /schemas/user declares twoFactorType as a read-only property") + void testUserSchema_declaresTwoFactorTypeAsReadOnly() { + JsonList properties = + GET("/schemas/user").content(OK).getList("properties", JsonObject.class); + + JsonObject twoFactorTypeProp = + properties.stream() + .filter(p -> "twoFactorType".equals(p.getString("name").string())) + .findFirst() + .orElse(null); + + assertNotNull(twoFactorTypeProp, "twoFactorType must be declared in the User schema"); + assertFalse( + twoFactorTypeProp.getBoolean("writable").booleanValue(true), + "twoFactorType must be schema-declared as read-only"); + } } From ac1e4e6b1042931807f5604fdfb4caebd867c038 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sat, 23 May 2026 03:20:06 +0800 Subject: [PATCH 2/8] fix: bump expected user property count after twoFactorType add [DHIS2-20097] FieldPathHelperTest#skipSharingFieldsExcludeCorrectFieldsTest asserts the total number of properties on the User schema. Adding twoFactorType in the previous commit bumped the count from 58 to 59. AI Assisted. --- .../java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java index a2fb5a23b585..3f181aa5abd6 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java @@ -119,7 +119,7 @@ void skipSharingFieldsExcludeCorrectFieldsTest() { // then only matching exclusions should have been applied // and fields starting with 'user' should still be present - assertEquals(58, result.size()); // all user properties + assertEquals(59, result.size()); // all user properties assertTrue( result.stream() .map(FieldPath::getName) From 6f6c94db207d06ad0ed7ef1476fcfd5f0ec92635 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sat, 23 May 2026 04:05:34 +0800 Subject: [PATCH 3/8] redesign: move twoFactorType audit to /api/users/twoFactor [DHIS2-20097] Replace the JSON-layer exposure on /api/users with a dedicated admin-only audit endpoint group. The User resource ACL is too coarse for this field: anyone who can read the User resource would have seen every other user's 2FA state. The new endpoints are gated by the ALL authority (superuser) and keep /api/users clean. - GET /api/users/twoFactor/summary -- totals, byType breakdown, privileged-user (ALL-authority) coverage stats. Computed live; no cache (used rarely). - GET /api/users/twoFactor -- per-user list filterable by ?status= (ALL|ENABLED|DISABLED) and ?type= (multi-value of TwoFactorType). Reverts the User.java / JsonUser.java / UserControllerTest.java changes from the previous attempt and the FieldPathHelperTest count bump. /api/me.twoFactorType is unaffected (it reads via MeDto, not the User Jackson stack). AI Assisted. --- .../main/java/org/hisp/dhis/user/User.java | 4 +- .../test/webapi/json/domain/JsonUser.java | 4 - .../fieldfiltering/FieldPathHelperTest.java | 2 +- .../webapi/controller/UserControllerTest.java | 76 -------- .../UserTwoFactorAuditControllerTest.java | 161 +++++++++++++++++ .../user/UserTwoFactorAuditController.java | 171 ++++++++++++++++++ 6 files changed, 334 insertions(+), 84 deletions(-) create mode 100644 dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java create mode 100644 dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java index b2488391d13a..92957f13d7a6 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java @@ -459,9 +459,7 @@ public void setSecret(String secret) { this.secret = secret; } - @JsonProperty(access = JsonProperty.Access.READ_ONLY) - @JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0) - @Property(access = Property.Access.READ_ONLY) + @JsonIgnore public TwoFactorType getTwoFactorType() { return this.twoFactorType == null ? TwoFactorType.NOT_ENABLED : this.twoFactorType; } diff --git a/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java b/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java index 110f85908d61..2cb1b83debc8 100644 --- a/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java +++ b/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/webapi/json/domain/JsonUser.java @@ -74,8 +74,4 @@ default LocalDateTime getLastLogin() { default LocalDateTime getAccountExpiry() { return get("accountExpiry", JsonDate.class).date(); } - - default String getTwoFactorType() { - return getString("twoFactorType").string(); - } } diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java index 3f181aa5abd6..a2fb5a23b585 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java @@ -119,7 +119,7 @@ void skipSharingFieldsExcludeCorrectFieldsTest() { // then only matching exclusions should have been applied // and fields starting with 'user' should still be present - assertEquals(59, result.size()); // all user properties + assertEquals(58, result.size()); // all user properties assertTrue( result.stream() .map(FieldPath::getName) diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java index d86f371140d7..04eeae212ea6 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java @@ -1547,80 +1547,4 @@ void testGetUsersFilterByOrgUnitMembershipWithChildren() { assertFalse( uids.contains(viewer.getUid()), "viewer (in orgA, not orgB) should not be returned"); } - - @Test - @DisplayName("GET /users?fields=twoFactorType exposes the per-user 2FA enrolment state") - void testGetUsers_includesTwoFactorTypeWhenRequested() { - User totpUser = userService.getUser(peter.getUid()); - totpUser.setTwoFactorType(org.hisp.dhis.security.twofa.TwoFactorType.TOTP_ENABLED); - userService.updateUser(totpUser); - - JsonList users = - GET("/users?fields=id,username,twoFactorType&filter=id:eq:" + peter.getUid()) - .content(OK) - .getList("users", JsonUser.class); - - assertEquals(1, users.size()); - assertEquals("TOTP_ENABLED", users.get(0).getTwoFactorType()); - } - - @Test - @DisplayName("GET /users/{uid}?fields=:all includes twoFactorType") - void testGetUser_byUidWithFieldsAllIncludesTwoFactorType() { - User emailUser = userService.getUser(peter.getUid()); - emailUser.setTwoFactorType(org.hisp.dhis.security.twofa.TwoFactorType.EMAIL_ENABLED); - userService.updateUser(emailUser); - - JsonUser user = GET("/users/{id}?fields=:all", peter.getUid()).content(OK).as(JsonUser.class); - - assertEquals("EMAIL_ENABLED", user.getTwoFactorType()); - } - - @Test - @DisplayName("twoFactorType defaults to NOT_ENABLED for users without 2FA") - void testGetUser_twoFactorTypeDefaultsToNotEnabled() { - JsonUser user = - GET("/users/{id}?fields=id,username,twoFactorType", peter.getUid()) - .content(OK) - .as(JsonUser.class); - - assertEquals("NOT_ENABLED", user.getTwoFactorType()); - } - - @Test - @DisplayName("PUT /users/{uid} does not allow setting twoFactorType (read-only)") - void testPutUser_doesNotAllowSettingTwoFactorType() { - User target = userService.getUser(peter.getUid()); - target.setTwoFactorType(org.hisp.dhis.security.twofa.TwoFactorType.NOT_ENABLED); - userService.updateUser(target); - - assertStatus( - HttpStatus.OK, - PATCH( - "/users/{id}?importReportMode=ERRORS", - peter.getUid(), - Body("[{'op': 'replace', 'path': '/twoFactorType', 'value': 'TOTP_ENABLED'}]"))); - - User reloaded = userService.getUser(peter.getUid()); - assertEquals( - org.hisp.dhis.security.twofa.TwoFactorType.NOT_ENABLED, reloaded.getTwoFactorType()); - } - - @Test - @DisplayName("GET /schemas/user declares twoFactorType as a read-only property") - void testUserSchema_declaresTwoFactorTypeAsReadOnly() { - JsonList properties = - GET("/schemas/user").content(OK).getList("properties", JsonObject.class); - - JsonObject twoFactorTypeProp = - properties.stream() - .filter(p -> "twoFactorType".equals(p.getString("name").string())) - .findFirst() - .orElse(null); - - assertNotNull(twoFactorTypeProp, "twoFactorType must be declared in the User schema"); - assertFalse( - twoFactorTypeProp.getBoolean("writable").booleanValue(true), - "twoFactorType must be schema-declared as read-only"); - } } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java new file mode 100644 index 000000000000..f52de563d2d8 --- /dev/null +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java @@ -0,0 +1,161 @@ +/* + * Copyright (c) 2004-2026, 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; + +import static org.hisp.dhis.http.HttpStatus.FORBIDDEN; +import static org.hisp.dhis.http.HttpStatus.OK; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import org.hisp.dhis.jsontree.JsonList; +import org.hisp.dhis.jsontree.JsonObject; +import org.hisp.dhis.security.twofa.TwoFactorType; +import org.hisp.dhis.test.webapi.H2ControllerIntegrationTestBase; +import org.hisp.dhis.user.User; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.transaction.annotation.Transactional; + +@Transactional +class UserTwoFactorAuditControllerTest extends H2ControllerIntegrationTestBase { + + private User totpUser; + private User emailUser; + private User plainUser; + + @BeforeEach + void setUpUsers() { + totpUser = createUserWithTwoFactorType("totp", TwoFactorType.TOTP_ENABLED); + emailUser = createUserWithTwoFactorType("email", TwoFactorType.EMAIL_ENABLED); + plainUser = createUserWithTwoFactorType("plain", TwoFactorType.NOT_ENABLED); + } + + @Test + @DisplayName("GET /users/twoFactor/summary returns counts grouped by 2FA type") + void testSummary_returnsCountsByType() { + JsonObject summary = GET("/users/twoFactor/summary").content(OK); + + assertTrue( + summary.getNumber("totalUsers").integer() >= 3, + "summary must count at least the three seeded users"); + assertTrue( + summary.getNumber("enabled").integer() >= 2, + "totp + email users must be counted as enabled"); + assertTrue( + summary.getNumber("disabled").integer() >= 1, "plain user must be counted as disabled"); + + JsonObject byType = summary.getObject("byType"); + assertTrue(byType.getNumber("TOTP_ENABLED").integer() >= 1); + assertTrue(byType.getNumber("EMAIL_ENABLED").integer() >= 1); + assertTrue(byType.getNumber("NOT_ENABLED").integer() >= 1); + + JsonObject privileged = summary.getObject("privileged"); + assertTrue(privileged.getNumber("withAllAuthority").integer() >= 1, "admin counts as super"); + } + + @Test + @DisplayName("GET /users/twoFactor/summary is forbidden for non-superusers") + void testSummary_forbiddenForNonSuperuser() { + switchToNewUser("regular"); + assertEquals(FORBIDDEN, GET("/users/twoFactor/summary").status()); + } + + @Test + @DisplayName("GET /users/twoFactor lists all users with their 2FA type by default") + void testList_returnsAllByDefault() { + JsonObject body = GET("/users/twoFactor").content(OK); + JsonList users = body.getList("users", JsonObject.class); + + assertContainsUid(users, totpUser.getUid(), "TOTP_ENABLED"); + assertContainsUid(users, emailUser.getUid(), "EMAIL_ENABLED"); + assertContainsUid(users, plainUser.getUid(), "NOT_ENABLED"); + } + + @Test + @DisplayName("GET /users/twoFactor?status=ENABLED hides users without 2FA") + void testList_filterByStatusEnabled() { + JsonList users = + GET("/users/twoFactor?status=ENABLED").content(OK).getList("users", JsonObject.class); + + List uids = users.stream().map(u -> u.getString("id").string()).toList(); + assertTrue(uids.contains(totpUser.getUid())); + assertTrue(uids.contains(emailUser.getUid())); + assertTrue(uids.stream().noneMatch(plainUser.getUid()::equals)); + } + + @Test + @DisplayName("GET /users/twoFactor?status=DISABLED returns only users without 2FA") + void testList_filterByStatusDisabled() { + JsonList users = + GET("/users/twoFactor?status=DISABLED").content(OK).getList("users", JsonObject.class); + + List uids = users.stream().map(u -> u.getString("id").string()).toList(); + assertTrue(uids.contains(plainUser.getUid())); + assertTrue(uids.stream().noneMatch(totpUser.getUid()::equals)); + assertTrue(uids.stream().noneMatch(emailUser.getUid()::equals)); + } + + @Test + @DisplayName("GET /users/twoFactor?type=TOTP_ENABLED filters by 2FA type") + void testList_filterByType() { + JsonList users = + GET("/users/twoFactor?type=TOTP_ENABLED").content(OK).getList("users", JsonObject.class); + + List uids = users.stream().map(u -> u.getString("id").string()).toList(); + assertTrue(uids.contains(totpUser.getUid())); + assertTrue(uids.stream().noneMatch(emailUser.getUid()::equals)); + assertTrue(uids.stream().noneMatch(plainUser.getUid()::equals)); + } + + @Test + @DisplayName("GET /users/twoFactor is forbidden for non-superusers") + void testList_forbiddenForNonSuperuser() { + switchToNewUser("regular"); + assertEquals(FORBIDDEN, GET("/users/twoFactor").status()); + } + + private User createUserWithTwoFactorType(String username, TwoFactorType type) { + User user = createUserWithAuth(username); + user.setTwoFactorType(type); + userService.updateUser(user); + return user; + } + + private static void assertContainsUid(JsonList users, String uid, String expectType) { + JsonObject match = + users.stream() + .filter(u -> uid.equals(u.getString("id").string())) + .findFirst() + .orElseThrow(() -> new AssertionError("user " + uid + " not in list")); + assertEquals(expectType, match.getString("twoFactorType").string()); + } +} diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java new file mode 100644 index 000000000000..fe717326e4fa --- /dev/null +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java @@ -0,0 +1,171 @@ +/* + * Copyright (c) 2004-2026, 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.user; + +import static org.hisp.dhis.security.Authorities.ALL; + +import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.Comparator; +import java.util.Date; +import java.util.EnumMap; +import java.util.List; +import java.util.Map; +import javax.annotation.CheckForNull; +import lombok.RequiredArgsConstructor; +import org.hisp.dhis.common.OpenApi; +import org.hisp.dhis.security.RequiresAuthority; +import org.hisp.dhis.security.twofa.TwoFactorType; +import org.hisp.dhis.user.User; +import org.hisp.dhis.user.UserService; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +/** + * Admin-only 2FA enrolment audit for the user base. Restricted to callers holding {@link + * org.hisp.dhis.security.Authorities#ALL}. + * + * @author Morten Svanaes + */ +@OpenApi.Document( + group = OpenApi.Document.GROUP_QUERY, + classifiers = {"team:platform", "purpose:security"}) +@RestController +@RequestMapping("/api/users/twoFactor") +@RequiredArgsConstructor +@RequiresAuthority(anyOf = ALL) +public class UserTwoFactorAuditController { + + private final UserService userService; + + @GetMapping("/summary") + public TwoFactorAuditSummary getSummary() { + Map byType = new EnumMap<>(TwoFactorType.class); + for (TwoFactorType type : TwoFactorType.values()) { + byType.put(type, 0L); + } + long total = 0; + long enabled = 0; + long withAllAuthority = 0; + long withAllAuthorityMissing2FA = 0; + for (User user : userService.getAllUsers()) { + total++; + TwoFactorType type = effectiveType(user); + byType.merge(type, 1L, Long::sum); + if (type.isEnabled()) { + enabled++; + } + if (user.isSuper()) { + withAllAuthority++; + if (!type.isEnabled()) { + withAllAuthorityMissing2FA++; + } + } + } + long disabled = total - enabled; + double coverage = total == 0 ? 0d : Math.round((double) enabled / total * 1000d) / 10d; + return new TwoFactorAuditSummary( + total, + enabled, + disabled, + coverage, + byType, + new PrivilegedUserStats(withAllAuthority, withAllAuthorityMissing2FA)); + } + + @GetMapping + public TwoFactorAuditList getList( + @RequestParam(required = false, defaultValue = "ALL") AuditStatus status, + @CheckForNull @RequestParam(required = false) List type) { + List entries = + userService.getAllUsers().stream() + .filter(u -> matchesStatus(u, status)) + .filter(u -> matchesType(u, type)) + .sorted(Comparator.comparing(User::getUsername, String.CASE_INSENSITIVE_ORDER)) + .map(UserTwoFactorAuditController::toEntry) + .toList(); + return new TwoFactorAuditList(entries.size(), entries); + } + + private static boolean matchesStatus(User user, AuditStatus status) { + TwoFactorType type = effectiveType(user); + return switch (status) { + case ALL -> true; + case ENABLED -> type.isEnabled(); + case DISABLED -> !type.isEnabled(); + }; + } + + private static boolean matchesType(User user, @CheckForNull List types) { + return types == null || types.isEmpty() || types.contains(effectiveType(user)); + } + + private static TwoFactorType effectiveType(User user) { + TwoFactorType type = user.getTwoFactorType(); + return type == null ? TwoFactorType.NOT_ENABLED : type; + } + + private static TwoFactorAuditEntry toEntry(User user) { + return new TwoFactorAuditEntry( + user.getUid(), + user.getUsername(), + user.getName(), + effectiveType(user), + user.getLastLogin()); + } + + public enum AuditStatus { + ALL, + ENABLED, + DISABLED + } + + public record TwoFactorAuditSummary( + @JsonProperty long totalUsers, + @JsonProperty long enabled, + @JsonProperty long disabled, + @JsonProperty double coveragePercent, + @JsonProperty Map byType, + @JsonProperty PrivilegedUserStats privileged) {} + + public record PrivilegedUserStats( + @JsonProperty long withAllAuthority, @JsonProperty long withAllAuthorityMissing2FA) {} + + public record TwoFactorAuditList( + @JsonProperty long total, @JsonProperty List users) {} + + public record TwoFactorAuditEntry( + @JsonProperty String id, + @JsonProperty String username, + @JsonProperty String name, + @JsonProperty TwoFactorType twoFactorType, + @JsonProperty Date lastLogin) {} +} From 73a19046f89cc2f6c79d93c9c0909d199d3ed548 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sat, 23 May 2026 05:30:06 +0800 Subject: [PATCH 4/8] perf: back /api/users/twoFactor with native SQL projections [DHIS2-20097] Replace the controller-side userService.getAllUsers() scan with a TwoFactorAuditQueryService that aggregates counts and projects user rows directly via JdbcTemplate. Avoids hydrating User entities and their lazy userRoles collections, so a single privileged call no longer pulls the full user-role-group graph into memory. - countByType(): one GROUP BY against userinfo - countPrivileged(): single join over userrolemembers + userroleauthorities with a FILTER aggregate for the missing-2FA subset - count(filter) / list(filter, offset, limit): conditional WHERE built from status + type filters, paged DB-side with LIMIT/OFFSET The list endpoint now returns a standard DHIS2 Pager wrapper and accepts ?page, ?pageSize, ?paging=false. Default pageSize=50, max=1000. Mirrors the JdbcStatisticsProvider pattern already used in dhis-service-administration. AI Assisted. --- .../audit/TwoFactorAuditQueryService.java | 182 ++++++++++++++++++ .../UserTwoFactorAuditControllerTest.java | 15 ++ .../user/UserTwoFactorAuditController.java | 103 ++++------ 3 files changed, 233 insertions(+), 67 deletions(-) create mode 100644 dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java new file mode 100644 index 000000000000..e2970f1803df --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java @@ -0,0 +1,182 @@ +/* + * Copyright (c) 2004-2026, 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.twofa.audit; + +import java.util.ArrayList; +import java.util.Date; +import java.util.EnumMap; +import java.util.List; +import java.util.Map; +import javax.annotation.CheckForNull; +import lombok.RequiredArgsConstructor; +import org.hisp.dhis.security.twofa.TwoFactorType; +import org.springframework.jdbc.core.JdbcTemplate; +import org.springframework.stereotype.Service; + +/** + * Native-SQL backed provider for the 2FA enrolment audit endpoints. Aggregates counts and lists + * users directly against the {@code userinfo} / {@code userrolemembers} / {@code + * userroleauthorities} tables, avoiding full-graph hydration of {@code User} entities and their + * lazy {@code userRoles} collections. + * + * @author Morten Svanaes + */ +@Service +@RequiredArgsConstructor +public class TwoFactorAuditQueryService { + + public enum Status { + ALL, + ENABLED, + DISABLED + } + + private static final String ENABLED_TYPES_SQL_LIST = "('TOTP_ENABLED','EMAIL_ENABLED')"; + + private final JdbcTemplate jdbcTemplate; + + /** Returns the row count of {@code userinfo} grouped by {@code twofactortype}. */ + public Map countByType() { + Map result = new EnumMap<>(TwoFactorType.class); + for (TwoFactorType type : TwoFactorType.values()) { + result.put(type, 0L); + } + jdbcTemplate.query( + "SELECT twofactortype, COUNT(*) FROM userinfo GROUP BY twofactortype", + rs -> { + String raw = rs.getString(1); + if (raw != null) { + try { + result.put(TwoFactorType.valueOf(raw), rs.getLong(2)); + } catch (IllegalArgumentException ignore) { + // Out-of-enum value in the column — drop it from the breakdown. + } + } + }); + return result; + } + + /** + * Returns the count of users holding the {@code ALL} authority and how many of them have no + * active 2FA. Done in a single query to keep the privileged-user detection on the DB side. + */ + public PrivilegedCounts countPrivileged() { + String sql = + "SELECT COUNT(DISTINCT urm.userid) AS with_all," + + " COUNT(DISTINCT urm.userid) FILTER (" + + " WHERE u.twofactortype NOT IN " + + ENABLED_TYPES_SQL_LIST + + " ) AS with_all_missing" + + " FROM userrolemembers urm" + + " JOIN userroleauthorities ura ON ura.userroleid = urm.userroleid" + + " JOIN userinfo u ON u.userinfoid = urm.userid" + + " WHERE ura.authority = 'ALL'"; + PrivilegedCounts counts = + jdbcTemplate.queryForObject( + sql, (rs, n) -> new PrivilegedCounts(rs.getLong(1), rs.getLong(2))); + return counts == null ? new PrivilegedCounts(0L, 0L) : counts; + } + + /** Returns the number of users matching the given filter. */ + public int count(Status status, @CheckForNull List types) { + StringBuilder sql = new StringBuilder("SELECT COUNT(*) FROM userinfo WHERE 1=1"); + List params = new ArrayList<>(); + appendStatusClause(sql, status); + appendTypeClause(sql, params, types); + Integer count = jdbcTemplate.queryForObject(sql.toString(), Integer.class, params.toArray()); + return count == null ? 0 : count; + } + + /** + * Returns the matching user rows projected to the audit-row shape. {@code offset}/{@code limit} + * are applied DB-side via {@code OFFSET} / {@code LIMIT}; pass {@code limit < 0} to return all + * matches. + */ + public List list( + Status status, @CheckForNull List types, int offset, int limit) { + StringBuilder sql = + new StringBuilder( + "SELECT uid, username, name, twofactortype, lastlogin FROM userinfo WHERE 1=1"); + List params = new ArrayList<>(); + appendStatusClause(sql, status); + appendTypeClause(sql, params, types); + sql.append(" ORDER BY LOWER(username)"); + if (limit >= 0) { + sql.append(" LIMIT ? OFFSET ?"); + params.add(limit); + params.add(Math.max(0, offset)); + } + return jdbcTemplate.query( + sql.toString(), + params.toArray(), + (rs, n) -> + new UserAuditRow( + rs.getString("uid"), + rs.getString("username"), + rs.getString("name"), + parseType(rs.getString("twofactortype")), + rs.getTimestamp("lastlogin"))); + } + + private static void appendStatusClause(StringBuilder sql, Status status) { + switch (status) { + case ENABLED -> sql.append(" AND twofactortype IN ").append(ENABLED_TYPES_SQL_LIST); + case DISABLED -> sql.append(" AND twofactortype NOT IN ").append(ENABLED_TYPES_SQL_LIST); + case ALL -> { + // no-op + } + } + } + + private static void appendTypeClause( + StringBuilder sql, List params, @CheckForNull List types) { + if (types == null || types.isEmpty()) return; + sql.append(" AND twofactortype IN ("); + for (int i = 0; i < types.size(); i++) { + sql.append(i == 0 ? "?" : ",?"); + params.add(types.get(i).name()); + } + sql.append(")"); + } + + private static TwoFactorType parseType(@CheckForNull String raw) { + if (raw == null) return TwoFactorType.NOT_ENABLED; + try { + return TwoFactorType.valueOf(raw); + } catch (IllegalArgumentException e) { + return TwoFactorType.NOT_ENABLED; + } + } + + public record PrivilegedCounts(long withAllAuthority, long withAllAuthorityMissing2FA) {} + + public record UserAuditRow( + String uid, String username, String name, TwoFactorType twoFactorType, Date lastLogin) {} +} diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java index f52de563d2d8..0ca89eda90df 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java @@ -143,10 +143,25 @@ void testList_forbiddenForNonSuperuser() { assertEquals(FORBIDDEN, GET("/users/twoFactor").status()); } + @Test + @DisplayName("GET /users/twoFactor pages the result with pager total reflecting full match set") + void testList_paging() { + JsonObject body = GET("/users/twoFactor?pageSize=2&page=1").content(OK); + + JsonObject pager = body.getObject("pager"); + assertTrue( + pager.getNumber("total").integer() >= 3, + "pager.total must include every matching row, not just the page"); + assertEquals(2, pager.getNumber("pageSize").integer()); + assertEquals(1, pager.getNumber("page").integer()); + assertEquals(2, body.getList("users", JsonObject.class).size()); + } + private User createUserWithTwoFactorType(String username, TwoFactorType type) { User user = createUserWithAuth(username); user.setTwoFactorType(type); userService.updateUser(user); + manager.flush(); // ensure twofactortype is visible to JDBC reads inside the controller return user; } diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java index fe717326e4fa..fc4dbfc3c55c 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java @@ -32,18 +32,19 @@ import static org.hisp.dhis.security.Authorities.ALL; import com.fasterxml.jackson.annotation.JsonProperty; -import java.util.Comparator; import java.util.Date; -import java.util.EnumMap; import java.util.List; import java.util.Map; import javax.annotation.CheckForNull; import lombok.RequiredArgsConstructor; import org.hisp.dhis.common.OpenApi; +import org.hisp.dhis.common.Pager; import org.hisp.dhis.security.RequiresAuthority; import org.hisp.dhis.security.twofa.TwoFactorType; -import org.hisp.dhis.user.User; -import org.hisp.dhis.user.UserService; +import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService; +import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService.PrivilegedCounts; +import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService.Status; +import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService.UserAuditRow; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; @@ -51,7 +52,9 @@ /** * Admin-only 2FA enrolment audit for the user base. Restricted to callers holding {@link - * org.hisp.dhis.security.Authorities#ALL}. + * org.hisp.dhis.security.Authorities#ALL}. All aggregation and filtering is delegated to {@link + * TwoFactorAuditQueryService}, which runs native SQL against the user tables — no User entities or + * lazy role collections are hydrated here. * * @author Morten Svanaes */ @@ -64,88 +67,54 @@ @RequiresAuthority(anyOf = ALL) public class UserTwoFactorAuditController { - private final UserService userService; + private static final int DEFAULT_PAGE_SIZE = 50; + private static final int MAX_PAGE_SIZE = 1000; + + private final TwoFactorAuditQueryService auditService; @GetMapping("/summary") public TwoFactorAuditSummary getSummary() { - Map byType = new EnumMap<>(TwoFactorType.class); - for (TwoFactorType type : TwoFactorType.values()) { - byType.put(type, 0L); - } - long total = 0; - long enabled = 0; - long withAllAuthority = 0; - long withAllAuthorityMissing2FA = 0; - for (User user : userService.getAllUsers()) { - total++; - TwoFactorType type = effectiveType(user); - byType.merge(type, 1L, Long::sum); - if (type.isEnabled()) { - enabled++; - } - if (user.isSuper()) { - withAllAuthority++; - if (!type.isEnabled()) { - withAllAuthorityMissing2FA++; - } - } - } + Map byType = auditService.countByType(); + long total = byType.values().stream().mapToLong(Long::longValue).sum(); + long enabled = + byType.getOrDefault(TwoFactorType.TOTP_ENABLED, 0L) + + byType.getOrDefault(TwoFactorType.EMAIL_ENABLED, 0L); long disabled = total - enabled; double coverage = total == 0 ? 0d : Math.round((double) enabled / total * 1000d) / 10d; + PrivilegedCounts privileged = auditService.countPrivileged(); return new TwoFactorAuditSummary( total, enabled, disabled, coverage, byType, - new PrivilegedUserStats(withAllAuthority, withAllAuthorityMissing2FA)); + new PrivilegedUserStats( + privileged.withAllAuthority(), privileged.withAllAuthorityMissing2FA())); } @GetMapping public TwoFactorAuditList getList( - @RequestParam(required = false, defaultValue = "ALL") AuditStatus status, - @CheckForNull @RequestParam(required = false) List type) { + @RequestParam(required = false, defaultValue = "ALL") Status status, + @CheckForNull @RequestParam(required = false) List type, + @RequestParam(required = false, defaultValue = "true") boolean paging, + @RequestParam(required = false, defaultValue = "1") int page, + @RequestParam(required = false, defaultValue = "" + DEFAULT_PAGE_SIZE) int pageSize) { + int total = auditService.count(status, type); + int effectivePage = Math.max(1, page); + int effectivePageSize = paging ? Math.min(Math.max(1, pageSize), MAX_PAGE_SIZE) : total; + int offset = paging ? (effectivePage - 1) * effectivePageSize : 0; + int limit = paging ? effectivePageSize : -1; List entries = - userService.getAllUsers().stream() - .filter(u -> matchesStatus(u, status)) - .filter(u -> matchesType(u, type)) - .sorted(Comparator.comparing(User::getUsername, String.CASE_INSENSITIVE_ORDER)) + auditService.list(status, type, offset, limit).stream() .map(UserTwoFactorAuditController::toEntry) .toList(); - return new TwoFactorAuditList(entries.size(), entries); - } - - private static boolean matchesStatus(User user, AuditStatus status) { - TwoFactorType type = effectiveType(user); - return switch (status) { - case ALL -> true; - case ENABLED -> type.isEnabled(); - case DISABLED -> !type.isEnabled(); - }; - } - - private static boolean matchesType(User user, @CheckForNull List types) { - return types == null || types.isEmpty() || types.contains(effectiveType(user)); + Pager pager = new Pager(effectivePage, total, paging ? effectivePageSize : Math.max(1, total)); + return new TwoFactorAuditList(pager, entries); } - private static TwoFactorType effectiveType(User user) { - TwoFactorType type = user.getTwoFactorType(); - return type == null ? TwoFactorType.NOT_ENABLED : type; - } - - private static TwoFactorAuditEntry toEntry(User user) { + private static TwoFactorAuditEntry toEntry(UserAuditRow row) { return new TwoFactorAuditEntry( - user.getUid(), - user.getUsername(), - user.getName(), - effectiveType(user), - user.getLastLogin()); - } - - public enum AuditStatus { - ALL, - ENABLED, - DISABLED + row.uid(), row.username(), row.name(), row.twoFactorType(), row.lastLogin()); } public record TwoFactorAuditSummary( @@ -160,7 +129,7 @@ public record PrivilegedUserStats( @JsonProperty long withAllAuthority, @JsonProperty long withAllAuthorityMissing2FA) {} public record TwoFactorAuditList( - @JsonProperty long total, @JsonProperty List users) {} + @JsonProperty Pager pager, @JsonProperty List users) {} public record TwoFactorAuditEntry( @JsonProperty String id, From 962851520e8084494d5acbfe9365e8cac0d5406d Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sat, 23 May 2026 06:07:58 +0800 Subject: [PATCH 5/8] fix: clamp out-of-bounds paging + NULL-safe SQL filters [DHIS2-20097] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from adversarial review: * The controller computed the DB offset from the raw request `page` while Pager's constructor silently clamps page to [1, pageCount]. Asking for page=999 returned an empty list with `pager.page` reporting a different page — a payload that contradicted itself. Build the Pager first and drive the query off `pager.getOffset()` / `pager.getPageSize()` so both metadata and rows agree. * Although the Postgres column is NOT NULL today, the Hibernate mapping declares twofactortype nullable. Under three-valued SQL logic a stray NULL would slip past `... NOT IN ('TOTP_ENABLED','EMAIL_ENABLED')` (UNKNOWN, excluded) and corrupt every privileged/disabled count. Coalesce defensively in countByType, countPrivileged, and the status/type filter clauses. Tests: new out-of-bounds-page case asserts pager.page matches pageCount and the response carries the last page's rows (not an empty list). AI Assisted. --- .../audit/TwoFactorAuditQueryService.java | 26 +++++++++++++++---- .../UserTwoFactorAuditControllerTest.java | 17 ++++++++++++ .../user/UserTwoFactorAuditController.java | 14 ++++++---- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java index e2970f1803df..08c21252bfb2 100644 --- a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java @@ -60,6 +60,11 @@ public enum Status { private static final String ENABLED_TYPES_SQL_LIST = "('TOTP_ENABLED','EMAIL_ENABLED')"; + // The DB column is NOT NULL today, but the Hibernate mapping declares it + // nullable; coalesce defensively so any drift can never silently misbucket + // rows under three-valued logic ({@code NULL NOT IN (...)} -> UNKNOWN). + private static final String EFFECTIVE_TYPE_SQL = "COALESCE(twofactortype, 'NOT_ENABLED')"; + private final JdbcTemplate jdbcTemplate; /** Returns the row count of {@code userinfo} grouped by {@code twofactortype}. */ @@ -69,7 +74,10 @@ public Map countByType() { result.put(type, 0L); } jdbcTemplate.query( - "SELECT twofactortype, COUNT(*) FROM userinfo GROUP BY twofactortype", + "SELECT " + + EFFECTIVE_TYPE_SQL + + " AS effective_type, COUNT(*) FROM userinfo GROUP BY " + + EFFECTIVE_TYPE_SQL, rs -> { String raw = rs.getString(1); if (raw != null) { @@ -91,7 +99,7 @@ public PrivilegedCounts countPrivileged() { String sql = "SELECT COUNT(DISTINCT urm.userid) AS with_all," + " COUNT(DISTINCT urm.userid) FILTER (" - + " WHERE u.twofactortype NOT IN " + + " WHERE COALESCE(u.twofactortype, 'NOT_ENABLED') NOT IN " + ENABLED_TYPES_SQL_LIST + " ) AS with_all_missing" + " FROM userrolemembers urm" @@ -147,8 +155,16 @@ public List list( private static void appendStatusClause(StringBuilder sql, Status status) { switch (status) { - case ENABLED -> sql.append(" AND twofactortype IN ").append(ENABLED_TYPES_SQL_LIST); - case DISABLED -> sql.append(" AND twofactortype NOT IN ").append(ENABLED_TYPES_SQL_LIST); + case ENABLED -> + sql.append(" AND ") + .append(EFFECTIVE_TYPE_SQL) + .append(" IN ") + .append(ENABLED_TYPES_SQL_LIST); + case DISABLED -> + sql.append(" AND ") + .append(EFFECTIVE_TYPE_SQL) + .append(" NOT IN ") + .append(ENABLED_TYPES_SQL_LIST); case ALL -> { // no-op } @@ -158,7 +174,7 @@ private static void appendStatusClause(StringBuilder sql, Status status) { private static void appendTypeClause( StringBuilder sql, List params, @CheckForNull List types) { if (types == null || types.isEmpty()) return; - sql.append(" AND twofactortype IN ("); + sql.append(" AND ").append(EFFECTIVE_TYPE_SQL).append(" IN ("); for (int i = 0; i < types.size(); i++) { sql.append(i == 0 ? "?" : ",?"); params.add(types.get(i).name()); diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java index 0ca89eda90df..1a664faec91b 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserTwoFactorAuditControllerTest.java @@ -157,6 +157,23 @@ void testList_paging() { assertEquals(2, body.getList("users", JsonObject.class).size()); } + @Test + @DisplayName( + "GET /users/twoFactor with out-of-bounds page returns clamped page with matching data") + void testList_outOfBoundsPage() { + JsonObject body = GET("/users/twoFactor?pageSize=2&page=999").content(OK); + + JsonObject pager = body.getObject("pager"); + int reportedPage = pager.getNumber("page").integer(); + int pageCount = pager.getNumber("pageCount").integer(); + assertEquals( + pageCount, reportedPage, "Out-of-bounds page must be clamped to the last available page"); + assertTrue( + body.getList("users", JsonObject.class).size() > 0, + "Response body must contain the rows for the page reported by pager.page," + + " not an empty list mismatched against pager.page"); + } + private User createUserWithTwoFactorType(String username, TwoFactorType type) { User user = createUserWithAuth(username); user.setTwoFactorType(type); diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java index fc4dbfc3c55c..295a60fd54ab 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java @@ -100,15 +100,19 @@ public TwoFactorAuditList getList( @RequestParam(required = false, defaultValue = "1") int page, @RequestParam(required = false, defaultValue = "" + DEFAULT_PAGE_SIZE) int pageSize) { int total = auditService.count(status, type); - int effectivePage = Math.max(1, page); - int effectivePageSize = paging ? Math.min(Math.max(1, pageSize), MAX_PAGE_SIZE) : total; - int offset = paging ? (effectivePage - 1) * effectivePageSize : 0; - int limit = paging ? effectivePageSize : -1; + int requestedPageSize = + paging ? Math.min(Math.max(1, pageSize), MAX_PAGE_SIZE) : Math.max(1, total); + // Build the Pager first so its built-in [1, pageCount] page clamp drives + // BOTH the response metadata and the DB offset/limit — an out-of-bounds + // page request can no longer return an empty list with pager.page pinned + // to a different (clamped) page. + Pager pager = new Pager(page, total, requestedPageSize); + int offset = paging ? pager.getOffset() : 0; + int limit = paging ? pager.getPageSize() : -1; List entries = auditService.list(status, type, offset, limit).stream() .map(UserTwoFactorAuditController::toEntry) .toList(); - Pager pager = new Pager(effectivePage, total, paging ? effectivePageSize : Math.max(1, total)); return new TwoFactorAuditList(pager, entries); } From 32586aa60b55d24ccfe31b484d92a71771b225ec Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Mon, 25 May 2026 02:30:08 +0800 Subject: [PATCH 6/8] fix: harden audit queries per adversarial review [DHIS2-20097] Four findings from the Antigravity adversarial review: * ORDER BY LOWER(username) defeated the unique index on username, forcing a full-table filesort. Removed the LOWER() wrapper since DHIS2 already enforces lowercase usernames at creation time. * countByType / countPrivileged / count / list scanned all rows including disabled accounts and uncompleted invitations, skewing 2FA coverage percentages and inflating the privileged-at-risk count. Added AND disabled = false AND invitation = false to every base WHERE clause. * The list projection lacked email, disabled, and invitation fields, making it impossible for auditors to contact non-compliant users or distinguish active from deactivated accounts without a second query. Added all three to UserAuditRow and TwoFactorAuditEntry. * paging=false set the SQL LIMIT to -1 (unbounded), risking OOM on large instances. Capped at UNPAGED_HARD_CEILING (10 000 rows). AI Assisted. --- .../audit/TwoFactorAuditQueryService.java | 51 ++++++++++++------- .../user/UserTwoFactorAuditController.java | 25 ++++++--- 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java index 08c21252bfb2..10de92237c24 100644 --- a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java @@ -60,14 +60,15 @@ public enum Status { private static final String ENABLED_TYPES_SQL_LIST = "('TOTP_ENABLED','EMAIL_ENABLED')"; - // The DB column is NOT NULL today, but the Hibernate mapping declares it - // nullable; coalesce defensively so any drift can never silently misbucket - // rows under three-valued logic ({@code NULL NOT IN (...)} -> UNKNOWN). private static final String EFFECTIVE_TYPE_SQL = "COALESCE(twofactortype, 'NOT_ENABLED')"; + // Only count accounts that can actually log in. + private static final String ACTIVE_ACCOUNT_FILTER = + " AND disabled = false AND invitation = false"; + private final JdbcTemplate jdbcTemplate; - /** Returns the row count of {@code userinfo} grouped by {@code twofactortype}. */ + /** Returns the row count of active users grouped by {@code twofactortype}. */ public Map countByType() { Map result = new EnumMap<>(TwoFactorType.class); for (TwoFactorType type : TwoFactorType.values()) { @@ -76,7 +77,9 @@ public Map countByType() { jdbcTemplate.query( "SELECT " + EFFECTIVE_TYPE_SQL - + " AS effective_type, COUNT(*) FROM userinfo GROUP BY " + + " AS effective_type, COUNT(*) FROM userinfo WHERE 1=1" + + ACTIVE_ACCOUNT_FILTER + + " GROUP BY " + EFFECTIVE_TYPE_SQL, rs -> { String raw = rs.getString(1); @@ -84,7 +87,7 @@ public Map countByType() { try { result.put(TwoFactorType.valueOf(raw), rs.getLong(2)); } catch (IllegalArgumentException ignore) { - // Out-of-enum value in the column — drop it from the breakdown. + // Out-of-enum value in the column. } } }); @@ -92,8 +95,8 @@ public Map countByType() { } /** - * Returns the count of users holding the {@code ALL} authority and how many of them have no - * active 2FA. Done in a single query to keep the privileged-user detection on the DB side. + * Returns the count of active users holding the {@code ALL} authority and how many of them have + * no active 2FA. */ public PrivilegedCounts countPrivileged() { String sql = @@ -105,16 +108,18 @@ public PrivilegedCounts countPrivileged() { + " FROM userrolemembers urm" + " JOIN userroleauthorities ura ON ura.userroleid = urm.userroleid" + " JOIN userinfo u ON u.userinfoid = urm.userid" - + " WHERE ura.authority = 'ALL'"; + + " WHERE ura.authority = 'ALL'" + + " AND u.disabled = false AND u.invitation = false"; PrivilegedCounts counts = jdbcTemplate.queryForObject( sql, (rs, n) -> new PrivilegedCounts(rs.getLong(1), rs.getLong(2))); return counts == null ? new PrivilegedCounts(0L, 0L) : counts; } - /** Returns the number of users matching the given filter. */ + /** Returns the number of active users matching the given filter. */ public int count(Status status, @CheckForNull List types) { StringBuilder sql = new StringBuilder("SELECT COUNT(*) FROM userinfo WHERE 1=1"); + sql.append(ACTIVE_ACCOUNT_FILTER); List params = new ArrayList<>(); appendStatusClause(sql, status); appendTypeClause(sql, params, types); @@ -123,19 +128,21 @@ public int count(Status status, @CheckForNull List types) { } /** - * Returns the matching user rows projected to the audit-row shape. {@code offset}/{@code limit} - * are applied DB-side via {@code OFFSET} / {@code LIMIT}; pass {@code limit < 0} to return all - * matches. + * Returns the matching active user rows projected to the audit-row shape. {@code offset}/{@code + * limit} are applied DB-side via {@code OFFSET} / {@code LIMIT}; pass {@code limit < 0} to return + * all matches. */ public List list( Status status, @CheckForNull List types, int offset, int limit) { StringBuilder sql = new StringBuilder( - "SELECT uid, username, name, twofactortype, lastlogin FROM userinfo WHERE 1=1"); + "SELECT uid, username, name, twofactortype, lastlogin," + + " email, disabled, invitation FROM userinfo WHERE 1=1"); + sql.append(ACTIVE_ACCOUNT_FILTER); List params = new ArrayList<>(); appendStatusClause(sql, status); appendTypeClause(sql, params, types); - sql.append(" ORDER BY LOWER(username)"); + sql.append(" ORDER BY username"); if (limit >= 0) { sql.append(" LIMIT ? OFFSET ?"); params.add(limit); @@ -150,7 +157,10 @@ public List list( rs.getString("username"), rs.getString("name"), parseType(rs.getString("twofactortype")), - rs.getTimestamp("lastlogin"))); + rs.getTimestamp("lastlogin"), + rs.getString("email"), + rs.getBoolean("disabled"), + rs.getBoolean("invitation"))); } private static void appendStatusClause(StringBuilder sql, Status status) { @@ -194,5 +204,12 @@ private static TwoFactorType parseType(@CheckForNull String raw) { public record PrivilegedCounts(long withAllAuthority, long withAllAuthorityMissing2FA) {} public record UserAuditRow( - String uid, String username, String name, TwoFactorType twoFactorType, Date lastLogin) {} + String uid, + String username, + String name, + TwoFactorType twoFactorType, + Date lastLogin, + String email, + boolean disabled, + boolean invitation) {} } diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java index 295a60fd54ab..ae630f7cfce7 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java @@ -69,6 +69,7 @@ public class UserTwoFactorAuditController { private static final int DEFAULT_PAGE_SIZE = 50; private static final int MAX_PAGE_SIZE = 1000; + private static final int UNPAGED_HARD_CEILING = 10_000; private final TwoFactorAuditQueryService auditService; @@ -101,14 +102,12 @@ public TwoFactorAuditList getList( @RequestParam(required = false, defaultValue = "" + DEFAULT_PAGE_SIZE) int pageSize) { int total = auditService.count(status, type); int requestedPageSize = - paging ? Math.min(Math.max(1, pageSize), MAX_PAGE_SIZE) : Math.max(1, total); - // Build the Pager first so its built-in [1, pageCount] page clamp drives - // BOTH the response metadata and the DB offset/limit — an out-of-bounds - // page request can no longer return an empty list with pager.page pinned - // to a different (clamped) page. + paging + ? Math.min(Math.max(1, pageSize), MAX_PAGE_SIZE) + : Math.max(1, Math.min(total, UNPAGED_HARD_CEILING)); Pager pager = new Pager(page, total, requestedPageSize); int offset = paging ? pager.getOffset() : 0; - int limit = paging ? pager.getPageSize() : -1; + int limit = paging ? pager.getPageSize() : UNPAGED_HARD_CEILING; List entries = auditService.list(status, type, offset, limit).stream() .map(UserTwoFactorAuditController::toEntry) @@ -118,7 +117,14 @@ public TwoFactorAuditList getList( private static TwoFactorAuditEntry toEntry(UserAuditRow row) { return new TwoFactorAuditEntry( - row.uid(), row.username(), row.name(), row.twoFactorType(), row.lastLogin()); + row.uid(), + row.username(), + row.name(), + row.twoFactorType(), + row.lastLogin(), + row.email(), + row.disabled(), + row.invitation()); } public record TwoFactorAuditSummary( @@ -140,5 +146,8 @@ public record TwoFactorAuditEntry( @JsonProperty String username, @JsonProperty String name, @JsonProperty TwoFactorType twoFactorType, - @JsonProperty Date lastLogin) {} + @JsonProperty Date lastLogin, + @JsonProperty String email, + @JsonProperty boolean disabled, + @JsonProperty boolean invitation) {} } From fb7089d2bd1e46193d3270d1ace1e1815a11b8f9 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Mon, 25 May 2026 02:37:00 +0800 Subject: [PATCH 7/8] fix: rename Status enum to avoid OpenAPI schema name collision [DHIS2-20097] The nested Status enum in TwoFactorAuditQueryService collided with another Status type in the generated OpenAPI document, failing OpenApiControllerTest. Renamed to TwoFactorAuditStatus. AI Assisted. --- .../security/twofa/audit/TwoFactorAuditQueryService.java | 9 +++++---- .../controller/user/UserTwoFactorAuditController.java | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java index 10de92237c24..bf47cb155d01 100644 --- a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java @@ -52,7 +52,7 @@ @RequiredArgsConstructor public class TwoFactorAuditQueryService { - public enum Status { + public enum TwoFactorAuditStatus { ALL, ENABLED, DISABLED @@ -117,7 +117,8 @@ public PrivilegedCounts countPrivileged() { } /** Returns the number of active users matching the given filter. */ - public int count(Status status, @CheckForNull List types) { + public int count( + TwoFactorAuditStatus status, @CheckForNull List types) { StringBuilder sql = new StringBuilder("SELECT COUNT(*) FROM userinfo WHERE 1=1"); sql.append(ACTIVE_ACCOUNT_FILTER); List params = new ArrayList<>(); @@ -133,7 +134,7 @@ public int count(Status status, @CheckForNull List types) { * all matches. */ public List list( - Status status, @CheckForNull List types, int offset, int limit) { + TwoFactorAuditStatus status, @CheckForNull List types, int offset, int limit) { StringBuilder sql = new StringBuilder( "SELECT uid, username, name, twofactortype, lastlogin," @@ -163,7 +164,7 @@ public List list( rs.getBoolean("invitation"))); } - private static void appendStatusClause(StringBuilder sql, Status status) { + private static void appendStatusClause(StringBuilder sql, TwoFactorAuditStatus status) { switch (status) { case ENABLED -> sql.append(" AND ") diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java index ae630f7cfce7..79a268b9b0fe 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/UserTwoFactorAuditController.java @@ -43,7 +43,7 @@ import org.hisp.dhis.security.twofa.TwoFactorType; import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService; import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService.PrivilegedCounts; -import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService.Status; +import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService.TwoFactorAuditStatus; import org.hisp.dhis.security.twofa.audit.TwoFactorAuditQueryService.UserAuditRow; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.RequestMapping; @@ -95,7 +95,7 @@ public TwoFactorAuditSummary getSummary() { @GetMapping public TwoFactorAuditList getList( - @RequestParam(required = false, defaultValue = "ALL") Status status, + @RequestParam(required = false, defaultValue = "ALL") TwoFactorAuditStatus status, @CheckForNull @RequestParam(required = false) List type, @RequestParam(required = false, defaultValue = "true") boolean paging, @RequestParam(required = false, defaultValue = "1") int page, From ed590909d6f108e9b3a6af59545b622fd676157a Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Mon, 25 May 2026 02:50:01 +0800 Subject: [PATCH 8/8] style: spotless format fix [DHIS2-20097] AI Assisted. --- .../dhis/security/twofa/audit/TwoFactorAuditQueryService.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java index bf47cb155d01..9a5fa6b1c9b0 100644 --- a/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/security/twofa/audit/TwoFactorAuditQueryService.java @@ -117,8 +117,7 @@ public PrivilegedCounts countPrivileged() { } /** Returns the number of active users matching the given filter. */ - public int count( - TwoFactorAuditStatus status, @CheckForNull List types) { + public int count(TwoFactorAuditStatus status, @CheckForNull List types) { StringBuilder sql = new StringBuilder("SELECT COUNT(*) FROM userinfo WHERE 1=1"); sql.append(ACTIVE_ACCOUNT_FILTER); List params = new ArrayList<>();