Re: [PR] fix(amber): hold result export to the same rules the export dialog applies [texera]

["Ma77Ball (via GitHub)" <gi...@apache.org>]

Ma77Ball commented on code in PR #7994:
URL: https://github.com/apache/texera/pull/7994#discussion_r4034516758


##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -857,22 +917,22 @@ class WorkflowExecutionsResource {
   @Path("/result/export/dataset")
   @RolesAllowed(Array("REGULAR", "ADMIN"))
   def exportResultToDataset(request: ResultExportRequest, @Auth user: SessionUser): Response = {
-    if (!WorkflowAccessResource.hasReadAccess(request.workflowId, user.getUser.getUid)) {
-      workflowAccessDeniedResponse
-    } else {
-      try {
-        val resultExportService =
-          new ResultExportService(WorkflowIdentity(request.workflowId), request.computingUnitId)
-        resultExportService.exportToDataset(user.user, request)
-
-      } catch {
-        case ex: Exception =>
-          Response
-            .status(Response.Status.INTERNAL_SERVER_ERROR)
-            .`type`(MediaType.APPLICATION_JSON)
-            .entity(Map("error" -> ex.getMessage).asJava)
-            .build()
-      }
+    validateUserCanExportResult(user.getUser, request) match {

Review Comment:
   `validateUserCanExportResult` runs outside the try/catch here, whereas in `exportResultToLocal` (line 960) it runs inside one. Its DB calls (`getComputingUnitAccess`, `getNonDownloadableOperatorMap`) can throw, and on this endpoint that exception escapes uncaught instead of becoming the `{"error": ...}` 500 JSON body this endpoint returns for every other failure.
   
   Why it matters: a transient DB error during authorization surfaces as a raw container 500 on `/export/dataset` but as a clean JSON 500 on `/export/local` -- inconsistent, and harder for the client to parse. This is not a regression (the old read-access check sat outside the try too), just a good moment to align the two endpoints; wrap the `match` in the existing try, or lift the shared try/catch around both.
   
   Nit, optional.



##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala:
##########
@@ -1668,4 +1727,111 @@ class WorkflowExecutionsResourceSpec
     assert(response.getStatus == Response.Status.UNAUTHORIZED.getStatusCode)
   }
 
+  // ─── export authorization beyond workflow read access ─────────────────────
+  // Read access to the workflow is not the whole gate: the execution is looked up by
+  // (wid, cuid), and an operator's results can carry data out of a dataset its owner
+  // marked non-downloadable. Both endpoints answer those two with a 403 — not the 401
+  // above, which the frontend's interceptor reads as an expired session.
+
+  private val computingUnitDenied = "No sufficient access privilege to the computing unit."
+
+  "export authorization" should "deny a download for a computing unit the caller cannot reach" in {
+    grantReadAccess()
+    val foreignUnit = insertComputingUnit(insertUser(testUserId + 2, "cu-owner@example.com").getUid)
+    insertExecution(cuid = foreignUnit.getCuid)
+
+    val response = resource.exportResultToLocal(
+      Json.stringify(
+        Json.toJson(exportRequest(List(OperatorExportInfo("op-1", "csv")), foreignUnit.getCuid))
+      ),
+      tokenFor(UserRoleEnum.REGULAR)
+    )
+    assert(response.getStatus == Response.Status.FORBIDDEN.getStatusCode)
+    assert(errorOf(response) == computingUnitDenied)
+  }
+
+  it should "deny an export to dataset for a computing unit the caller cannot reach" in {
+    grantReadAccess()
+    val foreignUnit = insertComputingUnit(insertUser(testUserId + 2, "cu-owner@example.com").getUid)
+    insertExecution(cuid = foreignUnit.getCuid)
+
+    val response = resource.exportResultToDataset(
+      exportRequest(List(OperatorExportInfo("op-1", "csv")), foreignUnit.getCuid),
+      session(testUser)
+    )
+    assert(response.getStatus == Response.Status.FORBIDDEN.getStatusCode)
+    assert(errorOf(response) == computingUnitDenied)
+  }
+
+  // Shared access, not ownership, is the predicate: a unit someone else owns but granted
+  // the caller READ on has to still export, or the check would break every shared unit.
+  it should "allow a download for a computing unit shared with the caller" in {
+    grantReadAccess()
+    val foreignUnit = insertComputingUnit(insertUser(testUserId + 2, "cu-owner@example.com").getUid)
+    grantComputingUnitAccess(foreignUnit.getCuid)
+    insertExecution(cuid = foreignUnit.getCuid)
+
+    val response = resource.exportResultToLocal(
+      Json.stringify(
+        Json.toJson(
+          exportRequest(
+            List(OperatorExportInfo("op-1", "csv"), OperatorExportInfo("op-2", "csv")),
+            foreignUnit.getCuid
+          )
+        )
+      ),
+      tokenFor(UserRoleEnum.REGULAR)
+    )
+    assert(response.getStatus == 200)
+  }
+
+  it should "deny a download of an operator fed by a non-downloadable dataset" in {
+    grantReadAccess()
+    val unit = insertComputingUnit()
+    insertExecution(cuid = unit.getCuid)
+    seedNonDownloadableWorkflow(testUserId + 3, "locked-owner@example.com")
+
+    // The downstream operator, not the scan itself: the restriction propagates along the
+    // links, and the data reaching downstreamB came out of the locked dataset.
+    val response = resource.exportResultToLocal(
+      Json.stringify(
+        Json.toJson(exportRequest(List(OperatorExportInfo("downstreamB", "csv")), unit.getCuid))
+      ),
+      tokenFor(UserRoleEnum.REGULAR)
+    )
+    assert(response.getStatus == Response.Status.FORBIDDEN.getStatusCode)
+    assert(errorOf(response).contains("LockedDS (locked-owner@example.com)"))
+  }
+
+  it should "deny an export to dataset of an operator fed by a non-downloadable dataset" in {
+    grantReadAccess()
+    val unit = insertComputingUnit()
+    insertExecution(cuid = unit.getCuid)
+    seedNonDownloadableWorkflow(testUserId + 3, "locked-owner@example.com")
+
+    val response = resource.exportResultToDataset(
+      exportRequest(List(OperatorExportInfo("scanA", "csv")), unit.getCuid),
+      session(testUser)
+    )
+    assert(response.getStatus == Response.Status.FORBIDDEN.getStatusCode)
+    assert(errorOf(response).contains("LockedDS (locked-owner@example.com)"))
+  }
+
+  // Only the operators carrying the restricted data are blocked; one restriction in the
+  // workflow must not close the whole workflow down.
+  it should "let an unrestricted operator of a restricted workflow through" in {

Review Comment:
   The new suite covers the deny paths and the shared-unit allow path, but not the dataset-owner allow path that `getNonDownloadableOperatorMap` implements: its `isOwner` check skips datasets the caller owns, so an operator fed by the caller's *own* non-downloadable dataset must still export. This PR is what newly makes the export endpoints enforce that map, so the allow side deserves a pin too.
   
   Why it matters: without it, a future change that dropped the `isOwner` exclusion, or otherwise over-blocked, would pass CI while silently breaking a workflow owner exporting results derived from their own non-downloadable dataset -- a real, common flow.
   
   Suggested fix: add a case that seeds `LockedDS` owned by `testUser` (reuse `seedNonDownloadableWorkflow` with `testUser`'s uid/email) and asserts `exportResultToLocal` on `scanA` is not `FORBIDDEN`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org