This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6446-874499fb167ce518657b6a734a62253ce17b5bc8 in repository https://gitbox.apache.org/repos/asf/texera.git
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") + } +}
