Require privileges for unprotected CohortService methods (backport to 2.8.x)#6323
Merged
dkayiwa merged 1 commit intoJul 15, 2026
Merged
Conversation
* Require Purge Cohorts privilege for CohortService.purgeCohort purgeCohort permanently and irreversibly removes a cohort from the database but carried no @Authorized annotation, so any authenticated user could reach it through the REST purge path (DELETE /ws/rest/v1/cohort/{uuid}?purge=true), even though the reversible voidCohort on the same resource requires Delete Cohorts. A dedicated Purge Cohorts privilege already exists in PrivilegeConstants but was never applied. Annotate purgeCohort with it, consistent with the rest of the service. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add regression test for Purge Cohorts authorization Guards the @Authorized gate on CohortService.purgeCohort so that dropping the annotation fails the build instead of silently reopening the purge path to unprivileged callers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Require Get Patient Cohorts privilege for CohortService.getCohorts getCohorts(String) was the only read on the service with no @Authorized, reading straight from the DAO while every sibling getter requires Get Patient Cohorts. A direct service-layer caller could therefore list cohorts (names, descriptions, member patient ids) with no authorization; the REST search path was only covered incidentally by the resource's required-get privilege at conversion time. Gate it at the service layer to match the other getters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Assert the specific privilege in cohort authorization tests Capture the thrown APIAuthenticationException and assert its message names the required privilege, so a future edit that swaps either annotation to a different privilege (for purgeCohort, the exact regression this PR fixes: gating on Get Patient Cohorts instead of Purge Cohorts) fails the build rather than staying green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Use containsString matcher for cohort authorization assertions assertThat(msg, containsString(privilege)) reports the actual message on failure, so a future privilege regression is self-describing in the CI log instead of the bare "expected true but was false" that assertTrue(msg.contains(..)) produces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit d8fe136)
|
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
1 participant
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.
Backport of #6317 to the 2.8.x release line.
CohortService.purgeCohort(permanent, irreversible cohort deletion) andCohortService.getCohorts(String)(cohort search) both lacked the@Authorizedannotation that guards every other mutating and read method on the service, so they were reachable with no privilege check:purgeCohortviaDELETE /ws/rest/v1/cohort/{uuid}?purge=true, andgetCohortsviaGET /ws/rest/v1/cohort?q=. This applies thePurge CohortsandGet Patient Cohortsprivileges respectively, matching the rest of the service, and adds regression tests asserting each gate. Addresses private advisory GHSA-4w59-mg6c-76ch.Cherry-picked from d8fe136 on master; the only backport adjustment was reconciling the test file's import block with the 2.8.x import ordering.