Yicong-Huang commented on code in PR #7994:
URL: https://github.com/apache/texera/pull/7994#discussion_r4058798860
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -794,6 +795,65 @@ class WorkflowExecutionsResource {
.entity(Map("error" -> "No sufficient access privilege.").asJava)
.build()
+ /** 403 rather than the 401 above, for a caller who *can* read the workflow
but may not take
+ * this particular data out of it. The distinction is also operational: the
frontend's
+ * UnauthorizedHttpInterceptor reads any 401 on an authenticated request as
an expired
+ * session and logs the user out, which is the wrong outcome for a live
session that simply
+ * asked for something it is not entitled to.
+ */
+ private def exportDeniedResponse(message: String): Response =
+ Response
+ .status(Response.Status.FORBIDDEN)
+ .`type`(MediaType.APPLICATION_JSON)
+ .entity(Map("error" -> message).asJava)
+ .build()
+
+ /** Authorizes one result-export request, for both the local and the dataset
endpoint.
+ *
+ * Three things have to hold before results leave the system:
+ *
+ * 1. the caller can read the workflow the results belong to;
+ * 2. the caller can reach the computing unit that produced them —
`getLatestExecutionID`
+ * selects on `(wid, cuid)` alone, so the unit is as much a
caller-chosen key as the
+ * workflow id is;
+ * 3. no requested operator carries data from a dataset whose owner
marked it
+ * non-downloadable. `getWorkflowResultDownloadability` hands the same
map to the UI,
+ * which greys those operators out — but that is advice to a
cooperating client, and
+ * both export endpoints are reachable directly.
+ *
+ * @return the response to send back, or None when the export may proceed
+ */
+ private def validateUserCanExportResult(
+ user: UserPojo,
+ request: ResultExportRequest
+ ): Option[Response] = {
+ if (!WorkflowAccessResource.hasReadAccess(request.workflowId,
user.getUid)) {
+ Some(workflowAccessDeniedResponse)
+ } else if (
+ ComputingUnitAccess
+ .getComputingUnitAccess(request.computingUnitId, user.getUid)
+ .eq(PrivilegeEnum.NONE)
+ ) {
+ Some(exportDeniedResponse("No sufficient access privilege to the
computing unit."))
+ } else {
+ val restrictions = getNonDownloadableOperatorMap(request.workflowId,
user)
Review Comment:
**Must fix:**
This map is computed from `WORKFLOW.CONTENT`, the workflow's current JSON
(`:132-136`). The rows released come from whatever `getLatestExecutionID`
resolves, and that query joins on `WORKFLOW_VERSION.WID.eq(wid)` (`:103`), so
it spans every version. The caller controls one input and not the other. Anyone
with WRITE (the owner always, and the owner is who this control targets) can
clear the locked scan's `fileName`, save, and re-post the same request. The old
execution's results still resolve, and the recomputed map no longer names the
operator. I would compute the map from the executed version, which
`WorkflowVersionResource.applyPatch` already reconstructs, and make the three
`Map.empty` returns (`:139`, `:148`, `:160`) deny. If that is too large for
this PR, say so in the Scaladoc.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -794,6 +795,65 @@ class WorkflowExecutionsResource {
.entity(Map("error" -> "No sufficient access privilege.").asJava)
.build()
+ /** 403 rather than the 401 above, for a caller who *can* read the workflow
but may not take
Review Comment:
**Advisory:**
`UnauthorizedHttpInterceptor` fires only on an `HttpErrorResponse` for a
request carrying an `Authorization` header
(`unauthorized-http-interceptor.service.ts:80-88`). That covers
`/export/dataset`, not `/export/local`: `DownloadService` posts the latter as a
hidden form into a hidden iframe (`download.service.ts:208-233`), so the
interceptor never sees the response. On that endpoint the 403 body lands in the
iframe, so a denied caller gets no feedback and the download simply never
starts. Fixing that is outside this diff, but narrowing the comment to name the
dataset endpoint costs one clause.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala:
##########
@@ -211,12 +211,14 @@ class WorkflowExecutionsResourceSpec
getDSLContext
.deleteFrom(WORKFLOW_COMPUTING_UNIT)
- .where(WORKFLOW_COMPUTING_UNIT.UID.eq(testUserId))
+ .where(WORKFLOW_COMPUTING_UNIT.UID.between(testUserId, testUserId + 10))
Review Comment:
**Advisory:**
A question, not a retitle demand. `CONTRIBUTING.md`'s type table routes "a
functionality or a form of support never existed and you are adding it" to
`feat`, even when the docs already described it as present. Server-side
enforcement of these two rules never existed, as the description says. Reading
a security gap as a `fix` is defensible, and the table does not settle it. But
the prefix is what the auto-label bot keys on: it has already labelled this PR
`release/v1.2` and queued the backport. Two new 403 refusals headed for a patch
release deserve a committer's explicit call.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala:
##########
@@ -211,12 +211,14 @@ class WorkflowExecutionsResourceSpec
getDSLContext
.deleteFrom(WORKFLOW_COMPUTING_UNIT)
- .where(WORKFLOW_COMPUTING_UNIT.UID.eq(testUserId))
+ .where(WORKFLOW_COMPUTING_UNIT.UID.between(testUserId, testUserId + 10))
Review Comment:
**Must fix:**
The body says "No issue", with no `Closes #N`, no `#N` mention and no
discussion link, so nothing is linked in the Development sidebar.
`CONTRIBUTING.md` step 2 asks for an issue and consensus before development.
Its minor typo and docs exception does not stretch to two new authorization
refusals on public endpoints, queued for a release backport. Open an issue and
promote the reference to `Closes #N`. Every other template section is filled,
including the ASF generative-AI disclosure.
--
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]
