This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 8df38afe77 fix(computing-unit): fix 500s in share/revoke (unknown
email + privilege change) (#6446)
8df38afe77 is described below
commit 8df38afe777b1240c5c9263fb16862ccb199f441
Author: Prateek Ganigi <[email protected]>
AuthorDate: Thu Jul 16 13:24:23 2026 -0700
fix(computing-unit): fix 500s in share/revoke (unknown email + privilege
change) (#6446)
### What changes were proposed in this PR?
Fixes two HTTP 500s in ComputingUnitAccessResource's sharing endpoints,
both hit by ordinary user actions.
1. Sharing/revoking to an unknown email -> 500 (NPE). grantAccess and
revokeAccess read userDao.fetchOneByEmail(email).getUid before
null-checking:
val granteeId = userDao.fetchOneByEmail(email).getUid // NPE when email
is unknown
if (granteeId == null) { ... } // dead code, never reached
fetchOneByEmail returns null for an unknown email, so .getUid threw an
NPE before the guard, surfacing as an opaque 500, which is the intended
"User with the given email does not exist" error was unreachable. Both
methods now null-check the fetched user first, so an unknown email
returns a clear IllegalArgumentException (4xx). Mistyping an email when
sharing is common, so this replaces a server error with an actionable
message.
2. Changing an existing collaborator's access level -> 500 (duplicate
key). grantAccess used insert, so re-granting a user who already had
access - what the share dialog does when you change a privilege via the
dropdown, violated the (cuid, uid) primary key. Switched to merge
(upsert) so re-granting updates the privilege in place, matching
DatasetAccessResource/WorkflowAccessResource/ProjectAccessResource, from
which this resource had diverged.
### Any related issues, documentation, discussions?
Closes #6445
### How was this PR tested?
Added ComputingUnitAccessResourceSpec (embedded Postgres via
MockTexeraDB), 8 tests covering both methods: happy path, unknown email
(fix 1), re-grant privilege change (fix 2), and no-write-access.
Verified red->green: against the old code the unknown-email cases fail
with NullPointerException and the re-grant case with a duplicate-key
IntegrityConstraintViolationException; after the fixes all 8 pass.
The module had no DB-backed test wiring, so build.sbt adds DAO %
"test->test" (for MockTexeraDB + embedded Postgres) and forks the test
JVM with COMPUTING_UNIT_SHARING_ENABLED=true and the repo root as the
working directory (so MockTexeraDB can resolve sql/texera_ddl.sql).
Run:
sbt "ComputingUnitManagingService/testOnly
org.apache.texera.service.resource.ComputingUnitAccessResourceSpec"
### Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8 in compliance with ASF.
---
build.sbt | 13 +-
.../resource/ComputingUnitAccessResource.scala | 33 ++--
.../resource/ComputingUnitAccessResourceSpec.scala | 204 +++++++++++++++++++++
3 files changed, 237 insertions(+), 13 deletions(-)
diff --git a/build.sbt b/build.sbt
index ee6882600d..47aea23e8b 100644
--- a/build.sbt
+++ b/build.sbt
@@ -160,6 +160,8 @@ lazy val WorkflowCore = (project in
file("common/workflow-core"))
.dependsOn(DAO % "test->test") // test scope dependency
lazy val ComputingUnitManagingService = (project in
file("computing-unit-managing-service"))
.dependsOn(WorkflowCore, Auth, Config, Resource)
+ .configs(Test)
+ .dependsOn(DAO % "test->test") // reuse MockTexeraDB embedded Postgres in
tests
.settings(commonModuleSettings)
.settings(
dependencyOverrides ++= Seq(
@@ -171,7 +173,16 @@ lazy val ComputingUnitManagingService = (project in
file("computing-unit-managin
// with "Scala module 2.18.8 requires Jackson Databind version >= 2.18.0
// and < 2.19.0 - Found jackson-databind version 2.21.0").
"com.fasterxml.jackson.core" % "jackson-databind" % jacksonVersion
- ) ++ nettyDependencyOverrides
+ ) ++ nettyDependencyOverrides,
+ // Fork the test JVM so the sharing feature flag can be enabled:
ComputingUnitConfig
+ // reads computing-unit.conf's sharing.enabled as a load-time val (default
false,
+ // overridable only via the COMPUTING_UNIT_SHARING_ENABLED env var), and
the
+ // access-resource tests need it on to reach the share/revoke code paths.
Also run
+ // from the repo root so MockTexeraDB can resolve sql/texera_ddl.sql by
relative path.
+ Test / fork := true,
+ Test / envVars += "COMPUTING_UNIT_SHARING_ENABLED" -> "true",
+ Test / forkOptions := (Test / forkOptions).value
+ .withWorkingDirectory((ThisBuild / baseDirectory).value)
)
lazy val FileService = (project in file("file-service"))
.settings(commonModuleSettings)
diff --git
a/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala
b/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala
index 4045de42c0..9ed77ead1f 100644
---
a/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala
+++
b/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala
@@ -113,6 +113,19 @@ class ComputingUnitAccessResource {
}
final private val userDao = new UserDao(context.configuration())
+ /**
+ * Resolves an email to its user id, throwing a JAX-RS BadRequestException
(400) when no
+ * account matches — the service registers no ExceptionMapper for
IllegalArgumentException,
+ * so that would otherwise surface as an opaque HTTP 500. Shared by
grant/revoke.
+ */
+ private def resolveUidByEmail(email: String): Integer = {
+ val user = userDao.fetchOneByEmail(email)
+ if (user == null) {
+ throw new BadRequestException("User with the given email does not exist")
+ }
+ user.getUid
+ }
+
@GET
@Produces(Array(MediaType.APPLICATION_JSON))
@Path("/computing-unit/list/{cuid}")
@@ -148,14 +161,10 @@ class ComputingUnitAccessResource {
): Unit = {
ensureSharingIsEnabled()
if (!hasWriteAccess(cuid, user.getUid)) {
- throw new IllegalArgumentException("User does not have permission to
grant access")
+ throw new ForbiddenException("User does not have permission to grant
access")
}
- // TODO: add try except and check how to display error message in the
frontend
- val granteeId = userDao.fetchOneByEmail(email).getUid
- if (granteeId == null) {
- throw new IllegalArgumentException("User with the given email does not
exist")
- }
+ val granteeId = resolveUidByEmail(email)
withTransaction(context) { ctx =>
val computingUnitUserAccessDao = new
ComputingUnitUserAccessDao(ctx.configuration())
@@ -163,7 +172,10 @@ class ComputingUnitAccessResource {
access.setCuid(cuid)
access.setUid(granteeId)
access.setPrivilege(privilege)
- computingUnitUserAccessDao.insert(access)
+ // merge (upsert) rather than insert: re-granting an existing grantee
updates
+ // their privilege in place instead of hitting a duplicate-primary-key
error
+ // (the (cuid, uid) PK). Mirrors
DatasetAccessResource/WorkflowAccessResource.
+ computingUnitUserAccessDao.merge(access)
}
}
@@ -176,13 +188,10 @@ class ComputingUnitAccessResource {
): Unit = {
ensureSharingIsEnabled()
if (!hasWriteAccess(cuid, user.getUid)) {
- throw new IllegalArgumentException("User does not have permission to
revoke access")
+ throw new ForbiddenException("User does not have permission to revoke
access")
}
- val granteeId = userDao.fetchOneByEmail(email).getUid
- if (granteeId == null) {
- throw new IllegalArgumentException("User with the given email does not
exist")
- }
+ val granteeId = resolveUidByEmail(email)
withTransaction(context) { ctx =>
ctx
diff --git
a/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessResourceSpec.scala
b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessResourceSpec.scala
new file mode 100644
index 0000000000..56c098049a
--- /dev/null
+++
b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessResourceSpec.scala
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.texera.service.resource
+
+import jakarta.ws.rs.{BadRequestException, ForbiddenException}
+import org.apache.texera.auth.SessionUser
+import org.apache.texera.common.config.ComputingUnitConfig
+import org.apache.texera.dao.MockTexeraDB
+import org.apache.texera.dao.jooq.generated.Tables.COMPUTING_UNIT_USER_ACCESS
+import org.apache.texera.dao.jooq.generated.enums.{
+ PrivilegeEnum,
+ UserRoleEnum,
+ WorkflowComputingUnitTypeEnum
+}
+import org.apache.texera.dao.jooq.generated.tables.daos.{UserDao,
WorkflowComputingUnitDao}
+import org.apache.texera.dao.jooq.generated.tables.pojos.{User,
WorkflowComputingUnit}
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach}
+
+/**
+ * Spec for [[ComputingUnitAccessResource]]'s share/revoke endpoints, backed
by an
+ * embedded Postgres (via [[MockTexeraDB]]).
+ *
+ * The suite runs with COMPUTING_UNIT_SHARING_ENABLED=true (set in
build.sbt), which
+ * `ensureSharingIsEnabled()` requires; this is asserted below so a missing
env var
+ * fails loudly instead of silently short-circuiting every case with a
ForbiddenException.
+ *
+ * The key regression these tests guard: granting/revoking to an unknown
email must
+ * surface a clear IllegalArgumentException, not a NullPointerException
(500), because
+ * `userDao.fetchOneByEmail` returns null for an address with no account.
+ */
+class ComputingUnitAccessResourceSpec
+ extends AnyFlatSpec
+ with Matchers
+ with MockTexeraDB
+ with BeforeAndAfterAll
+ with BeforeAndAfterEach {
+
+ private val ownerUser: User = {
+ val user = new User
+ user.setName("cu_owner")
+ user.setPassword("123")
+ user.setEmail("[email protected]")
+ user.setRole(UserRoleEnum.REGULAR)
+ user
+ }
+
+ private val granteeUser: User = {
+ val user = new User
+ user.setName("cu_grantee")
+ user.setPassword("123")
+ user.setEmail("[email protected]")
+ user.setRole(UserRoleEnum.REGULAR)
+ user
+ }
+
+ private val strangerUser: User = {
+ val user = new User
+ user.setName("cu_stranger")
+ user.setPassword("123")
+ user.setEmail("[email protected]")
+ user.setRole(UserRoleEnum.REGULAR)
+ user
+ }
+
+ private val ownedUnit: WorkflowComputingUnit = {
+ val unit = new WorkflowComputingUnit
+ unit.setName("owned-unit")
+ unit.setType(WorkflowComputingUnitTypeEnum.local)
+ unit.setUri("")
+ unit
+ }
+
+ private val nonExistentEmail: String = "[email protected]"
+
+ lazy val accessResource = new ComputingUnitAccessResource()
+
+ lazy val ownerSession = new SessionUser(ownerUser)
+ lazy val strangerSession = new SessionUser(strangerUser)
+
+ private def cuid: Integer = ownedUnit.getCuid
+
+ private def accessEmails(cuid: Integer): List[String] =
+ accessResource.getComputingUnitAccessList(ownerSession, cuid).map(_.email)
+
+ override protected def beforeAll(): Unit = {
+ super.beforeAll()
+ initializeDBAndReplaceDSLContext()
+
+ val userDao = new UserDao(getDSLContext.configuration())
+ userDao.insert(ownerUser)
+ userDao.insert(granteeUser)
+ userDao.insert(strangerUser)
+
+ ownedUnit.setUid(ownerUser.getUid)
+ new
WorkflowComputingUnitDao(getDSLContext.configuration()).insert(ownedUnit)
+ }
+
+ override protected def beforeEach(): Unit = {
+ super.beforeEach()
+ // every test starts with no explicit grants on the unit
+ getDSLContext.deleteFrom(COMPUTING_UNIT_USER_ACCESS).execute()
+ }
+
+ override protected def afterAll(): Unit = {
+ try shutdownDB()
+ finally super.afterAll()
+ }
+
+ "the test environment" should "have computing-unit sharing enabled" in {
+ // guards against a silent misconfiguration that would make every case
below
+ // short-circuit in ensureSharingIsEnabled rather than test the intended
paths
+ ComputingUnitConfig.sharingComputingUnitEnabled shouldBe true
+ }
+
+ //
===========================================================================
+ // grantAccess
+ //
===========================================================================
+
+ "grantAccess" should "add a grantee that appears in the access list" in {
+ accessResource.grantAccess(ownerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+
+ val entries = accessResource.getComputingUnitAccessList(ownerSession, cuid)
+ entries should have size 1
+ entries.head.email shouldEqual granteeUser.getEmail
+ entries.head.privilege shouldEqual PrivilegeEnum.READ
+ }
+
+ it should "reject an unknown email with a 400 instead of crashing" in {
+ val ex = intercept[BadRequestException] {
+ accessResource.grantAccess(ownerSession, cuid, nonExistentEmail,
PrivilegeEnum.READ)
+ }
+ ex.getResponse.getStatus shouldEqual 400
+ ex.getMessage should include("User with the given email does not exist")
+ accessEmails(cuid) shouldBe empty
+ }
+
+ it should "update the privilege in place when re-granting with a different
privilege" in {
+ accessResource.grantAccess(ownerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+ accessResource.grantAccess(ownerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.WRITE)
+
+ val entries = accessResource.getComputingUnitAccessList(ownerSession, cuid)
+ entries should have size 1
+ entries.head.email shouldEqual granteeUser.getEmail
+ entries.head.privilege shouldEqual PrivilegeEnum.WRITE
+ }
+
+ it should "reject a caller without write access with a 403" in {
+ val ex = intercept[ForbiddenException] {
+ accessResource.grantAccess(strangerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+ }
+ ex.getResponse.getStatus shouldEqual 403
+ ex.getMessage should include("does not have permission to grant access")
+ }
+
+ //
===========================================================================
+ // revokeAccess
+ //
===========================================================================
+
+ "revokeAccess" should "remove the grantee from the access list" in {
+ accessResource.grantAccess(ownerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+ accessEmails(cuid) should contain(granteeUser.getEmail)
+
+ accessResource.revokeAccess(ownerSession, cuid, granteeUser.getEmail)
+
+ accessEmails(cuid) shouldBe empty
+ }
+
+ it should "reject an unknown email with a 400 instead of crashing" in {
+ val ex = intercept[BadRequestException] {
+ accessResource.revokeAccess(ownerSession, cuid, nonExistentEmail)
+ }
+ ex.getResponse.getStatus shouldEqual 400
+ ex.getMessage should include("User with the given email does not exist")
+ }
+
+ it should "reject a caller without write access with a 403" in {
+ accessResource.grantAccess(ownerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+
+ val ex = intercept[ForbiddenException] {
+ accessResource.revokeAccess(strangerSession, cuid, granteeUser.getEmail)
+ }
+ ex.getResponse.getStatus shouldEqual 403
+ ex.getMessage should include("does not have permission to revoke access")
+ }
+}
