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-6714-6a1fedc1f8fb153b12b41ba42f9e98572ffe6207 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 4fd19be8634113565c423434efb9ffd086dcdd10 Author: Prateek Ganigi <[email protected]> AuthorDate: Tue Jul 21 17:35:21 2026 -0700 test(computing-unit): cover ComputingUnitAccessResource endpoints, privilege helpers, and the sharing-disabled path (#6714) ### What changes were proposed in this PR? This PR expands test coverage for ComputingUnitAccessResource. #6446 introduced ComputingUnitAccessResourceSpec but scoped it to grantAccess/revokeAccess; the rest of the resource was only exercised incidentally. This adds direct coverage for the remaining surface, all backed by embedded Postgres via MockTexeraDB: getComputingUnitAccessList: empty list, and multiple grantees with correct email / name / privilege mapping. Privilege helpers (companion object): isOwner (owner / non-owner / nonexistent cuid), getPrivilege (no grant -> null, granted -> value), and hasReadAccess / hasWriteAccess across owner, READ-grantee, WRITE-grantee, and no-grant users. getOwner: happy path (returns the owner's email). The nonexistent-unit -> 404 case is already covered by #6475. Sharing-disabled branch: a new ComputingUnitAccessSharingDisabledSpec asserting every endpoint returns 403 (ForbiddenException) when sharing is off. No production code changes: test-only, plus one build.sbt test-config change described below. ### Any related issues, documentation, discussions? Closes #6490. Builds on the test infrastructure added in #6446 and the getOwner 404 fix in #6475. ### How was this PR tested? sbt "ComputingUnitManagingService/test": 40 tests across all 6 module suites pass; no existing suite regressed. The sharing-disabled path needs special handling: ComputingUnitConfig.sharingComputingUnitEnabled is resolved once as a load-time val from the COMPUTING_UNIT_SHARING_ENABLED env var, and the module's test JVM sets it to true (needed by the sharing-enabled suites). So the disabled branch can only be exercised in a JVM where that var is absent. To do that without disturbing the other suites, build.sbt adds a Test / testGrouping that keeps all sharing-enabled suites in the existing single forked JVM and isolates any *SharingDisabledSpec into its own forked JVM with the env var removed. Each spec asserts the resolved flag (sharingComputingUnitEnabled shouldBe true / false) as a first case, so a misconfigured grouping fails loudly instead of silently passing. ### Was this PR authored or co-authored using generative AI tooling? Co-authored with Claude Opus 4.8 in compliance with ASF. --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> --- build.sbt | 17 +++- .../resource/ComputingUnitAccessResourceSpec.scala | 102 +++++++++++++++++++-- .../ComputingUnitAccessSharingDisabledSpec.scala | 99 ++++++++++++++++++++ 3 files changed, 208 insertions(+), 10 deletions(-) diff --git a/build.sbt b/build.sbt index 479bfe4631..b1ab4b6db3 100644 --- a/build.sbt +++ b/build.sbt @@ -183,7 +183,22 @@ lazy val ComputingUnitManagingService = (project in file("computing-unit-managin Test / fork := true, Test / envVars += "COMPUTING_UNIT_SHARING_ENABLED" -> "true", Test / forkOptions := (Test / forkOptions).value - .withWorkingDirectory((ThisBuild / baseDirectory).value) + .withWorkingDirectory((ThisBuild / baseDirectory).value), + // Isolate the sharing-disabled suite into its own forked JVM without + // COMPUTING_UNIT_SHARING_ENABLED: the config flag is a load-time val, so the disabled + // branch can only be exercised where the env var is absent (not merely unset per-test). + // All other suites keep running together in one forked JVM (the pre-grouping default). + Test / testGrouping := { + val opts = (Test / forkOptions).value + val (disabled, enabled) = + (Test / definedTests).value.partition(_.name.endsWith("SharingDisabledSpec")) + val enabledGroup = Tests.Group("sharing-enabled", enabled, Tests.SubProcess(opts)) + val disabledGroups = disabled.map { suite => + val disabledOpts = opts.withEnvVars(opts.envVars - "COMPUTING_UNIT_SHARING_ENABLED") + Tests.Group(suite.name, Seq(suite), Tests.SubProcess(disabledOpts)) + } + enabledGroup +: disabledGroups + } ) lazy val FileService = (project in file("file-service")) .settings(commonModuleSettings) 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 index 7637c99d26..4b5c78654e 100644 --- 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 @@ -29,23 +29,31 @@ import org.apache.texera.dao.jooq.generated.enums.{ 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.apache.texera.dao.jooq.generated.tables.daos.{ + ComputingUnitUserAccessDao, + UserDao, + WorkflowComputingUnitDao +} +import org.apache.texera.dao.jooq.generated.tables.pojos.{ + ComputingUnitUserAccess, + 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]]). + * Spec for [[ComputingUnitAccessResource]] with sharing ENABLED, backed by an embedded + * Postgres (via [[MockTexeraDB]]). Covers the endpoints — grantAccess, revokeAccess, + * getComputingUnitAccessList, getOwner — and the companion-object privilege helpers + * (isOwner / getPrivilege / hasReadAccess / hasWriteAccess). * * 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 + * `ensureSharingIsEnabled()` requires; the first case asserts this 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. + * The sharing-DISABLED branch is covered separately by ComputingUnitAccessSharingDisabledSpec, + * which forks without that env var (the flag is a load-time val). */ class ComputingUnitAccessResourceSpec extends AnyFlatSpec @@ -102,6 +110,15 @@ class ComputingUnitAccessResourceSpec private def accessEmails(cuid: Integer): List[String] = accessResource.getComputingUnitAccessList(ownerSession, cuid).map(_.email) + /** Inserts an access row directly, bypassing the grant endpoint, to set up helper tests. */ + private def grantDirectly(uid: Integer, privilege: PrivilegeEnum): Unit = { + val access = new ComputingUnitUserAccess + access.setCuid(cuid) + access.setUid(uid) + access.setPrivilege(privilege) + new ComputingUnitUserAccessDao(getDSLContext.configuration()).insert(access) + } + override protected def beforeAll(): Unit = { super.beforeAll() initializeDBAndReplaceDSLContext() @@ -203,6 +220,69 @@ class ComputingUnitAccessResourceSpec ex.getMessage should include("does not have permission to revoke access") } + // =========================================================================== + // getComputingUnitAccessList + // =========================================================================== + + "getComputingUnitAccessList" should "return an empty list when nothing is granted" in { + accessResource.getComputingUnitAccessList(ownerSession, cuid) shouldBe empty + } + + it should "list every grantee with their email, name, and privilege" in { + grantDirectly(granteeUser.getUid, PrivilegeEnum.READ) + grantDirectly(strangerUser.getUid, PrivilegeEnum.WRITE) + + val entries = accessResource.getComputingUnitAccessList(ownerSession, cuid) + entries should have size 2 + + val byEmail = entries.map(entry => entry.email -> entry).toMap + byEmail(granteeUser.getEmail).name shouldEqual granteeUser.getName + byEmail(granteeUser.getEmail).privilege shouldEqual PrivilegeEnum.READ + byEmail(strangerUser.getEmail).privilege shouldEqual PrivilegeEnum.WRITE + } + + // =========================================================================== + // Privilege helpers (companion object) + // =========================================================================== + + "isOwner" should "be true only for the owner of an existing unit" in { + ComputingUnitAccessResource.isOwner(cuid, ownerUser.getUid) shouldBe true + ComputingUnitAccessResource.isOwner(cuid, strangerUser.getUid) shouldBe false + ComputingUnitAccessResource.isOwner(nonExistentCuid, ownerUser.getUid) shouldBe false + } + + "getPrivilege" should "return null without a grant and the granted privilege otherwise" in { + ComputingUnitAccessResource.getPrivilege(cuid, strangerUser.getUid) shouldBe null + + grantDirectly(granteeUser.getUid, PrivilegeEnum.READ) + ComputingUnitAccessResource.getPrivilege( + cuid, + granteeUser.getUid + ) shouldEqual PrivilegeEnum.READ + } + + "the owner" should "have both read and write access" in { + ComputingUnitAccessResource.hasReadAccess(cuid, ownerUser.getUid) shouldBe true + ComputingUnitAccessResource.hasWriteAccess(cuid, ownerUser.getUid) shouldBe true + } + + "a READ grantee" should "have read but not write access" in { + grantDirectly(granteeUser.getUid, PrivilegeEnum.READ) + ComputingUnitAccessResource.hasReadAccess(cuid, granteeUser.getUid) shouldBe true + ComputingUnitAccessResource.hasWriteAccess(cuid, granteeUser.getUid) shouldBe false + } + + "a WRITE grantee" should "have both read and write access" in { + grantDirectly(granteeUser.getUid, PrivilegeEnum.WRITE) + ComputingUnitAccessResource.hasReadAccess(cuid, granteeUser.getUid) shouldBe true + ComputingUnitAccessResource.hasWriteAccess(cuid, granteeUser.getUid) shouldBe true + } + + "a user with no grant" should "have neither read nor write access" in { + ComputingUnitAccessResource.hasReadAccess(cuid, strangerUser.getUid) shouldBe false + ComputingUnitAccessResource.hasWriteAccess(cuid, strangerUser.getUid) shouldBe false + } + // =========================================================================== // getOwner // =========================================================================== @@ -214,4 +294,8 @@ class ComputingUnitAccessResourceSpec ex.getResponse.getStatus shouldEqual 404 ex.getMessage should include(s"Computing unit with cuid=$nonExistentCuid does not exist") } + + it should "return the owner's email for an existing unit" in { + accessResource.getOwner(ownerSession, cuid) shouldEqual ownerUser.getEmail + } } diff --git a/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessSharingDisabledSpec.scala b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessSharingDisabledSpec.scala new file mode 100644 index 0000000000..675ad5ccdd --- /dev/null +++ b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessSharingDisabledSpec.scala @@ -0,0 +1,99 @@ +/* + * 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.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.enums.{PrivilegeEnum, UserRoleEnum} +import org.apache.texera.dao.jooq.generated.tables.pojos.User +import org.scalatest.BeforeAndAfterAll +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Spec for [[ComputingUnitAccessResource]] when the sharing feature is DISABLED. + * + * `ComputingUnitConfig.sharingComputingUnitEnabled` is a load-time val resolved from the + * COMPUTING_UNIT_SHARING_ENABLED env var, so the disabled branch must be exercised in a + * forked JVM where that var resolves to false (build.sbt's `Test / testGrouping` isolates any + * suite whose name ends with "SharingDisabledSpec" and forces the flag off there). + * The first assertion guards that grouping — if it ever stops applying, this suite fails loudly + * rather than silently passing against a sharing-enabled JVM. + */ +class ComputingUnitAccessSharingDisabledSpec + extends AnyFlatSpec + with Matchers + with MockTexeraDB + with BeforeAndAfterAll { + + private val user: User = { + val u = new User + u.setName("cu_user") + u.setPassword("123") + u.setEmail("[email protected]") + u.setRole(UserRoleEnum.REGULAR) + u + } + + // Access resource construction needs an initialized SqlServer; the DB is otherwise unused + // here because ensureSharingIsEnabled() throws before any query runs. + lazy val accessResource = new ComputingUnitAccessResource() + lazy val session = new SessionUser(user) + + private val cuid: Integer = 1 + + override protected def beforeAll(): Unit = { + super.beforeAll() + initializeDBAndReplaceDSLContext() + } + + override protected def afterAll(): Unit = { + try shutdownDB() + finally super.afterAll() + } + + private def expectForbidden(call: => Any): Unit = { + val ex = intercept[ForbiddenException](call) + ex.getResponse.getStatus shouldEqual 403 + ex.getMessage should include("sharing feature is disabled") + } + + "the test environment" should "have computing-unit sharing disabled" in { + ComputingUnitConfig.sharingComputingUnitEnabled shouldBe false + } + + "grantAccess" should "be forbidden (403) when sharing is disabled" in { + expectForbidden(accessResource.grantAccess(session, cuid, user.getEmail, PrivilegeEnum.READ)) + } + + "revokeAccess" should "be forbidden (403) when sharing is disabled" in { + expectForbidden(accessResource.revokeAccess(session, cuid, user.getEmail)) + } + + "getComputingUnitAccessList" should "be forbidden (403) when sharing is disabled" in { + expectForbidden(accessResource.getComputingUnitAccessList(session, cuid)) + } + + "getOwner" should "be forbidden (403) when sharing is disabled" in { + expectForbidden(accessResource.getOwner(session, cuid)) + } +}
