EST: DHIS2 OAuth#11
Conversation
Adds OpenSpec proposals for DHIS2 OAuth2 SSO (dhis2-oauth-core) and iframe embedding (dhis2-iframe-embedding), plus the archived investigation spike (dhis2-oauth-spike) that gated them. The spike confirmed DHIS2 OAuth endpoints, /api/me shape, embedded Tomcat 8.5.88, the User.active=false rejection path in AuthController, HTTP client availability, and *.localtest.me resolution. Follow-up design confidence re-scored to 9/10. Adds DHIS2 OAuth + iframe config blocks to docker/openboxes.client-template.yml and a spike-only docker-compose for local DHIS2.
Cleanup of the previous commit, which accidentally committed both the pre-archive and post-archive spike directories due to stale index state.
Adds DHIS2 Authorization Code login alongside the existing username/password flow. First-login auto-registers users with active=false; admins activate via the existing user-admin screen (new "Pending DHIS2 access" filter). - Side-table custom_dhis2_user_link matches by stable DHIS2 UID - Hand-rolled client (no Spring Security plugin); hooks into AuthService - SecurityInterceptor gates inactive users to a pending-access page - Feature disabled by default; opt-in via per-deployment openboxes.yml
Rename to follow Grails service naming convention so the bean is auto-registered, removing the manual resources.groovy wiring. Reuse a single pooled HttpClient across calls, consolidate the request/response handling, and add a search filter + roles fetch to the pending-users admin query.
- Move pending.gsp to grails-app/views/custom/dhis2auth/ to comply with custom-package isolation; controller renders explicit view path. - Drop fetchMode JOIN on roles in Dhis2AdminService — Hibernate applies LIMIT before deduplication on collection joins, breaking pagination. Pending users are a small set so lazy fetch is fine. - Remove redundant @transactional on Dhis2RegistrationService (Grails services are transactional by default; the annotation narrows scope silently if future methods are added unannotated). - Add `static transactional = false` and `volatile` to Dhis2OAuthService fields backing the pooled HttpClient; drop now-redundant @NotTransactional annotations. Same treatment on Dhis2SessionService. - Rewrite Dhis2UserLinkSpec — beforeInsert/beforeUpdate tests called methods that don't exist; replaced with concrete unique-constraint saves and a GORM auto-timestamp assertion. - Date-prefix the Liquibase migration filename to match the custom-package-isolation convention and update the changelog include. - Tighten weak Spock assertions: assert full encoded URL in Dhis2OAuthServiceSpec with edge-case where: rows; assert the linked user id in Dhis2RegistrationServiceSpec. - design.md: fix the schema diagram (UUID CHAR(38), not BIGINT), the config key path (openboxes.dhis2.oauth.*), list the UrlMappings touch point, and replace the application.yml note with the external-config approach actually used.
grails.gorm.transactions.Transactional uses a compile-time AST transform that reliably binds the Hibernate session; without it the default Grails transaction wrapping does not cover flush() calls in private methods, causing TransactionRequiredException at runtime.
- Dhis2OAuthServiceSpec: stub GrailsApplication.getConfig() was failing a Spock cast (returns grails.config.Config, not ConfigObject); switch to grailsApplication.config property assignment instead. - Dhis2UserLinkSpec: User/Person not mocked caused validate() failures; add DataTest + mockDomains. Unique constraint tests and auto-timestamp tests removed — neither is enforced by in-memory GORM unit tests; those guarantees live at the DB schema level (SQL UNIQUE, Hibernate events). - Dhis2UserLink: add blank: false to dhis2Uid constraint so empty strings correctly fail validation.
Restore trailing whitespace on upstream docker-compose.yml lines so the only diff is the functional app port mapping. Add docker-compose.yml and docker/openboxes.yml to design.md upstream touch points.
Covers task 3.5: controller session/redirect/400 paths, SecurityInterceptor pending-access gate, and an @Integration spec exercising the real HTTP client against an embedded DHIS2 stub with real GORM persistence. Promote dhis2-auth spec to openspec/specs; sync tasks.md and design.md (Deploy status).
Implementation complete and tests green. Moved to archive/2026-06-04-dhis2-oauth-core.
Remove the 9090:8080 compose override (app falls back to base 8080:8080) so docker-compose.yml is upstream-pristine, and delete the spike-only DHIS2 stack docker/dhis2-sso/docker-compose.spike.yml. Neither is needed by the feature — both were local-dev/validation scaffolding. Drop the compose touch point from the archived design.md.
Move OAuth config from openboxes.dhis2.oauth.* to openboxes.custom.dhis2.oauth.* to mirror the org.pih.warehouse.custom.* isolation convention and avoid a collision if upstream adds its own openboxes.dhis2 config. Updates service, controller, login GSP, specs, client-template.yml, and openspec docs.
v42 SAS tokens expose no DHIS2 UID, so the link is keyed on the username. Make dhis2_uid nullable+unique, make dhis2_username the required, unique, case-sensitive (utf8mb4_bin) key, and add a deactivated_at tombstone for username recycling. Folded the additive change into the create-table changeset (not yet deployed) and renamed it to the date-based id format.
Add resolveIdentity branching on profile: v42 reads the username from the id_token sub claim (back-channel TLS, no signature verification), v40/v41 keep /api/me. Add PKCE (S256) on the authorize/token exchange and an optional client_secret_post auth method. Preserve the original cause when wrapping a malformed id_token in Dhis2OAuthException.
findOrRegister branches on the presence of a UID: v40/v41 keep the UID path, v42 looks up (or creates) an inactive user keyed by username with placeholder names an admin fills at approval. A tombstoned link forces re-approval rather than silently reusing the prior account. Username collisions now escalate the suffix (-dhis2, -dhis2-2, ...) and throw once exhausted instead of failing on a duplicate-key save.
The callback now stores the PKCE verifier on initiate, passes it to the token exchange, and resolves identity via resolveIdentity. Restrict the post-login targetUri redirect to local paths only, rejecting protocol- relative (//) and backslash forms that browsers normalise to off-domain.
Describe the profile: v40 | v42 switch, the v42 endpoint/scope set, the required DHIS2-side client PATCHes (scopes, auth method, bcrypt secret), and that v42 needs no userUrl and never requests the email scope.
Proposal, design, delta spec, and tasks for the v42 username-keyed identity pivot. Not yet archived (live e2e pending).
Sync the MODIFIED 'DHIS2 OAuth login option' requirement (v40/v42 profiles, PKCE, id_token sub identity) into the main spec, then move the completed change to openspec/changes/archive/2026-06-11-dhis2-oauth-v42.
- Guard active flag before re-writing User on tombstone re-approval to avoid a redundant UPDATE on every recycled-username login. - Drop no-op URLEncoder on the base64url PKCE code_challenge. - Assert lastLoginAt advances in the v42 returning-user spec.
MatiasArriola
left a comment
MatiasArriola
left a comment
There was a problem hiding this comment.
Impressive work, nice integration point!
I haven’t had time for a full deep review yet, but I’m leaving a few comments from an AI-assisted analysis that I reviewed and curated. Most important items first:
DHIS2-created users get a shared static password
grails-app/services/org/pih/warehouse/custom/dhis2auth/Dhis2RegistrationService.groovy:96 and grails-app/services/org/pih/warehouse/core/UserService.groovy:491
DHIS2-created users are saved with the static sentinel password DHIS2. However, local database authentication accepts either the encoded password or the cleartext password. Once a DHIS2-linked user is activated, that account may be able to log in through the normal username/password flow, or API login, using the literal password DHIS2.
Risk: account takeover for any activated DHIS2-linked user whose username is known, including privileged users.
Recommendation: block database-password authentication for users with a Dhis2UserLink, or store a per-user unguessable random password value instead of a shared sentinel. Add a regression test proving userService.authenticate(dhis2Username, '*DHIS2*') == false for DHIS2-linked users.
I haven't tested this myself, but saving a random long string for each DHIS2 user should be a safe bet.
v42 id_token is trusted without signature or claim validation
grails-app/services/org/pih/warehouse/custom/dhis2auth/Dhis2OAuthService.groovy:126
The v42 flow extracts sub directly from the JWT payload and explicitly skips signature verification. Even though the token is returned from the TLS token endpoint, this is still not a full OIDC validation. The app should verify at least signature/JWKS, issuer, audience/client ID, expiration, and nonce or equivalent binding where applicable.
Risk: a compromised/misconfigured token endpoint, proxy, or future refactor could allow forged identity assertions. Since sub becomes the OpenBoxes identity key, this is the most important security issue.
Recommendation: use a real JWT/OIDC validation library and validate iss, aud, exp, signature, and expected algorithm.
Since the same company would be in control of the environments end-to-end, the risk is much lower, but maybe we need to document this as an accepted risk. Maybe change the comment included there, since TLS protects the transport channel, but it does not validate the JWT as an identity assertion.
OAuth HTTP client has no connection/read timeouts
grails-app/services/org/pih/warehouse/custom/dhis2auth/Dhis2OAuthService.groovy:50
HttpClients.custom().setConnectionManager(...).build() uses default timeout behavior. A slow or unreachable DHIS2 token/user endpoint can hang request threads.
Recommendation: configure connect timeout, socket/read timeout, and connection request timeout. This is especially relevant because login is user-facing and can consume servlet threads.
Maybe it is worth reviewing this if it is an easy change, I would say this is low priority.
Token/user-info error bodies are logged via exception message
grails-app/services/org/pih/warehouse/custom/dhis2auth/Dhis2OAuthService.groovy:176 grails-app/controllers/org/pih/warehouse/custom/dhis2auth/Dhis2OAuthController.groovy:72
On non-200 responses, the full response body is included in the exception and then logged. OAuth servers sometimes include sensitive diagnostics, token fragments, client details, or user identifiers in error responses.
Recommendation: log status code and a sanitized/truncated error category. Keep raw body out of normal logs, or log it only at guarded debug level with redaction.
We should avoid logging sensitive information, maybe we can find a way to test how theses cases work and check what is actually being logged.
- Registration: give DHIS2-only users an unguessable random local password instead of the shared '*DHIS2*' sentinel, closing an account-takeover path (local DB/API login matches cleartext). - OAuth HTTP client: set connect/socket/connection-request timeouts so a slow DHIS2 endpoint can't hang servlet threads. - Token/userinfo errors: keep the raw response body out of the (logged) exception; expose it only at guarded debug level. - id_token: clarify the comment — TLS secures transport but does not validate the JWT as an identity assertion (accepted risk).
|
Thank you @MatiasArriola! I've addressed all four findings. The only one handled as a documented decision rather than a code change is full id_token validation. I agree the original comment was misleading, so I've corrected it. Doing that verification properly is a medium-sized change: it means adding a new third-party library (so a build.gradle change and an extra dependency to maintain in the fork) plus the logic to check the token's signature and claims. The real-world risk is low: the token comes straight from DHIS2's token endpoint over a secure connection and in a controlled environment by the company, as you mentioned. I've documented it as an accepted risk in the code and propose we track full validation as a follow-up to revisit if a third-party or untrusted system ever sits between DHIS2 and OpenBoxes. |
2 participants
📌 References
More info about setup here: https://app.clickup.com/4528615/v/dc/4a6f7-37352/4a6f7-7232
Purpose
v40(legacy UAA OAuth 2.0) andv42(Spring Authorization Server, OAuth 2.1 / OIDC).📝 Implementation
Dhis2OAuthController—initiate/callback/pending; state-CSRF check, local-only redirect guard.Dhis2OAuthService— builds the authorize URL, PKCE (S256) for v42, token exchange (Basic orclient_secret_post), and identity resolution: v40 from/api/me, v42 from the OIDCid_tokensub(the SAS token isn't accepted on/api/*).Dhis2RegistrationService— find-or-register, username-collision suffixing, tombstone re-approval;@Transactionalfor reliable Hibernate session binding.Dhis2UserLinkdomain +custom_dhis2_user_linkmigration — side-table with a UNIQUE FK toUser(no columns added to upstreamuser). v40 keys on UID; v42 keys on username.openboxes.custom.dhis2.oauth.*(profile,clientId/clientSecret, authorize/token/user URLs,scopes,clientAuth).✨ Description of Change
Adds DHIS2 OAuth2 SSO as an optional login path; local username/password login is unchanged. Profile-selectable (
v40default,v42OIDC). First login auto-registers an inactive user (no roles, no locations) linked viaDhis2UserLink; returning users refresh name/email only (roles and theactiveflag untouched).Key design decisions:
org.pih.warehouse.custom.dhis2auth.*; identity stored in a side-table, never as columns onuser.id_tokensub— the Spring-Auth-Server access token isn't valid on/api/*, so the username comes from the OIDC claim.statevalidated before any processing, open-redirect guard on the callback target.📷 Screenshots & Recordings (optional)
V40/41
2026-06-11.11-06-15.mp4
V42
2026-06-11.10-18-54.mp4
🔥 Notes to the tester
Configure
openboxes.custom.dhis2.oauth.*(seedocker/dhis2-sso/README.md) and register an OAuth client in DHIS2. Then click "Sign in with DHIS2" on the login page → consent → lands on the pending page (first login) or the dashboard (active user).