Skip to content

Fix [SDCOSMO-2543]: getVerifiedWorkspace is now based on organizationId #880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jreynard-code merged 1 commit into from
Mar 7, 2025
Merged

Fix [SDCOSMO-2543]: getVerifiedWorkspace is now based on organizationId #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions ...integrationTest/kotlin/com/cosmotech/worskpace/service/WorkspaceServiceIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import kotlin.test.assertNull
import kotlin.test.assertTrue
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertDoesNotThrow
import org.junit.jupiter.api.assertThrows
import org.junit.jupiter.api.extension.ExtendWith
import org.junit.runner.RunWith
Expand Down Expand Up @@ -500,6 +501,28 @@ class WorkspaceServiceIntegrationTest : CsmRedisTestBase() {
}
}

@Test
fun `SDCOSMO-2543 - test workspace is not accessible from another organization`() {
val workspace = makeWorkspace()
workspaceSaved = workspaceApiService.createWorkspace(organizationSaved.id!!, workspace)

val anotherOrganization = makeOrganization("Another Organization")
val anotherOrganizationSaved = organizationApiService.registerOrganization(anotherOrganization)

assertDoesNotThrow {
workspaceApiService.findWorkspaceById(organizationSaved.id!!, workspaceSaved.id!!)
}

val exception =
assertThrows<CsmResourceNotFoundException> {
workspaceApiService.findWorkspaceById(anotherOrganizationSaved.id!!, workspaceSaved.id!!)
}

assertEquals(
"Workspace ${workspaceSaved.id} not found in organization ${anotherOrganizationSaved.id}",
exception.message)
}

fun makeOrganization(
id: String,
userName: String = CONNECTED_ADMIN_USER,
Expand Down
7 changes: 4 additions & 3 deletions workspace/src/main/kotlin/com/cosmotech/workspace/service/WorkspaceServiceImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ import org.springframework.context.annotation.Scope
import org.springframework.context.event.EventListener
import org.springframework.core.io.Resource
import org.springframework.data.domain.Pageable
import org.springframework.data.repository.findByIdOrNull
import org.springframework.scheduling.annotation.Async
import org.springframework.stereotype.Service

Expand Down Expand Up @@ -458,8 +457,10 @@ internal class WorkspaceServiceImpl(
): Workspace {
organizationService.getVerifiedOrganization(organizationId)
val workspace =
workspaceRepository.findByIdOrNull(workspaceId)
?: throw CsmResourceNotFoundException("Workspace $workspaceId does not exist!")
workspaceRepository.findBy(organizationId, workspaceId).orElseThrow {
CsmResourceNotFoundException(
"Workspace $workspaceId not found in organization $organizationId")
}
csmRbac.verify(workspace.getRbac(), requiredPermission)
return workspace
}
Expand Down
42 changes: 21 additions & 21 deletions workspace/src/test/kotlin/com/cosmotech/workspace/service/WorkspaceServiceImplTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ class WorkspaceServiceImplTests {
every { workspace.id } returns WORKSPACE_ID
every { workspace.name } returns "test workspace"
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(workspace)

val file = mockk<Resource>(relaxed = true)
every { file.filename } returns "my_file.txt"
Expand Down Expand Up @@ -173,7 +173,7 @@ class WorkspaceServiceImplTests {
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
every { organizationService.getVerifiedOrganization(ORGANIZATION_ID) } returns
mockk<Organization>()
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(workspace)

val file = mockk<Resource>(relaxed = true)
every { file.filename } returns "my_file.txt"
Expand Down Expand Up @@ -203,7 +203,7 @@ class WorkspaceServiceImplTests {
every { workspace.id } returns WORKSPACE_ID
every { workspace.name } returns "test workspace"
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(workspace)

val file = mockk<Resource>(relaxed = true)
every { file.filename } returns "my_file.txt"
Expand Down Expand Up @@ -235,7 +235,7 @@ class WorkspaceServiceImplTests {
every { workspace.id } returns WORKSPACE_ID
every { workspace.name } returns "test workspace"
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(workspace)

val file = mockk<Resource>(relaxed = true)
every { file.filename } returns "my_file.txt"
Expand Down Expand Up @@ -267,7 +267,7 @@ class WorkspaceServiceImplTests {
every { workspace.id } returns WORKSPACE_ID
every { workspace.name } returns "test workspace"
every { workspace.security } returns WorkspaceSecurity(ROLE_ADMIN, mutableListOf())
every { workspaceRepository.findByIdOrNull(any()) } returns workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(workspace)

val file = mockk<Resource>(relaxed = true)
every { file.filename } returns "my_file.txt"
Expand Down Expand Up @@ -356,7 +356,7 @@ class WorkspaceServiceImplTests {
name = "my workspace name",
solution = WorkspaceSolution(solutionId = "SOL-my-solution-id"),
security = WorkspaceSecurity(ROLE_ADMIN, mutableListOf()))
every { workspaceRepository.findByIdOrNull(WORKSPACE_ID) } returns workspace
every { workspaceRepository.findBy(any(), WORKSPACE_ID) } returns Optional.of(workspace)
every { solutionService.findSolutionById(ORGANIZATION_ID, any()) } throws
CsmResourceNotFoundException("Solution not found")
assertThrows<CsmResourceNotFoundException> {
Expand All @@ -383,7 +383,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC read workspace: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
workspaceServiceImpl.getVerifiedWorkspace(it.organization.id!!, it.workspace.id!!)
}
}
Expand Down Expand Up @@ -420,7 +420,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC delete all workspace files: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
workspaceServiceImpl.deleteAllWorkspaceFiles(it.organization.id!!, it.workspace.id!!)
}
}
Expand All @@ -437,7 +437,7 @@ class WorkspaceServiceImplTests {
.map { (role, shouldThrow) ->
rbacTest("test RBAC update workspace: $role", role, shouldThrow) {
every { solutionService.findSolutionById(any(), any()) } returns it.solution
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { workspaceRepository.save(any()) } returns it.workspace
workspaceServiceImpl.updateWorkspace(
it.organization.id!!, it.workspace.id!!, it.workspace)
Expand All @@ -455,7 +455,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC delete workspace: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { secretManager.deleteSecret(any(), any()) } returns Unit
workspaceServiceImpl.deleteWorkspace(it.organization.id!!, it.workspace.id!!)
}
Expand All @@ -472,7 +472,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC delete workspace file: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every {
azureStorageBlobServiceClient
.getBlobContainerClient(any())
Expand All @@ -494,7 +494,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC download workspace file: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { azureStorageBlobProtocolResolver.getResource(any()) } returns mockk()
workspaceServiceImpl.downloadWorkspaceFile(
it.organization.id!!, it.workspace.id!!, "")
Expand All @@ -512,7 +512,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC upload workspace file: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { resource.filename } returns "name"
every {
azureStorageBlobServiceClient
Expand All @@ -538,7 +538,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC findAllWorkspaceFiles: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { azureStorageBlobServiceClient.getBlobContainerClient(any()) } returns mockk()
every { azureStorageBlobProtocolResolver.getResources(any()) } returns arrayOf()
workspaceServiceImpl.findAllWorkspaceFiles(it.organization.id!!, it.workspace.id!!)
Expand All @@ -556,7 +556,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC get workspace security: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
workspaceServiceImpl.getWorkspaceSecurity(it.organization.id!!, it.workspace.id!!)
}
}
Expand All @@ -572,7 +572,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("Test RBAC set workspace default security: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { workspaceRepository.save(any()) } returns it.workspace
workspaceServiceImpl.setWorkspaceDefaultSecurity(
it.organization.id!!, it.workspace.id!!, WorkspaceRole(ROLE_NONE))
Expand All @@ -590,7 +590,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("test RBAC get workspace access control: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
workspaceServiceImpl.getWorkspaceAccessControl(
it.organization.id!!, it.workspace.id!!, CONNECTED_DEFAULT_USER)
}
Expand All @@ -608,7 +608,7 @@ class WorkspaceServiceImplTests {
.map { (role, shouldThrow) ->
rbacTest("test RBAC add workspace access control: $role", role, shouldThrow) {
every { workspaceRepository.save(any()) } returns it.workspace
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
workspaceServiceImpl.addWorkspaceAccessControl(
it.organization.id!!,
it.workspace.id!!,
Expand All @@ -627,7 +627,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("test RBAC update workspace access control: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { workspaceRepository.save(any()) } returns it.workspace
workspaceServiceImpl.updateWorkspaceAccessControl(
it.organization.id!!,
Expand All @@ -648,7 +648,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("test RBAC remove workspace access control: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { workspaceRepository.save(any()) } returns it.workspace
workspaceServiceImpl.removeWorkspaceAccessControl(
it.organization.id!!, it.workspace.id!!, "2$CONNECTED_DEFAULT_USER")
Expand All @@ -665,7 +665,7 @@ class WorkspaceServiceImplTests {
ROLE_NONE to true)
.map { (role, shouldThrow) ->
rbacTest("test RBAC get workspace security users: $role", role, shouldThrow) {
every { workspaceRepository.findByIdOrNull(any()) } returns it.workspace
every { workspaceRepository.findBy(any(), any()) } returns Optional.of(it.workspace)
every { workspaceRepository.save(any()) } returns it.workspace
workspaceServiceImpl.getWorkspaceSecurityUsers(
it.organization.id!!, it.workspace.id!!)
Expand Down
Loading