diff --git a/build.gradle.kts b/build.gradle.kts index d143350dc..23b784251 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -48,7 +48,7 @@ group = "com.cosmotech" version = scmVersion.version val kotlinJvmTarget = 17 -val cosmotechApiCommonVersion = "0.3.2" +val cosmotechApiCommonVersion = "0.3.3" val cosmotechApiAzureVersion = "0.2.1" val azureSpringBootBomVersion = "3.14.0" val jedisVersion = "3.9.0" diff --git a/dataset/src/main/kotlin/com/cosmotech/dataset/service/DatasetServiceImpl.kt b/dataset/src/main/kotlin/com/cosmotech/dataset/service/DatasetServiceImpl.kt index 92461f86c..5d0386625 100644 --- a/dataset/src/main/kotlin/com/cosmotech/dataset/service/DatasetServiceImpl.kt +++ b/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!!) diff --git a/dataset/src/test/kotlin/com/cosmotech/dataset/service/DatasetServiceImplTests.kt b/dataset/src/test/kotlin/com/cosmotech/dataset/service/DatasetServiceImplTests.kt index 23264385d..26ddd3a82 100644 --- a/dataset/src/test/kotlin/com/cosmotech/dataset/service/DatasetServiceImplTests.kt +++ b/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 { - 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()) } + verify { csmRedisGraph.readOnlyQuery(any(), any(), any()) } } @Test diff --git a/runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceIntegrationTest.kt b/runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceIntegrationTest.kt index 667955fd2..898ef5a8b 100644 --- a/runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceIntegrationTest.kt +++ b/runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceIntegrationTest.kt @@ -359,18 +359,17 @@ class RunnerServiceIntegrationTest : CsmRedisTestBase() { RunnerRole(ROLE_EDITOR)) assertEquals(ROLE_EDITOR, runnerAccessControlRegistered.role) - logger.info( - "should update the Access Control and assert it has been updated in the linked datasets") + logger.info("Should not change the datasets access control because ACL already exist on it") 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) diff --git a/runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceRBACTest.kt b/runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceRBACTest.kt index 0357a814a..b4f0169eb 100644 --- a/runner/src/integrationTest/kotlin/com/cosmotech/runner/service/RunnerServiceRBACTest.kt +++ b/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 @@ -2776,76 +2777,140 @@ 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, + fun `test Dataset RBAC modification when addRunnerAccessControl is called on a runner`() = + listOf( + ROLE_VIEWER, + ROLE_EDITOR, + ROLE_VALIDATOR, + ROLE_USER, + ROLE_NONE, + ROLE_ADMIN, ) - .map { (role, shouldThrow) -> - dynamicTest("Test Dataset RBAC addRunnerAccessControl : $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 = Dataset.IngestionStatus.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) - val workspaceSaved = - workspaceApiService.createWorkspace(organizationSaved.id!!, workspace) - val runner = - makeRunnerWithRole( - organizationSaved.id!!, - workspaceSaved.id!!, - solutionSaved.id!!, - mutableListOf(datasetSaved.id!!), - id = TEST_USER_MAIL, - role = ROLE_ADMIN) - val runnerSaved = - runnerApiService.createRunner(organizationSaved.id!!, workspaceSaved.id!!, runner) - every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL - - if (shouldThrow) { - val exception = - assertThrows { - runnerApiService.addRunnerAccessControl( + .map { role -> + dynamicTest( + "Check Dataset RBAC modification " + + "when addRunnerAccessControl is called on a runner with role : $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 = Dataset.IngestionStatus.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) + val workspaceSaved = + workspaceApiService.createWorkspace(organizationSaved.id!!, workspace) + val runner = + makeRunnerWithRole( organizationSaved.id!!, workspaceSaved.id!!, - runnerSaved.id!!, - RunnerAccessControl("id", ROLE_ADMIN)) - } - 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)) + solutionSaved.id!!, + mutableListOf(datasetSaved.id!!), + id = TEST_USER_MAIL, + role = ROLE_ADMIN) + val runnerSaved = + runnerApiService.createRunner( + organizationSaved.id!!, workspaceSaved.id!!, runner) + + assertDoesNotThrow { + assertEquals( + 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 addRunnerAccessControl when called on a runner with an user that do not exist in Dataset RBAC`() { + 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 = "unknown_user@test.com", 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 = "unknown_user@test.com", + 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( diff --git a/runner/src/main/kotlin/com/cosmotech/runner/service/RunnerService.kt b/runner/src/main/kotlin/com/cosmotech/runner/service/RunnerService.kt index ad15567f6..c84f24c40 100644 --- a/runner/src/main/kotlin/com/cosmotech/runner/service/RunnerService.kt +++ b/runner/src/main/kotlin/com/cosmotech/runner/service/RunnerService.kt @@ -21,8 +21,7 @@ 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.domain.Dataset import com.cosmotech.dataset.service.getRbac import com.cosmotech.organization.OrganizationApiServiceInterface import com.cosmotech.organization.domain.Organization @@ -178,9 +177,12 @@ 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 { dataset -> + this.runner.security?.accessControlList?.forEach { roleDefinition -> + addUserAccessControlOnDataset(dataset, roleDefinition) } } } @@ -213,7 +215,16 @@ 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 -> + addUserAccessControlOnDataset(dataset, RunnerAccessControl(userId, datasetRole)) + } } fun getAccessControlFor(userId: String): RunnerAccessControl { @@ -257,26 +268,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 -> @@ -298,4 +289,12 @@ class RunnerService( this.setRbacSecurity(rbacSecurity) } } + + private fun addUserAccessControlOnDataset(dataset: Dataset, roleDefinition: RunnerAccessControl) { + val newDatasetAcl = dataset.getRbac().accessControlList + if (newDatasetAcl.none { it.id == roleDefinition.id }) { + datasetApiService.addOrUpdateAccessControl( + organization!!.id!!, dataset, roleDefinition.id, roleDefinition.role) + } + } }