[DT-4061] Centralize study read access in DatasetService - #3049
[DT-4061] Centralize study read access in DatasetService#3049otchet-broad wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated tests introduce a disallowed lenient Mockito stub and add new authorization tests that currently rely on mocked AuthUser email state without stubbing getEmail(), reducing test correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Centralizes study read-visibility enforcement by moving the gating logic out of StudyResource into DatasetService#verifyStudyVisibilityAccess, so downstream endpoints can reuse a single, consistent rule and avoid 500s caused by null unboxing of public_visibility. Also hardens authorization against users whose user_role query returns a null role list, ensuring @RolesAllowed checks deny rather than error.
Changes:
- Added
DatasetService#verifyStudyVisibilityAccess(Study, User)and updatedStudyResourceto delegate to it. - Added null guards in
AuthorizationHelperfor missing users / null role lists, with new tests. - Updated tests and internal AI docs to reflect the new access gate and Guice provider patterns.
File summaries
| File | Description |
|---|---|
| src/main/java/org/broadinstitute/consent/http/service/DatasetService.java | Introduces the centralized study visibility read gate. |
| src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java | Switches resource-level visibility check to call the shared service gate. |
| src/main/java/org/broadinstitute/consent/http/authentication/AuthorizationHelper.java | Prevents NPEs when the user or role list is null during authorization. |
| src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java | Adds unit tests covering public/private/null study visibility behavior. |
| src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java | Updates resource tests to account for the new service-level gate. |
| src/test/java/org/broadinstitute/consent/http/authentication/AuthorizationHelperTest.java | Adds test coverage for null/empty roles and null user authorization cases. |
| docs/ai/CLAUDE.md | Corrects/clarifies ConsentModule singleton/provider guidance. |
Review details
Suppressed comments (2)
src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java:80
- The mock implementation of verifyStudyVisibilityAccess doesn't handle a null Study argument, so a null would cause a NullPointerException via study.getPublicVisibility(). The real DatasetService#verifyStudyVisibilityAccess throws NotFoundException on null; the test stub should mirror that to avoid accidental 500s or brittle tests.
invocation -> {
Study study = invocation.getArgument(0);
User requestingUser = invocation.getArgument(1);
if (!datasetService.isCreatorCustodianOrAdmin(requestingUser, study)
&& !Boolean.TRUE.equals(study.getPublicVisibility())) {
throw new NotFoundException("Study not found");
src/test/java/org/broadinstitute/consent/http/authentication/AuthorizationHelperTest.java:136
- Same issue here: unauthorizedUser/unauthorizedDuosUser are Mockito mocks, so setEmail(...) (used in other tests) won't affect getEmail() unless it is explicitly stubbed. Without stubbing, this test may not be validating the intended behavior.
User user = new User();
user.setEmail("email");
when(userService.findUserByEmail(unauthorizedUser.getEmail())).thenReturn(user);
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
1f57fc7 to
0233a80
Compare
kevinmarete
left a comment
kevinmarete
left a comment
There was a problem hiding this comment.
verifyStudyVisibilityAccess treats null visibility as private, but canReadStudy still treats every value except false as public. A null-visibility study can therefore return 404 through the new study gate while remaining readable through dataset routes. Please define this rule once, ideally by changing canReadStudy to require Boolean.TRUE.equals(...) and delegating the new gate to it. Please also cover the dataset path with a null-visibility test.
68ba657 to
3d192ec
Compare
fboulnois
left a comment
fboulnois
left a comment
There was a problem hiding this comment.
Some issues I filtered that Claude and Codex identified:
Duplicate authorization gates return different statuses for the same predicate
DatasetService.java:375 defines findStudyByIdForRead, which wraps canReadStudy and throws ForbiddenException. FileStorageObjectService.java:442 uses this method for study files. The new verifyStudyVisibilityAccess wraps the same predicate but throws NotFoundException.
In the downstream stack, otchet-dt-4061-study-asset-endpoints adds a StudyAssetService that uses the 404 gate, while FileStorageObjectService continues using the 403 gate. As a result, two study-asset routes will disagree about whether a hidden study is forbidden or absent. That distinction matters because a 403 confirms that the study exists, while a 404 does not.
Redundant initService() calls
DatasetServiceTest.java contains redundant initService() calls at lines 1197, 1211, 1228, 1249, and 1270. The method already runs under @beforeeach, and the other tests in this class do not call it explicitly.
Extracts the study read-visibility rule out of StudyResource into DatasetService#verifyStudyVisibilityAccess so the study asset, comment, and metrics endpoints added later in this stack share one gate. The rule now has a single definition, in canReadStudy, of which the new gate is just the throwing form. canReadStudy read a null public_visibility as public, while the dataset study summaries already required Boolean.TRUE, so the two disagreed about a study with an unset flag. It now reads as "not published" in both: readable by its creator, its custodians and admins, and by anyone else only once public_visibility is TRUE. That disagreement was unreachable rather than live, so this is a consistency fix and not a change in behavior. study.public_visibility is NOT NULL in the production schema, so no row can hold one: across the 353 studies in the most recent restored dump, 322 are TRUE, 31 are FALSE, and none are NULL. Nor can a null reach the rule from a partially selected row - every query that materializes a Study includes the column. findStudyDetailsById, which findStudyById assembles from, and findStudyByName both select s.*, and DatasetDAO.findDatasetStudyById, the only query that attaches a Study to a Dataset, selects s.public_visibility explicitly. The nullability is an artifact of the Boolean field, so the handling here is defensive. Two existing tests asserted the old reading of a null and now assert the new one, and the dataset route's null-visibility case is covered in both directions, so the intent is pinned either way. For the same reason the 500 this also fixes was latent: StudyResource unboxed public_visibility before checking the caller's role, so a null would have thrown rather than denied. findStudyByIdForRead now delegates to the same gate instead of re-deciding. It was a second check over the same predicate that answered 403 where the new gate answers 404, so two routes over one study - its files and its registration assets - disagreed about whether a study the caller may not read is forbidden or absent. They now agree on absent, which is also what the visibility flag is for: a 403 confirms the study exists. This changes the study-file route's answer for a hidden study from 403 to 404; the test that pinned the old status now pins the new one. The dataset-file route still answers 403 for a hidden dataset, which is a separate pre-existing difference and untouched here. Guards AuthorizationHelper against a user with no user_role rows, whose role list is null rather than empty. That NPE was reachable, and surfaced as a 500 on every @RolesAllowed endpoint instead of a plain denial. Also corrects the ConsentModule singleton guidance in docs/ai/CLAUDE.md to describe the provider-parameter pattern the module actually uses. No migration. The PATCH ownership gate that shares this refactor lives on otchet-dt-4061-study-patch-ownership, so it can be reviewed separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3d192ec to
92299b1
Compare
|
Base:
develop· 7 files, +238/-16 · Migration: no · UI impact: noneExtracts the study read-visibility rule out of
StudyResourceintoDatasetService#verifyStudyVisibilityAccess, so the asset, comment, andmetrics endpoints later in the stack all share one gate rather than each
re-deriving it. A study whose
public_visibilityis NULL now reads as "notpublic" — 404 for an unapproved caller — instead of throwing on the unboxing
and returning 500.
Also guards
AuthorizationHelperagainst a user with nouser_rolerows,whose role list is null rather than empty. That NPE surfaced as a 500 on
every
@RolesAllowedendpoint instead of a plain denial. Corrects theConsentModulesingleton guidance indocs/ai/CLAUDE.mdto describe theprovider-parameter pattern the module actually uses.
No behavior change for any caller who could already read a study, and no
migration. The PATCH ownership gate that originally shared this refactor is
now branch 1b.
Depends on: nothing. Every other branch depends on this one.
Where this sits in the DT-4061 stack
otchet-dt-3990-study-ratings-pi-detailswas too large to review meaningfully(95 files, +8168), so it was split into eight PRs. This PR targets
develop,not
develop, so its diff shows only its own work.otchet-dt-4061-study-visibility-authz← this PRdevelopotchet-dt-4061-study-patch-ownershipotchet-dt-4061-study-visibility-authzotchet-dt-4061-study-pi-detailsotchet-dt-4061-study-visibility-authzotchet-dt-4061-study-commentsotchet-dt-4061-study-pi-detailsotchet-dt-4061-study-dar-metricsotchet-dt-4061-study-commentsotchet-dt-4061-study-recommendationsotchet-dt-4061-study-dar-metricsotchet-dt-4061-study-asset-fieldsotchet-dt-4061-study-recommendationsotchet-dt-4061-study-asset-endpointsotchet-dt-4061-study-asset-fieldsBranch 1b is a sibling rather than a link in the chain: nothing depends on it,
so it can be held or dropped without blocking the others. Land the numbered
chain in order — merging out of order, or squash-merging, will require rebasing
the descendants.
The split is verified lossless: branch 7 plus 1b reproduces the original branch
exactly, apart from five
scripts/verify-study-*.shhelper scripts that weredropped at the author's request.
./mvnw test-compilepasses on each branchindependently.
🤖 Generated with Claude Code