Copilot commented on code in PR #7592:
URL: https://github.com/apache/texera/pull/7592#discussion_r3768324336
##########
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:
This fixture creates a private workflow owned by `sessionUser1`, gives
`sessionUser2` no access, and then asserts that `sessionUser2` can clone its
full content. That codifies an authorization bypass: `cloneWorkflow` reads the
workflow directly without the `hasReadAccess` guard used by
retrieval/duplication. Make the success case public (or grant read access), add
a rejection case for an inaccessible private workflow, and add the
corresponding production guard before copying.
--
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]
