refactor AbstractContainerResource; fixes #131

server/src/main/kotlin/nl/knaw/huc/annorepo/resources/AbstractContainerResource.kt

@@ -31,11 +31,10 @@ abstract class AbstractContainerResource(
     protected fun SecurityContext.checkUserHasReadRightsInThisContainer(containerName: String) {
         checkContainerExists(containerName)
         val anonymousHasAccess = checkContainerIsReadOnlyForAnonymous(containerName)
-        if (configuration.withAuthentication) {
+        if (!anonymousHasAccess && configuration.withAuthentication) {
             containerAccessChecker.checkUserHasReadRightsInThisContainer(
                 userPrincipal,
-                containerName,
-                anonymousHasAccess
+                containerName
             )
         }
     }

server/src/main/kotlin/nl/knaw/huc/annorepo/resources/tools/ContainerAccessChecker.kt

@@ -13,29 +13,26 @@ class ContainerAccessChecker(private val containerUserDAO: ContainerUserDAO) {
     val log: Logger = LoggerFactory.getLogger(javaClass)
 
     fun checkUserHasAdminRightsInThisContainer(userPrincipal: Principal?, containerName: String) {
-        checkUserRightsInThisContainer(userPrincipal, containerName, setOf(Role.ADMIN), false)
+        checkUserRightsInThisContainer(userPrincipal, containerName, setOf(Role.ADMIN))
     }
 
     fun checkUserHasEditRightsInThisContainer(userPrincipal: Principal?, containerName: String) {
-        checkUserRightsInThisContainer(userPrincipal, containerName, setOf(Role.ADMIN, Role.EDITOR), false)
+        checkUserRightsInThisContainer(userPrincipal, containerName, setOf(Role.ADMIN, Role.EDITOR))
     }
 
     fun checkUserHasReadRightsInThisContainer(
         userPrincipal: Principal?,
-        containerName: String,
-        anonymousHasAccess: Boolean
+        containerName: String
     ) = checkUserRightsInThisContainer(
         userPrincipal,
         containerName,
-        setOf(Role.ADMIN, Role.EDITOR, Role.GUEST),
-        anonymousHasAccess
+        setOf(Role.ADMIN, Role.EDITOR, Role.GUEST)
     )
 
     private fun checkUserRightsInThisContainer(
         userPrincipal: Principal?,
         containerName: String,
-        rolesWithAccessRights: Set<Role>,
-        anonymousHasAccess: Boolean = false,
+        rolesWithAccessRights: Set<Role>
     ) {
 //        log.info("userPrincipal={}", userPrincipal)
 //        log.info("anonymousHasAccess={}", anonymousHasAccess)
@@ -52,9 +49,7 @@ class ContainerAccessChecker(private val containerUserDAO: ContainerUserDAO) {
             throw NotAuthorizedException("User $userName does not have access rights to this endpoint")
         }
 
-        if (!anonymousHasAccess) {
-            throw NotAuthorizedException("No authentication found")
-        }
+        throw NotAuthorizedException("No authentication found")
     }
 
 }

server/src/test/java/nl/knaw/huc/annorepo/resources/AbstractContainerResourceTest.kt

@@ -0,0 +1,93 @@
+package nl.knaw.huc.annorepo.resources
+
+import java.security.Principal
+import jakarta.ws.rs.NotAuthorizedException
+import jakarta.ws.rs.core.SecurityContext
+import org.junit.jupiter.api.Test
+import io.mockk.every
+import io.mockk.mockk
+import org.assertj.core.api.Assertions.assertThat
+import org.assertj.core.api.Assertions.assertThatExceptionOfType
+import nl.knaw.huc.annorepo.api.ContainerMetadata
+import nl.knaw.huc.annorepo.config.AnnoRepoConfiguration
+import nl.knaw.huc.annorepo.dao.ContainerDAO
+import nl.knaw.huc.annorepo.dao.ContainerUserDAO
+import nl.knaw.huc.annorepo.resources.tools.ContainerAccessChecker
+
+class AbstractContainerResourceTest {
+    private val containerName = "an-existing-container"
+
+    class TestResource(
+        configuration: AnnoRepoConfiguration,
+        containerDAO: ContainerDAO,
+        containerAccessChecker: ContainerAccessChecker
+    ) : AbstractContainerResource(configuration, containerDAO, containerAccessChecker) {
+        fun isReadable(containerName: String, principal: Principal): Boolean {
+            val securityContext = mockk<SecurityContext>()
+            every { securityContext.userPrincipal } returns principal
+            securityContext.checkUserHasReadRightsInThisContainer(containerName)
+            return true
+        }
+    }
+
+    @Test
+    fun `A user with a valid api key should have read access to a container that is read-only for anyone`() {
+        val userName = "a-user"
+
+        val mockPrincipal = mockk<Principal>()
+        every { mockPrincipal.name } returns userName
+
+        val mockContainerUserDAO = mockk<ContainerUserDAO>()
+        every {
+            mockContainerUserDAO.getUserRole(
+                containerName,
+                userName
+            )
+        } returns null // user has no role in this container
+
+        val containerAccessChecker = ContainerAccessChecker(mockContainerUserDAO)
+
+        val mockContainerMetadata = mockk<ContainerMetadata>()
+        every { mockContainerMetadata.isReadOnlyForAnonymous } returns true
+
+        val mockContainerDAO = mockk<ContainerDAO>()
+        every { mockContainerDAO.containerExists(containerName) } returns true
+        every { mockContainerDAO.getContainerMetadata(containerName) } returns mockContainerMetadata
+
+        val mockConfiguration = mockk<AnnoRepoConfiguration>()
+
+        val resource = TestResource(mockConfiguration, mockContainerDAO, containerAccessChecker)
+        val hasReadAccess: Boolean = resource.isReadable(containerName, mockPrincipal)
+
+        assertThat(hasReadAccess).isTrue()
+    }
+
+    @Test
+    fun `A user with a valid api key should not have read access to a container that is not read-only for anyone`() {
+        val userName = "a-user"
+
+        val mockPrincipal = mockk<Principal>()
+        every { mockPrincipal.name } returns userName
+
+        val mockContainerUserDAO = mockk<ContainerUserDAO>()
+        every { mockContainerUserDAO.getUserRole(containerName, userName) } returns null
+
+        val containerAccessChecker = ContainerAccessChecker(mockContainerUserDAO)
+
+        val mockContainerMetadata = mockk<ContainerMetadata>()
+        every { mockContainerMetadata.isReadOnlyForAnonymous } returns false
+
+        val mockContainerDAO = mockk<ContainerDAO>()
+        every { mockContainerDAO.containerExists(containerName) } returns true
+        every { mockContainerDAO.getContainerMetadata(containerName) } returns mockContainerMetadata
+
+        val mockConfiguration = mockk<AnnoRepoConfiguration>()
+        every { mockConfiguration.withAuthentication } returns true
+
+        val resource = TestResource(mockConfiguration, mockContainerDAO, containerAccessChecker)
+        assertThatExceptionOfType(NotAuthorizedException::class.java).isThrownBy {
+            resource.isReadable(containerName, mockPrincipal)
+        }.withMessage("HTTP 401 Unauthorized")
+    }
+
+}