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,
"[email protected]").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,
"[email protected]").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,
"[email protected]").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, "[email protected]")
+
+ // 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 ([email protected])"))
+ }
+
+ 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, "[email protected]")
+
+ 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 ([email protected])"))
+ }
+
+ // 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
