Skip to content

Commit 3b1778f

Browse files
Leopold-CramerLeopold-Cramer
committed
feat: apply PR review comments
1 parent 994b193 commit 3b1778f

File tree

8 files changed

+123
-73
lines changed

8 files changed

+123
-73
lines changed

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,4 +43,12 @@ interface DatasetApiServiceInterface : DatasetApiService {
4343
identity: String,
4444
role: String
4545
): DatasetAccessControl
46+
47+
/**
48+
* Update the default security of the dataset passed in parameter
49+
* @param organizationId an organization id
50+
* @param dataset a dataset to update
51+
* @param role new dataset role
52+
*/
53+
fun updateDefaultSecurity(organizationId: String, dataset: Dataset, role: String)
4654
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,6 +1064,12 @@ class DatasetServiceImpl(
10641064
return dataset.security as DatasetSecurity
10651065
}
10661066

1067+
override fun updateDefaultSecurity(organizationId: String, dataset: Dataset, role: String) {
1068+
val rbacSecurity = csmRbac.setDefault(dataset.getRbac(), role)
1069+
dataset.setRbac(rbacSecurity)
1070+
datasetRepository.save(dataset)
1071+
}
1072+
10671073
override fun addDatasetAccessControl(
10681074
organizationId: String,
10691075
datasetId: String,

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

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import com.cosmotech.api.utils.getCurrentAuthenticatedUserName
2020
import com.cosmotech.connector.api.ConnectorApiService
2121
import com.cosmotech.connector.domain.Connector
2222
import com.cosmotech.connector.domain.IoTypesEnum
23-
import com.cosmotech.dataset.api.DatasetApiService
23+
import com.cosmotech.dataset.DatasetApiServiceInterface
2424
import com.cosmotech.dataset.domain.*
2525
import com.cosmotech.dataset.repository.DatasetRepository
2626
import com.cosmotech.organization.api.OrganizationApiService
@@ -39,6 +39,7 @@ import com.cosmotech.workspace.domain.WorkspaceSolution
3939
import com.ninjasquad.springmockk.SpykBean
4040
import com.redis.om.spring.RediSearchIndexer
4141
import com.redis.testcontainers.RedisStackContainer
42+
import io.mockk.clearAllMocks
4243
import io.mockk.every
4344
import io.mockk.junit5.MockKExtension
4445
import io.mockk.mockkStatic
@@ -80,7 +81,7 @@ class RunnerServiceIntegrationTest : CsmRedisTestBase() {
8081
@Autowired lateinit var rediSearchIndexer: RediSearchIndexer
8182
@Autowired lateinit var connectorApiService: ConnectorApiService
8283
@Autowired lateinit var organizationApiService: OrganizationApiService
83-
@SpykBean @Autowired lateinit var datasetApiService: DatasetApiService
84+
@SpykBean @Autowired lateinit var datasetApiService: DatasetApiServiceInterface
8485
@Autowired lateinit var datasetRepository: DatasetRepository
8586
@Autowired lateinit var solutionApiService: SolutionApiService
8687
@Autowired lateinit var workspaceApiService: WorkspaceApiService
@@ -128,6 +129,7 @@ class RunnerServiceIntegrationTest : CsmRedisTestBase() {
128129

129130
@BeforeEach
130131
fun setUp() {
132+
clearAllMocks()
131133
mockkStatic("com.cosmotech.api.utils.SecurityUtilsKt")
132134
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
133135
every { getCurrentAuthenticatedUserName(csmPlatformProperties) } returns "test.user"
@@ -1099,51 +1101,55 @@ class RunnerServiceIntegrationTest : CsmRedisTestBase() {
10991101
@Test
11001102
fun `when sharing a runner, the linked dataset default security should be set to at least viewer if the dataset isn't main`() {
11011103
runnerSaved.datasetList!!.removeLast()
1104+
val linkedDatasets = mutableListOf<Dataset>()
11021105
listOf(ROLE_NONE, ROLE_VIEWER, ROLE_USER, ROLE_EDITOR, ROLE_ADMIN).forEach { role ->
1103-
var linkedDataset =
1106+
linkedDatasets.add(
11041107
datasetApiService.createDataset(
1105-
organizationSaved.id!!, makeDataset(isMain = false, default = role))
1106-
runnerSaved.datasetList!!.add(linkedDataset.id!!)
1107-
linkedDataset =
1108+
organizationSaved.id!!, makeDataset(isMain = false, default = role)))
1109+
linkedDatasets.add(
11081110
datasetApiService.createDataset(
1109-
organizationSaved.id!!, makeDataset(isMain = true, default = role))
1110-
runnerSaved.datasetList!!.add(linkedDataset.id!!)
1111+
organizationSaved.id!!, makeDataset(isMain = true, default = role)))
11111112
}
1113+
linkedDatasets.forEach { runnerSaved.datasetList!!.add(it.id!!) }
11121114
runnerSaved =
11131115
runnerApiService.updateRunner(
11141116
organizationSaved.id!!, workspaceSaved.id!!, runnerSaved.id!!, runnerSaved)
1117+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
1118+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returnsMany
1119+
linkedDatasets
11151120

11161121
runnerApiService.setRunnerDefaultSecurity(
11171122
organizationSaved.id!!, workspaceSaved.id!!, runnerSaved.id!!, RunnerRole(ROLE_EDITOR))
11181123

1119-
verify(exactly = 1) {
1120-
datasetApiService.setDatasetDefaultSecurity(any(), any(), DatasetRole(ROLE_VIEWER))
1121-
}
1124+
// Only one result correspond, the dataset with default security none and main=false
1125+
verify(exactly = 1) { datasetApiService.updateDefaultSecurity(any(), any(), ROLE_VIEWER) }
11221126
}
11231127

11241128
@Test
11251129
fun `when stopping sharing a scenario, the dataset default security should be set to none it's not higher than viewer and the dataset isn't main`() {
11261130
runnerSaved.datasetList!!.removeLast()
1131+
val linkedDatasets = mutableListOf<Dataset>()
11271132
listOf(ROLE_NONE, ROLE_VIEWER, ROLE_USER, ROLE_EDITOR, ROLE_ADMIN).forEach { role ->
1128-
var linkedDataset =
1133+
linkedDatasets.add(
11291134
datasetApiService.createDataset(
1130-
organizationSaved.id!!, makeDataset(isMain = false, default = role))
1131-
runnerSaved.datasetList!!.add(linkedDataset.id!!)
1132-
linkedDataset =
1135+
organizationSaved.id!!, makeDataset(isMain = false, default = role)))
1136+
linkedDatasets.add(
11331137
datasetApiService.createDataset(
1134-
organizationSaved.id!!, makeDataset(isMain = true, default = role))
1135-
runnerSaved.datasetList!!.add(linkedDataset.id!!)
1138+
organizationSaved.id!!, makeDataset(isMain = true, default = role)))
11361139
}
1140+
linkedDatasets.forEach { runnerSaved.datasetList!!.add(it.id!!) }
11371141
runnerSaved =
11381142
runnerApiService.updateRunner(
11391143
organizationSaved.id!!, workspaceSaved.id!!, runnerSaved.id!!, runnerSaved)
1144+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
1145+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returnsMany
1146+
linkedDatasets
11401147

11411148
runnerApiService.setRunnerDefaultSecurity(
11421149
organizationSaved.id!!, workspaceSaved.id!!, runnerSaved.id!!, RunnerRole(ROLE_NONE))
11431150

1144-
verify(exactly = 1) {
1145-
datasetApiService.setDatasetDefaultSecurity(any(), any(), DatasetRole(ROLE_NONE))
1146-
}
1151+
// Only one result correspond, the dataset with default security viewer and main=false
1152+
verify(exactly = 1) { datasetApiService.updateDefaultSecurity(any(), any(), ROLE_NONE) }
11471153
}
11481154

11491155
private fun makeConnector(name: String = "name"): Connector {

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import com.cosmotech.api.utils.getCurrentAuthenticatedUserName
2525
import com.cosmotech.connector.api.ConnectorApiService
2626
import com.cosmotech.connector.domain.Connector
2727
import com.cosmotech.connector.domain.IoTypesEnum
28-
import com.cosmotech.dataset.api.DatasetApiService
28+
import com.cosmotech.dataset.DatasetApiServiceInterface
2929
import com.cosmotech.dataset.domain.Dataset
3030
import com.cosmotech.dataset.domain.DatasetAccessControl
3131
import com.cosmotech.dataset.domain.DatasetConnector
@@ -56,6 +56,7 @@ import com.cosmotech.workspace.domain.WorkspaceSolution
5656
import com.ninjasquad.springmockk.SpykBean
5757
import com.redis.om.spring.RediSearchIndexer
5858
import com.redis.testcontainers.RedisStackContainer
59+
import io.mockk.clearAllMocks
5960
import io.mockk.every
6061
import io.mockk.junit5.MockKExtension
6162
import io.mockk.mockk
@@ -96,7 +97,7 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
9697
@Autowired lateinit var rediSearchIndexer: RediSearchIndexer
9798
@Autowired lateinit var connectorApiService: ConnectorApiService
9899
@Autowired lateinit var organizationApiService: OrganizationApiService
99-
@SpykBean @Autowired lateinit var datasetApiService: DatasetApiService
100+
@SpykBean @Autowired lateinit var datasetApiService: DatasetApiServiceInterface
100101
@Autowired lateinit var solutionApiService: SolutionApiService
101102
@Autowired lateinit var workspaceApiService: WorkspaceApiService
102103
@Autowired lateinit var runnerApiService: RunnerApiService
@@ -106,6 +107,7 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
106107

107108
@BeforeEach
108109
fun setUp() {
110+
clearAllMocks()
109111
mockkStatic("com.cosmotech.api.utils.SecurityUtilsKt")
110112
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
111113
every { getCurrentAuthenticatedUserName(csmPlatformProperties) } returns "test.user"
@@ -2369,6 +2371,9 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
23692371
datasetRepository.save(
23702372
datasetSaved.apply { ingestionStatus = IngestionStatusEnum.SUCCESS })
23712373
materializeTwingraph(datasetSaved)
2374+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
2375+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returns
2376+
datasetSaved
23722377
val solution = makeSolution(organizationSaved.id!!, TEST_USER_MAIL, ROLE_ADMIN)
23732378
val solutionSaved =
23742379
solutionApiService.createSolution(organizationSaved.id!!, solution)
@@ -2440,6 +2445,9 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
24402445
datasetRepository.save(
24412446
datasetSaved.apply { ingestionStatus = IngestionStatusEnum.SUCCESS })
24422447
materializeTwingraph(datasetSaved)
2448+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
2449+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returns
2450+
datasetSaved
24432451
val solution = makeSolution(organizationSaved.id!!, TEST_USER_MAIL, ROLE_ADMIN)
24442452
val solutionSaved =
24452453
solutionApiService.createSolution(organizationSaved.id!!, solution)
@@ -2511,6 +2519,9 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
25112519
datasetRepository.save(
25122520
datasetSaved.apply { ingestionStatus = IngestionStatusEnum.SUCCESS })
25132521
materializeTwingraph(datasetSaved)
2522+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
2523+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returns
2524+
datasetSaved
25142525
val solution = makeSolution(organizationSaved.id!!, TEST_USER_MAIL, role)
25152526
val solutionSaved =
25162527
solutionApiService.createSolution(organizationSaved.id!!, solution)
@@ -2581,6 +2592,9 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
25812592
datasetSaved =
25822593
datasetRepository.save(
25832594
datasetSaved.apply { ingestionStatus = IngestionStatusEnum.SUCCESS })
2595+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
2596+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returns
2597+
datasetSaved
25842598
val solution = makeSolution(organizationSaved.id!!, TEST_USER_MAIL, ROLE_ADMIN)
25852599
val solutionSaved =
25862600
solutionApiService.createSolution(organizationSaved.id!!, solution)
@@ -2647,6 +2661,9 @@ class RunnerServiceRBACTest : CsmRedisTestBase() {
26472661
datasetSaved =
26482662
datasetRepository.save(
26492663
datasetSaved.apply { ingestionStatus = IngestionStatusEnum.SUCCESS })
2664+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
2665+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returns
2666+
datasetSaved
26502667
val solution = makeSolution(organizationSaved.id!!, TEST_USER_MAIL, ROLE_ADMIN)
26512668
val solutionSaved =
26522669
solutionApiService.createSolution(organizationSaved.id!!, solution)

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

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import com.cosmotech.api.utils.getCurrentAuthenticatedRoles
2727
import com.cosmotech.api.utils.getCurrentAuthenticatedUserName
2828
import com.cosmotech.dataset.DatasetApiServiceInterface
2929
import com.cosmotech.dataset.domain.Dataset
30-
import com.cosmotech.dataset.domain.DatasetRole
3130
import com.cosmotech.dataset.service.getRbac
3231
import com.cosmotech.organization.OrganizationApiServiceInterface
3332
import com.cosmotech.organization.domain.Organization
@@ -508,33 +507,29 @@ class RunnerService(
508507
// create a rbacSecurity object from runner Rbac by changing default value
509508
val rbacSecurity = csmRbac.setDefault(this.getRbacSecurity(), role, this.roleDefinition)
510509
this.setRbacSecurity(rbacSecurity)
511-
// this.runner.datasetList!!
512-
// .mapNotNull {
513-
// datasetApiService.findByOrganizationIdAndDatasetId(this.runner.organizationId!!,
514-
// it)
515-
// }
516-
// .forEach { dataset ->
517510
updateLinkedDatasetDefaultSecurity(role)
518-
// }
519511
}
520512

521513
private fun updateLinkedDatasetDefaultSecurity(role: String) {
522-
var datasetRole = ROLE_NONE
523-
if (role != ROLE_NONE) datasetRole = ROLE_VIEWER
514+
var datasetRole = ROLE_VIEWER
515+
if (role == ROLE_NONE) datasetRole = ROLE_NONE
524516
this.runner.datasetList!!.forEach { datasetId ->
525517
val linkedDataset =
526-
datasetApiService.findDatasetById(this.runner.organizationId!!, datasetId)
527-
// We do not want to lower the default security if it's higher than viewer
528-
if (linkedDataset.security!!.default != ROLE_NONE && datasetRole == ROLE_VIEWER)
529-
return@forEach Unit
530-
if (linkedDataset.security!!.default != ROLE_NONE && datasetRole == ROLE_NONE)
531-
return@forEach Unit
518+
datasetApiService.findByOrganizationIdAndDatasetId(
519+
this.runner.organizationId!!, datasetId)
520+
if (linkedDataset!!.main == true) return@forEach
521+
// If the dataset default security is different from none,
522+
// Then it's already viewer or higher and doesn't need to change
523+
if (datasetRole == ROLE_VIEWER && linkedDataset.security!!.default != ROLE_NONE)
524+
return@forEach
525+
// If the dataset default security is different from viewer,
526+
// Then it's either already none or higher than viewer and doesn't need to change
527+
if (datasetRole == ROLE_NONE && linkedDataset.security!!.default != ROLE_VIEWER)
528+
return@forEach
532529
// Filter on dataset copy (because we do not want to update main dataset as it can be shared
533-
// between scenarios)
534-
if (linkedDataset.main != true) {
535-
datasetApiService.setDatasetDefaultSecurity(
536-
this.runner.organizationId!!, datasetId, DatasetRole(datasetRole))
537-
}
530+
// between runners)
531+
datasetApiService.updateDefaultSecurity(
532+
this.runner.organizationId!!, linkedDataset, datasetRole)
538533
}
539534
}
540535
}

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

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import com.ninjasquad.springmockk.MockkBean
5050
import com.ninjasquad.springmockk.SpykBean
5151
import com.redis.om.spring.RediSearchIndexer
5252
import com.redis.testcontainers.RedisStackContainer
53+
import io.mockk.clearAllMocks
5354
import io.mockk.every
5455
import io.mockk.impl.annotations.MockK
5556
import io.mockk.junit5.MockKExtension
@@ -143,6 +144,7 @@ class ScenarioServiceIntegrationTest : CsmRedisTestBase() {
143144

144145
@BeforeEach
145146
fun setUp() {
147+
clearAllMocks()
146148
mockkStatic("com.cosmotech.api.utils.SecurityUtilsKt")
147149
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
148150
every { getCurrentAuthenticatedUserName(csmPlatformProperties) } returns "test.user"
@@ -1261,51 +1263,55 @@ class ScenarioServiceIntegrationTest : CsmRedisTestBase() {
12611263
@Test
12621264
fun `when sharing a scenario, the linked dataset default security should be set to at least viewer if the dataset isn't main`() {
12631265
scenarioSaved.datasetList!!.removeLast()
1266+
val linkedDatasets = mutableListOf<Dataset>()
12641267
listOf(ROLE_NONE, ROLE_VIEWER, ROLE_USER, ROLE_EDITOR, ROLE_ADMIN).forEach { role ->
1265-
var linkedDataset =
1268+
linkedDatasets.add(
12661269
datasetApiService.createDataset(
1267-
organizationSaved.id!!, makeDataset(isMain = false, default = role))
1268-
scenarioSaved.datasetList!!.add(linkedDataset.id!!)
1269-
linkedDataset =
1270+
organizationSaved.id!!, makeDataset(isMain = false, default = role)))
1271+
linkedDatasets.add(
12701272
datasetApiService.createDataset(
1271-
organizationSaved.id!!, makeDataset(isMain = true, default = role))
1272-
scenarioSaved.datasetList!!.add(linkedDataset.id!!)
1273+
organizationSaved.id!!, makeDataset(isMain = true, default = role)))
12731274
}
1275+
linkedDatasets.forEach { scenarioSaved.datasetList!!.add(it.id!!) }
12741276
scenarioSaved =
12751277
scenarioApiService.updateScenario(
12761278
organizationSaved.id!!, workspaceSaved.id!!, scenarioSaved.id!!, scenarioSaved)
1279+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
1280+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returnsMany
1281+
linkedDatasets
12771282

12781283
scenarioApiService.setScenarioDefaultSecurity(
12791284
organizationSaved.id!!, workspaceSaved.id!!, scenarioSaved.id!!, ScenarioRole(ROLE_EDITOR))
12801285

1281-
verify(exactly = 1) {
1282-
datasetApiService.setDatasetDefaultSecurity(any(), any(), DatasetRole(ROLE_VIEWER))
1283-
}
1286+
// Only one result correspond, the dataset with default security none and main=false
1287+
verify(exactly = 1) { datasetApiService.updateDefaultSecurity(any(), any(), ROLE_VIEWER) }
12841288
}
12851289

12861290
@Test
12871291
fun `when stopping sharing a scenario, the dataset default security should be set to none it's not higher than viewer and the dataset isn't main`() {
12881292
scenarioSaved.datasetList!!.removeLast()
1293+
val linkedDatasets = mutableListOf<Dataset>()
12891294
listOf(ROLE_NONE, ROLE_VIEWER, ROLE_USER, ROLE_EDITOR, ROLE_ADMIN).forEach { role ->
1290-
var linkedDataset =
1295+
linkedDatasets.add(
12911296
datasetApiService.createDataset(
1292-
organizationSaved.id!!, makeDataset(isMain = false, default = role))
1293-
scenarioSaved.datasetList!!.add(linkedDataset.id!!)
1294-
linkedDataset =
1297+
organizationSaved.id!!, makeDataset(isMain = false, default = role)))
1298+
linkedDatasets.add(
12951299
datasetApiService.createDataset(
1296-
organizationSaved.id!!, makeDataset(isMain = true, default = role))
1297-
scenarioSaved.datasetList!!.add(linkedDataset.id!!)
1300+
organizationSaved.id!!, makeDataset(isMain = true, default = role)))
12981301
}
1302+
linkedDatasets.forEach { scenarioSaved.datasetList!!.add(it.id!!) }
12991303
scenarioSaved =
13001304
scenarioApiService.updateScenario(
13011305
organizationSaved.id!!, workspaceSaved.id!!, scenarioSaved.id!!, scenarioSaved)
1306+
every { datasetApiService.updateDefaultSecurity(any(), any(), any()) } returns Unit
1307+
every { datasetApiService.findByOrganizationIdAndDatasetId(any(), any()) } returnsMany
1308+
linkedDatasets
13021309

13031310
scenarioApiService.setScenarioDefaultSecurity(
13041311
organizationSaved.id!!, workspaceSaved.id!!, scenarioSaved.id!!, ScenarioRole(ROLE_NONE))
13051312

1306-
verify(exactly = 1) {
1307-
datasetApiService.setDatasetDefaultSecurity(any(), any(), DatasetRole(ROLE_NONE))
1308-
}
1313+
// Only one result correspond, the dataset with default security viewer and main=false
1314+
verify(exactly = 1) { datasetApiService.updateDefaultSecurity(any(), any(), ROLE_NONE) }
13091315
}
13101316

13111317
private fun makeWorkspaceEventHubInfo(eventHubAvailable: Boolean): WorkspaceEventHubInfo {

0 commit comments

Comments
 (0)