From 92299b12956e811a7a7a25cdbb30824e3ff6a8b0 Mon Sep 17 00:00:00 2001 From: Elliot Otchet Date: Tue, 8 Sep 2026 18:17:30 +0000 Subject: [PATCH] [DT-4061] Centralize study read access in DatasetService 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) --- docs/ai/CLAUDE.md | 26 ++- .../authentication/AuthorizationHelper.java | 6 + .../consent/http/resources/StudyResource.java | 7 +- .../consent/http/service/DatasetService.java | 53 ++++-- .../AuthorizationHelperTest.java | 60 +++++++ .../http/resources/StudyResourceTest.java | 62 +++++-- .../http/service/DatasetServiceTest.java | 165 +++++++++++++++++- 7 files changed, 330 insertions(+), 49 deletions(-) diff --git a/docs/ai/CLAUDE.md b/docs/ai/CLAUDE.md index 0b315a0832..c79ecd7479 100644 --- a/docs/ai/CLAUDE.md +++ b/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. diff --git a/src/main/java/org/broadinstitute/consent/http/authentication/AuthorizationHelper.java b/src/main/java/org/broadinstitute/consent/http/authentication/AuthorizationHelper.java index 6ea4ca746a..676e257f6b 100644 --- a/src/main/java/org/broadinstitute/consent/http/authentication/AuthorizationHelper.java +++ b/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())); diff --git a/src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java b/src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java index 0c0aaa310b..31d3ccbb14 100644 --- a/src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java +++ b/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); } } diff --git a/src/main/java/org/broadinstitute/consent/http/service/DatasetService.java b/src/main/java/org/broadinstitute/consent/http/service/DatasetService.java index f4f4ccfb21..e99750a5fc 100644 --- a/src/main/java/org/broadinstitute/consent/http/service/DatasetService.java +++ b/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. + * + *

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. + * + *

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 findAllDatasetStudySummaries(User user) { diff --git a/src/test/java/org/broadinstitute/consent/http/authentication/AuthorizationHelperTest.java b/src/test/java/org/broadinstitute/consent/http/authentication/AuthorizationHelperTest.java index 553e8a6661..d4a5fd619a 100644 --- a/src/test/java/org/broadinstitute/consent/http/authentication/AuthorizationHelperTest.java +++ b/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")); diff --git a/src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java b/src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java index cb93179550..ef90f5a2a8 100644 --- a/src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java @@ -128,7 +128,7 @@ void testGetStudyByIdNoDatasets() { study.setName("asdfasdfasdfasdfasdfasdf"); when(datasetService.getStudyWithDatasetsById(user, study.getStudyId())).thenReturn(study); when(duosUser.getUser()).thenReturn(user); - when(datasetService.isCreatorCustodianOrAdmin(user, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, user)).thenReturn(study); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); @@ -154,7 +154,7 @@ void testGetStudyByIdWithDatasets() { when(datasetService.getStudyWithDatasetsById(user, study.getStudyId())).thenReturn(study); when(duosUser.getUser()).thenReturn(user); - when(datasetService.isCreatorCustodianOrAdmin(user, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, user)).thenReturn(study); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); @@ -181,7 +181,8 @@ void testGetStudyByIdNotPublicGeneralUser() { when(duosUser.getUser()).thenReturn(generalUser); when(datasetService.getStudyWithDatasetsById(duosUser.getUser(), study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(generalUser, study)).thenReturn(false); + when(datasetService.verifyStudyVisibilityAccess(study, generalUser)) + .thenThrow(new NotFoundException("Study not found")); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_NOT_FOUND, response.getStatus()); @@ -197,7 +198,7 @@ void testGetStudyByIdNotPublicCreateUser() { when(duosUser.getUser()).thenReturn(createUser); when(datasetService.getStudyWithDatasetsById(duosUser.getUser(), study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(createUser, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, createUser)).thenReturn(study); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); @@ -209,7 +210,7 @@ void testGetRegistrationFromStudy() { Study study = createMockStudy(); when(datasetService.getStudyWithDatasetsById(user, study.getStudyId())).thenReturn(study); when(duosUser.getUser()).thenReturn(user); - when(datasetService.isCreatorCustodianOrAdmin(user, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, user)).thenReturn(study); try (var response = resource.getRegistrationFromStudy(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); @@ -222,7 +223,7 @@ void testGetRegistrationFromStudyNoDatasets() { study.getDatasets().clear(); when(datasetService.getStudyWithDatasetsById(user, study.getStudyId())).thenReturn(study); when(duosUser.getUser()).thenReturn(user); - when(datasetService.isCreatorCustodianOrAdmin(user, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, user)).thenReturn(study); try (var response = resource.getRegistrationFromStudy(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); @@ -249,7 +250,8 @@ void testGetRegistrationFromStudyNotPublicGeneralUser() { when(duosUser.getUser()).thenReturn(generalUser); when(datasetService.getStudyWithDatasetsById(generalUser, study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(generalUser, study)).thenReturn(false); + when(datasetService.verifyStudyVisibilityAccess(study, generalUser)) + .thenThrow(new NotFoundException("Study not found")); try (var response = resource.getRegistrationFromStudy(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_NOT_FOUND, response.getStatus()); @@ -265,7 +267,7 @@ void testGetRegistrationFromStudyNotPublicCreateUser() { when(duosUser.getUser()).thenReturn(createUser); when(datasetService.getStudyWithDatasetsById(duosUser.getUser(), study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(createUser, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, createUser)).thenReturn(study); try (var response = resource.getRegistrationFromStudy(duosUser, study.getStudyId())) { assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); @@ -593,7 +595,7 @@ void testCheckPublicVisibilityForUser_PublicStudy_ApprovedRole() { approvedUser.setUserId(study.getCreateUserId()); when(datasetService.getStudyWithDatasetsById(approvedUser, study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(approvedUser, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, approvedUser)).thenReturn(study); when(duosUser.getUser()).thenReturn(approvedUser); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { @@ -609,7 +611,7 @@ void testCheckPublicVisibilityForUser_PublicStudy_NoApprovedRole() { generalUser.setUserId(randomInt(1000, 1100)); when(datasetService.getStudyWithDatasetsById(generalUser, study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(generalUser, study)).thenReturn(false); + when(datasetService.verifyStudyVisibilityAccess(study, generalUser)).thenReturn(study); when(duosUser.getUser()).thenReturn(generalUser); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { @@ -624,7 +626,7 @@ void testCheckPublicVisibilityForUser_PrivateStudy_Creator() { User creator = new User(); creator.setUserId(study.getCreateUserId()); when(datasetService.getStudyWithDatasetsById(creator, study.getStudyId())).thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(creator, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, creator)).thenReturn(study); when(duosUser.getUser()).thenReturn(creator); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { @@ -639,7 +641,7 @@ void testCheckPublicVisibilityForUser_PrivateStudy_Custodian() { User custodian = new User(); custodian.setUserId(randomInt(1000, 1100)); when(datasetService.getStudyWithDatasetsById(custodian, study.getStudyId())).thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(custodian, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, custodian)).thenReturn(study); when(duosUser.getUser()).thenReturn(custodian); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { @@ -655,7 +657,7 @@ void testCheckPublicVisibilityForUser_PrivateStudy_Admin() { admin.setUserId(randomInt(1000, 1100)); admin.setAdminRole(); when(datasetService.getStudyWithDatasetsById(admin, study.getStudyId())).thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(admin, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, admin)).thenReturn(study); when(duosUser.getUser()).thenReturn(admin); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { @@ -671,7 +673,8 @@ void testCheckPublicVisibilityForUser_PrivateStudy_NoApprovedRole() { generalUser.setUserId(randomInt(1000, 1100)); when(datasetService.getStudyWithDatasetsById(generalUser, study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(generalUser, study)).thenReturn(false); + when(datasetService.verifyStudyVisibilityAccess(study, generalUser)) + .thenThrow(new NotFoundException("Study not found")); when(duosUser.getUser()).thenReturn(generalUser); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { @@ -679,19 +682,39 @@ void testCheckPublicVisibilityForUser_PrivateStudy_NoApprovedRole() { } } + // A study whose public_visibility is NULL (the column is nullable) reads as "not public". + // An approved user still sees it; anyone else gets a 404 rather than the 500 this used to + // produce by unboxing the null before checking the role. @Test - void testCheckPublicVisibilityForUser_PublicVisibilityNull_CausesError() { + void testCheckPublicVisibilityForUser_PublicVisibilityNull() { Study study = createMockStudy(); study.setPublicVisibility(null); User approvedUser = new User(); approvedUser.setUserId(study.getCreateUserId()); when(datasetService.getStudyWithDatasetsById(approvedUser, study.getStudyId())) .thenReturn(study); - when(datasetService.isCreatorCustodianOrAdmin(approvedUser, study)).thenReturn(true); + when(datasetService.verifyStudyVisibilityAccess(study, approvedUser)).thenReturn(study); when(duosUser.getUser()).thenReturn(approvedUser); try (var response = resource.getStudyById(duosUser, study.getStudyId())) { - assertEquals(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, response.getStatus()); + assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); + } + } + + @Test + void testCheckPublicVisibilityForUser_PublicVisibilityNull_NoApprovedRole() { + Study study = createMockStudy(); + study.setPublicVisibility(null); + User generalUser = new User(); + generalUser.setUserId(randomInt(1000, 1100)); + when(datasetService.getStudyWithDatasetsById(generalUser, study.getStudyId())) + .thenReturn(study); + when(datasetService.verifyStudyVisibilityAccess(study, generalUser)) + .thenThrow(new NotFoundException("Study not found")); + when(duosUser.getUser()).thenReturn(generalUser); + + try (var response = resource.getStudyById(duosUser, study.getStudyId())) { + assertEquals(HttpStatusCodes.STATUS_CODE_NOT_FOUND, response.getStatus()); } } @@ -702,6 +725,7 @@ void testPatchStudyById() { admin.setAdminRole(); admin.setUserId(study.getCreateUserId()); when(datasetService.findStudy(study.getStudyId())).thenReturn(study); + when(datasetService.verifyStudyVisibilityAccess(study, admin)).thenReturn(study); when(duosUser.getUser()).thenReturn(admin); String patchJson = """ @@ -736,6 +760,8 @@ void testPatchStudyByIdNotModified() { admin.setAdminRole(); admin.setUserId(study.getCreateUserId()); when(datasetService.findStudy(study.getStudyId())).thenReturn(study); + when(datasetService.verifyStudyVisibilityAccess(study, admin)).thenReturn(study); + when(duosUser.getUser()).thenReturn(admin); try (var response = resource.patchStudyById(duosUser, study.getStudyId(), "{}")) { assertEquals(HttpStatusCodes.STATUS_CODE_NOT_MODIFIED, response.getStatus()); } @@ -758,6 +784,8 @@ void testPatchStudyByIdInvalidPatch(String json) { admin.setAdminRole(); admin.setUserId(study.getCreateUserId()); when(datasetService.findStudy(study.getStudyId())).thenReturn(study); + when(datasetService.verifyStudyVisibilityAccess(study, admin)).thenReturn(study); + when(duosUser.getUser()).thenReturn(admin); try (var response = resource.patchStudyById(duosUser, study.getStudyId(), json)) { assertEquals(HttpStatusCodes.STATUS_CODE_BAD_REQUEST, response.getStatus()); } diff --git a/src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java b/src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java index e44bf615f5..cfbb1dc16c 100644 --- a/src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java @@ -230,8 +230,12 @@ void testFindStudyByIdForReadNotFound() { assertThrows(NotFoundException.class, () -> datasetService.findStudyByIdForRead(mockUser, 99)); } + /** + * A study the caller may not read is reported absent, not forbidden - the same answer + * verifyStudyVisibilityAccess gives, so the study's files and its registration assets agree. + */ @Test - void testFindStudyByIdForReadForbidden() { + void testFindStudyByIdForReadHiddenStudyIsNotFound() { User user = new User(); user.setUserId(1); user.setEmail("user@email.com"); @@ -247,8 +251,7 @@ void testFindStudyByIdForReadForbidden() { int studyId = study.getStudyId(); when(studyDAO.findStudyById(study.getStudyId())).thenReturn(study); - assertThrows( - ForbiddenException.class, () -> datasetService.findStudyByIdForRead(user, studyId)); + assertThrows(NotFoundException.class, () -> datasetService.findStudyByIdForRead(user, studyId)); } @Test @@ -1097,8 +1100,11 @@ void testVerifyPublicVisibilityAccess_VisibleNull() { dataset.setStudy(study); dataset.setStudyId(study.getStudyId()); - Dataset verfiedDataset = datasetService.verifyPublicVisibilityAccess(dataset, user); - assertEquals(dataset.getDatasetId(), verfiedDataset.getDatasetId()); + // An unset public_visibility is no longer treated as published: the caller is neither the + // dataset's creator nor the study's, so the dataset is withheld. It previously came back, + // which is what let a null-visibility study stay readable here while the study endpoints + // returned 404 for it. + assertNull(datasetService.verifyPublicVisibilityAccess(dataset, user)); } @Test @@ -1187,6 +1193,87 @@ void testVerifyPublicVisibilityAccess_DatasetCreatorWithHiddenStudy() { assertEquals(dataset.getDatasetId(), verifiedDataset.getDatasetId()); } + // verifyStudyVisibilityAccess is the single read-access gate shared by StudyResource and the + // study asset, comment, and metrics endpoints. + @Test + void testVerifyStudyVisibilityAccess_PublicStudyIsReadableByAnyone() { + Study study = new Study(); + study.setStudyId(1); + study.setCreateUserId(1); + study.setPublicVisibility(true); + User generalUser = new User(); + generalUser.setUserId(2); + generalUser.setEmail("general@email.com"); + + assertEquals(study, datasetService.verifyStudyVisibilityAccess(study, generalUser)); + } + + @Test + void testVerifyStudyVisibilityAccess_PrivateStudyIsHiddenFromOtherUsers() { + Study study = new Study(); + study.setStudyId(1); + study.setCreateUserId(1); + study.setCreateUserEmail("creator@email.com"); + study.setPublicVisibility(false); + User generalUser = new User(); + generalUser.setUserId(2); + generalUser.setEmail("general@email.com"); + + assertThrows( + NotFoundException.class, + () -> datasetService.verifyStudyVisibilityAccess(study, generalUser)); + } + + @Test + void testVerifyStudyVisibilityAccess_PrivateStudyIsReadableByCreatorAndAdmin() { + Study study = new Study(); + study.setStudyId(1); + study.setCreateUserId(1); + study.setCreateUserEmail("creator@email.com"); + study.setPublicVisibility(false); + User creator = new User(); + creator.setUserId(1); + creator.setEmail("creator@email.com"); + User admin = new User(); + admin.setUserId(3); + admin.setEmail("admin@email.com"); + admin.setAdminRole(); + + assertEquals(study, datasetService.verifyStudyVisibilityAccess(study, creator)); + assertEquals(study, datasetService.verifyStudyVisibilityAccess(study, admin)); + } + + // The public_visibility column is nullable; a null reads as "not public". + @Test + void testVerifyStudyVisibilityAccess_NullVisibilityIsNotPublic() { + Study study = new Study(); + study.setStudyId(1); + study.setCreateUserId(1); + study.setCreateUserEmail("creator@email.com"); + study.setPublicVisibility(null); + User generalUser = new User(); + generalUser.setUserId(2); + generalUser.setEmail("general@email.com"); + User creator = new User(); + creator.setUserId(1); + creator.setEmail("creator@email.com"); + + assertThrows( + NotFoundException.class, + () -> datasetService.verifyStudyVisibilityAccess(study, generalUser)); + assertEquals(study, datasetService.verifyStudyVisibilityAccess(study, creator)); + } + + @Test + void testVerifyStudyVisibilityAccess_NullStudyIsNotFound() { + User generalUser = new User(); + generalUser.setUserId(2); + + assertThrows( + NotFoundException.class, + () -> datasetService.verifyStudyVisibilityAccess(null, generalUser)); + } + @Test void testIsCreatorOrCustodian_DatasetCreator() { User datasetCreator = new User(); @@ -1832,6 +1919,55 @@ void testFindDatasetsByIds_FilteredByVisibility() { assertEquals(0, result.size()); } + // ============= verifyPublicVisibilityAccess(Dataset, User) – null visibility ============= + + /** + * The dataset route reaches the same rule through canReadStudy, so a study whose + * public_visibility is null is hidden here too. Before the rule was unified, such a study + * returned 404 from the study endpoints while its datasets stayed readable. + */ + @Test + void testVerifyPublicVisibilityAccess_Dataset_PublicVisibilityNull_NotCreator() { + User user = new User(); + user.setUserId(1); + user.setEmail("user@test.com"); + Study study = studyWithNullVisibility(99); + Dataset dataset = new Dataset(); + dataset.setDatasetId(5); + dataset.setCreateUserId(3); + dataset.setStudyId(study.getStudyId()); + dataset.setStudy(study); + + assertNull(datasetService.verifyPublicVisibilityAccess(dataset, user)); + } + + @Test + void testVerifyPublicVisibilityAccess_Dataset_PublicVisibilityNull_Creator() { + User creator = new User(); + creator.setUserId(1); + creator.setEmail("creator@test.com"); + Study study = studyWithNullVisibility(creator.getUserId()); + Dataset dataset = new Dataset(); + dataset.setDatasetId(5); + dataset.setCreateUserId(creator.getUserId()); + dataset.setStudyId(study.getStudyId()); + dataset.setStudy(study); + + assertEquals(dataset, datasetService.verifyPublicVisibilityAccess(dataset, creator)); + } + + private Study studyWithNullVisibility(Integer createUserId) { + Study study = new Study(); + study.setStudyId(7); + study.setCreateUserId(createUserId); + study.setPublicVisibility(null); + StudyProperty property = new StudyProperty(); + property.setKey("other"); + property.setValue("[]"); + study.addProperties(property); + return study; + } + // ==================== canReadStudy ==================== @Test @@ -1849,13 +1985,28 @@ void testCanReadStudy_Admin() { assertTrue(datasetService.canReadStudy(admin, study)); } + /** + * public_visibility is nullable, and a null now reads as "not published" rather than as public. + * It previously 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. + */ @Test void testCanReadStudy_PublicVisibilityNull() { User user = new User(); user.setUserId(1); + user.setEmail("user@test.com"); + Study study = new Study(); + study.setCreateUserId(99); + assertFalse(datasetService.canReadStudy(user, study)); + } + + @Test + void testCanReadStudy_PublicVisibilityNullForCreator() { + User creator = new User(); + creator.setUserId(1); Study study = new Study(); - // publicVisibility null → !Boolean.FALSE.equals(null) is true → readable - assertTrue(datasetService.canReadStudy(user, study)); + study.setCreateUserId(creator.getUserId()); + assertTrue(datasetService.canReadStudy(creator, study)); } @Test