Skip to content

[DT-3989] Restrict election open and cancel actions for admins - #3019

Merged
kevinmarete merged 7 commits into
developfrom
km-dt-3989-remove-admin-access-to-apis
Aug 14, 2026
Merged

[DT-3989] Restrict election open and cancel actions for admins#3019
kevinmarete merged 7 commits into
developfrom
km-dt-3989-remove-admin-access-to-apis

Conversation

@kevinmarete

@kevinmarete kevinmarete commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Addresses

https://broadworkbench.atlassian.net/browse/DT-3989

Follows duos-ui #3852 (merged), which removed the Action column from the Admin DAR table.

Security risk: low — this change only removes authorization; no endpoint gains access.

Summary

  • Remove Admin access from election open and cancellation APIs.
  • Hide election open and cancel actions from Admin collection summaries.
  • Preserve Chairperson and Researcher access.
  • Imply only the Researcher role when roleName is omitted on cancel; a Chairperson must request the role explicitly, since cancelling as a chair cancels the elections for every dataset that chair governs.
  • Require a Chairperson to govern at least one of the collection's datasets before creating elections (previously a chair of an unrelated DAC got a 500 instead of a 403).
  • Reject election creation for a DAR with no dataset ids with a 400 instead of skipping the governing-DAC check (which then failed as a 500 downstream).
  • Note on both endpoints that Admins escalate to the governing DAC Chairperson.
  • Add authorization and regression tests.

Notes for reviewers

  • Breaking API contract: Admin is removed from the roleName enum on PUT /api/collections/{id}/cancel, and Admin callers now get 403. No duos-ui call site passes AdminAdminManageDarCollections passes no cancel/open handlers after #3852.
  • Enforcement lives in DarCollectionService, with @RolesAllowed at the boundary and a reflection test pinning the annotation. The cancel endpoint still resolves the acting role in the resource because roleName selects it.
  • The escalation path for the operational risk raised on duos-ui #3852 is a note on the two endpoint descriptions: Admins escalate to the governing DAC Chairperson.

Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens authorization around DAR collection election “open” (create elections) and “cancel” actions by removing Admin capabilities and ensuring Admin-facing collection summaries no longer advertise those actions, while preserving Chairperson/Researcher behavior and documenting the operational cleanup path.

Changes:

  • Restrict election creation and collection cancellation to DAC Chairpersons and (for cancellation) Researchers; Admin becomes read-only for these actions.
  • Remove Open/Cancel actions from Admin DAR collection summaries.
  • Update OpenAPI docs and add/adjust regression + authorization tests; document exceptional election cleanup process.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java Enforces Chairperson-only election creation, restricts cancellation to Chairperson/Researcher, clears Admin summary actions.
src/main/java/org/broadinstitute/consent/http/resources/DarCollectionResource.java Narrows @RolesAllowed for cancel/create-elections endpoints and adds explicit runtime role checks.
src/test/java/org/broadinstitute/consent/http/service/DarCollectionServiceTest.java Updates tests to assert Admin is forbidden for cancel/create-elections and that Admin summaries have no actions.
src/test/java/org/broadinstitute/consent/http/resources/DarCollectionResourceTest.java Adds/updates endpoint authorization tests (including annotation checks) for cancel/create-elections.
src/main/resources/assets/paths/createCollectionElectionsByCollectionId.yaml Documents Chairperson-only authorization and adds explicit 401/403 responses.
src/main/resources/assets/paths/cancelCollectionByCollectionId.yaml Removes Admin from roleName enum and documents updated cancel authorization + 403 response.
docs/README.md Links the new election operations runbook doc.
docs/ELECTION_OPERATIONS.md Documents ownership/escalation and the exceptional cleanup process without Admin API fallback.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java Outdated
- Collapse the unreachable default branch in cancelDarCollectionByRole and
  extract the duplicated authorization messages into constants.
- Imply only the Researcher role when roleName is omitted on cancel. A
  chairperson must request the role explicitly, since canceling as a chair
  cancels the elections for every dataset that chair governs.
- Require a chairperson to govern at least one of the collection's datasets
  before creating elections, returning 403 instead of 500.
- Update the OpenAPI paths and mark the exceptional cleanup process in
  ELECTION_OPERATIONS.md as pending support sign-off.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java:796

  • darDatasetIds.stream().noneMatch(governedDatasetIds::contains) does a linear lookup in a List for each dataset, which can become O(n*m) if a DAR includes many datasets. Converting the governed IDs to a Set makes the membership check O(1) and keeps this authorization check cheap.
      List<Integer> governedDatasetIds = datasetDAO.findDatasetIdsByDACUserId(user.getUserId());
      if (darDatasetIds.stream().noneMatch(governedDatasetIds::contains)) {
        throw new ForbiddenException(CREATE_ELECTION_DAC_ERROR);

src/main/resources/assets/paths/createCollectionElectionsByCollectionId.yaml:27

  • The 403 response description is narrower than the actual behavior: this endpoint can return 403 either because the caller is not a Chairperson (role-based) or because the Chairperson does not govern any dataset in the collection (dataset-governance check). Consider documenting both cases to match the implementation.
    401:
      description: Authentication required.
    403:
      description: Forbidden. Not a Chairperson for any dataset in the collection.

src/main/resources/assets/paths/cancelCollectionByCollectionId.yaml:31

  • The 403 response description suggests the caller lacks Chairperson/Researcher, but the implementation can also return 403 when a caller with those roles requests an unsupported roleName (e.g., Admin). Updating the description helps keep the OpenAPI contract aligned with actual error conditions.
    400:
      description: Bad Request (result of elections present on DARs, or a role name that is missing or invalid for this user)
    403:
      description: Forbidden. Not a Chairperson or Researcher.

- Use a Set for the chairperson's governed dataset ids, matching
  cancelDarCollectionElectionsAsChair.
- Correct the cancel 403 description; a Chairperson requesting an
  unsupported role also gets a 403.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java:798

  • When the most recent DAR has no datasetIds, this method skips the governance check and proceeds to create elections. createElectionsForDarByUser will then create nothing and this method throws an IllegalStateException (500) instead of returning a client error. This should be rejected as a Bad Request up front, consistent with other DAR validations (e.g., DataAccessRequestService throws "At least one dataset is required").
    DataAccessRequest dar = collection.getMostRecentDar();
    List<Integer> darDatasetIds = dar.getDatasetIds();
    if (!darDatasetIds.isEmpty()) {
      Set<Integer> governedDatasetIds =
          Set.copyOf(datasetDAO.findDatasetIdsByDACUserId(user.getUserId()));

src/main/java/org/broadinstitute/consent/http/resources/DarCollectionResource.java:275

  • createElectionsForCollection is already protected by @RolesAllowed({CHAIRPERSON}), and DarCollectionService#createElectionsForDarCollection now enforces the same role check. Keeping a third role check here is redundant and risks drift (e.g., if annotation/service rules change but this check isn’t updated). Consider relying on @RolesAllowed + service-layer authorization and keeping the role restriction covered by the reflection-based annotation test.
      User user = duosUser.getUser();
      if (!user.hasUserRole(UserRoles.CHAIRPERSON)) {
        throw new ForbiddenException(CREATE_ELECTION_ROLE_ERROR);
      }

kevinmarete and others added 2 commits August 13, 2026 18:30
@RolesAllowed and DarCollectionService already enforce the role, so a
third check only risks drift. The resource test now covers the service's
ForbiddenException mapping instead of the removed check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extract the pre-flight checks so createElectionsForDarCollection stays
under the cognitive complexity limit, and reject a collection with no
DARs instead of dereferencing the null from getMostRecentDar.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java:801

  • validateElectionCreation skips the governing-DAC check when the DAR has no dataset IDs, which can allow election creation to proceed and later fail (e.g., with "No elections were created...") instead of returning a clear 4xx. Since other request flows treat empty dataset IDs as a bad request, this should be rejected here as well.
    List<Integer> darDatasetIds = dar.getDatasetIds();
    if (!darDatasetIds.isEmpty()) {
      Set<Integer> governedDatasetIds =
          Set.copyOf(datasetDAO.findDatasetIdsByDACUserId(user.getUserId()));
      if (darDatasetIds.stream().noneMatch(governedDatasetIds::contains)) {

@kevinmarete
kevinmarete marked this pull request as ready for review August 13, 2026 23:00
@kevinmarete
kevinmarete requested a review from a team as a code owner August 13, 2026 23:00
@kevinmarete
kevinmarete requested review from fboulnois, otchet-broad and rushtong and removed request for a team August 13, 2026 23:00

@otchet-broad otchet-broad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rushtong rushtong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, couple questions inline. 👍🏽

Comment thread src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java Outdated

@fboulnois fboulnois left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Fail closed on an empty dataset id list instead of skipping the
governing DAC check, and share the cancel role error message with
the resource.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java Outdated
findDatasetIdsByDACUserId matches any user_role row for a DAC, so a
member of one DAC and chair of another could open elections for the
DAC they only sit on. Reuse the role-scoped lookup instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@kevinmarete
kevinmarete merged commit e20fe76 into develop Aug 14, 2026
12 checks passed
@kevinmarete
kevinmarete deleted the km-dt-3989-remove-admin-access-to-apis branch August 14, 2026 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants