aglinxinyuan commented on code in PR #7592:
URL: https://github.com/apache/texera/pull/7592#discussion_r3771557105
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala:
##########
@@ -1041,4 +1060,232 @@ class WorkflowResourceSpec
assert(names.contains("dup-src") && names.contains("dup-src_copy"))
}
+ // ─── shared-access and failure paths (issue #7224)
──────────────────────────
+
+ // Content with an operators array: duplicate/clone reassign operator ids
and fail
+ // on content that has none.
+ private val contentWithOperator =
+
"{\"operators\":[{\"operatorID\":\"op1\",\"operatorType\":\"CSVFileScan\"}]}"
+
+ private def grantAccess(wid: Integer, user: User, privilege: PrivilegeEnum):
Unit =
+ new WorkflowUserAccessDao(getDSLContext.configuration())
+ .merge(new WorkflowUserAccess(user.getUid, wid, privilege))
+
+ // A request whose remote address is a valid IPv4 so recordCloneAction
stores it.
+ private def cloneRequest: HttpServletRequest = {
+ val r = stub[HttpServletRequest]
+ (r.getRemoteAddr _).when().returns("127.0.0.1")
+ r
+ }
+
+ private def workflowNamesOf(user: SessionUser): List[String] =
+
workflowResource.retrieveWorkflowsBySessionUser(user).map(_.workflow.getName)
+
+ private def versionCount(wid: Integer): Int =
+ getDSLContext.fetchCount(WORKFLOW_VERSION, WORKFLOW_VERSION.WID.eq(wid))
+
+ "WorkflowResource.getWorkflowName (companion)" should "return the stored
name" in {
+ val wid = seedWorkflow(sessionUser1, "companion-name-wf").workflow.getWid
+ assert(WorkflowResource.getWorkflowName(wid) == "companion-name-wf")
+ }
+
+ it should "throw NotFoundException for a wid that does not exist" in {
+ val wid = seedWorkflow(sessionUser1,
"companion-missing-wf").workflow.getWid
+ assertThrows[NotFoundException](WorkflowResource.getWorkflowName(wid +
100000))
+ }
+
+ "WorkflowResource.persistWorkflow" should "reject the guest user" in {
+ val workflow = seedWorkflow(sessionUser1, "guest-wf", "d",
"{\"a\":1}").workflow
+ workflow.setContent("{\"a\":2}")
+
+ // the message distinguishes the guest rejection from the access-privilege
one
+ val thrown = intercept[ForbiddenException](
+ workflowResource.persistWorkflow(workflow, new
SessionUser(GuestAuthFilter.GUEST))
+ )
+ assert(thrown.getMessage.contains("Guest user"))
+ assert(workflowResource.retrieveWorkflow(workflow.getWid,
sessionUser1).content == "{\"a\":1}")
+ }
+
+ it should "update the workflow in place for its owner and record a version"
in {
+ val workflow = seedWorkflow(sessionUser1, "persist-owner", "d",
"{\"a\":1}").workflow
+ val versionsBefore = versionCount(workflow.getWid)
+ workflow.setContent("{\"a\":2}")
+
+ val persisted = workflowResource.persistWorkflow(workflow, sessionUser1)
+
+ assert(persisted.getContent == "{\"a\":2}")
+ assert(workflowResource.retrieveWorkflow(workflow.getWid,
sessionUser1).content == "{\"a\":2}")
+ assert(versionCount(workflow.getWid) == versionsBefore + 1)
+ // updating must not create a second workflow
+ assert(workflowNamesOf(sessionUser1) == List("persist-owner"))
+ }
+
+ it should "let a non-owner with write access update the workflow and record
a version" in {
+ val workflow = seedWorkflow(sessionUser1, "persist-writer", "d",
"{\"a\":1}").workflow
+ grantAccess(workflow.getWid, testUser2, PrivilegeEnum.WRITE)
+ val versionsBefore = versionCount(workflow.getWid)
+ workflow.setContent("{\"a\":3}")
+
+ workflowResource.persistWorkflow(workflow, sessionUser2)
+
+ assert(workflowResource.retrieveWorkflow(workflow.getWid,
sessionUser1).content == "{\"a\":3}")
+ assert(versionCount(workflow.getWid) == versionsBefore + 1)
+ // Guards rather than pins: both already hold before persistWorkflow is
called, since
+ // seedWorkflow and grantAccess establish them and the write-access branch
touches only
+ // WORKFLOW and WORKFLOW_VERSION. They document the intent -- the writer
updates the owner's
+ // workflow rather than getting a copy of its own -- while the two
assertions above carry the
+ // actual pin.
+ assert(workflowResource.getOwnerName(workflow.getWid) == testUser.getName)
+ assert(workflowNamesOf(sessionUser2) == List("persist-writer"))
+ }
+
+ it should "reject a non-owner with only read access" in {
+ val workflow = seedWorkflow(sessionUser1, "persist-reader", "d",
"{\"a\":1}").workflow
+ grantAccess(workflow.getWid, testUser2, PrivilegeEnum.READ)
+ workflow.setContent("{\"a\":9}")
+
+
assertThrows[ForbiddenException](workflowResource.persistWorkflow(workflow,
sessionUser2))
+ assert(workflowResource.retrieveWorkflow(workflow.getWid,
sessionUser1).content == "{\"a\":1}")
+ }
+
+ it should "reject a user with no access to an existing workflow" in {
+ val workflow = seedWorkflow(sessionUser1, "persist-no-access", "d",
"{\"a\":1}").workflow
+ workflow.setContent("{\"a\":9}")
+
+
assertThrows[ForbiddenException](workflowResource.persistWorkflow(workflow,
sessionUser2))
+ assert(workflowResource.retrieveWorkflow(workflow.getWid,
sessionUser1).content == "{\"a\":1}")
+ // the rejected persist must not silently create a copy owned by the caller
+ assert(workflowNamesOf(sessionUser2).isEmpty)
+ }
+
+ "WorkflowResource.createWorkflow" should "reject a workflow that already
carries an id" in {
+ val existing = seedWorkflow(sessionUser1, "already-has-id").workflow
+
+
assertThrows[BadRequestException](workflowResource.createWorkflow(existing,
sessionUser1))
+ assert(workflowNamesOf(sessionUser1) == List("already-has-id"))
+ }
+
+ "WorkflowResource.updateWorkflowName" should "accept a non-owner with write
access" in {
+ val wid = seedWorkflow(sessionUser1, "writer-rename").workflow.getWid
+ grantAccess(wid, testUser2, PrivilegeEnum.WRITE)
+ val update = new Workflow()
+ update.setWid(wid)
+ update.setName("renamed-by-writer")
+
+ workflowResource.updateWorkflowName(update, sessionUser2)
+
+ assert(workflowResource.getWorkflowName(wid) == "renamed-by-writer")
+ }
+
+ it should "reject a user with neither ownership nor write access" in {
+ val wid = seedWorkflow(sessionUser1, "no-access-rename").workflow.getWid
+ val update = new Workflow()
+ update.setWid(wid)
+ update.setName("should-not-apply")
+
+
assertThrows[ForbiddenException](workflowResource.updateWorkflowName(update,
sessionUser2))
+ assert(workflowResource.getWorkflowName(wid) == "no-access-rename")
+ }
+
+ "WorkflowResource.makePrivate" should "reject a user without write access"
in {
+ val wid = seedWorkflow(sessionUser1, "private-forbidden").workflow.getWid
+ workflowResource.makePublic(wid, sessionUser1)
+
+ assertThrows[ForbiddenException](workflowResource.makePrivate(wid,
sessionUser2))
+ assert(workflowResource.getWorkflowType(wid) == "Public")
+ }
+
+ "WorkflowResource.cloneWorkflow" should "copy the workflow to the caller and
record the clone" in {
+ val wid = seedWorkflow(sessionUser1, "clone-src", "d",
contentWithOperator).workflow.getWid
+
+ val newWid = workflowResource.cloneWorkflow(wid, sessionUser2,
cloneRequest)
Review Comment:
Fixed in c3c0bcafa7 — the guard really is missing, and the test really did
pin its absence.
`cloneWorkflow` fetches by wid and copies the content with no check. Its
siblings all guard: `retrieveWorkflow` and `duplicateWorkflow` call
`hasReadAccess` directly, and `cloneVersion` inherits it through
`retrieveWorkflowVersion`. This one endpoint was the odd one out, which reads
as an oversight rather than a decision.
Split across two PRs, since the production half is a behaviour change and
does not belong in a test-only PR:
- **Here.** The source is published before the clone, which is the flow the
endpoint actually serves — the hub's clone button, on someone else's published
workflow. The success case no longer asserts that a caller with no access can
read a private workflow. `cloneWorkflow` branches on nothing, so the covered
lines are unchanged, and `!clone.isPublished` gets stronger: with a public
source it now pins that publicness is not inherited by the clone.
- **#7605**, stacked on this one, adds the guard along with the rejection
case you asked for, plus a READ-grant case showing it does not over-block. Red
before the guard and green after, so the test pins the guard rather than a
fixture.
Also applied the suppressed comment — the section header now points at #7591
rather than the earlier #7224.
--
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]
