Skip to content

Commit 60962b9

Browse files
jreynard-codejreynard-code
committed
Refactor solution and workspace file management handling
- Replaced `deleteAllS3WorkspaceObjects` and `deleteAllS3SolutionObjects` with `deleteWorkspaceFiles` and `deleteSolutionFiles` respectively - Enhanced error handling during S3 file deletions with specific exceptions. - Expanded supported MIME types for solution and workspace file uploads. - Added RBAC tests for `listSolutionFiles` functionality with comprehensive role validation. - Updated integration tests for file deletion operations in solution and workspace services.
1 parent 576d8b8 commit 60962b9

File tree

8 files changed

+161
-45
lines changed

8 files changed

+161
-45
lines changed

‎solution/src/integrationTest/kotlin/com/cosmotech/solution/service/SolutionServiceIntegrationTest.kt‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2225,10 +2225,18 @@ class SolutionServiceIntegrationTest : CsmTestBase() {
22252225
solutionApiService.createSolutionFile(
22262226
organizationSaved.id, solutionSaved.id, multipartFile, true, null)
22272227

2228-
solutionApiService.deleteAllS3SolutionObjects(organizationSaved.id, solutionSaved)
2229-
2230-
assertEquals(
2231-
0, solutionApiService.listSolutionFiles(organizationSaved.id, solutionSaved.id).size)
2228+
solutionApiService.deleteSolutionFiles(organizationSaved.id, solutionSaved.id)
2229+
2230+
var numberOfTry = 0
2231+
var solutionFilesIsEmpty: Boolean
2232+
do {
2233+
solutionFilesIsEmpty =
2234+
solutionApiService.listSolutionFiles(organizationSaved.id, solutionSaved.id).isEmpty()
2235+
numberOfTry++
2236+
Thread.sleep(50)
2237+
} while (numberOfTry < 100 && !solutionFilesIsEmpty)
2238+
2239+
assertTrue(solutionFilesIsEmpty)
22322240
}
22332241

22342242
@Test

‎solution/src/integrationTest/kotlin/com/cosmotech/solution/service/SolutionServiceRBACTest.kt‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,99 @@ class SolutionServiceRBACTest : CsmTestBase() {
694694
}
695695
}
696696

697+
@TestFactory
698+
fun `test Organization RBAC listSolutionFiles`() =
699+
mapOf(
700+
ROLE_VIEWER to false,
701+
ROLE_EDITOR to false,
702+
ROLE_USER to false,
703+
ROLE_NONE to true,
704+
ROLE_ADMIN to false,
705+
)
706+
.map { (role, shouldThrow) ->
707+
DynamicTest.dynamicTest("Test Organization RBAC listSolutionFiles : $role") {
708+
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
709+
710+
val organization = makeOrganizationCreateRequest(id = TEST_USER_MAIL, role = role)
711+
organizationSaved = organizationApiService.createOrganization(organization)
712+
val solution = makeSolutionWithRole(id = TEST_USER_MAIL, role = ROLE_ADMIN)
713+
solutionSaved = solutionApiService.createSolution(organizationSaved.id, solution)
714+
val fileName = "my_file.txt"
715+
val multipartFile =
716+
MockMultipartFile(
717+
"file",
718+
fileName,
719+
MediaType.MULTIPART_FORM_DATA_VALUE,
720+
InputStream.nullInputStream())
721+
722+
solutionApiService.createSolutionFile(
723+
organizationSaved.id, solutionSaved.id, multipartFile, false, null)
724+
725+
every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
726+
727+
if (shouldThrow) {
728+
val exception =
729+
assertThrows<CsmAccessForbiddenException> {
730+
solutionApiService.listSolutionFiles(organizationSaved.id, solutionSaved.id)
731+
}
732+
assertEquals(
733+
"RBAC ${organizationSaved.id} - User does not have permission $PERMISSION_READ",
734+
exception.message)
735+
} else {
736+
assertDoesNotThrow {
737+
solutionApiService.listSolutionFiles(organizationSaved.id, solutionSaved.id)
738+
}
739+
}
740+
}
741+
}
742+
743+
@TestFactory
744+
fun `test Solution RBAC listSolutionFiles`() =
745+
mapOf(
746+
ROLE_VIEWER to false,
747+
ROLE_EDITOR to false,
748+
ROLE_USER to false,
749+
ROLE_NONE to true,
750+
ROLE_ADMIN to false,
751+
)
752+
.map { (role, shouldThrow) ->
753+
DynamicTest.dynamicTest("Test Solution RBAC listSolutionFiles : $role") {
754+
every { getCurrentAccountIdentifier(any()) } returns CONNECTED_ADMIN_USER
755+
756+
val organization =
757+
makeOrganizationCreateRequest(id = TEST_USER_MAIL, role = ROLE_ADMIN)
758+
organizationSaved = organizationApiService.createOrganization(organization)
759+
val solution = makeSolutionWithRole(id = TEST_USER_MAIL, role = role)
760+
solutionSaved = solutionApiService.createSolution(organizationSaved.id, solution)
761+
val fileName = "my_file.txt"
762+
val multipartFile =
763+
MockMultipartFile(
764+
"file",
765+
fileName,
766+
MediaType.MULTIPART_FORM_DATA_VALUE,
767+
InputStream.nullInputStream())
768+
769+
solutionApiService.createSolutionFile(
770+
organizationSaved.id, solutionSaved.id, multipartFile, false, null)
771+
772+
every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
773+
774+
if (shouldThrow) {
775+
val exception =
776+
assertThrows<CsmAccessForbiddenException> {
777+
solutionApiService.listSolutionFiles(organizationSaved.id, solutionSaved.id)
778+
}
779+
assertEquals(
780+
"RBAC ${solutionSaved.id} - User does not have permission $PERMISSION_READ",
781+
exception.message)
782+
} else {
783+
assertDoesNotThrow {
784+
solutionApiService.listSolutionFiles(organizationSaved.id, solutionSaved.id)
785+
}
786+
}
787+
}
788+
}
789+
697790
@TestFactory
698791
fun `test Organization RBAC updateSolution`() =
699792
mapOf(

‎solution/src/integrationTest/resources/application-solution-test.yml‎

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -55,23 +55,6 @@ csm:
5555
type: hashid
5656
event-publisher:
5757
type: in_process
58-
images:
59-
scenario-fetch-parameters: cosmo-tech/fetch-scenario-parameters
60-
send-datawarehouse: cosmo-tech/azure-data-explorer-connector
61-
scenario-data-upload: cosmo-tech/azure-storage-publish:latest
62-
containers:
63-
- name: "ADTTwingraphImport"
64-
imageRegistry: "ghcr.io"
65-
imageName: "cosmo-tech/adt-twincache-connector"
66-
imageVersion: "0.3.0"
67-
- name: "AzureStorageTwingraphImport"
68-
imageRegistry: "ghcr.io"
69-
imageName: "cosmo-tech/azstorage-twincache-connector"
70-
imageVersion: "1.2.0"
71-
- name: "TwincacheConnector"
72-
imageRegistry: "ghcr.io"
73-
imageName: "cosmo-tech/twincache-connector"
74-
imageVersion: "0.4.1"
7558
upload:
7659
authorized-mime-types:
7760
workspaces:
@@ -83,6 +66,15 @@ csm:
8366
- text/plain
8467
- text/x-yaml
8568
- application/json
69+
solutions:
70+
- application/zip
71+
- application/xml
72+
- application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
73+
- application/x-tika-ooxml
74+
- text/csv
75+
- text/plain
76+
- text/x-yaml
77+
- application/json
8678
authorization:
8779
tenant-id-jwt-claim: "iss"
8880
# Note that the way @Value works in Spring does not make it possible to inject this sole YAML list.

‎solution/src/main/kotlin/com/cosmotech/solution/SolutionApiServiceInterface.kt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,4 @@ interface SolutionApiServiceInterface : SolutionApiService {
2020
solutionId: String,
2121
runTemplateId: String
2222
): Boolean
23-
24-
fun deleteAllS3SolutionObjects(organizationId: String, solution: Solution)
2523
}

‎solution/src/main/kotlin/com/cosmotech/solution/service/SolutionServiceImpl.kt‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT license.
33
package com.cosmotech.solution.service
44

5+
import com.amazonaws.SdkClientException
56
import com.cosmotech.api.CsmPhoenixService
67
import com.cosmotech.api.containerregistry.ContainerRegistryService
78
import com.cosmotech.api.events.OrganizationUnregistered
@@ -44,6 +45,7 @@ import com.cosmotech.solution.domain.SolutionRole
4445
import com.cosmotech.solution.domain.SolutionSecurity
4546
import com.cosmotech.solution.domain.SolutionUpdateRequest
4647
import com.cosmotech.solution.repository.SolutionRepository
48+
import io.awspring.cloud.s3.S3Exception
4749
import io.awspring.cloud.s3.S3Template
4850
import java.time.Instant
4951
import kotlin.collections.mutableListOf
@@ -56,6 +58,7 @@ import org.springframework.data.domain.Pageable
5658
import org.springframework.scheduling.annotation.Async
5759
import org.springframework.stereotype.Service
5860
import org.springframework.web.multipart.MultipartFile
61+
import software.amazon.awssdk.awscore.exception.AwsServiceException
5962
import software.amazon.awssdk.services.s3.S3Client
6063
import software.amazon.awssdk.services.s3.model.ListObjectsV2Request
6164

@@ -71,7 +74,7 @@ class SolutionServiceImpl(
7174
private val containerRegistryService: ContainerRegistryService,
7275
private val s3Client: S3Client,
7376
private val s3Template: S3Template,
74-
private val resourceScanner: ResourceScanner,
77+
private val resourceScanner: ResourceScanner
7578
) : CsmPhoenixService(), SolutionApiServiceInterface {
7679

7780
override fun listSolutions(organizationId: String, page: Int?, size: Int?): List<Solution> {
@@ -193,7 +196,7 @@ class SolutionServiceImpl(
193196
}
194197

195198
override fun listSolutionFiles(organizationId: String, solutionId: String): List<SolutionFile> {
196-
val solution = getSolution(organizationId, solutionId)
199+
val solution = getVerifiedSolution(organizationId, solutionId)
197200

198201
logger.debug("List all files for solution #{} ({})", solution.id, solution.name)
199202
return getSolutionFiles(organizationId, solutionId)
@@ -418,7 +421,7 @@ class SolutionServiceImpl(
418421
file.originalFilename,
419422
destination)
420423

421-
resourceScanner.scanMimeTypes(file, csmPlatformProperties.upload.authorizedMimeTypes.workspaces)
424+
resourceScanner.scanMimeTypes(file, csmPlatformProperties.upload.authorizedMimeTypes.solutions)
422425
val fileRelativeDestinationBuilder = StringBuilder()
423426
if (destination.isNullOrBlank()) {
424427
fileRelativeDestinationBuilder.append(file.originalFilename)
@@ -433,9 +436,8 @@ class SolutionServiceImpl(
433436
val objectKey =
434437
"$organizationId/$solutionId/$SOLUTION_FILES_BASE_FOLDER/$fileRelativeDestinationBuilder"
435438

436-
if (!overwrite && s3Template.objectExists(csmPlatformProperties.s3.bucketName, objectKey)) {
437-
throw IllegalArgumentException(
438-
"File '$fileRelativeDestinationBuilder' already exists, not overwriting it")
439+
require(overwrite || !s3Template.objectExists(csmPlatformProperties.s3.bucketName, objectKey)) {
440+
"File '$fileRelativeDestinationBuilder' already exists, not overwriting it"
439441
}
440442

441443
s3Template.upload(csmPlatformProperties.s3.bucketName, objectKey, file.inputStream)
@@ -705,7 +707,7 @@ class SolutionServiceImpl(
705707
return solution
706708
}
707709

708-
override fun deleteAllS3SolutionObjects(organizationId: String, solution: Solution) {
710+
fun deleteAllS3SolutionObjects(organizationId: String, solution: Solution) {
709711
logger.debug("Deleting all files for solution #{} ({})", solution.id, solution.name)
710712
val solutionFiles = getSolutionFiles(organizationId, solution.id)
711713
if (solutionFiles.isEmpty()) {
@@ -720,10 +722,17 @@ class SolutionServiceImpl(
720722
private fun deleteS3SolutionObject(organizationId: String, solution: Solution, fileName: String) {
721723
logger.debug(
722724
"Deleting file resource from solution #{} ({}): {}", solution.id, solution.name, fileName)
723-
724-
s3Template.deleteObject(
725-
csmPlatformProperties.s3.bucketName,
726-
"$organizationId/${solution.id}/$SOLUTION_FILES_BASE_FOLDER/$fileName")
725+
try {
726+
s3Template.deleteObject(
727+
csmPlatformProperties.s3.bucketName,
728+
"$organizationId/${solution.id}/$SOLUTION_FILES_BASE_FOLDER/$fileName")
729+
} catch (e: AwsServiceException) {
730+
logger.error("Something wrong happened during $fileName deletion", e)
731+
} catch (e: SdkClientException) {
732+
logger.error("Something wrong happened during $fileName deletion", e)
733+
} catch (e: S3Exception) {
734+
logger.error("Something wrong happened during $fileName deletion", e)
735+
}
727736
}
728737

729738
private fun getSolutionFiles(organizationId: String, solutionId: String): List<SolutionFile> {

‎workspace/src/integrationTest/kotlin/com/cosmotech/workspace/service/WorkspaceServiceIntegrationTest.kt‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -233,10 +233,18 @@ class WorkspaceServiceIntegrationTest : CsmTestBase() {
233233
workspaceApiService.createWorkspaceFile(
234234
organizationSaved.id, workspaceSaved.id, multipartFile, true, null)
235235

236-
workspaceApiService.deleteAllS3WorkspaceObjects(organizationSaved.id, workspaceSaved)
237-
238-
assertEquals(
239-
0, workspaceApiService.listWorkspaceFiles(organizationSaved.id, workspaceSaved.id).size)
236+
workspaceApiService.deleteWorkspaceFiles(organizationSaved.id, workspaceSaved.id)
237+
238+
var numberOfTry = 0
239+
var workspaceFilesIsEmpty: Boolean
240+
do {
241+
workspaceFilesIsEmpty =
242+
workspaceApiService.listWorkspaceFiles(organizationSaved.id, workspaceSaved.id).isEmpty()
243+
numberOfTry++
244+
Thread.sleep(50)
245+
} while (numberOfTry < 100 && !workspaceFilesIsEmpty)
246+
247+
assertTrue(workspaceFilesIsEmpty)
240248
}
241249

242250
@Test

‎workspace/src/main/kotlin/com/cosmotech/workspace/WorkspaceApiServiceInterface.kt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,4 @@ interface WorkspaceApiServiceInterface : WorkspaceApiService {
1313
workspaceId: String,
1414
requiredPermission: String = PERMISSION_READ
1515
): Workspace
16-
17-
fun deleteAllS3WorkspaceObjects(organizationId: String, workspace: Workspace)
1816
}

‎workspace/src/main/kotlin/com/cosmotech/workspace/service/WorkspaceServiceImpl.kt‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT license.
33
package com.cosmotech.workspace.service
44

5+
import com.amazonaws.SdkClientException
56
import com.cosmotech.api.CsmPhoenixService
67
import com.cosmotech.api.events.OrganizationUnregistered
78
import com.cosmotech.api.events.WorkspaceDeleted
@@ -37,6 +38,7 @@ import com.cosmotech.workspace.domain.WorkspaceRole
3738
import com.cosmotech.workspace.domain.WorkspaceSecurity
3839
import com.cosmotech.workspace.domain.WorkspaceUpdateRequest
3940
import com.cosmotech.workspace.repository.WorkspaceRepository
41+
import io.awspring.cloud.s3.S3Exception
4042
import io.awspring.cloud.s3.S3Template
4143
import java.time.Instant
4244
import kotlinx.coroutines.CoroutineScope
@@ -50,6 +52,7 @@ import org.springframework.data.domain.Pageable
5052
import org.springframework.scheduling.annotation.Async
5153
import org.springframework.stereotype.Service
5254
import org.springframework.web.multipart.MultipartFile
55+
import software.amazon.awssdk.awscore.exception.AwsServiceException
5356
import software.amazon.awssdk.services.s3.S3Client
5457
import software.amazon.awssdk.services.s3.model.ListObjectsV2Request
5558

@@ -350,13 +353,20 @@ internal class WorkspaceServiceImpl(
350353
workspace.id,
351354
workspace.name,
352355
fileName)
353-
354-
s3Template.deleteObject(
355-
csmPlatformProperties.s3.bucketName,
356-
"$organizationId/${workspace.id}/$WORKSPACE_FILES_BASE_FOLDER/$fileName")
356+
try {
357+
s3Template.deleteObject(
358+
csmPlatformProperties.s3.bucketName,
359+
"$organizationId/${workspace.id}/$WORKSPACE_FILES_BASE_FOLDER/$fileName")
360+
} catch (e: AwsServiceException) {
361+
logger.error("Something wrong happened during $fileName deletion", e)
362+
} catch (e: SdkClientException) {
363+
logger.error("Something wrong happened during $fileName deletion", e)
364+
} catch (e: S3Exception) {
365+
logger.error("Something wrong happened during $fileName deletion", e)
366+
}
357367
}
358368

359-
override fun deleteAllS3WorkspaceObjects(organizationId: String, workspace: Workspace) {
369+
fun deleteAllS3WorkspaceObjects(organizationId: String, workspace: Workspace) {
360370
logger.debug("Deleting all files for workspace #{} ({})", workspace.id, workspace.name)
361371
val workspaceFiles = getWorkspaceFiles(organizationId, workspace.id)
362372
if (workspaceFiles.isEmpty()) {

0 commit comments

Comments
 (0)