Skip to content

Commit d031573

Browse files
jreynard-codejreynard-code
committed
[RMZ-413] upgrade runner ACL lead to server error on ACL dataset management
1 parent d74c76d commit d031573

File tree

4 files changed

+115
-58
lines changed

4 files changed

+115
-58
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -435,7 +435,7 @@ class DatasetServiceImplTests {
435435
val twinGraphQuery = DatasetTwinGraphQuery("MATCH(n) RETURN n")
436436
datasetService.twingraphQuery(ORGANIZATION_ID, DATASET_ID, twinGraphQuery)
437437

438-
verify { csmRedisGraph.query(any(), any(), any<Long>()) }
438+
verify { csmRedisGraph.readOnlyQuery(any(), any(), any<Long>()) }
439439
}
440440

441441
@Test

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -360,17 +360,17 @@ class RunnerServiceIntegrationTest : CsmRedisTestBase() {
360360
assertEquals(ROLE_EDITOR, runnerAccessControlRegistered.role)
361361

362362
logger.info(
363-
"should update the Access Control and assert it has been updated in the linked datasets")
363+
"should let the Access Control as-is cause dataset ACL already contains info for this user")
364364
runnerSaved.datasetList!!.forEach {
365365
assertEquals(
366-
ROLE_EDITOR,
366+
ROLE_VIEWER,
367367
datasetApiService
368368
.getDatasetAccessControl(organizationSaved.id!!, it, TEST_USER_MAIL)
369369
.role)
370370
}
371371

372372
logger.info("should get the list of users and assert there are 2")
373-
var userList =
373+
val userList =
374374
runnerApiService.getRunnerSecurityUsers(
375375
organizationSaved.id!!, workspaceSaved.id!!, runnerSaved.id!!)
376376
assertEquals(3, userList.size)

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

Lines changed: 87 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ import io.mockk.junit5.MockKExtension
6262
import io.mockk.mockk
6363
import io.mockk.mockkStatic
6464
import java.util.*
65+
import kotlin.test.Test
6566
import kotlin.test.assertEquals
6667
import org.junit.jupiter.api.BeforeEach
6768
import org.junit.jupiter.api.DynamicTest.dynamicTest
@@ -2777,15 +2778,15 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
27772778

27782779
@TestFactory
27792780
fun `test Dataset RBAC addRunnerAccessControl`() =
2780-
mapOf(
2781-
ROLE_VIEWER to true,
2782-
ROLE_EDITOR to true,
2783-
ROLE_VALIDATOR to true,
2784-
ROLE_USER to true,
2785-
ROLE_NONE to true,
2786-
ROLE_ADMIN to false,
2781+
listOf(
2782+
ROLE_VIEWER,
2783+
ROLE_EDITOR,
2784+
ROLE_VALIDATOR,
2785+
ROLE_USER,
2786+
ROLE_NONE,
2787+
ROLE_ADMIN,
27872788
)
2788-
.map { (role, shouldThrow) ->
2789+
.map { role ->
27892790
dynamicTest("Test Dataset RBAC addRunnerAccessControl : $role") {
27902791
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
27912792
val connector = makeConnector()
@@ -2820,32 +2821,90 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
28202821
role = ROLE_ADMIN)
28212822
val runnerSaved =
28222823
runnerApiService.createRunner(organizationSaved.id!!, workspaceSaved.id!!, runner)
2823-
every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
28242824

2825-
if (shouldThrow) {
2826-
val exception =
2827-
assertThrows<CsmAccessForbiddenException> {
2828-
runnerApiService.addRunnerAccessControl(
2829-
organizationSaved.id!!,
2830-
workspaceSaved.id!!,
2831-
runnerSaved.id!!,
2832-
RunnerAccessControl("id", ROLE_ADMIN))
2833-
}
2825+
assertDoesNotThrow {
28342826
assertEquals(
2835-
"RBAC ${datasetSaved.id!!} - User does not have permission $PERMISSION_WRITE_SECURITY",
2836-
exception.message)
2837-
} else {
2838-
assertDoesNotThrow {
2839-
runnerApiService.addRunnerAccessControl(
2840-
organizationSaved.id!!,
2841-
workspaceSaved.id!!,
2842-
runnerSaved.id!!,
2843-
RunnerAccessControl("id", ROLE_ADMIN))
2844-
}
2827+
true,
2828+
datasetSaved.security
2829+
?.accessControlList
2830+
?.filter { datasetAccessControl ->
2831+
datasetAccessControl.id == TEST_USER_MAIL
2832+
}
2833+
?.any { datasetAccessControl -> datasetAccessControl.role == role })
2834+
2835+
runnerApiService.addRunnerAccessControl(
2836+
organizationSaved.id!!,
2837+
workspaceSaved.id!!,
2838+
runnerSaved.id!!,
2839+
RunnerAccessControl(TEST_USER_MAIL, ROLE_ADMIN))
2840+
2841+
val datasetWithUpgradedACL =
2842+
datasetApiService.findDatasetById(organizationSaved.id!!, datasetSaved.id!!)
2843+
assertEquals(
2844+
true,
2845+
datasetWithUpgradedACL.security
2846+
?.accessControlList
2847+
?.filter { datasetAccessControl ->
2848+
datasetAccessControl.id == TEST_USER_MAIL
2849+
}
2850+
?.any { datasetAccessControl -> datasetAccessControl.role == role })
28452851
}
28462852
}
28472853
}
28482854

2855+
@Test
2856+
fun `test Dataset RBAC addRunnerAccessControl with new ACL entry`() {
2857+
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
2858+
val connector = makeConnector()
2859+
val connectorSaved = connectorApiService.registerConnector(connector)
2860+
val organization = makeOrganizationWithRole(id = TEST_USER_MAIL, role = ROLE_ADMIN)
2861+
val organizationSaved = organizationApiService.registerOrganization(organization)
2862+
val dataset =
2863+
makeDataset(
2864+
organizationSaved.id!!, connectorSaved, id = "[email protected]", role = ROLE_NONE)
2865+
var datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
2866+
datasetSaved =
2867+
datasetRepository.save(
2868+
datasetSaved.apply { ingestionStatus = Dataset.IngestionStatus.SUCCESS })
2869+
every { datasetApiService.createSubDataset(any(), any(), any()) } returns datasetSaved
2870+
2871+
assertEquals(
2872+
false,
2873+
datasetSaved.security?.accessControlList?.any { datasetAccessControl ->
2874+
datasetAccessControl.id == TEST_USER_MAIL
2875+
})
2876+
2877+
val solution = makeSolution(organizationSaved.id!!, TEST_USER_MAIL, ROLE_ADMIN)
2878+
val solutionSaved = solutionApiService.createSolution(organizationSaved.id!!, solution)
2879+
val workspace =
2880+
makeWorkspaceWithRole(
2881+
organizationSaved.id!!, solutionSaved.id!!, id = TEST_USER_MAIL, role = ROLE_ADMIN)
2882+
val workspaceSaved = workspaceApiService.createWorkspace(organizationSaved.id!!, workspace)
2883+
val runner =
2884+
makeRunnerWithRole(
2885+
organizationSaved.id!!,
2886+
workspaceSaved.id!!,
2887+
solutionSaved.id!!,
2888+
mutableListOf(datasetSaved.id!!),
2889+
id = "[email protected]",
2890+
role = ROLE_ADMIN)
2891+
val runnerSaved =
2892+
runnerApiService.createRunner(organizationSaved.id!!, workspaceSaved.id!!, runner)
2893+
2894+
runnerApiService.addRunnerAccessControl(
2895+
organizationSaved.id!!,
2896+
workspaceSaved.id!!,
2897+
runnerSaved.id!!,
2898+
RunnerAccessControl(TEST_USER_MAIL, ROLE_ADMIN))
2899+
val datasetWithUpgradedACL =
2900+
datasetApiService.findDatasetById(organizationSaved.id!!, datasetSaved.id!!)
2901+
assertEquals(
2902+
true,
2903+
datasetWithUpgradedACL.security?.accessControlList?.any { datasetAccessControl ->
2904+
datasetAccessControl.id == TEST_USER_MAIL
2905+
})
2906+
}
2907+
28492908
@TestFactory
28502909
fun `test Solution RBAC addRunnerAccessControl`() =
28512910
mapOf(

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

Lines changed: 24 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ import com.cosmotech.api.utils.compareToAndMutateIfNeeded
2121
import com.cosmotech.api.utils.getCurrentAccountIdentifier
2222
import com.cosmotech.api.utils.getCurrentAuthenticatedUserName
2323
import com.cosmotech.dataset.DatasetApiServiceInterface
24-
import com.cosmotech.dataset.domain.DatasetAccessControl
25-
import com.cosmotech.dataset.domain.DatasetRole
2624
import com.cosmotech.dataset.service.getRbac
2725
import com.cosmotech.organization.OrganizationApiServiceInterface
2826
import com.cosmotech.organization.domain.Organization
@@ -178,9 +176,16 @@ class RunnerService(
178176
// take newly added datasets and propagate existing ACL on it
179177
this.runner.datasetList
180178
?.filterNot { beforeMutateDatasetList?.contains(it) ?: false }
181-
?.forEach { newDatasetId ->
182-
this.runner.security?.accessControlList?.forEach {
183-
this.propagateAccessControlToDataset(newDatasetId, it.id, it.role)
179+
?.mapNotNull {
180+
datasetApiService.findByOrganizationIdAndDatasetId(organization!!.id!!, it)
181+
}
182+
?.forEach { newDataset ->
183+
this.runner.security?.accessControlList?.forEach { roleDefinition ->
184+
val newDatasetAcl = newDataset.getRbac().accessControlList
185+
if (newDatasetAcl.none { it.id == roleDefinition.id }) {
186+
datasetApiService.addOrUpdateAccessControl(
187+
organization!!.id!!, newDataset, roleDefinition.id, roleDefinition.role)
188+
}
184189
}
185190
}
186191
}
@@ -213,7 +218,20 @@ class RunnerService(
213218
this.roleDefinition)
214219
this.setRbacSecurity(rbacSecurity)
215220

216-
this.propagateAccessControlToDatasets(runnerAccessControl.id, runnerAccessControl.role)
221+
val userId = runnerAccessControl.id
222+
val datasetRole = runnerAccessControl.role.takeUnless { it == ROLE_VALIDATOR } ?: ROLE_USER
223+
val organizationId = this.runner.organizationId!!
224+
225+
// Assign roles on linked datasets if not already present on dataset resource
226+
this.runner.datasetList!!
227+
.mapNotNull { datasetApiService.findByOrganizationIdAndDatasetId(organizationId, it) }
228+
.forEach { dataset ->
229+
val datasetAcl = dataset.getRbac().accessControlList
230+
if (datasetAcl.none { it.id == userId }) {
231+
datasetApiService.addOrUpdateAccessControl(
232+
organizationId, dataset, userId, datasetRole)
233+
}
234+
}
217235
}
218236

219237
fun getAccessControlFor(userId: String): RunnerAccessControl {
@@ -257,26 +275,6 @@ class RunnerService(
257275
?: mutableListOf())
258276
}
259277

260-
private fun propagateAccessControlToDatasets(userId: String, role: String) {
261-
this.runner.datasetList!!.forEach { datasetId ->
262-
propagateAccessControlToDataset(datasetId, userId, role)
263-
}
264-
}
265-
266-
private fun propagateAccessControlToDataset(datasetId: String, userId: String, role: String) {
267-
val datasetRole = role.takeUnless { it == ROLE_VALIDATOR } ?: ROLE_USER
268-
val organizationId = this.runner.organizationId!!
269-
270-
val datasetUsers = datasetApiService.getDatasetSecurityUsers(organizationId, datasetId)
271-
if (datasetUsers.contains(userId)) {
272-
datasetApiService.updateDatasetAccessControl(
273-
organizationId, datasetId, userId, DatasetRole(datasetRole))
274-
} else {
275-
datasetApiService.addDatasetAccessControl(
276-
organizationId, datasetId, DatasetAccessControl(userId, datasetRole))
277-
}
278-
}
279-
280278
private fun removeAccessControlToDatasets(userId: String) {
281279
val organizationId = this.runner.organizationId!!
282280
this.runner.datasetList!!.forEach { datasetId ->

0 commit comments

Comments
 (0)