[DT-3788] Migrate eligible AuthUser resource endpoints to DuosUser#2979
Open
rushtong wants to merge 8 commits into
Open
[DT-3788] Migrate eligible AuthUser resource endpoints to DuosUser#2979rushtong wants to merge 8 commits into
rushtong wants to merge 8 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request migrates eligible authenticated resource endpoints from @Auth AuthUser to @Auth DuosUser, removing redundant UserService.findUserByEmail(...) lookups in resources and documenting the resulting 401 Unauthorized behavior when a caller has no Consent User row.
Changes:
- Updated resource method signatures/constructors to accept
DuosUserand useduosUser.getUser()/getUserId()instead of re-looking up by email. - Updated resource tests to pass
DuosUserprincipals and removed now-deadfindUserByEmailstubbing paths. - Updated OpenAPI path docs (and inline
api-docs.yaml) to include401 Unauthorizedon affected operations.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/org/broadinstitute/consent/http/resources/VoteResourceTest.java | Switches tests to DuosUser and removes UserService stubbing. |
| src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java | Updates user-resource tests to pass DuosUser and removes user-not-found cases now handled at auth. |
| src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java | Updates study-resource tests to pass DuosUser and removes UserService dependency. |
| src/test/java/org/broadinstitute/consent/http/resources/MatchResourceTest.java | Uses DuosUser for the admin reprocess endpoint test. |
| src/test/java/org/broadinstitute/consent/http/resources/MailResourceTest.java | Updates mail-resource tests to call endpoints with DuosUser. |
| src/test/java/org/broadinstitute/consent/http/resources/FeatureFlagResourceTest.java | Updates feature-flag tests to use DuosUser and removes UserService dependency. |
| src/test/java/org/broadinstitute/consent/http/resources/EmailNotifierResourceTest.java | Updates email-notifier tests to pass DuosUser. |
| src/test/java/org/broadinstitute/consent/http/resources/DraftResourceTest.java | Updates draft-resource tests to pass DuosUser and removes UserService dependency. |
| src/test/java/org/broadinstitute/consent/http/resources/DatasetResourceTest.java | Updates dataset-resource tests to pass DuosUser (including data use and indexing endpoints). |
| src/test/java/org/broadinstitute/consent/http/resources/DataAccessRequestResourceTest.java | Updates DAR v2 tests to pass DuosUser (including “user-with-cards” fixtures). |
| src/test/java/org/broadinstitute/consent/http/resources/DacResourceTest.java | Updates DAC tests to pass DuosUser and adds filterUsers coverage. |
| src/test/java/org/broadinstitute/consent/http/resources/DACAutomationRuleResourceTest.java | Updates automation-rule tests to pass DuosUser and removes UserService dependency. |
| src/main/resources/assets/paths/votesRationale.yaml | Documents 401 Unauthorized for vote rationale updates. |
| src/main/resources/assets/paths/votes.yaml | Documents 401 Unauthorized for vote updates. |
| src/main/resources/assets/paths/userInstitutionSigningOfficials.yaml | Documents 401 Unauthorized for signing officials lookup. |
| src/main/resources/assets/paths/userInstitutionByInstitutionId.yaml | Documents 401 Unauthorized for users-by-institution endpoint. |
| src/main/resources/assets/paths/studyByIdCustodians.yaml | Documents 401 Unauthorized for updating custodians. |
| src/main/resources/assets/paths/studyById.yaml | Documents 401 Unauthorized for update/delete study operations. |
| src/main/resources/assets/paths/progressReport.yaml | Documents 401 Unauthorized for progress report creation. |
| src/main/resources/assets/paths/getDatasetsFromUserDacsV2.yaml | Documents 401 Unauthorized for DAC dataset listing v2. |
| src/main/resources/assets/paths/getDatasetsFromUserDacs.yaml | Documents 401 Unauthorized for DAC dataset listing v1. |
| src/main/resources/assets/paths/datasetV3.yaml | Documents 401 Unauthorized for dataset registration. |
| src/main/resources/assets/paths/datasetIndexById.yaml | Documents 401 Unauthorized for per-dataset indexing operations. |
| src/main/resources/assets/paths/datasetIndex.yaml | Documents 401 Unauthorized for bulk dataset indexing. |
| src/main/resources/assets/paths/datasetByIdDataUse.yaml | Documents 401 Unauthorized for updating dataset data use. |
| src/main/resources/assets/paths/darV2ByReferenceId.yaml | Documents 401 Unauthorized for DAR update-by-reference-id. |
| src/main/resources/assets/paths/darV2.yaml | Documents 401 Unauthorized for DAR v2 create/list operations. |
| src/main/resources/assets/paths/dacById.yaml | Documents 401 Unauthorized for DAC-by-id. |
| src/main/resources/assets/paths/dac.yaml | Documents 401 Unauthorized for DAC list. |
| src/main/resources/assets/paths/adminFeatureById.yaml | Documents 401 Unauthorized for feature flag create/delete. |
| src/main/resources/assets/api-docs.yaml | Adds inline 401 Unauthorized docs for affected operations. |
| src/main/java/org/broadinstitute/consent/http/resources/VoteResource.java | Switches vote endpoints to DuosUser and removes UserService lookup/dependency. |
| src/main/java/org/broadinstitute/consent/http/resources/UserResource.java | Switches selected user endpoints to DuosUser and removes redundant lookups. |
| src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java | Switches custodians/update/delete study endpoints to DuosUser and removes UserService dependency. |
| src/main/java/org/broadinstitute/consent/http/resources/MatchResource.java | Switches admin reprocess endpoint to DuosUser. |
| src/main/java/org/broadinstitute/consent/http/resources/MailResource.java | Switches admin mail endpoints to DuosUser. |
| src/main/java/org/broadinstitute/consent/http/resources/FeatureFlagResource.java | Switches feature flag endpoints to DuosUser and removes UserService dependency. |
| src/main/java/org/broadinstitute/consent/http/resources/EmailNotifierResource.java | Switches notifier endpoints to DuosUser. |
| src/main/java/org/broadinstitute/consent/http/resources/DraftResource.java | Switches draft endpoints to DuosUser and removes UserService dependency. |
| src/main/java/org/broadinstitute/consent/http/resources/DatasetResource.java | Switches dataset registration/index/datause endpoints to DuosUser and uses duosUser.getUser(). |
| src/main/java/org/broadinstitute/consent/http/resources/DataAccessRequestResource.java | Switches DAR v2 endpoints to DuosUser and removes internal email lookup helper. |
| src/main/java/org/broadinstitute/consent/http/resources/DacResource.java | Switches DAC list/get/filterUsers endpoints to DuosUser. |
| src/main/java/org/broadinstitute/consent/http/resources/DACAutomationRuleResource.java | Switches DAC automation-rule endpoints to DuosUser and removes UserService dependency. |
Addresses Copilot review feedback on PR #2979: tests were passing a mocked DuosUser without stubbing getUser(), defaulting to null and making the tests less representative of production callers. DraftResource always dereferences duosUser.getUser() as the first statement in every method, so the default is added once in initResource() and used by every test. StudyResource's patchStudyById/getStudyById short-circuit before touching the user on some not-found paths, so a blanket default there would trip Mockito's strict-stubbing check; only the three updateCustodians tests (which unconditionally consume the user) got an explicit stub. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # src/main/java/org/broadinstitute/consent/http/resources/DatasetResource.java # src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java # src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java
… change The develop merge (DT-3595/3596/3597) switched updateStudyByRegistration to fetch the existing study via datasetService.getStudyWithDatasetsById instead of datasetRegistrationService.findStudyById. Three tests were still stubbing the old lookup method, and one was missing the duosUser.getUser() stub entirely, causing Mockito strict-stubbing argument-mismatch exceptions (surfaced as unexpected 500s). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rushtong
marked this pull request as ready for review
July 17, 2026 18:26
rushtong
requested review from
fboulnois,
kevinmarete and
otchet-broad
and removed request for
a team
July 17, 2026 18:26
kevinmarete
approved these changes
Jul 17, 2026
kevinmarete
left a comment
kevinmarete
left a comment
Contributor
There was a problem hiding this comment.
Very clean, handle the nit sonar warnings and all good!
|
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
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.
Addresses
https://broadworkbench.atlassian.net/browse/DT-3788
Summary
Migrates 51 of the 52
@Auth AuthUserresource endpoints to@Auth DuosUseracross 12 resourceclasses (
EmailNotifierResource,DatasetResource,DacResource,MatchResource,StudyResource,DraftResource,MailResource,UserResource,VoteResource,FeatureFlagResource,DataAccessRequestResource,DACAutomationRuleResource), removing theredundant
UserService.findUserByEmail(authUser.getEmail())lookup from each migrated method infavor of
duosUser.getUser()/duosUser.getUserId().UserResource.createResearcheris the one endpoint left onAuthUser— it's the registrationflow and explicitly depends on the caller not having a Consent user yet, which
DuosUserdoesn't allow.
No business logic moved into the resource layer as part of this cleanup; only the principal type
and the now-redundant lookup changed. Response bodies and success status codes are unchanged.
Behavior change (documented, not a bug): for role-restricted endpoints, an authenticated
caller who has no Consent user row now gets 401 from
DuosUserAuthenticatorat theauthentication layer, instead of whatever status the resource/authorizer produced before (usually
403, from
AuthorizationHelper.authorize()silently swallowing the lookup failure). Roleauthorization already performed its own independent
findUserByEmaillookup regardless of theresource's principal type, so this migration doesn't introduce new lookup risk — it only shifts
when the "no Consent user" case is caught.
src/main/resources/assets/paths/*.yaml(and theinline paths in
api-docs.yaml) have been updated to document401on the ~24 affected operationsthat didn't already list it.
Tests: updated ~15 resource test files to use
DuosUserfixtures in place ofAuthUser,dropping now-dead
findUserByEmailstubs (would otherwise trip Mockito'sUnnecessaryStubbingException) and the handful of tests that specifically exercised theresource-level
NotFoundExceptioncatch — that scenario is now handled upstream by theauthenticator and is already covered generically in
AuthorizationHelperTest. Added afilterUserstest toDacResourceTest(previously untested). Full suite: 3379 tests, 0failures/errors.