[RMZ-413] upgrade runner ACL lead to server error on ACL dataset management

dataset/src/main/kotlin/com/cosmotech/dataset/service/DatasetServiceImpl.kt

@@ -560,14 +560,6 @@ class DatasetServiceImpl(
   override fun deleteDataset(organizationId: String, datasetId: String) {
     val dataset = getVerifiedDataset(organizationId, datasetId, PERMISSION_DELETE)
 
-    val isPlatformAdmin =
-        getCurrentAuthenticatedRoles(csmPlatformProperties).contains(ROLE_PLATFORM_ADMIN)
-    if (dataset.ownerId != getCurrentAuthenticatedUserName(csmPlatformProperties) &&
-        !isPlatformAdmin) {
-      // TODO Only the owner or an admin should be able to perform this operation
-      throw CsmAccessForbiddenException("You are not allowed to delete this Resource")
-    }
-
     csmJedisPool.resource.use { jedis ->
       if (jedis.exists(dataset.twingraphId!!)) {
         jedis.del(dataset.twingraphId!!)

dataset/src/test/kotlin/com/cosmotech/dataset/service/DatasetServiceImplTests.kt

@@ -5,7 +5,6 @@ package com.cosmotech.dataset.service
 import com.cosmotech.api.config.CsmPlatformProperties
 import com.cosmotech.api.events.CsmEventPublisher
 import com.cosmotech.api.events.TwingraphImportJobInfoRequest
-import com.cosmotech.api.exceptions.CsmAccessForbiddenException
 import com.cosmotech.api.exceptions.CsmResourceNotFoundException
 import com.cosmotech.api.id.CsmIdGenerator
 import com.cosmotech.api.rbac.CsmAdmin
@@ -390,13 +389,12 @@ class DatasetServiceImplTests {
   }
 
   @Test
-  fun `deleteDataset should throw CsmAccessForbiddenException`() {
-    val dataset = baseDataset()
+  fun `deleteDataset do not throw error - rbac is disabled`() {
+    val dataset = baseDataset().apply { twingraphId = "mytwingraphId" }
     every { datasetRepository.findBy(ORGANIZATION_ID, DATASET_ID) } returns Optional.of(dataset)
     every { getCurrentAuthenticatedUserName(csmPlatformProperties) } returns "my.account-tester"
-    assertThrows<CsmAccessForbiddenException> {
-      datasetService.deleteDataset(ORGANIZATION_ID, DATASET_ID)
-    }
+    datasetService.deleteDataset(ORGANIZATION_ID, DATASET_ID)
+    verify(exactly = 1) { datasetRepository.delete(any()) }
   }
 
   @Test
@@ -435,7 +433,7 @@ class DatasetServiceImplTests {
     val twinGraphQuery = DatasetTwinGraphQuery("MATCH(n) RETURN n")
     datasetService.twingraphQuery(ORGANIZATION_ID, DATASET_ID, twinGraphQuery)
 
-    verify { csmRedisGraph.query(any(), any(), any<Long>()) }
+    verify { csmRedisGraph.readOnlyQuery(any(), any(), any<Long>()) }
   }
 
   @Test

runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceIntegrationTest.kt

@@ -360,17 +360,17 @@ class RunnerServiceIntegrationTest : CsmRedisTestBase() {
     assertEquals(ROLE_EDITOR, runnerAccessControlRegistered.role)
 
     logger.info(
-        "should update the Access Control and assert it has been updated in the linked datasets")
+        "should let the Access Control as-is cause dataset ACL already contains info for this user")
     runnerSaved.datasetList!!.forEach {
       assertEquals(
-          ROLE_EDITOR,
+          ROLE_VIEWER,
           datasetApiService
               .getDatasetAccessControl(organizationSaved.id!!, it, TEST_USER_MAIL)
               .role)
     }
 
     logger.info("should get the list of users and assert there are 2")
-    var userList =
+    val userList =
         runnerApiService.getRunnerSecurityUsers(
             organizationSaved.id!!, workspaceSaved.id!!, runnerSaved.id!!)
     assertEquals(3, userList.size)

runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceRBACTest.kt

@@ -62,6 +62,7 @@ import io.mockk.junit5.MockKExtension
 import io.mockk.mockk
 import io.mockk.mockkStatic
 import java.util.*
+import kotlin.test.Test
 import kotlin.test.assertEquals
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.DynamicTest.dynamicTest
@@ -2777,15 +2778,15 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
 
   @TestFactory
   fun `test Dataset RBAC addRunnerAccessControl`() =
-      mapOf(
-              ROLE_VIEWER to true,
-              ROLE_EDITOR to true,
-              ROLE_VALIDATOR to true,
-              ROLE_USER to true,
-              ROLE_NONE to true,
-              ROLE_ADMIN to false,
+      listOf(
+              ROLE_VIEWER,
+              ROLE_EDITOR,
+              ROLE_VALIDATOR,
+              ROLE_USER,
+              ROLE_NONE,
+              ROLE_ADMIN,
           )
-          .map { (role, shouldThrow) ->
+          .map { role ->
             dynamicTest("Test Dataset RBAC addRunnerAccessControl : $role") {
               every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
               val connector = makeConnector()
@@ -2820,32 +2821,90 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
                       role = ROLE_ADMIN)
               val runnerSaved =
                   runnerApiService.createRunner(organizationSaved.id!!, workspaceSaved.id!!, runner)
-              every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
 
-              if (shouldThrow) {
-                val exception =
-                    assertThrows<CsmAccessForbiddenException> {
-                      runnerApiService.addRunnerAccessControl(
-                          organizationSaved.id!!,
-                          workspaceSaved.id!!,
-                          runnerSaved.id!!,
-                          RunnerAccessControl("id", ROLE_ADMIN))
-                    }
+              assertDoesNotThrow {
                 assertEquals(
-                    "RBAC ${datasetSaved.id!!} - User does not have permission $PERMISSION_WRITE_SECURITY",
-                    exception.message)
-              } else {
-                assertDoesNotThrow {
-                  runnerApiService.addRunnerAccessControl(
-                      organizationSaved.id!!,
-                      workspaceSaved.id!!,
-                      runnerSaved.id!!,
-                      RunnerAccessControl("id", ROLE_ADMIN))
-                }
+                    true,
+                    datasetSaved.security
+                        ?.accessControlList
+                        ?.filter { datasetAccessControl ->
+                          datasetAccessControl.id == TEST_USER_MAIL
+                        }
+                        ?.any { datasetAccessControl -> datasetAccessControl.role == role })
+
+                runnerApiService.addRunnerAccessControl(
+                    organizationSaved.id!!,
+                    workspaceSaved.id!!,
+                    runnerSaved.id!!,
+                    RunnerAccessControl(TEST_USER_MAIL, ROLE_ADMIN))
+
+                val datasetWithUpgradedACL =
+                    datasetApiService.findDatasetById(organizationSaved.id!!, datasetSaved.id!!)
+                assertEquals(
+                    true,
+                    datasetWithUpgradedACL.security
+                        ?.accessControlList
+                        ?.filter { datasetAccessControl ->
+                          datasetAccessControl.id == TEST_USER_MAIL
+                        }
+                        ?.any { datasetAccessControl -> datasetAccessControl.role == role })
               }
             }
           }
 
+  @Test
+  fun `test Dataset RBAC addRunnerAccessControl with new ACL entry`() {
+    every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
+    val connector = makeConnector()
+    val connectorSaved = connectorApiService.registerConnector(connector)
+    val organization = makeOrganizationWithRole(id = TEST_USER_MAIL, role = ROLE_ADMIN)
+    val organizationSaved = organizationApiService.registerOrganization(organization)
+    val dataset =
+        makeDataset(
+            organizationSaved.id!!, connectorSaved, id = "[email protected]", role = ROLE_NONE)
+    var datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
+    datasetSaved =
+        datasetRepository.save(
+            datasetSaved.apply { ingestionStatus = Dataset.IngestionStatus.SUCCESS })
+    every { datasetApiService.createSubDataset(any(), any(), any()) } returns datasetSaved
+
+    assertEquals(
+        false,
+        datasetSaved.security?.accessControlList?.any { datasetAccessControl ->
+          datasetAccessControl.id == TEST_USER_MAIL
+        })
+
+    val solution = makeSolution(organizationSaved.id!!, TEST_USER_MAIL, ROLE_ADMIN)
+    val solutionSaved = solutionApiService.createSolution(organizationSaved.id!!, solution)
+    val workspace =
+        makeWorkspaceWithRole(
+            organizationSaved.id!!, solutionSaved.id!!, id = TEST_USER_MAIL, role = ROLE_ADMIN)
+    val workspaceSaved = workspaceApiService.createWorkspace(organizationSaved.id!!, workspace)
+    val runner =
+        makeRunnerWithRole(
+            organizationSaved.id!!,
+            workspaceSaved.id!!,
+            solutionSaved.id!!,
+            mutableListOf(datasetSaved.id!!),
+            id = "[email protected]",
+            role = ROLE_ADMIN)
+    val runnerSaved =
+        runnerApiService.createRunner(organizationSaved.id!!, workspaceSaved.id!!, runner)
+
+    runnerApiService.addRunnerAccessControl(
+        organizationSaved.id!!,
+        workspaceSaved.id!!,
+        runnerSaved.id!!,
+        RunnerAccessControl(TEST_USER_MAIL, ROLE_ADMIN))
+    val datasetWithUpgradedACL =
+        datasetApiService.findDatasetById(organizationSaved.id!!, datasetSaved.id!!)
+    assertEquals(
+        true,
+        datasetWithUpgradedACL.security?.accessControlList?.any { datasetAccessControl ->
+          datasetAccessControl.id == TEST_USER_MAIL
+        })
+  }
+
   @TestFactory
   fun `test Solution RBAC addRunnerAccessControl`() =
       mapOf(

runner/src/main/kotlin/com/cosmotech/runner/service/RunnerService.kt

@@ -21,8 +21,6 @@ import com.cosmotech.api.utils.compareToAndMutateIfNeeded
 import com.cosmotech.api.utils.getCurrentAccountIdentifier
 import com.cosmotech.api.utils.getCurrentAuthenticatedUserName
 import com.cosmotech.dataset.DatasetApiServiceInterface
-import com.cosmotech.dataset.domain.DatasetAccessControl
-import com.cosmotech.dataset.domain.DatasetRole
 import com.cosmotech.dataset.service.getRbac
 import com.cosmotech.organization.OrganizationApiServiceInterface
 import com.cosmotech.organization.domain.Organization
@@ -178,9 +176,16 @@ class RunnerService(
       // take newly added datasets and propagate existing ACL on it
       this.runner.datasetList
           ?.filterNot { beforeMutateDatasetList?.contains(it) ?: false }
-          ?.forEach { newDatasetId ->
-            this.runner.security?.accessControlList?.forEach {
-              this.propagateAccessControlToDataset(newDatasetId, it.id, it.role)
+          ?.mapNotNull {
+            datasetApiService.findByOrganizationIdAndDatasetId(organization!!.id!!, it)
+          }
+          ?.forEach { newDataset ->
+            this.runner.security?.accessControlList?.forEach { roleDefinition ->
+              val newDatasetAcl = newDataset.getRbac().accessControlList
+              if (newDatasetAcl.none { it.id == roleDefinition.id }) {
+                datasetApiService.addOrUpdateAccessControl(
+                    organization!!.id!!, newDataset, roleDefinition.id, roleDefinition.role)
+              }
             }
           }
     }
@@ -213,7 +218,20 @@ class RunnerService(
               this.roleDefinition)
       this.setRbacSecurity(rbacSecurity)
 
-      this.propagateAccessControlToDatasets(runnerAccessControl.id, runnerAccessControl.role)
+      val userId = runnerAccessControl.id
+      val datasetRole = runnerAccessControl.role.takeUnless { it == ROLE_VALIDATOR } ?: ROLE_USER
+      val organizationId = this.runner.organizationId!!
+
+      // Assign roles on linked datasets if not already present on dataset resource
+      this.runner.datasetList!!
+          .mapNotNull { datasetApiService.findByOrganizationIdAndDatasetId(organizationId, it) }
+          .forEach { dataset ->
+            val datasetAcl = dataset.getRbac().accessControlList
+            if (datasetAcl.none { it.id == userId }) {
+              datasetApiService.addOrUpdateAccessControl(
+                  organizationId, dataset, userId, datasetRole)
+            }
+          }
     }
 
     fun getAccessControlFor(userId: String): RunnerAccessControl {
@@ -257,26 +275,6 @@ class RunnerService(
               ?: mutableListOf())
     }
 
-    private fun propagateAccessControlToDatasets(userId: String, role: String) {
-      this.runner.datasetList!!.forEach { datasetId ->
-        propagateAccessControlToDataset(datasetId, userId, role)
-      }
-    }
-
-    private fun propagateAccessControlToDataset(datasetId: String, userId: String, role: String) {
-      val datasetRole = role.takeUnless { it == ROLE_VALIDATOR } ?: ROLE_USER
-      val organizationId = this.runner.organizationId!!
-
-      val datasetUsers = datasetApiService.getDatasetSecurityUsers(organizationId, datasetId)
-      if (datasetUsers.contains(userId)) {
-        datasetApiService.updateDatasetAccessControl(
-            organizationId, datasetId, userId, DatasetRole(datasetRole))
-      } else {
-        datasetApiService.addDatasetAccessControl(
-            organizationId, datasetId, DatasetAccessControl(userId, datasetRole))
-      }
-    }
-
     private fun removeAccessControlToDatasets(userId: String) {
       val organizationId = this.runner.organizationId!!
       this.runner.datasetList!!.forEach { datasetId ->