[DT-3107, DT-3108, DT-3110, DT-3112, DT-3113] Add entity-scoped FSO endpoints and access-controlled FileStorageObjectService by kevinmarete · Pull Request #2867 · DataBiosphere/consent

kevinmarete · 2026-04-16T22:33:32Z

Addresses

https://broadworkbench.atlassian.net/browse/DT-3107
https://broadworkbench.atlassian.net/browse/DT-3108
https://broadworkbench.atlassian.net/browse/DT-3110
https://broadworkbench.atlassian.net/browse/DT-3112
https://broadworkbench.atlassian.net/browse/DT-3113

Summary

This PR introduces core API functionality for managing entity-scoped File Storage Objects (FSOs). It includes endpoints for uploading files, streaming file content, soft-deleting FSOs, and updating file categories. Additionally, it adds a centralized FileStorageObjectService to enforce consistent access control policies based on entity, category, and user roles across all FSO operations.

Have you read CONTRIBUTING.md lately? If not, do that first.

Label PR with a Jira ticket number and include a link to the ticket
Label PR with a security risk modifier [no, low, medium, high]
PR describes scope of changes
Get a minimum of one thumbs worth of review, preferably two if enough team members are available
Get PO sign-off for all non-trivial UI or workflow changes
Verify all tests go green
Test this change deployed correctly and works on dev environment after deployment

sonarqubecloud · 2026-04-16T22:40:27Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

eweitz

Looks mostly good!

The only substantive question I have is in the comment beginning "Above notes".

Beyond that, I suggest some potential refinements for code clarity, but defer to you. I also flag code for "update" operations, which IIUC we'll remove pending Greg's confirmation.

eweitz · 2026-04-17T20:48:49Z

+  /**
+   * Downloads the binary content of a document.
+   *
+   * <ol>
+   *   <li>Resolves entity ID.
+   *   <li>Fetches FSO metadata (including category).
+   *   <li>Calls {@link #checkAccess} with the file's actual category (READ).
+   *   <li>Streams file from GCS.
+   * </ol>
+   */


Incidentally, I'm curious about the pros / cons of Consent's pre-existing bucket-to-server-to-client approach vs. a the bucket-to-cilent approach used in Terra and SCP. Added as a parking lot topic for Tuesday!

eweitz · 2026-04-17T20:58:22Z

+  /**
+   * Updates the category of an existing document.
+   *
+   * <ol>
+   *   <li>Parses and validates the new category for the entity.
+   *   <li>Calls {@link #checkAccess} with the new category (CRUD).
+   *   <li>Resolves entity ID, fetches FSO, updates category.
+   * </ol>
+   */
+  public FileStorageObject updateDocumentCategory(


IIUC, undesired pending Greg's confirmation.

eweitz · 2026-04-17T21:36:37Z

+   *         <li>CRUD: DAR creator only.
+   *         <li>READ: ADMIN, CHAIRPERSON, MEMBER, or DAR creator.
+   *       </ul>
+   *   <li><b>DAC</b> – {@link FileCategory#DATA_ACCESS_AGREEMENT}:
+   *       <ul>
+   *         <li>CRUD: CHAIRPERSON of the specific DAC (entity-scoped) or ADMIN.
+   *         <li>READ: any authenticated user.
+   *       </ul>
+   *   <li><b>DATASET</b> – {@link FileCategory#NIH_INSTITUTIONAL_CERTIFICATION}:
+   *       <ul>
+   *         <li>CRUD: ADMIN, DATASUBMITTER, or CHAIRPERSON.
+   *         <li>READ: ADMIN, DATASUBMITTER, CHAIRPERSON, or MEMBER.
+   *       </ul>
+   *   <li><b>STUDY</b> – {@link FileCategory#ALTERNATIVE_DATA_SHARING_PLAN}:
+   *       <ul>
+   *         <li>CRUD: ADMIN, DATASUBMITTER, or CHAIRPERSON.
+   *         <li>READ: ADMIN, DATASUBMITTER, or CHAIRPERSON (study must be DAC-linked; enforced by


Changing CRUD to CREATE, DELETE seems like it would be clearer, since we'll likely not allow "UPDATE" via API and "READ" is cover in the line below.

eweitz · 2026-04-17T22:05:13Z

+    ensureHasAnyRole(
+        user, UserRoles.ADMIN, UserRoles.DATASUBMITTER, UserRoles.CHAIRPERSON, UserRoles.MEMBER);


Above notes:

* <li><b>DATASET</b> – {@link FileCategory#NIH_INSTITUTIONAL_CERTIFICATION}: * <ul> * <li>CRUD: ADMIN, DATASUBMITTER, or CHAIRPERSON. * <li>READ: ADMIN, DATASUBMITTER, CHAIRPERSON, or MEMBER. * </ul> * <li><b>STUDY</b> – {@link FileCategory#ALTERNATIVE_DATA_SHARING_PLAN}: * <ul> * <li>CRUD: ADMIN, DATASUBMITTER, or CHAIRPERSON. * <li>READ: ADMIN, DATASUBMITTER, or CHAIRPERSON (study must be DAC-linked; enforced by * entity resolution via DatasetService). * </ul> * </ul>

I interpret that to mean alternative data sharing plans should not permit READ to MEMBER. Naively, the UserRoles.MEMBER argument on this line seems like would permit READ to MEMBER.

eweitz · 2026-04-17T22:08:25Z

+  private void checkDarAccess(User user, String entityId, OperationType op) {
+    boolean isCreator = isDarCreator(user, entityId);
+
+    if (op == OperationType.CRUD && !isCreator) {


Following my other suggestion to refine "CRUD" (i.e. CREATE, READ, UPDATE, DELETE) to "CREATE, DELETE", I think refining this operation type to something like CREATE_OR_DELETE would help avoid maintainer confusion.

eweitz · 2026-04-17T22:38:52Z

+      description: Not Found
+
+put:
+  summary: Update document category by entity and file id


IIUC we'll presumably remove this, pending Greg's confirmation.

eweitz · 2026-04-17T23:04:20Z

@@ -0,0 +1,15 @@
+type: object


IIUC we'll presumably remove this, pending Greg's approval.

eweitz · 2026-04-17T23:10:04Z

+  }
+
+  @Test
+  void testUpdateCategory() {


Flagging for removal pending Greg's confirmation.

eweitz · 2026-04-17T23:14:16Z

+    private static final String DAR_ID = "DAR-100";
+
+    @Test
+    void crudAllowedForCreator() {


Similar to my other suggestions to refine usage of "CRUD" -- I recommend changing crud to e.g. createAndDelete throughout this PR's tests.

eweitz · 2026-04-17T23:24:09Z

+          ForbiddenException.class,
+          () -> service.checkAccess(researcher, "study", "1", null, OperationType.READ));
+    }
+  }


Depending on the answer to my question in the comment beginning "Above notes", it might be worth testing that case here.

kevinmarete added 7 commits April 16, 2026 14:31

Prompt 1 - Upload

9940e4a

Prompt 2 - Get File

2bd73ed

Prompt 3 - Soft Delete

0d546d0

Prompt 4 - Update Category

9e921c3

Prompt 4 - Allow READ for DAC, DAR in GET endpoints

4ea3008

Prompt 5 - Refactor FileStorageObjectService

12735f8

Rename path files

077c4de

kevinmarete self-assigned this Apr 16, 2026

kevinmarete changed the title ~~Km dt 3113 file storage apis~~ [DT-3107, DT-3108, DT-3110, DT-3112, DT-3113] Add entity-scoped FSO endpoints and access-controlled FileStorageObjectService Apr 16, 2026

This was referenced Apr 16, 2026

[DT-3108] Add PUT endpoint to Update FileStorageObject category #2865

Closed

[DT-3112] Add DELETE endpoint to Soft delete FileStorageObject #2864

Closed

kevinmarete marked this pull request as ready for review April 17, 2026 18:35

kevinmarete requested a review from a team as a code owner April 17, 2026 18:35

kevinmarete requested review from Copilot, eweitz, otchet-broad and rushtong and removed request for a team, Copilot and eweitz April 17, 2026 18:35

Copilot started reviewing on behalf of kevinmarete April 17, 2026 18:35 View session

kevinmarete requested a review from eweitz April 17, 2026 18:35

eweitz reviewed Apr 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DT-3107, DT-3108, DT-3110, DT-3112, DT-3113] Add entity-scoped FSO endpoints and access-controlled FileStorageObjectService#2867

[DT-3107, DT-3108, DT-3110, DT-3112, DT-3113] Add entity-scoped FSO endpoints and access-controlled FileStorageObjectService#2867
kevinmarete wants to merge 7 commits intodevelopfrom
km-dt-3113-file-storage-apis

kevinmarete commented Apr 16, 2026 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Apr 16, 2026

Uh oh!

eweitz left a comment

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

eweitz Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		ensureHasAnyRole(
		user, UserRoles.ADMIN, UserRoles.DATASUBMITTER, UserRoles.CHAIRPERSON, UserRoles.MEMBER);

Conversation

kevinmarete commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Addresses

Summary

Uh oh!

sonarqubecloud bot commented Apr 16, 2026

Quality Gate passed

Uh oh!

eweitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kevinmarete commented Apr 16, 2026 •

edited

Loading