Fix SDCOSMO-1820

dataset/src/integrationTest/kotlin/com/cosmotech/dataset/service/DatasetServiceIntegrationTest.kt

@@ -194,6 +194,30 @@ class DatasetServiceIntegrationTest : CsmRedisTestBase() {
     assertTrue { datasetList.size == 1 }
   }
 
+  @Test
+  fun `test Dataset - findByOrganizationIdAndDatasetId`() {
+    organizationSaved = organizationApiService.registerOrganization(organization)
+    datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
+
+    logger.info("Fetch dataset...")
+    val datasetRetrieved =
+        datasetApiService.findByOrganizationIdAndDatasetId(
+            organizationSaved.id!!, datasetSaved.id!!)
+    assertNotNull(datasetRetrieved)
+    assertEquals(datasetSaved, datasetRetrieved)
+  }
+
+  @Test
+  fun `test Dataset - findByOrganizationIdAndDatasetId wrong dataset id`() {
+    organizationSaved = organizationApiService.registerOrganization(organization)
+    datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
+
+    logger.info("Fetch dataset...")
+    val datasetRetrieved =
+        datasetApiService.findByOrganizationIdAndDatasetId(organizationSaved.id!!, "wrong_id")
+    assertNull(datasetRetrieved)
+  }
+
   @Test
   fun `can delete dataset when user is not the owner and is Platform Admin`() {
 

dataset/src/main/kotlin/com/cosmotech/dataset/DatasetApiServiceInterface.kt

@@ -7,6 +7,7 @@ package com.cosmotech.dataset
 import com.cosmotech.api.rbac.PERMISSION_READ
 import com.cosmotech.dataset.api.DatasetApiService
 import com.cosmotech.dataset.domain.Dataset
+import com.cosmotech.dataset.domain.DatasetAccessControl
 import redis.clients.jedis.graph.ResultSet
 
 interface DatasetApiServiceInterface : DatasetApiService {
@@ -18,4 +19,28 @@ interface DatasetApiServiceInterface : DatasetApiService {
   ): Dataset
 
   fun query(dataset: Dataset, query: String, isReadOnly: Boolean = false): ResultSet
+
+  /**
+   * Find Dataset by Organization Id and Dataset Id (checking ONLY READ_PERMISSION on targeted
+   * organization)
+   * @param organizationId an organization Id
+   * @param datasetId a dataset Id
+   * @return a Dataset or null
+   */
+  fun findByOrganizationIdAndDatasetId(organizationId: String, datasetId: String): Dataset?
+
+  /**
+   * Add a new entry (ou update existing one) on dataset passed in parameter
+   * @param organizationId an organization id
+   * @param dataset a dataset to update
+   * @param identity a user/application identity
+   * @param role new dataset role
+   * @return new DatasetAccessControl with update
+   */
+  fun addOrUpdateAccessControl(
+      organizationId: String,
+      dataset: Dataset,
+      identity: String,
+      role: String
+  ): DatasetAccessControl
 }

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

@@ -79,6 +79,7 @@ import com.cosmotech.organization.service.getRbac
 import java.io.InputStream
 import java.nio.charset.StandardCharsets
 import java.time.Instant
+import kotlin.jvm.optionals.getOrNull
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.launch
 import org.apache.commons.compress.archivers.ArchiveStreamFactory
@@ -632,6 +633,27 @@ class DatasetServiceImpl(
     }
   }
 
+  override fun findByOrganizationIdAndDatasetId(
+      organizationId: String,
+      datasetId: String
+  ): Dataset? {
+    organizationService.getVerifiedOrganization(organizationId)
+    return datasetRepository.findBy(organizationId, datasetId).getOrNull()
+  }
+
+  override fun addOrUpdateAccessControl(
+      organizationId: String,
+      dataset: Dataset,
+      identity: String,
+      role: String
+  ): DatasetAccessControl {
+    val rbacSecurity = csmRbac.setUserRole(dataset.getRbac(), identity, role)
+    dataset.setRbac(rbacSecurity)
+    datasetRepository.save(dataset)
+    val rbacAccessControl = csmRbac.getAccessControl(dataset.getRbac(), identity)
+    return DatasetAccessControl(rbacAccessControl.id, rbacAccessControl.role)
+  }
+
   fun <T> trx(dataset: Dataset, actionLambda: (Dataset) -> T): T {
     dataset.ingestionStatus?.takeUnless { it == IngestionStatusEnum.PENDING }
         ?: throw CsmClientException("Dataset in use, cannot update. Retry later")

scenario/src/integrationTest/kotlin/com/cosmotech/scenario/service/ScenarioServiceIntegrationTest.kt

@@ -27,8 +27,10 @@ import com.cosmotech.dataset.domain.DatasetAccessControl
 import com.cosmotech.dataset.domain.DatasetConnector
 import com.cosmotech.dataset.domain.DatasetSecurity
 import com.cosmotech.dataset.domain.IngestionStatusEnum
+import com.cosmotech.dataset.domain.SubDatasetGraphQuery
 import com.cosmotech.dataset.domain.TwincacheStatusEnum
 import com.cosmotech.dataset.repository.DatasetRepository
+import com.cosmotech.dataset.service.getRbac
 import com.cosmotech.organization.OrganizationApiServiceInterface
 import com.cosmotech.organization.domain.Organization
 import com.cosmotech.organization.domain.OrganizationAccessControl
@@ -63,6 +65,7 @@ import io.mockk.mockkStatic
 import java.time.Instant
 import java.util.*
 import kotlin.test.assertEquals
+import kotlin.test.assertNotNull
 import kotlin.test.assertTrue
 import org.junit.jupiter.api.BeforeAll
 import org.junit.jupiter.api.BeforeEach
@@ -747,7 +750,12 @@ class ScenarioServiceIntegrationTest : CsmRedisTestBase() {
 
   @Test
   fun `test updating datasetList and assert the accessControlList has the correct users`() {
-    val newDataset = datasetApiService.createDataset(organizationSaved.id!!, makeDataset())
+    val newDataset =
+        datasetApiService.createSubDataset(
+            organizationSaved.id!!,
+            datasetSaved.id!!,
+            SubDatasetGraphQuery(
+                name = "Copy of datasetSaved", queries = mutableListOf("FAKE Query"), main = false))
     scenarioSaved =
         scenarioApiService.updateScenario(
             organizationSaved.id!!,
@@ -760,9 +768,15 @@ class ScenarioServiceIntegrationTest : CsmRedisTestBase() {
             organizationSaved.id!!, workspaceSaved.id!!, scenarioSaved.id!!)
 
     scenarioSaved.datasetList!!.forEach { thisDataset ->
-      val datasetUserList =
-          datasetApiService.getDatasetSecurityUsers(organizationSaved.id!!, thisDataset)
-      scenarioUserList.forEach { user -> assertTrue(datasetUserList.contains(user)) }
+      val dataset =
+          datasetApiService.findByOrganizationIdAndDatasetId(organizationSaved.id!!, thisDataset)
+      assertNotNull(dataset)
+      val datasetUserList = dataset.getRbac().accessControlList
+      if (dataset.main == null || dataset.main == true) {
+        datasetUserList.map { it.id }.forEach { assertTrue(scenarioUserList.contains(it)) }
+      } else {
+        assertTrue(datasetUserList.map { it.id }.containsAll(scenarioUserList))
+      }
     }
   }
 

scenario/src/integrationTest/kotlin/com/cosmotech/scenario/service/ScenarioServiceRBACTest.kt

@@ -5216,16 +5216,16 @@ class ScenarioServiceRBACTest : CsmRedisTestBase() {
           }
 
   @TestFactory
-  fun `test Dataset RBAC addScenarioAccessControl`() =
-      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,
+  fun `test Dataset RBAC addScenarioAccessControl with workspace datasetCopy to true`() =
+      listOf(
+              ROLE_VIEWER,
+              ROLE_EDITOR,
+              ROLE_VALIDATOR,
+              ROLE_USER,
+              ROLE_NONE,
+              ROLE_ADMIN,
           )
-          .map { (role, shouldThrow) ->
+          .forEach { role ->
             dynamicTest("Test Dataset RBAC addScenarioAccessControl : $role") {
               every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
               val connector = makeConnector()
@@ -5263,26 +5263,71 @@ class ScenarioServiceRBACTest : CsmRedisTestBase() {
                       organizationSaved.id!!, workspaceSaved.id!!, scenario)
               every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
 
-              if (shouldThrow) {
-                val exception =
-                    assertThrows<CsmAccessForbiddenException> {
-                      scenarioApiService.addScenarioAccessControl(
-                          organizationSaved.id!!,
-                          workspaceSaved.id!!,
-                          scenarioSaved.id!!,
-                          ScenarioAccessControl("id", ROLE_ADMIN))
-                    }
-                assertEquals(
-                    "RBAC ${datasetSaved.id!!} - User does not have permission $PERMISSION_WRITE_SECURITY",
-                    exception.message)
-              } else {
-                assertDoesNotThrow {
-                  scenarioApiService.addScenarioAccessControl(
+              assertDoesNotThrow {
+                scenarioApiService.addScenarioAccessControl(
+                    organizationSaved.id!!,
+                    workspaceSaved.id!!,
+                    scenarioSaved.id!!,
+                    ScenarioAccessControl("id", ROLE_ADMIN))
+              }
+            }
+          }
+
+  @TestFactory
+  fun `test Dataset RBAC addScenarioAccessControl with workspace datasetCopy to false`() =
+      listOf(
+              ROLE_VIEWER,
+              ROLE_EDITOR,
+              ROLE_VALIDATOR,
+              ROLE_USER,
+              ROLE_NONE,
+              ROLE_ADMIN,
+          )
+          .map { role ->
+            dynamicTest("Test Dataset RBAC addScenarioAccessControl : $role") {
+              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, TEST_USER_MAIL, role)
+              var datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
+              datasetSaved =
+                  datasetRepository.save(
+                      datasetSaved.apply { ingestionStatus = IngestionStatusEnum.SUCCESS })
+              every { datasetApiService.createSubDataset(any(), any(), any()) } returns datasetSaved
+              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)
+              workspace.datasetCopy = false
+              val workspaceSaved =
+                  workspaceApiService.createWorkspace(organizationSaved.id!!, workspace)
+              val scenario =
+                  makeScenarioWithRole(
                       organizationSaved.id!!,
                       workspaceSaved.id!!,
-                      scenarioSaved.id!!,
-                      ScenarioAccessControl("id", ROLE_ADMIN))
-                }
+                      solutionSaved.id!!,
+                      mutableListOf(datasetSaved.id!!),
+                      id = TEST_USER_MAIL,
+                      role = ROLE_ADMIN)
+              val scenarioSaved =
+                  scenarioApiService.createScenario(
+                      organizationSaved.id!!, workspaceSaved.id!!, scenario)
+              every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
+
+              assertDoesNotThrow {
+                scenarioApiService.addScenarioAccessControl(
+                    organizationSaved.id!!,
+                    workspaceSaved.id!!,
+                    scenarioSaved.id!!,
+                    ScenarioAccessControl("id", ROLE_ADMIN))
               }
             }
           }

scenario/src/main/kotlin/com/cosmotech/scenario/service/ScenarioServiceImpl.kt

@@ -41,9 +41,7 @@ import com.cosmotech.api.utils.constructPageRequest
 import com.cosmotech.api.utils.findAllPaginated
 import com.cosmotech.api.utils.getCurrentAccountIdentifier
 import com.cosmotech.api.utils.getCurrentAuthenticatedUserName
-import com.cosmotech.dataset.api.DatasetApiService
-import com.cosmotech.dataset.domain.DatasetAccessControl
-import com.cosmotech.dataset.domain.DatasetRole
+import com.cosmotech.dataset.DatasetApiServiceInterface
 import com.cosmotech.dataset.domain.IngestionStatusEnum
 import com.cosmotech.dataset.domain.SubDatasetGraphQuery
 import com.cosmotech.dataset.service.getRbac
@@ -84,7 +82,7 @@ import org.springframework.stereotype.Service
 @Service
 @Suppress("LargeClass", "TooManyFunctions", "LongParameterList")
 internal class ScenarioServiceImpl(
-    private val datasetService: DatasetApiService,
+    private val datasetService: DatasetApiServiceInterface,
     private val solutionService: SolutionApiServiceInterface,
     private val workspaceService: WorkspaceApiServiceInterface,
     private val azureDataExplorerClient: ResultDataClient,
@@ -1040,29 +1038,41 @@ internal class ScenarioServiceImpl(
       scenario: Scenario,
       scenarioAccessControl: ScenarioAccessControl
   ) {
-    val id = scenarioAccessControl.id
-    // Scenario and Dataset don't have the same roles
-    // This function translates the role set from one to another
+    val userIdForAcl = scenarioAccessControl.id
+
     val workspace = workspaceService.getVerifiedWorkspace(organizationId, scenario.workspaceId!!)
 
-    // The role in the datasets should be only be similar to scenarios if they are a copy
-    val role: String =
-        if (workspace.datasetCopy == true) {
+    if (workspace.datasetCopy == true) {
+      // Scenario and Dataset don't have the same roles
+      // This function translates the role set from one to another
+      val datasetRoleFromScenarioAcl: String =
           if (scenarioAccessControl.role == ROLE_VALIDATOR) {
             ROLE_USER
           } else {
             scenarioAccessControl.role
           }
-        } else {
-          ROLE_VIEWER
-        }
-    scenario.datasetList!!.forEach {
-      val datasetUsers = datasetService.getDatasetSecurityUsers(organizationId, it)
-      if (datasetUsers.contains(id)) {
-        datasetService.updateDatasetAccessControl(organizationId, it, id, DatasetRole(role))
-      } else {
-        datasetService.addDatasetAccessControl(organizationId, it, DatasetAccessControl(id, role))
-      }
+
+      scenario.datasetList!!
+          .mapNotNull { datasetService.findByOrganizationIdAndDatasetId(organizationId, it) }
+          // Filter on dataset copy (cause we do not want update main dataset when
+          // workspace.dataCopy is true)
+          .filter { it.main == false }
+          .forEach { dataset ->
+            // Dataset roles should be similar to scenario ones if dataset are copy of a master one
+            // This is possible when workspace.datasetCopy is true
+            datasetService.addOrUpdateAccessControl(
+                organizationId, dataset, userIdForAcl, datasetRoleFromScenarioAcl)
+          }
+    } else {
+      scenario.datasetList!!
+          .mapNotNull { datasetService.findByOrganizationIdAndDatasetId(organizationId, it) }
+          .forEach { dataset ->
+            val datasetAcl = dataset.getRbac().accessControlList
+            if (datasetAcl.none { it.id == userIdForAcl }) {
+              datasetService.addOrUpdateAccessControl(
+                  organizationId, dataset, userIdForAcl, ROLE_VIEWER)
+            }
+          }
     }
   }