Skip to content

Commit 7e7a2c0

Browse files
Leopold-CramerLeopold-Cramer
committed
add PR review feedbacks
1 parent 36de462 commit 7e7a2c0

File tree

11 files changed

+216
-229
lines changed

11 files changed

+216
-229
lines changed

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

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1020,42 +1020,22 @@ class DatasetServiceIntegrationTest : CsmRedisTestBase() {
10201020
}
10211021

10221022
@Test
1023-
fun `viewerRole has limited vision on security`() {
1023+
fun `As viewer, I can only see my information in security property for findDatasetById`() {
10241024
dataset = makeDatasetWithRole(role = ROLE_VIEWER)
1025-
10261025
datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
1027-
assertEquals(
1028-
DatasetSecurity(
1029-
default = ROLE_NONE, mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
1030-
datasetSaved.security)
10311026

10321027
datasetSaved = datasetApiService.findDatasetById(organizationSaved.id!!, datasetSaved.id!!)
10331028
assertEquals(
10341029
DatasetSecurity(
10351030
default = ROLE_NONE, mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
10361031
datasetSaved.security)
1032+
assertEquals(1, datasetSaved.security!!.accessControlList.size)
1033+
}
10371034

1038-
datasetSaved = datasetApiService.getVerifiedDataset(organizationSaved.id!!, datasetSaved.id!!)
1039-
assertEquals(
1040-
DatasetSecurity(
1041-
default = ROLE_NONE, mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
1042-
datasetSaved.security)
1043-
1044-
datasetSaved =
1045-
datasetApiService.linkWorkspace(
1046-
organizationSaved.id!!, datasetSaved.id!!, workspaceSaved.id!!)
1047-
assertEquals(
1048-
DatasetSecurity(
1049-
default = ROLE_NONE, mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
1050-
datasetSaved.security)
1051-
1052-
datasetSaved =
1053-
datasetApiService.unlinkWorkspace(
1054-
organizationSaved.id!!, datasetSaved.id!!, workspaceSaved.id!!)
1055-
assertEquals(
1056-
DatasetSecurity(
1057-
default = ROLE_NONE, mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
1058-
datasetSaved.security)
1035+
@Test
1036+
fun `As viewer, I can only see my information in security property for findByOrganizationIdAndDatasetId`() {
1037+
dataset = makeDatasetWithRole(role = ROLE_VIEWER)
1038+
datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
10591039

10601040
datasetSaved =
10611041
datasetApiService.findByOrganizationIdAndDatasetId(
@@ -1064,25 +1044,46 @@ class DatasetServiceIntegrationTest : CsmRedisTestBase() {
10641044
DatasetSecurity(
10651045
default = ROLE_NONE, mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
10661046
datasetSaved.security)
1047+
assertEquals(1, datasetSaved.security!!.accessControlList.size)
1048+
}
10671049

1068-
var datasets = datasetApiService.findAllDatasets(organizationSaved.id!!, 0, 10)
1050+
@Test
1051+
fun `As viewer, I can only see my information in security property for findAllDatasets`() {
1052+
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
1053+
datasetApiService.deleteDataset(organizationSaved.id!!, datasetSaved.id!!)
1054+
every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
1055+
dataset = makeDatasetWithRole(role = ROLE_VIEWER)
1056+
datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
1057+
1058+
val datasets = datasetApiService.findAllDatasets(organizationSaved.id!!, null, null)
10691059
datasets.forEach {
10701060
assertEquals(
10711061
DatasetSecurity(
10721062
default = ROLE_NONE,
10731063
mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
1074-
datasetSaved.security)
1064+
it.security)
1065+
assertEquals(1, it.security!!.accessControlList.size)
10751066
}
1067+
}
1068+
1069+
@Test
1070+
fun `As viewer, I can only see my information in security property for searchDatasets`() {
1071+
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
1072+
datasetApiService.deleteDataset(organizationSaved.id!!, datasetSaved.id!!)
1073+
every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
1074+
dataset = makeDatasetWithRole(role = ROLE_VIEWER)
1075+
datasetSaved = datasetApiService.createDataset(organizationSaved.id!!, dataset)
10761076

1077-
datasets =
1077+
val datasets =
10781078
datasetApiService.searchDatasets(
10791079
organizationSaved.id!!, DatasetSearch(mutableListOf("dataset")), 0, 10)
10801080
datasets.forEach {
10811081
assertEquals(
10821082
DatasetSecurity(
10831083
default = ROLE_NONE,
10841084
mutableListOf(DatasetAccessControl(TEST_USER_MAIL, ROLE_VIEWER))),
1085-
datasetSaved.security)
1085+
it.security)
1086+
assertEquals(1, it.security!!.accessControlList.size)
10861087
}
10871088
}
10881089

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

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import com.cosmotech.api.rbac.PERMISSION_READ_SECURITY
2626
import com.cosmotech.api.rbac.PERMISSION_WRITE
2727
import com.cosmotech.api.rbac.PERMISSION_WRITE_SECURITY
2828
import com.cosmotech.api.rbac.ROLE_NONE
29-
import com.cosmotech.api.rbac.ROLE_VIEWER
3029
import com.cosmotech.api.rbac.model.RbacAccessControl
3130
import com.cosmotech.api.rbac.model.RbacSecurity
3231
import com.cosmotech.api.security.ROLE_PLATFORM_ADMIN
@@ -168,12 +167,12 @@ class DatasetServiceImpl(
168167
datasetRepository.findAll(pageable).toList()
169168
}
170169
}
171-
result.forEach { checkReadSecurity(it) }
170+
result.forEach { it.security = updateSecurityVisibility(it).security }
172171
return result
173172
}
174173

175174
override fun findDatasetById(organizationId: String, datasetId: String): Dataset {
176-
return getVerifiedDataset(organizationId, datasetId)
175+
return updateSecurityVisibility(getVerifiedDataset(organizationId, datasetId))
177176
}
178177

179178
override fun removeAllDatasetCompatibilityElements(organizationId: String, datasetId: String) {
@@ -238,7 +237,7 @@ class DatasetServiceImpl(
238237
version = existingConnector.version
239238
}
240239
}
241-
return checkReadSecurity(datasetRepository.save(createdDataset))
240+
return datasetRepository.save(createdDataset)
242241
}
243242

244243
override fun createSubDataset(
@@ -301,7 +300,7 @@ class DatasetServiceImpl(
301300
})
302301
}
303302
}
304-
return checkReadSecurity(datasetSaved)
303+
return datasetSaved
305304
}
306305

307306
private fun checkIfGraphFunctionalityIsAvailable() {
@@ -674,7 +673,7 @@ class DatasetServiceImpl(
674673
organizationService.getVerifiedOrganization(organizationId)
675674
var dataset = datasetRepository.findBy(organizationId, datasetId).getOrNull()
676675
if (dataset != null) {
677-
dataset = checkReadSecurity(dataset)
676+
dataset = updateSecurityVisibility(dataset)
678677
}
679678
return dataset
680679
}
@@ -869,12 +868,15 @@ class DatasetServiceImpl(
869868
datasetId: String,
870869
workspaceId: String
871870
): Dataset {
871+
this.getVerifiedDataset(organizationId, datasetId, PERMISSION_WRITE)
872872
sendAddDatasetToWorkspaceEvent(organizationId, workspaceId, datasetId)
873873
return addWorkspaceToLinkedWorkspaceIdList(organizationId, datasetId, workspaceId)
874874
}
875875

876876
@EventListener(AddWorkspaceToDataset::class)
877877
fun processEventAddWorkspace(addWorkspaceToDataset: AddWorkspaceToDataset) {
878+
this.getVerifiedDataset(
879+
addWorkspaceToDataset.organizationId, addWorkspaceToDataset.datasetId, PERMISSION_WRITE)
878880
addWorkspaceToLinkedWorkspaceIdList(
879881
addWorkspaceToDataset.organizationId,
880882
addWorkspaceToDataset.datasetId,
@@ -905,14 +907,17 @@ class DatasetServiceImpl(
905907
datasetId: String,
906908
workspaceId: String
907909
): Dataset {
908-
910+
this.getVerifiedDataset(organizationId, datasetId, PERMISSION_WRITE)
909911
sendRemoveDatasetFromWorkspaceEvent(organizationId, workspaceId, datasetId)
910-
911912
return removeWorkspaceFromLinkedWorkspaceIdList(organizationId, datasetId, workspaceId)
912913
}
913914

914915
@EventListener(RemoveWorkspaceFromDataset::class)
915916
fun processEventRemoveWorkspace(removeWorkspaceFromDataset: RemoveWorkspaceFromDataset) {
917+
this.getVerifiedDataset(
918+
removeWorkspaceFromDataset.organizationId,
919+
removeWorkspaceFromDataset.datasetId,
920+
PERMISSION_WRITE)
916921
removeWorkspaceFromLinkedWorkspaceIdList(
917922
removeWorkspaceFromDataset.organizationId,
918923
removeWorkspaceFromDataset.datasetId,
@@ -1045,7 +1050,7 @@ class DatasetServiceImpl(
10451050
.findDatasetByTags(organizationId, datasetSearch.datasetTags.toSet(), it)
10461051
.toList()
10471052
}
1048-
datasetList.forEach { checkReadSecurity(it) }
1053+
datasetList.forEach { it.security = updateSecurityVisibility(it).security }
10491054
return datasetList
10501055
}
10511056

@@ -1217,6 +1222,7 @@ class DatasetServiceImpl(
12171222
}
12181223
}
12191224
}
1225+
12201226
private fun sendTwingraphImportJobInfoRequestEvent(
12211227
dataset: Dataset,
12221228
organizationId: String
@@ -1276,37 +1282,31 @@ class DatasetServiceImpl(
12761282
requiredPermission: String
12771283
): Dataset {
12781284
organizationService.getVerifiedOrganization(organizationId)
1279-
val dataset = findBy(organizationId, datasetId)
1280-
csmRbac.verify(dataset.getRbac(), requiredPermission)
1281-
return dataset
1282-
}
1283-
1284-
fun findBy(organizationId: String, datasetId: String): Dataset {
1285-
var dataset =
1285+
val dataset =
12861286
datasetRepository.findBy(organizationId, datasetId).orElseThrow {
12871287
CsmResourceNotFoundException(
12881288
"Dataset $datasetId not found in organization $organizationId")
12891289
}
1290-
dataset = checkReadSecurity(dataset)
1290+
csmRbac.verify(dataset.getRbac(), requiredPermission)
12911291
return dataset
12921292
}
12931293

1294-
fun checkReadSecurity(dataset: Dataset): Dataset {
1295-
val username = getCurrentAccountIdentifier(csmPlatformProperties)
1296-
val retrievedAC = dataset.security!!.accessControlList.firstOrNull { it.id == username }
1297-
if (retrievedAC != null) {
1298-
if (retrievedAC.role == ROLE_VIEWER) {
1294+
fun updateSecurityVisibility(dataset: Dataset): Dataset {
1295+
if (csmRbac.check(dataset.getRbac(), PERMISSION_READ_SECURITY).not()) {
1296+
val username = getCurrentAccountIdentifier(csmPlatformProperties)
1297+
val retrievedAC = dataset.security!!.accessControlList.firstOrNull { it.id == username }
1298+
if (retrievedAC != null) {
12991299
return dataset.copy(
13001300
security =
13011301
DatasetSecurity(
13021302
default = dataset.security!!.default,
13031303
accessControlList = mutableListOf(retrievedAC)))
1304+
} else {
1305+
return dataset.copy(
1306+
security =
1307+
DatasetSecurity(
1308+
default = dataset.security!!.default, accessControlList = mutableListOf()))
13041309
}
1305-
} else if (dataset.security!!.default == ROLE_VIEWER) {
1306-
return dataset.copy(
1307-
security =
1308-
DatasetSecurity(
1309-
default = dataset.security!!.default, accessControlList = mutableListOf()))
13101310
}
13111311
return dataset
13121312
}

‎organization/src/integrationTest/kotlin/com/cosmotech/organization/service/OrganizationServiceIntegrationTest.kt‎

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,37 +1059,32 @@ class OrganizationServiceIntegrationTest : CsmRedisTestBase() {
10591059
}
10601060

10611061
@Test
1062-
fun `viewerRole has limited vision on security`() {
1062+
fun `As a viewer, I can only see my information in security property for findOrganizationById`() {
10631063
val organization = makeOrganization(role = ROLE_VIEWER)
1064-
10651064
var organizationSaved = organizationApiService.registerOrganization(organization)
1066-
assertEquals(
1067-
OrganizationSecurity(
1068-
default = ROLE_NONE,
1069-
mutableListOf(OrganizationAccessControl(TEST_USER_ID, ROLE_VIEWER))),
1070-
organizationSaved.security)
10711065

10721066
organizationSaved = organizationApiService.findOrganizationById(organizationSaved.id!!)
10731067
assertEquals(
10741068
OrganizationSecurity(
10751069
default = ROLE_NONE,
10761070
mutableListOf(OrganizationAccessControl(TEST_USER_ID, ROLE_VIEWER))),
10771071
organizationSaved.security)
1072+
assertEquals(1, organizationSaved.security!!.accessControlList.size)
1073+
}
10781074

1079-
organizationSaved = organizationApiService.getVerifiedOrganization(organizationSaved.id!!)
1080-
assertEquals(
1081-
OrganizationSecurity(
1082-
default = ROLE_NONE,
1083-
mutableListOf(OrganizationAccessControl(TEST_USER_ID, ROLE_VIEWER))),
1084-
organizationSaved.security)
1075+
@Test
1076+
fun `As a viewer, I can only see my information in security property for findAllOrganizations`() {
1077+
val organization = makeOrganization(role = ROLE_VIEWER)
1078+
organizationApiService.registerOrganization(organization)
10851079

1086-
var organizations = organizationApiService.findAllOrganizations(0, 10)
1080+
val organizations = organizationApiService.findAllOrganizations(null, null)
10871081
organizations.forEach {
10881082
assertEquals(
10891083
OrganizationSecurity(
10901084
default = ROLE_NONE,
10911085
mutableListOf(OrganizationAccessControl(TEST_USER_ID, ROLE_VIEWER))),
1092-
organizationSaved.security)
1086+
it.security)
1087+
assertEquals(1, it.security!!.accessControlList.size)
10931088
}
10941089
}
10951090
}

‎organization/src/main/kotlin/com/cosmotech/organization/service/OrganizationServiceImpl.kt‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import com.cosmotech.api.rbac.PERMISSION_READ_SECURITY
1313
import com.cosmotech.api.rbac.PERMISSION_WRITE
1414
import com.cosmotech.api.rbac.PERMISSION_WRITE_SECURITY
1515
import com.cosmotech.api.rbac.ROLE_NONE
16-
import com.cosmotech.api.rbac.ROLE_VIEWER
1716
import com.cosmotech.api.rbac.getAllRolesDefinition
1817
import com.cosmotech.api.rbac.getCommonRolesDefinition
1918
import com.cosmotech.api.rbac.model.RbacAccessControl
@@ -68,12 +67,12 @@ class OrganizationServiceImpl(
6867
organizationRepository.findAll(pageable).toList()
6968
}
7069
}
71-
result.forEach { checkReadSecurity(it) }
70+
result.forEach { it.security = updateSecurityVisibility(it).security }
7271
return result
7372
}
7473

7574
override fun findOrganizationById(organizationId: String): Organization {
76-
return getVerifiedOrganization(organizationId, PERMISSION_READ)
75+
return updateSecurityVisibility(getVerifiedOrganization(organizationId, PERMISSION_READ))
7776
}
7877

7978
override fun registerOrganization(organization: Organization): Organization {
@@ -89,7 +88,7 @@ class OrganizationServiceImpl(
8988
ownerId = getCurrentAuthenticatedUserName(csmPlatformProperties))
9089
createdOrganization.setRbac(csmRbac.initSecurity(organization.getRbac()))
9190

92-
return checkReadSecurity(organizationRepository.save(createdOrganization))
91+
return organizationRepository.save(createdOrganization)
9392
}
9493

9594
override fun unregisterOrganization(organizationId: String) {
@@ -117,9 +116,9 @@ class OrganizationServiceImpl(
117116
}
118117

119118
return if (hasChanged) {
120-
checkReadSecurity(organizationRepository.save(existingOrganization))
119+
organizationRepository.save(existingOrganization)
121120
} else {
122-
checkReadSecurity(existingOrganization)
121+
existingOrganization
123122
}
124123
}
125124

@@ -217,7 +216,7 @@ class OrganizationServiceImpl(
217216
organizationRepository.findByIdOrNull(organizationId)
218217
?: throw CsmResourceNotFoundException("Organization $organizationId does not exist!")
219218
csmRbac.verify(organization.getRbac(), requiredPermission)
220-
return checkReadSecurity(organization)
219+
return organization
221220
}
222221

223222
override fun getVerifiedOrganization(
@@ -229,22 +228,22 @@ class OrganizationServiceImpl(
229228
return organization
230229
}
231230

232-
fun checkReadSecurity(organization: Organization): Organization {
233-
val username = getCurrentAccountIdentifier(csmPlatformProperties)
234-
val retrievedAC = organization.security!!.accessControlList.firstOrNull { it.id == username }
235-
if (retrievedAC != null) {
236-
if (retrievedAC.role == ROLE_VIEWER) {
237-
return organization.copy(
231+
fun updateSecurityVisibility(organization: Organization): Organization {
232+
if (csmRbac.check(organization.getRbac(), PERMISSION_READ_SECURITY).not()) {
233+
val username = getCurrentAccountIdentifier(csmPlatformProperties)
234+
val retrievedAC = organization.security!!.accessControlList.firstOrNull { it.id == username }
235+
return if (retrievedAC != null) {
236+
organization.copy(
238237
security =
239238
OrganizationSecurity(
240239
default = organization.security!!.default,
241240
accessControlList = mutableListOf(retrievedAC)))
241+
} else {
242+
organization.copy(
243+
security =
244+
OrganizationSecurity(
245+
default = organization.security!!.default, accessControlList = mutableListOf()))
242246
}
243-
} else if (organization.security!!.default == ROLE_VIEWER) {
244-
return organization.copy(
245-
security =
246-
OrganizationSecurity(
247-
default = organization.security!!.default, accessControlList = mutableListOf()))
248247
}
249248
return organization
250249
}

0 commit comments

Comments
 (0)