Skip to content

Commit 0b86274

Browse files
jreynard-codejreynard-code
committed
Refactor WorkspaceService to use MultipartFile for file upload handling
- Replaced `Resource` with `MultipartFile` in `uploadWorkspaceFile` for improved flexibility. - Updated related service, test, and integration test implementations to accommodate this change. - Adjusted validations on file paths and names for enhanced security. - Improved import paths for `RediSearchIndexer` in relevant modules.
1 parent 1a448bf commit 0b86274

File tree

5 files changed

+53
-29
lines changed

5 files changed

+53
-29
lines changed

‎run/src/main/kotlin/com/cosmotech/run/workflow/WorkflowService.kt‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ interface WorkflowService : HealthIndicator {
2020

2121
/**
2222
* Launch a new Scenario run, using the request specified
23+
*
2324
* @param runStartContainers the scenario run start request
2425
* @param executionTimeout the duration in which the workflow is allowed to run
2526
* @param alwaysPull the image pull policy
@@ -35,27 +36,31 @@ interface WorkflowService : HealthIndicator {
3536

3637
/**
3738
* Find WorkflowStatus by label
39+
*
3840
* @param labelSelector a label used to filter workflow
3941
* @return a list of all WorkflowStatus corresponding to the labelSelector
4042
*/
4143
fun findWorkflowStatusByLabel(labelSelector: String): List<WorkflowStatus>
4244

4345
/**
4446
* Get a Run status
47+
*
4548
* @param run the Run
4649
* @return the Run status
4750
*/
4851
fun getRunStatus(run: Run): RunStatus
4952

5053
/**
5154
* Get all logs of a Run, as a structured object
55+
*
5256
* @param run the Run
5357
* @return the RunLogs object
5458
*/
5559
fun getRunLogs(run: Run): RunLogs
5660

5761
/**
5862
* Stop the running workflow
63+
*
5964
* @param run the run object
6065
* @return the Run status
6166
*/

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import com.cosmotech.workspace.domain.WorkspaceAccessControl
3636
import com.cosmotech.workspace.domain.WorkspaceRole
3737
import com.cosmotech.workspace.domain.WorkspaceSecurity
3838
import com.cosmotech.workspace.domain.WorkspaceSolution
39-
import com.redis.om.spring.RediSearchIndexer
39+
import com.redis.om.spring.indexing.RediSearchIndexer
4040
import io.mockk.every
4141
import io.mockk.junit5.MockKExtension
4242
import io.mockk.mockkStatic
@@ -559,6 +559,7 @@ class WorkspaceServiceIntegrationTest : CsmRedisTestBase() {
559559
version = "1.0",
560560
ioTypes = listOf(IoTypesEnum.read))
561561
}
562+
562563
fun makeDataset(
563564
organizationId: String = organizationSaved.id!!,
564565
name: String = "name",

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ import com.cosmotech.workspace.domain.WorkspaceAccessControl
3434
import com.cosmotech.workspace.domain.WorkspaceRole
3535
import com.cosmotech.workspace.domain.WorkspaceSecurity
3636
import com.cosmotech.workspace.domain.WorkspaceSolution
37-
import com.redis.om.spring.RediSearchIndexer
37+
import com.redis.om.spring.indexing.RediSearchIndexer
3838
import io.mockk.every
3939
import io.mockk.impl.annotations.RelaxedMockK
4040
import io.mockk.junit5.MockKExtension
@@ -54,11 +54,11 @@ import org.junit.jupiter.api.extension.ExtendWith
5454
import org.junit.runner.RunWith
5555
import org.springframework.beans.factory.annotation.Autowired
5656
import org.springframework.boot.test.context.SpringBootTest
57-
import org.springframework.core.io.Resource
5857
import org.springframework.test.context.ActiveProfiles
5958
import org.springframework.test.context.junit.jupiter.SpringExtension
6059
import org.springframework.test.context.junit4.SpringRunner
6160
import org.springframework.test.util.ReflectionTestUtils
61+
import org.springframework.web.multipart.MultipartFile
6262

6363
@ActiveProfiles(profiles = ["workspace-test"])
6464
@ExtendWith(MockKExtension::class)
@@ -71,7 +71,7 @@ class WorkspaceServiceRBACTest : CsmRedisTestBase() {
7171
val TEST_USER_MAIL = "[email protected]"
7272
val CONNECTED_ADMIN_USER = "[email protected]"
7373

74-
@RelaxedMockK private lateinit var resource: Resource
74+
@RelaxedMockK private lateinit var resource: MultipartFile
7575

7676
@RelaxedMockK private lateinit var resourceScanner: ResourceScanner
7777

@@ -626,7 +626,11 @@ class WorkspaceServiceRBACTest : CsmRedisTestBase() {
626626
role = ROLE_ADMIN))
627627
every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
628628
ReflectionTestUtils.setField(workspaceApiService, "resourceScanner", resourceScanner)
629-
every { resourceScanner.scanMimeTypes(any(), any()) } returns Unit
629+
every { resource.originalFilename } returns "fakeName"
630+
every {
631+
resourceScanner.scanMimeTypes(
632+
"fakeName", any(), csmPlatformProperties.upload.authorizedMimeTypes.workspaces)
633+
} returns Unit
630634

631635
if (shouldThrow) {
632636
val exception =
@@ -674,7 +678,11 @@ class WorkspaceServiceRBACTest : CsmRedisTestBase() {
674678
role = role))
675679
every { getCurrentAccountIdentifier(any()) } returns TEST_USER_MAIL
676680
ReflectionTestUtils.setField(workspaceApiService, "resourceScanner", resourceScanner)
677-
every { resourceScanner.scanMimeTypes(any(), any()) } returns Unit
681+
every { resource.originalFilename } returns "fakeName"
682+
every {
683+
resourceScanner.scanMimeTypes(
684+
"fakeName", any(), csmPlatformProperties.upload.authorizedMimeTypes.workspaces)
685+
} returns Unit
678686

679687
if (shouldThrow) {
680688
val exception =

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

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ import org.springframework.data.domain.Pageable
5858
import org.springframework.data.repository.findByIdOrNull
5959
import org.springframework.scheduling.annotation.Async
6060
import org.springframework.stereotype.Service
61+
import org.springframework.web.multipart.MultipartFile
6162

6263
@Service
6364
@Scope(value = ConfigurableBeanFactory.SCOPE_PROTOTYPE)
@@ -208,36 +209,41 @@ internal class WorkspaceServiceImpl(
208209
override fun uploadWorkspaceFile(
209210
organizationId: String,
210211
workspaceId: String,
211-
file: Resource,
212+
file: MultipartFile,
212213
overwrite: Boolean,
213214
destination: String?
214215
): WorkspaceFile {
215-
if (destination?.contains("..") == true) {
216-
throw IllegalArgumentException("Invalid destination: '$destination'. '..' is not allowed")
216+
require(destination?.contains("..") != true) {
217+
"Invalid destination: '$destination'. '..' is not allowed"
217218
}
219+
require(file.originalFilename?.isBlank() != true) { "File name must not be blank" }
220+
require(
221+
file.originalFilename?.contains("..") != true &&
222+
file.originalFilename?.startsWith("/") != true) {
223+
"Invalid filename: '${file.originalFilename}'. '..' and '/' are not allowed"
224+
}
218225
val workspace = getVerifiedWorkspace(organizationId, workspaceId, PERMISSION_WRITE)
219-
if (file?.filename?.contains("..") == true || file?.filename?.contains("/") == true) {
220-
throw IllegalArgumentException(
221-
"Invalid filename: '${file.filename}'. '..' and '/' are not allowed")
222-
}
223226

224227
logger.debug(
225228
"Uploading file resource to workspace #{} ({}): {} => {}",
226229
workspace.id,
227230
workspace.name,
228-
file.filename,
231+
file.originalFilename,
229232
destination)
230233

231-
resourceScanner.scanMimeTypes(file, csmPlatformProperties.upload.authorizedMimeTypes.workspaces)
234+
resourceScanner.scanMimeTypes(
235+
file.originalFilename!!,
236+
file.inputStream,
237+
csmPlatformProperties.upload.authorizedMimeTypes.workspaces)
232238
val fileRelativeDestinationBuilder = StringBuilder()
233239
if (destination.isNullOrBlank()) {
234-
fileRelativeDestinationBuilder.append(file.filename)
240+
fileRelativeDestinationBuilder.append(file.originalFilename)
235241
} else {
236242
// Using multiple consecutive '/' in the path is not supported in the storage upload
237243
val destinationSanitized = destination.removePrefix("/").replace(Regex("(/)\\1+"), "/")
238244
fileRelativeDestinationBuilder.append(destinationSanitized)
239245
if (destinationSanitized.endsWith("/")) {
240-
fileRelativeDestinationBuilder.append(file.filename)
246+
fileRelativeDestinationBuilder.append(file.originalFilename)
241247
}
242248
}
243249

@@ -548,6 +554,7 @@ internal class WorkspaceServiceImpl(
548554
this.eventPublisher.publishEvent(
549555
AddWorkspaceToDataset(this, organizationId, datasetId, workspaceId))
550556
}
557+
551558
private fun sendDeleteHistoricalDataWorkspaceEvent(
552559
organizationId: String,
553560
it: Workspace,

‎workspace/src/test/kotlin/com/cosmotech/workspace/service/WorkspaceServiceImplTests.kt‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,8 @@ import org.junit.jupiter.api.TestFactory
6262
import org.junit.jupiter.api.assertDoesNotThrow
6363
import org.junit.jupiter.api.assertThrows
6464
import org.junit.jupiter.api.extension.ExtendWith
65-
import org.springframework.core.io.Resource
6665
import org.springframework.data.repository.findByIdOrNull
66+
import org.springframework.web.multipart.MultipartFile
6767

6868
const val ORGANIZATION_ID = "O-AbCdEf123"
6969
const val WORKSPACE_ID = "W-BcDeFg123"
@@ -145,8 +145,8 @@ class WorkspaceServiceImplTests {
145145
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
146146
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
147147

148-
val file = mockk<Resource>(relaxed = true)
149-
every { file.filename } returns "my_file.txt"
148+
val file = mockk<MultipartFile>(relaxed = true)
149+
every { file.originalFilename } returns "my_file.txt"
150150

151151
val workspaceFile =
152152
workspaceServiceImpl.uploadWorkspaceFile(ORGANIZATION_ID, WORKSPACE_ID, file, false, null)
@@ -166,8 +166,8 @@ class WorkspaceServiceImplTests {
166166
mockk<Organization>()
167167
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
168168

169-
val file = mockk<Resource>(relaxed = true)
170-
every { file.filename } returns "my_file.txt"
169+
val file = mockk<MultipartFile>(relaxed = true)
170+
every { file.originalFilename } returns "my_file.txt"
171171

172172
val workspaceFile =
173173
workspaceServiceImpl.uploadWorkspaceFile(ORGANIZATION_ID, WORKSPACE_ID, file, false, " ")
@@ -185,8 +185,8 @@ class WorkspaceServiceImplTests {
185185
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
186186
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
187187

188-
val file = mockk<Resource>(relaxed = true)
189-
every { file.filename } returns "my_file.txt"
188+
val file = mockk<MultipartFile>(relaxed = true)
189+
every { file.originalFilename } returns "my_file.txt"
190190

191191
val workspaceFile =
192192
workspaceServiceImpl.uploadWorkspaceFile(
@@ -207,8 +207,8 @@ class WorkspaceServiceImplTests {
207207
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
208208
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
209209

210-
val file = mockk<Resource>(relaxed = true)
211-
every { file.filename } returns "my_file.txt"
210+
val file = mockk<MultipartFile>(relaxed = true)
211+
every { file.originalFilename } returns "my_file.txt"
212212

213213
val workspaceFile =
214214
workspaceServiceImpl.uploadWorkspaceFile(
@@ -228,8 +228,8 @@ class WorkspaceServiceImplTests {
228228
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
229229
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
230230

231-
val file = mockk<Resource>(relaxed = true)
232-
every { file.filename } returns "my_file.txt"
231+
val file = mockk<MultipartFile>(relaxed = true)
232+
every { file.originalFilename } returns "my_file.txt"
233233

234234
val workspaceFile =
235235
workspaceServiceImpl.uploadWorkspaceFile(
@@ -443,8 +443,10 @@ class WorkspaceServiceImplTests {
443443
.map { (role, shouldThrow) ->
444444
rbacTest("Test RBAC upload workspace file: $role", role, shouldThrow) {
445445
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
446+
val file = mockk<MultipartFile>(relaxed = true)
447+
every { file.originalFilename } returns "fakeName"
446448
workspaceServiceImpl.uploadWorkspaceFile(
447-
it.organization.id!!, it.workspace.id!!, mockk(relaxed = true), true, "name")
449+
it.organization.id!!, it.workspace.id!!, file, true, "name")
448450
}
449451
}
450452

@@ -573,6 +575,7 @@ class WorkspaceServiceImplTests {
573575
it.organization.id!!, it.workspace.id!!, "2$CONNECTED_DEFAULT_USER")
574576
}
575577
}
578+
576579
@TestFactory
577580
fun `test RBAC get workspace security users`() =
578581
mapOf(

0 commit comments

Comments
 (0)