Copilot commented on code in PR #6446:
URL: https://github.com/apache/texera/pull/6446#discussion_r3593386245
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala:
##########
@@ -152,18 +152,22 @@ class ComputingUnitAccessResource {
}
Review Comment:
`grantAccess` throws `IllegalArgumentException` for client errors (no
permission / unknown email). There is no `ExceptionMapper` for
`IllegalArgumentException` in this repo (only `UnauthorizedExceptionMapper`),
so these exceptions are likely to surface as HTTP 500 instead of a 4xx. Use
JAX-RS exceptions to guarantee the intended status codes.
##########
computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessResourceSpec.scala:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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 clear error instead of crashing"
in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.grantAccess(ownerSession, cuid, nonExistentEmail,
PrivilegeEnum.READ)
+ }
+ 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" in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.grantAccess(strangerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+ }
+ ex.getMessage should include("does not have permission to grant access")
+ }
Review Comment:
If `grantAccess` uses `ForbiddenException` for authorization failures (so
the endpoint returns HTTP 403), this test should intercept `ForbiddenException`
rather than `IllegalArgumentException`.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala:
##########
@@ -179,10 +183,11 @@ class ComputingUnitAccessResource {
throw new IllegalArgumentException("User does not have permission to
revoke access")
}
Review Comment:
Same issue in `revokeAccess`: throwing `IllegalArgumentException` for
permission/unknown-email errors will likely be treated as an unhandled runtime
exception and returned as HTTP 500. Prefer explicit JAX-RS exceptions (403/400)
like other access resources.
##########
computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessResourceSpec.scala:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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 clear error instead of crashing"
in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.grantAccess(ownerSession, cuid, nonExistentEmail,
PrivilegeEnum.READ)
+ }
+ ex.getMessage should include("User with the given email does not exist")
+ accessEmails(cuid) shouldBe empty
+ }
Review Comment:
If `ComputingUnitAccessResource` is changed to throw `BadRequestException`
(so the REST endpoint returns 4xx instead of 500), this test should intercept
that JAX-RS exception type rather than `IllegalArgumentException`.
##########
computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessResourceSpec.scala:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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 clear error instead of crashing"
in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.grantAccess(ownerSession, cuid, nonExistentEmail,
PrivilegeEnum.READ)
+ }
+ 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" in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.grantAccess(strangerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+ }
+ 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 clear error instead of crashing"
in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.revokeAccess(ownerSession, cuid, nonExistentEmail)
+ }
+ ex.getMessage should include("User with the given email does not exist")
+ }
Review Comment:
If `revokeAccess` is updated to throw `BadRequestException` for unknown
emails (to avoid HTTP 500), update the intercepted exception type here
accordingly.
##########
computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitAccessResourceSpec.scala:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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 clear error instead of crashing"
in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.grantAccess(ownerSession, cuid, nonExistentEmail,
PrivilegeEnum.READ)
+ }
+ 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" in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.grantAccess(strangerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+ }
+ 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 clear error instead of crashing"
in {
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.revokeAccess(ownerSession, cuid, nonExistentEmail)
+ }
+ ex.getMessage should include("User with the given email does not exist")
+ }
+
+ it should "reject a caller without write access" in {
+ accessResource.grantAccess(ownerSession, cuid, granteeUser.getEmail,
PrivilegeEnum.READ)
+
+ val ex = intercept[IllegalArgumentException] {
+ accessResource.revokeAccess(strangerSession, cuid, granteeUser.getEmail)
+ }
+ ex.getMessage should include("does not have permission to revoke access")
+ }
Review Comment:
If `revokeAccess` is updated to throw `ForbiddenException` for
no-write-access cases (HTTP 403), update the intercepted exception type here to
match.
--
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]
