[DT-4061] Centralize study read access in DatasetService

docs/ai/CLAUDE.md

@@ -34,17 +34,27 @@ instantiated via `jdbi.onDemand()`.
 
 ## ConsentModule singleton pattern
 
-Every `@Provides` method in `ConsentModule` that creates a new service or DAO instance uses
-`@Singleton` + `synchronized` + a lazy null-guard field to guarantee a single instance
-on both the Guice injection path and the direct inter-provider call path:
+Every `@Provides` method in `ConsentModule` that creates a new service or DAO instance is
+annotated `@Singleton`, and takes each of its dependencies as a method parameter so Guice
+resolves them. Guice caches the singleton itself, so no lazy field or `synchronized` guard
+is needed:
 
 ```java
 @Provides
 @Singleton
-synchronized EmailService providesEmailService() {
-  if (emailService == null) {
-    emailService = new EmailService(...);
-  }
-  return emailService;
+private DatasetService providesDatasetService(
+    Jdbi jdbi,
+    DatasetServiceDAO datasetServiceDAO,
+    ElasticSearchService elasticSearchService,
+    EmailService emailService,
+    OntologyService ontologyService) {
+  return new DatasetService(
+      jdbi, datasetServiceDAO, elasticSearchService, emailService, ontologyService);
 }
 ```
+
+Never call one `@Provides` method from another. A direct call bypasses Guice's scoping and
+builds a second instance with its own `jdbi.onDemand` DAOs. Declare the dependency as a
+parameter instead. Adding a new service means adding a provider here — a service that is
+only JIT-bound (constructed by Guice without a declared provider) is unscoped, so a second
+injection point silently creates a second instance.

src/main/java/org/broadinstitute/consent/http/authentication/AuthorizationHelper.java

@@ -105,6 +105,12 @@ protected boolean authorize(AuthUser authUser, String role) {
     boolean authorize = false;
     try {
       User user = userService.findUserByEmail(authUser.getEmail());
+      // A user with no user_role rows has a null role list, not an empty one. Without this guard
+      // the authorizer throws a NullPointerException and Jersey turns what should be a plain
+      // denial into a 500 on every @RolesAllowed endpoint. Mirrors User#hasAnyUserRole.
+      if (user == null || user.getRoles() == null) {
+        return false;
+      }
       return user.getRoles().stream().anyMatch(r -> r.getName().equalsIgnoreCase(role));
     } catch (NotFoundException e) {
       logWarn("User not found, authorization incomplete: %s".formatted(authUser.getEmail()));

src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java

@@ -298,11 +298,6 @@ private StudyUpdateValidationResult validateRegistrationUpdate(String json, Stud
   }
 
   private void checkPublicVisibilityForUser(Study study, User user) {
-    boolean isApprovedRole = datasetService.isCreatorCustodianOrAdmin(user, study);
-    boolean isPubliclyVisible = study.getPublicVisibility();
-    // If approved role or publicly visible, the user can see the study, otherwise throw
-    if (!isApprovedRole && !isPubliclyVisible) {
-      throw new NotFoundException("Study not found");
-    }
+    datasetService.verifyStudyVisibilityAccess(study, user);
   }
 }

src/main/java/org/broadinstitute/consent/http/service/DatasetService.java

@@ -201,17 +201,23 @@ protected Dataset verifyPublicVisibilityAccess(Dataset dataset, User user) {
     return null;
   }
 
+  /**
+   * Whether the user may read this study and anything derived from it: its creator, its custodians
+   * and admins always may, anyone may when it is publicly visible.
+   *
+   * <p>public_visibility is nullable, and a null is not a grant - it is an unset flag on a study
+   * nobody has published. This used to read as public here while the dataset study summaries
+   * treated it as private, so the same study was readable through one route and hidden on another.
+   * The single definition lives here; {@link #verifyStudyVisibilityAccess(Study, User)} is the
+   * throwing form of it.
+   */
   protected boolean canReadStudy(User user, Study study) {
     if (study == null) {
       return false;
     }
-    if (user.hasUserRole(UserRoles.ADMIN)) {
-      return true;
-    }
-    if (!Boolean.FALSE.equals(study.getPublicVisibility())) {
-      return true;
-    }
-    return isCreatorOrCustodian(user, study);
+    // Visibility first: a published study is readable without reading its creator or properties.
+    return Boolean.TRUE.equals(study.getPublicVisibility())
+        || isCreatorCustodianOrAdmin(user, study);
   }
 
   protected boolean isCreatorOrCustodian(User user, Dataset dataset) {
@@ -251,6 +257,25 @@ public boolean isCreatorCustodianOrAdmin(User user, Study study) {
     return user.hasUserRole(UserRoles.ADMIN) || isCreatorOrCustodian(user, study);
   }
 
+  /**
+   * Enforces read access to a study and everything derived from it. A study that is not publicly
+   * visible is readable only by its creator, its custodians, and admins. Mirrors {@link
+   * #verifyPublicVisibilityAccess(Dataset, User)} for datasets.
+   *
+   * @param study The study to check. Must be populated with creator and properties.
+   * @param user The requesting user
+   * @return The same study, when the user may read it
+   * @throws NotFoundException if the study is not visible to the user
+   */
+  public Study verifyStudyVisibilityAccess(Study study, User user) {
+    // A study the caller may not read is reported as absent rather than forbidden, as is a study
+    // that does not exist. canReadStudy holds the rule; see it for the null-visibility case.
+    if (!canReadStudy(user, study)) {
+      throw new NotFoundException("Study not found");
+    }
+    return study;
+  }
+
   public Dataset getDatasetByName(String name) {
     String lowercaseName = name.toLowerCase();
     return datasetDAO.getDatasetByName(lowercaseName);
@@ -347,15 +372,21 @@ public Study findStudy(Integer studyId) {
     return studyDAO.findStudyById(studyId);
   }
 
+  /**
+   * Loads a study for reading, or reports it absent.
+   *
+   * <p>Delegates to {@link #verifyStudyVisibilityAccess(Study, User)} rather than re-deciding: a
+   * second gate over the same predicate used to answer 403 here and 404 there, so two routes over
+   * the same study - its registration assets and its files - disagreed about whether a study the
+   * caller may not read is forbidden or absent. A 403 also confirms the study exists, which is what
+   * the visibility flag is meant to withhold.
+   */
   public Study findStudyByIdForRead(User user, Integer studyId) {
     Study study = studyDAO.findStudyById(studyId);
     if (study == null) {
       throw new NotFoundException("Entity not found");
     }
-    if (!canReadStudy(user, study)) {
-      throw new ForbiddenException("User does not have permission");
-    }
-    return study;
+    return verifyStudyVisibilityAccess(study, user);
   }
 
   public List<DatasetStudySummary> findAllDatasetStudySummaries(User user) {

src/test/java/org/broadinstitute/consent/http/authentication/AuthorizationHelperTest.java

@@ -4,6 +4,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
@@ -99,6 +100,65 @@ void testNotAuthorized(String roleName) {
     assertFalse(authorizationHelper.authorize(unauthorizedDuosUser, roleName));
   }
 
+  /**
+   * A user with no user_role rows comes back with a null role list, not an empty one. Before the
+   * null guard this threw a NullPointerException out of the authorizer, which Jersey reported as a
+   * 500 - so every @RolesAllowed endpoint answered a roleless caller with a server error instead of
+   * denying them. Authorization must simply be false.
+   */
+  @ParameterizedTest
+  @ValueSource(
+      strings = {
+        Resource.MEMBER,
+        Resource.CHAIRPERSON,
+        Resource.RESEARCHER,
+        Resource.SIGNINGOFFICIAL,
+        Resource.ADMIN,
+        Resource.DATASUBMITTER,
+        Resource.ITDIRECTOR
+      })
+  void testNotAuthorizedWhenUserHasNoRoles(String roleName) {
+    User user = new User();
+    assertNull(user.getRoles(), "a user with no roles must have a null role list for this test");
+    // The email is incidental here: what matters is that the lookup yields a user with no roles.
+    when(userService.findUserByEmail(any())).thenReturn(user);
+
+    assertFalse(authorizationHelper.authorize(unauthorizedUser, roleName));
+    assertFalse(authorizationHelper.authorize(unauthorizedDuosUser, roleName));
+  }
+
+  /** The same case, reached through the authorizers Dropwizard actually wires up. */
+  @Test
+  void testAuthorizersDenyUserWithNoRoles() {
+    User user = new User();
+    when(userService.findUserByEmail(any())).thenReturn(user);
+
+    assertFalse(
+        new UserAuthorizer(authorizationHelper)
+            .authorize(unauthorizedUser, Resource.RESEARCHER, null));
+    assertFalse(
+        new DuosUserAuthorizer(authorizationHelper)
+            .authorize(unauthorizedDuosUser, Resource.RESEARCHER, null));
+  }
+
+  /** An explicitly empty role list behaves the same way as a null one. */
+  @Test
+  void testNotAuthorizedWhenUserHasEmptyRoleList() {
+    User user = new User();
+    user.setRoles(List.of());
+    when(userService.findUserByEmail(any())).thenReturn(user);
+
+    assertFalse(authorizationHelper.authorize(unauthorizedUser, Resource.RESEARCHER));
+  }
+
+  /** findUserByEmail returning null must deny rather than throw. */
+  @Test
+  void testNotAuthorizedWhenUserIsNull() {
+    when(userService.findUserByEmail(any())).thenReturn(null);
+
+    assertFalse(authorizationHelper.authorize(unauthorizedUser, Resource.RESEARCHER));
+  }
+
   @Test
   void testAuthenticateWithToken() {
     headerMap.put(ClaimsCache.OAUTH2_CLAIM_email, List.of("email"));