Skip to content
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

[CORE-211] Convert disk operations to new Sam permissions model #4822

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

[CORE-211] Convert disk operations to new Sam permissions model #4822

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
08950a1
listDisks
marctalbott Jan 8, 2025
9194755
createDisk
marctalbott Jan 8, 2025
d7c68d9
getDiskv1
marctalbott Jan 10, 2025
03e9d12
getDiskv2
marctalbott Jan 10, 2025
9c5918e
deleteDiskv2
marctalbott Jan 10, 2025
c61d5a6
deleteDiskv1
marctalbott Jan 10, 2025
154ffcd
updateDisk
marctalbott Jan 10, 2025
06bd6ae
deleteDiskRecords
marctalbott Jan 10, 2025
9a7e442
attachDisk
marctalbott Jan 13, 2025
814a783
switch to object instead of trait
marctalbott Jan 13, 2025
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
1 change: 0 additions & 1 deletion ...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AzureDependenciesBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ class AzureDependenciesBuilder extends CloudDependenciesBuilder {
val runtimeService = RuntimeService(
baselineDependencies.runtimeServicesConfig,
ConfigReader.appConfig.persistentDisk,
baselineDependencies.authProvider,
baselineDependencies.dockerDAO,
None,
None,
Expand Down
1 change: 0 additions & 1 deletion ...c/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/GcpDependenciesBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,6 @@ class GcpDependencyBuilder extends CloudDependenciesBuilder {
val runtimeService = RuntimeService(
baselineDependencies.runtimeServicesConfig,
ConfigReader.appConfig.persistentDisk,
baselineDependencies.authProvider,
baselineDependencies.dockerDAO,
Some(gcpDependencies.googleStorageService),
Some(gcpDependencies.googleComputeService),
Expand Down
2 changes: 0 additions & 2 deletions ...n/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig,
userEmail,
petSA,
appTypeToFormattedByType(req.appType),
authProvider,
samService,
config.leoKubernetesConfig.diskConfig,
parentWorkspaceId
Expand Down Expand Up @@ -827,7 +826,6 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig,
userEmail,
petSA,
appTypeToFormattedByType(req.appType),
authProvider,
samService,
config.leoKubernetesConfig.diskConfig
)
Expand Down
3 changes: 0 additions & 3 deletions ...c/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import org.broadinstitute.dsde.workbench.leonardo.config.PersistentDiskConfig
import org.broadinstitute.dsde.workbench.leonardo.dao.DockerDAO
import org.broadinstitute.dsde.workbench.leonardo.dao.sam.SamService
import org.broadinstitute.dsde.workbench.leonardo.db.DbReference
import org.broadinstitute.dsde.workbench.leonardo.model.LeoAuthProvider
import org.broadinstitute.dsde.workbench.leonardo.monitor.LeoPubsubMessage
import org.broadinstitute.dsde.workbench.model.UserInfo
import org.broadinstitute.dsde.workbench.model.google.GoogleProject
Expand Down Expand Up @@ -69,7 +68,6 @@ trait RuntimeService[F[_]] {
object RuntimeService {
def apply[F[_]: Parallel](config: RuntimeServiceConfig,
diskConfig: PersistentDiskConfig,
authProvider: LeoAuthProvider[F],
dockerDAO: DockerDAO[F],
googleStorageService: Option[GoogleStorageService[F]],
googleComputeService: Option[GoogleComputeService[F]],
Expand All @@ -85,7 +83,6 @@ object RuntimeService {
new RuntimeServiceInterp(
config,
diskConfig,
authProvider,
dockerDAO,
googleStorageService,
googleComputeService,
Expand Down
80 changes: 45 additions & 35 deletions .../scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,8 @@ import org.broadinstitute.dsde.workbench.leonardo.dao.DockerDAO
import org.broadinstitute.dsde.workbench.leonardo.dao.sam.{SamException, SamService, SamUtils}
import org.broadinstitute.dsde.workbench.leonardo.db._
import org.broadinstitute.dsde.workbench.leonardo.http.service.DiskServiceInterp.getDiskSamPolicyMap
import org.broadinstitute.dsde.workbench.leonardo.model.SamResourceAction.{
projectSamResourceAction,
workspaceSamResourceAction
}
import org.broadinstitute.dsde.workbench.leonardo.http.service.RuntimeServiceInterp._
import org.broadinstitute.dsde.workbench.leonardo.model.SamResource.RuntimeSamResource
import org.broadinstitute.dsde.workbench.leonardo.model.SamResourceAction._
import org.broadinstitute.dsde.workbench.leonardo.model._
import org.broadinstitute.dsde.workbench.leonardo.monitor.LeoPubsubMessage._
import org.broadinstitute.dsde.workbench.leonardo.monitor.{
Expand All @@ -61,7 +56,6 @@ import scala.concurrent.ExecutionContext
class RuntimeServiceInterp[F[_]: Parallel](
config: RuntimeServiceConfig,
diskConfig: PersistentDiskConfig,
authProvider: LeoAuthProvider[F],
dockerDAO: DockerDAO[F],
googleStorageService: Option[GoogleStorageService[F]],
googleComputeService: Option[GoogleComputeService[F]],
Expand Down Expand Up @@ -163,7 +157,6 @@ class RuntimeServiceInterp[F[_]: Parallel](
userEmail,
petSA,
FormattedBy.GCE,
authProvider,
samService,
diskConfig,
parentWorkspaceId
Expand Down Expand Up @@ -926,7 +919,6 @@ object RuntimeServiceInterp {
userEmail: WorkbenchEmail,
serviceAccount: WorkbenchEmail,
willBeUsedBy: FormattedBy,
authProvider: LeoAuthProvider[F],
samService: SamService[F],
diskConfig: PersistentDiskConfig,
workspaceId: Option[WorkspaceId]
Expand All @@ -943,6 +935,7 @@ object RuntimeServiceInterp {
disk <- diskOpt match {
case Some(pd) =>
for {
_ <- checkAttachAction(userInfo, samService, pd, cloudContext, req.name, ctx.traceId)
_ <-
if (pd.zone == targetZone) F.unit
else
Expand Down Expand Up @@ -977,23 +970,19 @@ object RuntimeServiceInterp {
if (isAttached)
F.raiseError[Unit](DiskAlreadyAttachedException(CloudContext.Gcp(googleProject), req.name, ctx.traceId))
else F.unit
hasPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction](
pd.samResource,
PersistentDiskAction.AttachPersistentDisk,
userInfo
)

_ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail))
} yield PersistentDiskRequestResult(pd, false)

case None =>
for {
hasPermission <- authProvider.hasPermission[ProjectSamResourceId, ProjectAction](
ProjectSamResourceId(googleProject),
ProjectAction.CreatePersistentDisk,
userInfo
)
_ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail))
_ <- samService
.checkAuthorized(userInfo.accessToken.token,
ProjectSamResourceId(googleProject),
ProjectAction.CreatePersistentDisk
)
.adaptError {
case e: SamException if e.statusCode == StatusCodes.Forbidden => ForbiddenError(userEmail)
}

samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString))
diskBeforeSave <- F.fromEither(
DiskServiceInterp.convertToDisk(
Expand Down Expand Up @@ -1032,7 +1021,6 @@ object RuntimeServiceInterp {
userEmail: WorkbenchEmail,
serviceAccount: WorkbenchEmail,
willBeUsedBy: FormattedBy,
authProvider: LeoAuthProvider[F],
samService: SamService[F],
diskConfig: PersistentDiskConfig
)(implicit
Expand All @@ -1047,6 +1035,7 @@ object RuntimeServiceInterp {
disk <- diskOpt match {
case Some(pd) =>
for {
_ <- checkAttachAction(userInfo, samService, pd, cloudContext, req.name, ctx.traceId)
_ <-
if (pd.zone == targetZone) F.unit
else
Expand Down Expand Up @@ -1081,23 +1070,18 @@ object RuntimeServiceInterp {
if (isAttached)
F.raiseError[Unit](DiskAlreadyAttachedException(cloudContext, req.name, ctx.traceId))
else F.unit
hasPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction](
pd.samResource,
PersistentDiskAction.AttachPersistentDisk,
userInfo
)

_ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail))
} yield PersistentDiskRequestResult(pd, false)

case None =>
for {
hasPermission <- authProvider.hasPermission[WorkspaceResourceSamResourceId, WorkspaceAction](
WorkspaceResourceSamResourceId(workspaceId),
WorkspaceAction.CreateControlledApplicationResource,
userInfo
) // TODO: Correct check?
_ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail))
_ <- samService
.checkAuthorized(userInfo.accessToken.token,
WorkspaceResourceSamResourceId(workspaceId),
WorkspaceAction.Compute
)
.adaptError {
case e: SamException if e.statusCode == StatusCodes.Forbidden => ForbiddenError(userEmail)
}
samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString))
diskBeforeSave <- F.fromEither(
DiskServiceInterp.convertToDisk(
Expand Down Expand Up @@ -1126,6 +1110,32 @@ object RuntimeServiceInterp {
}
} yield disk

private def checkAttachAction[F[_]](userInfo: UserInfo,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, would it make sense to move this with the other checkAction function in SamUtils? Might not be in the scope for this PR, but disks can also be attached to apps and so it could make sense to move this logic out of the RuntimeServiceInterp file

samService: SamService[F],
pd: PersistentDisk,
cloudContext: CloudContext,
diskName: DiskName,
traceId: TraceId
)(implicit
as: Ask[F, AppContext],
F: Async[F]
): F[Unit] =
samService
.checkAuthorized(userInfo.accessToken.token, pd.samResource, PersistentDiskAction.AttachPersistentDisk)
.handleErrorWith {
case e: SamException if e.statusCode == StatusCodes.Forbidden =>
samService
.checkAuthorized(userInfo.accessToken.token, pd.samResource, PersistentDiskAction.ReadPersistentDisk)
.attempt
.flatMap {
case Left(e: SamException) if e.statusCode == StatusCodes.Forbidden =>
F.raiseError(DiskNotFoundException(cloudContext, diskName, traceId))
case Right(_) => F.raiseError(ForbiddenError(userInfo.userEmail))
case Left(e) => F.raiseError(e)
}
case e => F.raiseError(e)
}

private[service] def calculateAutopauseThreshold(
autopause: Option[Boolean],
autopauseThreshold: Option[Int],
Expand Down
1 change: 0 additions & 1 deletion http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ trait TestLeoRoutes {
val runtimeService = RuntimeService(
serviceConfig,
ConfigReader.appConfig.persistentDisk,
allowListAuthProvider,
new MockDockerDAO,
Some(FakeGoogleStorageInterpreter),
Some(FakeGoogleComputeService),
Expand Down
1 change: 1 addition & 0 deletions .../scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le
val publisherQueue = QueueFactory.makePublisherQueue()
val mockSamService = mock[SamService[IO]]
when(mockSamService.createResource(any, any, any, any, any)(any)).thenReturn(IO.unit)
when(mockSamService.checkAuthorized(any, any, any)(any)).thenReturn(IO.unit)
when(mockSamService.deleteResource(any, any)(any)).thenReturn(IO.unit)
when(mockSamService.getUserEmail(any)(any)).thenReturn(IO.pure(userInfo.userEmail))
when(mockSamService.lookupWorkspaceParentForGoogleProject(any, any)(any)).thenReturn(IO.none)
Expand Down
17 changes: 6 additions & 11 deletions ...la/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ trait RuntimeServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with T
azureServiceConfig
),
ConfigReader.appConfig.persistentDisk,
authProvider,
new MockDockerDAO,
Some(FakeGoogleStorageInterpreter),
Some(computeService),
Expand Down Expand Up @@ -2307,7 +2306,6 @@ class RuntimeServiceInterpTest
userEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand Down Expand Up @@ -2361,7 +2359,6 @@ class RuntimeServiceInterpTest
userEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand Down Expand Up @@ -2391,7 +2388,6 @@ class RuntimeServiceInterpTest
userEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand All @@ -2418,7 +2414,6 @@ class RuntimeServiceInterpTest
userEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand All @@ -2442,7 +2437,6 @@ class RuntimeServiceInterpTest
unauthorizedEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand Down Expand Up @@ -2478,7 +2472,6 @@ class RuntimeServiceInterpTest
userEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand All @@ -2503,7 +2496,6 @@ class RuntimeServiceInterpTest
userEmail,
serviceAccount,
FormattedBy.Galaxy,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand All @@ -2520,7 +2512,6 @@ class RuntimeServiceInterpTest
userEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
Expand All @@ -2539,6 +2530,11 @@ class RuntimeServiceInterpTest
}

it should "fail to attach a disk when caller has no attach permission" in isolatedDbTest {
val samService = mock[SamService[IO]]
when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.AttachPersistentDisk))(any()))
.thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, TraceId(""))))
when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.ReadPersistentDisk))(any()))
.thenReturn(IO.unit)
val res = for {
savedDisk <- makePersistentDisk(None).save()
req = PersistentDiskRequest(savedDisk.name, Some(savedDisk.size), Some(savedDisk.diskType), savedDisk.labels)
Expand All @@ -2550,8 +2546,7 @@ class RuntimeServiceInterpTest
unauthorizedEmail,
serviceAccount,
FormattedBy.GCE,
allowListAuthProvider,
MockSamService,
samService,
ConfigReader.appConfig.persistentDisk,
Some(workspaceId)
)(implicitly, implicitly, implicitly, scala.concurrent.ExecutionContext.global)
Expand Down