Require Get Alerts privilege to read all users' alerts

[dkayiwa]

What

AlertService.getAllAlerts() and getAllAlerts(boolean) return every user's alerts, but were guarded only by @Authorized (authentication) — so any authenticated user could read alerts addressed to others through any consumer (REST, legacy UI, modules, direct API).

This introduces a dedicated Get Alerts privilege (following the GET_* read-privilege convention) and gates both methods with it. The per-user reads (getAlert, getAlerts, getAlertsByUser, getAllActiveAlerts) stay open for a caller reading their own alerts; reading another user's alerts through them now also requires Get Alerts, closing the same cross-user disclosure at the by-id and per-user paths rather than only at getAllAlerts.

Why a dedicated privilege (not Manage Alerts)

Reads should use a GET_* privilege rather than the write-oriented MANAGE_ALERTS, and a dedicated privilege allows granting read-all access without write. The privilege is registered via @AddOnStartup and created at startup by checkCoreDataset(); it is not auto-assigned to any role, so it does not reintroduce the leak — only super users and explicitly-granted roles receive it.

AlertReminderTask

The scheduled AlertReminderTask reads all alerts as its configured job user (the JobRunr path clears the daemon-thread flag before invoking the task, so @Authorized is enforced). It now grants itself a proxy Get Alerts privilege around that read.

Upgrade note

A new Get Alerts privilege is created at startup (via @AddOnStartup / checkCoreDataset()) and is not assigned to any role. After upgrade, grant Get Alerts to any role that must read other users' alerts:

• Roles that relied on Manage Alerts to list every user's alerts (e.g. the legacy "Manage Alerts" page, or any code calling getAllAlerts() / getAllAlerts(boolean)).
• Roles or integrations that read another user's alerts via getAlert(Integer), getAlerts(User, …), getAlertsByUser(User) or getAllActiveAlerts(User).

Reading one's own alerts is unaffected and needs no new privilege. Super users are unaffected (they bypass privilege checks). Via REST, GET /ws/rest/v1/alert now returns only the caller's own alerts unless they hold Get Alerts (paired with openmrs/openmrs-module-webservices.rest#749).

Tests

AlertServiceTest: callers without Get Alerts get APIAuthenticationException when reading another user's alerts through getAllAlerts, getAlerts, getAlertsByUser and getAllActiveAlerts, while getAlert(Integer) returns null for another user's alert — the same as for a non-existent id, so it cannot be used as an existence oracle; reading their own succeeds; and an unprivileged caller holding a proxy Get Alerts privilege succeeds (mirrors the task). mvn -pl api test -Dtest=AlertServiceTest → 9/9.

Notes

• Please link the corresponding TRUNK- ticket.
• Core-side fix for cross-user alert disclosure; the REST module's presentation-layer change is Restrict alert listing and retrieval to the alert's recipients openmrs-module-webservices.rest#749.

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

❌ Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.41%. Comparing base (c98b7f0) to head (3636a8d).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...org/openmrs/scheduler/tasks/AlertReminderTask.java	0.00%	3 Missing ⚠️
...rg/openmrs/notification/impl/AlertServiceImpl.java	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6186      +/-   ##
============================================
+ Coverage     59.32%   59.41%   +0.08%     
- Complexity     9332     9352      +20     
============================================
  Files           695      695              
  Lines         37451    37471      +20     
  Branches       5515     5521       +6     
============================================
+ Hits          22219    22263      +44     
+ Misses        13234    13213      -21     
+ Partials       1998     1995       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dkayiwa]

Verdict: merge — clean, focused fix for the cross-user alert disclosure, with runtime tests that assert the actual authorization effect. No blocking issues. One non-blocking question inline.

What I verified:

• AlertServiceTest is green on the PR head (mvn -pl api test -Dtest=AlertServiceTest → 9/9), and the assertions are real behavior checks (a non-super user gets APIAuthenticationException reading another user's alerts, succeeds on their own and via a proxy Get Alerts) — they would fail on master, where the bare @Authorized on getAllAlerts only required authentication.
• The three delegating overloads (getAllActiveAlerts, getAlertsByUser, and getAllAlerts()) route through Context.getAlertService(), so the in-method check in getAlerts(User, …) and the @Authorized(GET_ALERTS) on getAllAlerts(boolean) are enforced on those paths via the Spring proxy — not just on the directly-annotated methods. The tests confirm this for all three.
• The only in-repo consumer of a gated method is AlertReminderTask, which now brackets the read with addProxyPrivilege/removeProxyPrivilege in a finally. The daemon path is also safe: Context.hasPrivilege and AuthorizationAdvice both short-circuit for daemon threads, so the proxy privilege is simply redundant there and load-bearing only on the JobRunr (non-daemon) path — matching the PR description. Reading one's own alerts (e.g. getAlertsByUser(null)) needs no new privilege, so normal users are unaffected.

Needs action before merge: none.

[dkayiwa]

question: this gates content disclosure correctly, but it also turns getAlert into an existence oracle for IDs the caller doesn't own — it throws APIAuthenticationException when the alert exists and returns null when it doesn't, so an authenticated user can probe which (sequential) alert IDs exist. The getAlerts / getAlertsByUser / getAllActiveAlerts paths don't have this asymmetry because they gate on the target user identity before querying. Low sensitivity (only the existence of a notification leaks, not its content or recipient), so non-blocking — but since this PR is specifically about cross-user disclosure: is exposing existence intended, or would you rather return null for not-yours, the same as for not-found?

[dkayiwa]

Good catch — closed the oracle in 3636a8d.

getAlert now returns null when the alert is addressed to another user and the caller lacks Get Alerts, identical to the not-found path, so it can no longer be used to probe which (sequential) ids exist. Reading one's own alert, and privileged reads via Get Alerts, are unchanged.

I kept getAlerts / getAlertsByUser / getAllActiveAlerts throwing: there the caller supplies the target user identity, so the exception reveals nothing they didn't already assert — the asymmetry only mattered for the opaque-id lookup. I documented that contrast on getAlert's Javadoc so a caller used to the throwing overloads doesn't misread the null.

The test now asserts assertNull for both a not-yours id and a non-existent id, pinning the two as indistinguishable. mvn -pl api test -Dtest=AlertServiceTest → 9/9.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud