Require allergy privileges for getAllergies and setAllergies#6324
Conversation
PatientService.getAllergies(Patient) and setAllergies(Patient, Allergies) carried no @Authorized annotation. AuthorizationAdvice only enforces when a method declares a privilege, so both fell through with no check at all, letting any authenticated caller read or modify any patient's allergy list. The per-item siblings are already guarded (getAllergy and getAllergyByUuid require Get Allergies; saveAllergy requires Add/Edit Allergies). Annotate getAllergies with Get Allergies and setAllergies with Add/Edit Allergies, and add regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6324 +/- ##
============================================
+ Coverage 59.47% 59.63% +0.15%
- Complexity 9541 9574 +33
============================================
Files 731 731
Lines 38323 38409 +86
Branches 5587 5599 +12
============================================
+ Hits 22794 22906 +112
+ Misses 13489 13455 -34
- Partials 2040 2048 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR openmrs#6324 added @Authorized(GET_ALLERGIES) to PatientService.getAllergies. Two internal callers reach getAllergies through the AOP service proxy, so they now transitively demand Get Allergies from the outer caller. This is a behavior change the existing tests did not catch: - AllergyValidator reads the patient's existing allergies to reject a duplicate allergen whenever a new allergy is saved (allergyId == null). That runs inside saveAllergy (RequiredDataAdvice -> ValidateUtil.validate -> AllergyValidator) and on the encounter-save path, so a user holding only Add/Edit Allergies could no longer add an allergy. - PatientServiceImpl.mergeAllergies reads both patients' allergies, but mergePatients is only @Authorized(EDIT_PATIENTS), so merging began to require Get Allergies as well. Wrap each internal read in the standard addProxyPrivilege / removeProxyPrivilege idiom so the read is done on the caller's behalf without forcing the outer operation to hold Get Allergies. The interface @Authorized on getAllergies is left unchanged, so direct reads still require the privilege. Add a revert-sensitive PatientServiceTest that saves a new allergy as a user holding only Add Allergies (not Get Allergies); without the fix it fails with an APIAuthenticationException for Get Allergies raised from the validator's duplicate check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TpKNfxm3QjwUBDePEtFm2H
| try { | ||
| Context.addProxyPrivilege(PrivilegeConstants.GET_ALLERGIES); | ||
| preferredAllergies = patientService.getAllergies(preferred); | ||
| notPreferredAllergies = patientService.getAllergies(notPreferred); | ||
| } finally { | ||
| Context.removeProxyPrivilege(PrivilegeConstants.GET_ALLERGIES); | ||
| } |
There was a problem hiding this comment.
This proxy block is the one piece of the change that no test actually exercises. I confirmed it by reverting both wrappings on top of the new annotations: the saveAllergy duplicate-check test then fails with Privileges required: Get Allergies, but all four mergePatients allergy tests still pass, because they run as the super-user who already holds Get Allergies. If this block were dropped in a later refactor, CI would stay green while mergePatients started throwing APIAuthenticationException for any user who has Edit Patients but not Get Allergies.
The validator half of the same fix does have a guard test (the butch / Add-Allergies-only case), so this is the asymmetric spot. I would mirror that test here: become a user who holds the merge privileges but not Get Allergies, merge a patient who has an allergy, and assert it succeeds. The wrinkle is that mergePatients pulls in a fairly broad privilege set (relationships, obs, conditions, medication dispenses, patient programs, and so on), so the setup is bulkier than the saveAllergy test. If that is more scaffolding than it is worth, even a short comment here noting that the wrapping is only covered under super-user privileges would flag it for the next reader. Not blocking.
There was a problem hiding this comment.
It might be useful to ensure that the patient merge workflows are run with a non-super user user (which would patch the missing regression test here).
There was a problem hiding this comment.
Done in bf4ef22. mergeAndRetrieveAudit now runs the merge itself as butch (whose role grants no privileges) with proxy grants for only the privileges the merge legitimately requires, and Get Allergies deliberately left out. That closes the gap flagged above: with the proxy block reverted, all 27 audit-based merge workflow tests fail with Privileges required: Get Allergies, where previously all of them passed.
Running the merge without super-user privileges immediately paid off beyond allergies: it surfaced that voiding the losing patient demands Get Patient Cohorts from the merging user. PatientDataVoidHandler proxies Edit Cohorts around notifyPatientVoided, but the getCohortMemberships call inside that method goes through the service proxy, and its @Authorized annotation predates #6317, so any user voiding a patient without that privilege has been failing silently for a while. I gave that read the same proxy treatment as the allergy reads, and the test's privilege set omits Get Patient Cohorts too, so both internal reads are now guarded the same way (reverting the cohort proxy also fails all 27 tests, with Privileges required: Get Patient Cohorts).
The privileges that remain in the set are genuine demands of the workflow, including a few metadata reads (visit attribute types, the overlapping-visits global property, concepts, locations) that validators and change checks make through service proxies while the moved data is saved. Some of those could arguably get the internal-read treatment as well, but they are pre-existing and benign to hold, so the test documents them as-is rather than growing this PR further.
| try { | ||
| Context.addProxyPrivilege(PrivilegeConstants.GET_ALLERGIES); | ||
| preferredAllergies = patientService.getAllergies(preferred); | ||
| notPreferredAllergies = patientService.getAllergies(notPreferred); | ||
| } finally { | ||
| Context.removeProxyPrivilege(PrivilegeConstants.GET_ALLERGIES); | ||
| } |
There was a problem hiding this comment.
It might be useful to ensure that the patient merge workflows are run with a non-super user user (which would patch the missing regression test here).
mergeAndRetrieveAudit now executes mergePatients as a user who holds only the privileges the merge legitimately requires, with Get Allergies deliberately absent, so every audit-based merge workflow test guards the Get Allergies proxy block in mergeAllergies: removing that block fails all 27 of them with "Privileges required: Get Allergies". Running the merge without super-user privileges surfaced one more internal read that leaked its privilege onto the caller: voiding the losing patient triggers PatientDataVoidHandler, which proxies Edit Cohorts for CohortService.notifyPatientVoided, but the membership read inside that method goes through the service proxy and demanded Get Patient Cohorts from the voiding user. That read now acquires its own proxy privilege, matching the allergy reads, and the test privilege set omits Get Patient Cohorts so both internal reads stay covered. Suggested by @ibacher during review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
3 participants
Summary
PatientService.getAllergies(Patient)andsetAllergies(Patient, Allergies)have no@Authorizedannotation. Authorization is enforced inAuthorizationAdvice.before()only when a method declares at least one privilege; a method with none falls through with no check, not even authentication. So any authenticated caller who can reach these entry points can read another patient's allergy list and persist allergy changes for a patient, even though the dedicated per-item methods are guarded (getAllergy/getAllergyByUuidrequireGet Allergies;saveAllergyrequiresAdd/Edit Allergies).Change
getAllergies→@Authorized({ GET_ALLERGIES })setAllergies→@Authorized({ ADD_ALLERGIES, EDIT_ALLERGIES })Get Allergiesfrom callers acquire it as a proxy privilege instead: the duplicate-allergen check inAllergyValidator, and the allergy readsmergePatientsmakes while reconciling the two patients' allergy lists.mergePatientsas a non-super user holding only the privileges the merge legitimately requires, withGet AllergiesandGet Patient Cohortsdeliberately absent, so the internal-read proxying is guarded by all 27 audit-based merge tests (suggested by @ibacher in review).CohortServiceImpl.notifyPatientVoidedreads cohort memberships through the service proxy, so voiding any patient demandedGet Patient Cohortsfrom the voiding user even thoughPatientDataVoidHandleralready proxiesEdit Cohortsfor it (the annotation predates Require privileges for unprotected CohortService methods #6317). That read now acquires its own proxy privilege, matching the allergy reads.Scope note
The advisory also raises that
setAllergiesdoes not bind each submittedAllergyto the outerpatientargument (the DAOpersists each allergy as-is). That is a separate data-integrity hardening that interacts with the void/recreate reconciliation logic insetAllergies, so I'm handling it in a follow-up with its own tests rather than bundling it here. This PR closes the missing-authorization half.Addresses GHSA-jxvr-3qjq-vfqg.