[DT-4061] Centralize study read access in DatasetService

[otchet-broad]

Base: develop · 7 files, +238/-16 · Migration: no · UI impact: none

Extracts the study read-visibility rule out of StudyResource into
DatasetService#verifyStudyVisibilityAccess, so the asset, comment, and
metrics endpoints later in the stack all share one gate rather than each
re-deriving it. A study whose public_visibility is NULL now reads as "not
public" — 404 for an unapproved caller — instead of throwing on the unboxing
and returning 500.

Also guards AuthorizationHelper against a user with no user_role rows,
whose role list is null rather than empty. That NPE surfaced as a 500 on
every @RolesAllowed endpoint instead of a plain denial. Corrects the
ConsentModule singleton guidance in docs/ai/CLAUDE.md to describe the
provider-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-details was 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.

#	Branch	Base
1	otchet-dt-4061-study-visibility-authz ← this PR	develop
1b	otchet-dt-4061-study-patch-ownership	otchet-dt-4061-study-visibility-authz
2	otchet-dt-4061-study-pi-details	otchet-dt-4061-study-visibility-authz
3	otchet-dt-4061-study-comments	otchet-dt-4061-study-pi-details
4	otchet-dt-4061-study-dar-metrics	otchet-dt-4061-study-comments
5	otchet-dt-4061-study-recommendations	otchet-dt-4061-study-dar-metrics
6	otchet-dt-4061-study-asset-fields	otchet-dt-4061-study-recommendations
7	otchet-dt-4061-study-asset-endpoints	otchet-dt-4061-study-asset-fields

Branch 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-*.sh helper scripts that were
dropped at the author's request. ./mvnw test-compile passes on each branch
independently.

🤖 Generated with Claude Code

[Copilot]

🟡 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 updated StudyResource to delegate to it.
• Added null guards in AuthorizationHelper for 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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[kevinmarete]

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.