feat: admin-only /api/users/twoFactor audit endpoints [DHIS2-20097] - #23925
Merged
netroms merged 8 commits intoMay 28, 2026
Conversation
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.
…-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.
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.
netroms
changed the title
netroms
marked this pull request as ready for review
May 22, 2026 20:39
…097] 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.
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.
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.
…-20097] The nested Status enum in TwoFactorAuditQueryService collided with another Status type in the generated OpenAPI document, failing OpenApiControllerTest. Renamed to TwoFactorAuditStatus. AI Assisted.
AI Assisted.
|
vietnguyen
approved these changes
May 25, 2026
jbee
approved these changes
May 28, 2026
This was referenced Jun 18, 2026
netroms
added a commit
that referenced
this pull request
Jun 30, 2026
netroms
added a commit
that referenced
this pull request
Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a superuser-only endpoint group for auditing 2FA enrolment across the user base. Replaces the earlier attempt that exposed
twoFactorTypedirectly on/api/users— the User resource ACL was too coarse, anyone who could read users would have seen every user's 2FA state.GET /api/users/twoFactor/summary— totals,byTypebreakdown, privileged-user (ALL-authority) coverage stats. No cache (used rarely).GET /api/users/twoFactor— per-user list, filterable by?status=ALL|ENABLED|DISABLEDand?type=TOTP_ENABLED,EMAIL_ENABLED,…(multi-value).Both endpoints are gated by
@RequiresAuthority(anyOf = ALL)./api/me.twoFactorTypeis unaffected — it reads viaMeDto, not the User Jackson stack.JIRA: DHIS2-20097
Why
v42 removed every admin-readable surface for another user's 2FA state, blocking:
AI Assisted.