From 431b9c3b8a350bccb39157e923183b4f66e49653 Mon Sep 17 00:00:00 2001 From: LizBaldo Date: Wed, 19 Feb 2025 11:31:20 -0500 Subject: [PATCH] [AN-385] new sam disk permissions (#4828) --- .../leonardo/LeonardoApiClient.scala | 9 +- .../runtimes/RuntimeCreationDiskSpec.scala | 7 +- http/src/main/resources/swagger/api-docs.yaml | 21 -- .../workbench/leonardo/dao/sam/SamUtils.scala | 104 +++++-- .../leonardo/db/DiskServiceDbQueries.scala | 41 ++- .../http/AppDependenciesBuilder.scala | 4 +- .../http/AzureDependenciesBuilder.scala | 2 - .../http/GcpDependenciesBuilder.scala | 2 - .../http/service/DiskServiceInterp.scala | 245 +++++----------- .../http/service/DiskV2ServiceInterp.scala | 65 ++--- .../http/service/LeoAppServiceInterp.scala | 2 - .../leonardo/http/service/ProxyService.scala | 13 +- .../http/service/RuntimeService.scala | 3 - .../http/service/RuntimeServiceInterp.scala | 139 ++++----- .../http/service/RuntimeV2ServiceInterp.scala | 39 +-- .../leonardo/http/api/HttpRoutesSpec.scala | 2 +- .../leonardo/http/api/TestLeoRoutes.scala | 1 - .../http/service/AppServiceInterpSpec.scala | 1 + .../http/service/DiskServiceInterpSpec.scala | 267 ++++++++++++------ .../service/DiskV2ServiceInterpSpec.scala | 156 +++++----- .../service/RuntimeServiceInterpSpec.scala | 17 +- .../service/RuntimeV2ServiceInterpSpec.scala | 16 +- 22 files changed, 594 insertions(+), 562 deletions(-) diff --git a/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/LeonardoApiClient.scala b/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/LeonardoApiClient.scala index bb5400783c8..adabacb13ab 100644 --- a/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/LeonardoApiClient.scala +++ b/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/LeonardoApiClient.scala @@ -458,16 +458,11 @@ object LeonardoApiClient { } yield r def listDisk( - googleProject: GoogleProject, - includeDeleted: Boolean = false + googleProject: GoogleProject )(implicit client: Client[IO], authorization: IO[Authorization]): IO[List[ListPersistentDiskResponse]] = { val uriWithoutQueryParam = rootUri .withPath(Uri.Path.unsafeFromString(s"/api/google/v1/disks/${googleProject.value}")) - val uri = - if (includeDeleted) uriWithoutQueryParam.withQueryParam("includeDeleted", "true") - else uriWithoutQueryParam - for { traceIdHeader <- genTraceIdHeader() authHeader <- authorization @@ -475,7 +470,7 @@ object LeonardoApiClient { Request[IO]( method = Method.GET, headers = Headers(authHeader, traceIdHeader), - uri = uri + uri = uriWithoutQueryParam ) )(onError(s"Failed to list disks in project ${googleProject.value}")) } yield r diff --git a/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/runtimes/RuntimeCreationDiskSpec.scala b/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/runtimes/RuntimeCreationDiskSpec.scala index 42ad3d3be9e..fe658fe86d5 100644 --- a/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/runtimes/RuntimeCreationDiskSpec.scala +++ b/automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/runtimes/RuntimeCreationDiskSpec.scala @@ -173,10 +173,9 @@ class RuntimeCreationDiskSpec extends BillingProjectFixtureSpec with ParallelTes _ <- IO(disk.status shouldBe DiskStatus.Ready) _ <- IO(disk.size shouldBe diskSize) _ <- LeonardoApiClient.deleteDiskWithWait(googleProject, diskName) - listofDisks <- LeonardoApiClient.listDisk(googleProject, true) - } yield listofDisks.collect { case resp if resp.name == diskName => resp.status } shouldBe List( - DiskStatus.Deleted - ) // assume we won't have multiple disks with same name in the same project in tests + listofDisks <- LeonardoApiClient.listDisk(googleProject) + } yield listofDisks.collect { case resp if resp.name == diskName => resp.status } shouldBe List.empty + // assume we won't have multiple disks with same name in the same project in tests } res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } diff --git a/http/src/main/resources/swagger/api-docs.yaml b/http/src/main/resources/swagger/api-docs.yaml index 0589ec7e60d..739b14cf4f6 100644 --- a/http/src/main/resources/swagger/api-docs.yaml +++ b/http/src/main/resources/swagger/api-docs.yaml @@ -988,13 +988,6 @@ paths: required: false schema: type: string - - in: query - name: includeDeleted - description: Optional filter that includes any persistent disks with a Deleted status. - required: false - schema: - type: boolean - default: false - in: query name: includeLabels description: > @@ -1058,13 +1051,6 @@ paths: required: false schema: type: string - - in: query - name: includeDeleted - description: Optional filter that includes any persistent disks with a Deleted status. - required: false - schema: - type: boolean - default: false - in: query name: includeLabels description: > @@ -1357,13 +1343,6 @@ paths: required: false schema: type: string - - in: query - name: includeDeleted - description: Optional filter that includes any apps with a Deleted status. - required: false - schema: - type: boolean - default: false - in: query name: includeLabels description: > diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamUtils.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamUtils.scala index ea8578f0396..b6114da135e 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamUtils.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamUtils.scala @@ -5,6 +5,8 @@ import akka.http.scaladsl.model.headers.OAuth2BearerToken import cats.effect.Async import cats.implicits.{catsSyntaxApplicativeError, toFlatMapOps} import cats.mtl.Ask +import org.broadinstitute.dsde.workbench.google2.DiskName +import org.broadinstitute.dsde.workbench.leonardo.http.service.{DiskNotFoundByIdException, DiskNotFoundException} import org.broadinstitute.dsde.workbench.leonardo.model.{ ForbiddenError, LeoException, @@ -14,66 +16,110 @@ import org.broadinstitute.dsde.workbench.leonardo.model.{ import org.broadinstitute.dsde.workbench.leonardo.{ AppContext, CloudContext, + DiskId, + PersistentDiskAction, RuntimeAction, RuntimeName, + SamResourceAction, SamResourceId, WorkspaceId } -import org.broadinstitute.dsde.workbench.model.{UserInfo, WorkbenchEmail} +import org.broadinstitute.dsde.workbench.model.{TraceId, UserInfo, WorkbenchEmail} -trait SamUtils[F[_]] { - val samService: SamService[F] - - def checkRuntimeAction(userInfo: UserInfo, - cloudContext: CloudContext, - runtimeName: RuntimeName, - samResourceId: SamResourceId, - action: RuntimeAction, - userEmail: Option[WorkbenchEmail] = None +object SamUtils { + def checkRuntimeAction[F[_]](samService: SamService[F], + userInfo: UserInfo, + cloudContext: CloudContext, + runtimeName: RuntimeName, + samResourceId: SamResourceId, + action: RuntimeAction, + userEmail: Option[WorkbenchEmail] = None )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = - checkRuntimeActionInternal( + checkActionInternal( + samService, userInfo.accessToken, userEmail.getOrElse(userInfo.userEmail), samResourceId, action, + RuntimeAction.GetRuntimeStatus, RuntimeNotFoundException(cloudContext, runtimeName, "Not found in database") ) - def checkRuntimeAction(userInfo: UserInfo, - workspaceId: WorkspaceId, - runtimeName: RuntimeName, - samResourceId: SamResourceId, - action: RuntimeAction + def checkRuntimeAction[F[_]](samService: SamService[F], + userInfo: UserInfo, + workspaceId: WorkspaceId, + runtimeName: RuntimeName, + samResourceId: SamResourceId, + action: RuntimeAction )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = - checkRuntimeActionInternal( + checkActionInternal( + samService, userInfo.accessToken, userInfo.userEmail, samResourceId, action, + RuntimeAction.GetRuntimeStatus, RuntimeNotFoundByWorkspaceIdException(workspaceId, runtimeName, "Not found in database") ) - private def checkRuntimeActionInternal(userToken: OAuth2BearerToken, - userEmail: WorkbenchEmail, - samResourceId: SamResourceId, - action: RuntimeAction, - notFoundException: LeoException + def checkDiskAction[F[_]](samService: SamService[F], + userInfo: UserInfo, + cloudContext: CloudContext, + diskName: DiskName, + samResourceId: SamResourceId, + action: SamResourceAction, + traceId: TraceId + )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = + checkActionInternal( + samService, + userInfo.accessToken, + userInfo.userEmail, + samResourceId, + action, + PersistentDiskAction.ReadPersistentDisk, + DiskNotFoundException(cloudContext, diskName, traceId) + ) + + def checkDiskAction[F[_]](samService: SamService[F], + userInfo: UserInfo, + diskId: DiskId, + samResourceId: SamResourceId, + action: SamResourceAction, + traceId: TraceId + )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = + checkActionInternal( + samService, + userInfo.accessToken, + userInfo.userEmail, + samResourceId, + action, + PersistentDiskAction.ReadPersistentDisk, + DiskNotFoundByIdException(diskId, traceId) + ) + + private def checkActionInternal[F[_]](samService: SamService[F], + userToken: OAuth2BearerToken, + userEmail: WorkbenchEmail, + samResourceId: SamResourceId, + actionToCheck: SamResourceAction, + resourceReadAction: SamResourceAction, + notFoundException: LeoException )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = samService - .checkAuthorized(userToken.token, samResourceId, action) + .checkAuthorized(userToken.token, samResourceId, actionToCheck) .handleErrorWith { - // If we've already checked read access and the user doesn't have it, pretend the runtime doesn't exist to avoid leaking its existence - case e: SamException if e.statusCode == StatusCodes.Forbidden && action == RuntimeAction.GetRuntimeStatus => + // If we've already checked read access and the user doesn't have it, pretend the resource doesn't exist to avoid leaking its existence + case e: SamException if e.statusCode == StatusCodes.Forbidden && actionToCheck == resourceReadAction => F.raiseError(notFoundException) - // Check if the user can read the runtime to determine which error to raise + // Check if the user can read the resource to determine which error to raise case e: SamException if e.statusCode == StatusCodes.Forbidden => samService - .checkAuthorized(userToken.token, samResourceId, RuntimeAction.GetRuntimeStatus) + .checkAuthorized(userToken.token, samResourceId, resourceReadAction) .attempt .flatMap { - // The user can read the runtime, but they don't have the required action. Raise the original Forbidden action from Sam + // The user can read the resource, but they don't have the required action. Raise the original Forbidden action from Sam case Right(_) => F.raiseError(ForbiddenError(userEmail)) - // The user can't read the runtime, pretend it doesn't exist to avoid leaking its existence + // The user can't read the resource, pretend it doesn't exist to avoid leaking its existence case Left(_) => F.raiseError(notFoundException) } } diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/DiskServiceDbQueries.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/DiskServiceDbQueries.scala index 6ea66d18526..216a2b67b09 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/DiskServiceDbQueries.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/DiskServiceDbQueries.scala @@ -10,29 +10,52 @@ import org.broadinstitute.dsde.workbench.leonardo.db.persistentDiskQuery.unmarsh import org.broadinstitute.dsde.workbench.leonardo.http.{GetPersistentDiskResponse, GetPersistentDiskV2Response} import org.broadinstitute.dsde.workbench.leonardo.http.service.{DiskNotFoundByIdException, DiskNotFoundException} import org.broadinstitute.dsde.workbench.model.{TraceId, WorkbenchEmail} +import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.PersistentDiskSamResourceId import scala.concurrent.ExecutionContext object DiskServiceDbQueries { - def listDisks(labelMap: LabelMap, - includeDeleted: Boolean, - creatorOnly: Option[WorkbenchEmail], - cloudContextOpt: Option[CloudContext] = None, - workspaceOpt: Option[WorkspaceId] = None + def listDisksBySamIds(samDiskResourceIds: List[PersistentDiskSamResourceId], + labelMap: LabelMap, + creatorOnly: Option[WorkbenchEmail], + cloudContextOpt: Option[CloudContext] = None, + workspaceOpt: Option[WorkspaceId] = None + )(implicit + ec: ExecutionContext + ): DBIO[List[PersistentDisk]] = { + val listDiskQuery = persistentDiskQuery.tableQuery.filter(_.samResourceId inSetBind samDiskResourceIds) + + filterListDisks(listDiskQuery, labelMap, creatorOnly, cloudContextOpt, workspaceOpt) + } + def listDisks( + labelMap: LabelMap, + creatorOnly: Option[WorkbenchEmail], + cloudContextOpt: Option[CloudContext] = None, + workspaceOpt: Option[WorkspaceId] = None + )(implicit + ec: ExecutionContext + ): DBIO[List[PersistentDisk]] = + filterListDisks(persistentDiskQuery.tableQuery, labelMap, creatorOnly, cloudContextOpt, workspaceOpt) + + private def filterListDisks( + baseQuery: Query[PersistentDiskTable, PersistentDiskRecord, Seq], + labelMap: LabelMap, + creatorOnly: Option[WorkbenchEmail], + cloudContextOpt: Option[CloudContext] = None, + workspaceOpt: Option[WorkspaceId] = None )(implicit ec: ExecutionContext ): DBIO[List[PersistentDisk]] = { // filtered by creator first as it may have great impact val diskQueryFilteredByCreator = creatorOnly match { - case Some(email) => persistentDiskQuery.tableQuery.filter(_.creator === email) - case None => persistentDiskQuery.tableQuery + case Some(email) => baseQuery.filter(_.creator === email) + case None => baseQuery } val diskQueryFilteredByDeletion = - if (includeDeleted) diskQueryFilteredByCreator - else diskQueryFilteredByCreator.filterNot(_.status === (DiskStatus.Deleted: DiskStatus)) + diskQueryFilteredByCreator.filterNot(_.status === (DiskStatus.Deleted: DiskStatus)) val diskQueryFilteredByProject = cloudContextOpt.fold(diskQueryFilteredByDeletion)(p => diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AppDependenciesBuilder.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AppDependenciesBuilder.scala index 0b9c5d6756c..8e78839e2f7 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AppDependenciesBuilder.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AppDependenciesBuilder.scala @@ -91,9 +91,9 @@ class AppDependenciesBuilder(baselineDependenciesBuilder: BaselineDependenciesBu ): Resource[IO, ServicesDependencies] = { val statusService = new StatusService(baselineDependencies.samDAO, dbReference) val diskV2Service = new DiskV2ServiceInterp[IO]( - baselineDependencies.authProvider, baselineDependencies.publisherQueue, - baselineDependencies.wsmClientProvider + baselineDependencies.wsmClientProvider, + baselineDependencies.samService ) val azureService = new RuntimeV2ServiceInterp[IO]( diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AzureDependenciesBuilder.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AzureDependenciesBuilder.scala index 5d2faeaf24c..19038d6938b 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AzureDependenciesBuilder.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/AzureDependenciesBuilder.scala @@ -93,7 +93,6 @@ class AzureDependenciesBuilder extends CloudDependenciesBuilder { // Needed for v1 APIs val diskService = new DiskServiceInterp[IO]( ConfigReader.appConfig.persistentDisk, - baselineDependencies.authProvider, baselineDependencies.publisherQueue, None, None, @@ -103,7 +102,6 @@ class AzureDependenciesBuilder extends CloudDependenciesBuilder { val runtimeService = RuntimeService( baselineDependencies.runtimeServicesConfig, ConfigReader.appConfig.persistentDisk, - baselineDependencies.authProvider, baselineDependencies.dockerDAO, None, None, diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/GcpDependenciesBuilder.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/GcpDependenciesBuilder.scala index eacc768daf8..ce501eb2899 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/GcpDependenciesBuilder.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/GcpDependenciesBuilder.scala @@ -274,7 +274,6 @@ class GcpDependencyBuilder extends CloudDependenciesBuilder { val diskService = new DiskServiceInterp[IO]( ConfigReader.appConfig.persistentDisk, - baselineDependencies.authProvider, baselineDependencies.publisherQueue, Some(gcpDependencies.googleDiskService), Some(gcpDependencies.googleProjectDAO), @@ -284,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), diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala index b46638dce31..b67780fd5dc 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala @@ -4,7 +4,6 @@ package service import akka.http.scaladsl.model.StatusCodes import cats.Parallel -import cats.data.NonEmptyList import cats.effect.Async import cats.effect.std.Queue import cats.mtl.Ask @@ -12,11 +11,9 @@ import cats.syntax.all._ import com.google.api.services.cloudresourcemanager.model.Ancestor import org.broadinstitute.dsde.workbench.google.GoogleProjectDAO import org.broadinstitute.dsde.workbench.google2.{DiskName, GoogleDiskService} -import org.broadinstitute.dsde.workbench.leonardo.JsonCodec._ import org.broadinstitute.dsde.workbench.leonardo.config.PersistentDiskConfig import org.broadinstitute.dsde.workbench.leonardo.db._ import org.broadinstitute.dsde.workbench.leonardo.http.service.DiskServiceInterp._ -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.LeoPubsubMessage.{ @@ -30,12 +27,11 @@ import org.broadinstitute.dsde.workbench.model.{TraceId, UserInfo, WorkbenchEmai import java.time.Instant import java.util.UUID import org.broadinstitute.dsde.workbench.leonardo.SamResourceId._ -import org.broadinstitute.dsde.workbench.leonardo.dao.sam.SamService +import org.broadinstitute.dsde.workbench.leonardo.dao.sam.{SamException, SamService, SamUtils} import scala.concurrent.ExecutionContext class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, - authProvider: LeoAuthProvider[F], publisherQueue: Queue[F, LeoPubsubMessage], googleDiskService: Option[GoogleDiskService[F]], googleProjectDAO: Option[GoogleProjectDAO], @@ -58,12 +54,14 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, // Resolve the user email in Sam from the user token. This translates a pet token to the owner email. userEmail <- samService.getUserEmail(userInfo.accessToken.token) - 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) + } // Grab the pet service account for the user petSA <- samService.getPetServiceAccount(userInfo.accessToken.token, googleProject) @@ -85,18 +83,16 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, googleProject ) - disk <- F.fromEither( - convertToDisk(userEmail, - petSA, - cloudContext, - diskName, - samResource, - config, - req, - ctx.now, - sourceDiskOpt, - parentWorkspaceId - ) + disk = convertToDisk(userEmail, + petSA, + cloudContext, + diskName, + samResource, + config, + req, + ctx.now, + sourceDiskOpt, + parentWorkspaceId ) // Create a persistent-disk Sam resource with a creator policy and the google project as the parent _ <- samService.createResource(userInfo.accessToken.token, @@ -180,24 +176,15 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, for { ctx <- as.ask - // throw 403 if no project-level permission - hasProjectPermission <- authProvider.isUserProjectReader( - cloudContext, - userInfo - ) - _ <- F.raiseWhen(!hasProjectPermission)(ForbiddenError(userInfo.userEmail, Some(ctx.traceId))) - resp <- DiskServiceDbQueries.getGetPersistentDiskResponse(cloudContext, diskName, ctx.traceId).transaction - hasPermission <- authProvider.hasPermissionWithProjectFallback[PersistentDiskSamResourceId, PersistentDiskAction]( - resp.samResource, - PersistentDiskAction.ReadPersistentDisk, - ProjectAction.ReadPersistentDisk, - userInfo, - GoogleProject(cloudContext.asString) - ) // TODO: update this to support azure - _ <- - if (hasPermission) F.unit - else F.raiseError[Unit](DiskNotFoundException(cloudContext, diskName, ctx.traceId)) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + diskName, + resp.samResource, + PersistentDiskAction.ReadPersistentDisk, + ctx.traceId + ) } yield resp @@ -207,72 +194,29 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, for { ctx <- as.ask - // throw 403 if user doesn't have project permission - hasProjectPermission <- cloudContext.traverse(cc => - authProvider.isUserProjectReader( - cc, - userInfo - ) - ) - _ <- F.raiseWhen(!hasProjectPermission.getOrElse(true))(ForbiddenError(userInfo.userEmail, Some(ctx.traceId))) + samDiskIds <- samService.listResources(userInfo.accessToken.token, SamResourceType.PersistentDisk) paramMap <- F.fromEither(processListParameters(params)) creatorOnly <- F.fromEither(processCreatorOnlyParameter(userInfo.userEmail, params, ctx.traceId)) - disks <- DiskServiceDbQueries.listDisks(paramMap._1, paramMap._2, creatorOnly, cloudContext).transaction - partition = disks.partition(_.cloudContext.isInstanceOf[CloudContext.Gcp]) - _ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done DB call"))) - - gcpDiskAndProjects = partition._1.map(d => (GoogleProject(d.cloudContext.asString), d.samResource)) - gcpSamVisibleDisksOpt <- NonEmptyList.fromList(gcpDiskAndProjects).traverse { ds => - authProvider - .filterResourceProjectVisible(ds, userInfo) - } - - // TODO: use filterUserVisible (and remove old function) or make filterResourceProjectVisible handle both Azure and GCP - azureDiskAndProjects = partition._2.map(d => (GoogleProject(d.cloudContext.asString), d.samResource)) - azureSamVisibleDisksOpt <- NonEmptyList.fromList(azureDiskAndProjects).traverse { ds => - authProvider - .filterUserVisibleWithProjectFallback(ds, userInfo) - } - - samVisibleDisksOpt = (gcpSamVisibleDisksOpt, azureSamVisibleDisksOpt) match { - case (Some(a), Some(b)) => Some(a ++ b) - case (Some(a), None) => Some(a) - case (None, Some(b)) => Some(b) - case (None, None) => None - } - - _ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done checking Sam permission"))) - res = samVisibleDisksOpt match { - case None => Vector.empty - case Some(samVisibleDisks) => - val samVisibleDisksSet = samVisibleDisks.toSet - disks - .filter(d => - samVisibleDisksSet.contains( - (GoogleProject(d.cloudContext.asString), d.samResource) - ) - ) - .map(d => - ListPersistentDiskResponse(d.id, - d.cloudContext, - d.zone, - d.name, - d.status, - d.auditInfo, - d.size, - d.diskType, - d.blockSize, - d.labels.filter(l => paramMap._3.contains(l._1)), - d.workspaceId - ) - ) - .toVector - } - // We authenticate actions on resources. If there are no visible disks, - // we need to check if user should be able to see the empty list. - _ <- if (res.isEmpty) authProvider.checkUserEnabled(userInfo) else F.unit - } yield res + disks <- DiskServiceDbQueries + .listDisksBySamIds(samDiskIds.map(PersistentDiskSamResourceId), paramMap._1, creatorOnly, cloudContext) + .transaction + } yield disks + .map(d => + ListPersistentDiskResponse(d.id, + d.cloudContext, + d.zone, + d.name, + d.status, + d.auditInfo, + d.size, + d.diskType, + d.blockSize, + d.labels.filter(l => paramMap._3.contains(l._1)), + d.workspaceId + ) + ) + .toVector override def deleteDisk(userInfo: UserInfo, googleProject: GoogleProject, diskName: DiskName)(implicit as: Ask[F, AppContext] @@ -281,33 +225,19 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, ctx <- as.ask cloudContext = CloudContext.Gcp(googleProject) - // throw 403 if no project-level permission - hasProjectPermission <- authProvider.isUserProjectReader( - cloudContext, - userInfo - ) - _ <- F.raiseWhen(!hasProjectPermission)(ForbiddenError(userInfo.userEmail, Some(ctx.traceId))) - // throw 404 if not existent diskOpt <- persistentDiskQuery.getActiveByName(cloudContext, diskName).transaction disk <- diskOpt.fold(F.raiseError[PersistentDisk](DiskNotFoundException(cloudContext, diskName, ctx.traceId)))( F.pure ) - // throw 404 if no ReadPersistentDisk permission - // Note: the general pattern is to 404 (e.g. pretend the disk doesn't exist) if the caller doesn't have - // ReadPersistentDisk permission. We return 403 if the user can view the disk but can't perform some other action. - listOfPermissions <- authProvider.getActionsWithProjectFallback(disk.samResource, googleProject, userInfo) - hasReadPermission = listOfPermissions._1.toSet - .contains(PersistentDiskAction.ReadPersistentDisk) || listOfPermissions._2.toSet - .contains(ProjectAction.ReadPersistentDisk) - _ <- - if (hasReadPermission) F.unit - else F.raiseError[Unit](DiskNotFoundException(cloudContext, diskName, ctx.traceId)) - // throw 403 if no DeleteDisk permission - hasDeletePermission = listOfPermissions._1.toSet - .contains(PersistentDiskAction.DeletePersistentDisk) || listOfPermissions._2.toSet - .contains(ProjectAction.DeletePersistentDisk) - _ <- if (hasDeletePermission) F.unit else F.raiseError[Unit](ForbiddenError(userInfo.userEmail)) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + diskName, + disk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId + ) // throw 409 if the disk is not deletable _ <- if (disk.status.isDeletable) F.unit @@ -365,20 +295,14 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, .getGetPersistentDiskResponse(cloudContext, disk.name, ctx.traceId) .transaction - listOfPermissions <- authProvider.getActions(dbdisk.samResource, userInfo) - - // throw 404 if no ReadDiskStatus permission - hasPermission = listOfPermissions.toSet.contains(PersistentDiskAction.ReadPersistentDisk) - _ <- - if (hasPermission) F.unit - else - F.raiseError[Unit]( - DiskNotFoundException(cloudContext, disk.name, ctx.traceId) - ) - - // throw 403 if no DeleteDisk permission - hasDeletePermission = listOfPermissions.toSet.contains(PersistentDiskAction.DeletePersistentDisk) - _ <- if (hasDeletePermission) F.unit else F.raiseError[Unit](ForbiddenError(userInfo.userEmail)) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + dbdisk.name, + dbdisk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId + ) // Mark the resource as deleted in Leo's DB _ <- dbReference.inTransaction(persistentDiskQuery.delete(disk.id, ctx.now)) @@ -409,34 +333,22 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, ctx <- as.ask cloudContext = CloudContext.Gcp(googleProject) - // throw 403 if no project-level permission - hasProjectPermission <- authProvider.isUserProjectReader( - cloudContext, - userInfo - ) - _ <- F.raiseWhen(!hasProjectPermission)(ForbiddenError(userInfo.userEmail, Some(ctx.traceId))) - // throw 404 if not existent diskOpt <- persistentDiskQuery.getActiveByName(cloudContext, diskName).transaction disk <- diskOpt.fold(F.raiseError[PersistentDisk](DiskNotFoundException(cloudContext, diskName, ctx.traceId)))( F.pure ) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + diskName, + disk.samResource, + PersistentDiskAction.ModifyPersistentDisk, + ctx.traceId + ) // throw 400 if UpdateDiskRequest new size is smaller than disk's current size _ <- if (req.size.gb > disk.size.gb) for { - // throw 404 if no ReadPersistentDisk permission - // Note: the general pattern is to 404 (e.g. pretend the disk doesn't exist) if the caller doesn't have - // ReadPersistentDisk permission. We return 403 if the user can view the disk but can't perform some other action. - listOfPermissions <- authProvider.getActionsWithProjectFallback(disk.samResource, googleProject, userInfo) - hasReadPermission = listOfPermissions._1.toSet - .contains(PersistentDiskAction.ReadPersistentDisk) || listOfPermissions._2.toSet - .contains(ProjectAction.ReadPersistentDisk) - _ <- - if (hasReadPermission) F.unit - else F.raiseError[Unit](DiskNotFoundException(cloudContext, diskName, ctx.traceId)) - // throw 403 if no ModifyPersistentDisk permission - hasModifyPermission = listOfPermissions._1.contains(PersistentDiskAction.ModifyPersistentDisk) - _ <- if (hasModifyPermission) F.unit else F.raiseError[Unit](ForbiddenError(userInfo.userEmail)) // throw 409 if the disk is not updatable _ <- if (disk.status.isUpdatable) F.unit @@ -465,7 +377,7 @@ object DiskServiceInterp { now: Instant, sourceDisk: Option[SourceDisk], workspaceId: Option[WorkspaceId] - ): Either[Throwable, PersistentDisk] = + ): PersistentDisk = convertToDisk(userEmail, serviceAccount, cloudContext, @@ -490,7 +402,7 @@ object DiskServiceInterp { willBeUsedByGalaxy: Boolean, sourceDisk: Option[SourceDisk], workspaceId: Option[WorkspaceId] - ): Either[Throwable, PersistentDisk] = { + ): PersistentDisk = { // create a LabelMap of default labels val defaultLabels = DefaultDiskLabels( diskName, @@ -502,14 +414,7 @@ object DiskServiceInterp { // combine default and given labels val allLabels = req.labels ++ defaultLabels - for { - // check the labels do not contain forbidden keys - labels <- - if (allLabels.contains(includeDeletedKey)) - Left(IllegalLabelKeyException(includeDeletedKey)) - else - Right(allLabels) - } yield PersistentDisk( + PersistentDisk( DiskId(0), cloudContext, req.zone.getOrElse(config.defaultZone), @@ -524,7 +429,7 @@ object DiskServiceInterp { req.blockSize.getOrElse(config.defaultBlockSizeBytes), sourceDisk.flatMap(_.formattedBy), None, - labels, + allLabels, sourceDisk.map(_.diskLink), None, workspaceId diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterp.scala index 2bc4345aa6f..f4640e4245a 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterp.scala @@ -8,11 +8,8 @@ import cats.effect.Async import cats.effect.std.Queue import cats.mtl.Ask import cats.syntax.all._ -import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.{ - PersistentDiskSamResourceId, - WorkspaceResourceSamResourceId -} import org.broadinstitute.dsde.workbench.leonardo.dao._ +import org.broadinstitute.dsde.workbench.leonardo.dao.sam.{SamService, SamUtils} import org.broadinstitute.dsde.workbench.leonardo.db._ import org.broadinstitute.dsde.workbench.leonardo.model._ import org.broadinstitute.dsde.workbench.leonardo.monitor.LeoPubsubMessage @@ -23,9 +20,9 @@ import org.typelevel.log4cats.StructuredLogger import scala.concurrent.ExecutionContext class DiskV2ServiceInterp[F[_]: Parallel]( - authProvider: LeoAuthProvider[F], publisherQueue: Queue[F, LeoPubsubMessage], - wsmClientProvider: WsmApiClientProvider[F] + wsmClientProvider: WsmApiClientProvider[F], + samService: SamService[F] )(implicit F: Async[F], dbReference: DbReference[F], @@ -44,28 +41,17 @@ class DiskV2ServiceInterp[F[_]: Parallel]( .transaction // check that workspaceId is not null - workspaceId <- F.fromOption(diskResp.workspaceId, DiskWithoutWorkspaceException(diskId, ctx.traceId)) - - hasWorkspacePermission <- authProvider.isUserWorkspaceReader( - WorkspaceResourceSamResourceId(workspaceId), - userInfo + _ <- F.fromOption(diskResp.workspaceId, DiskWithoutWorkspaceException(diskId, ctx.traceId)) + + // check that user has read action on disk + _ <- SamUtils.checkDiskAction(samService, + userInfo, + diskId, + diskResp.samResource, + PersistentDiskAction.ReadPersistentDisk, + ctx.traceId ) - _ <- F.raiseUnless(hasWorkspacePermission)(ForbiddenError(userInfo.userEmail)) - - hasDiskPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction]( - diskResp.samResource, - PersistentDiskAction.ReadPersistentDisk, - userInfo - ) - - _ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done auth call for get azure disk permission"))) - _ <- F - .raiseError[Unit]( - DiskNotFoundByIdException(diskId, ctx.traceId) - ) - .whenA(!hasDiskPermission) - } yield diskResp override def deleteDisk(userInfo: UserInfo, diskId: DiskId)(implicit @@ -77,34 +63,19 @@ class DiskV2ServiceInterp[F[_]: Parallel]( disk <- F.fromOption(diskOpt, DiskNotFoundByIdException(diskId, ctx.traceId)) - // check read permission first - listOfPermissions <- authProvider.getActions(disk.samResource, userInfo) - hasReadPermission = listOfPermissions.toSet.contains( - PersistentDiskAction.ReadPersistentDisk + _ <- SamUtils.checkDiskAction(samService, + userInfo, + diskId, + disk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId ) - _ <- F - .raiseError[Unit](DiskNotFoundByIdException(diskId, ctx.traceId)) - .whenA(!hasReadPermission) - - // check delete permission - hasDeletePermission = listOfPermissions.toSet.contains( - PersistentDiskAction.DeletePersistentDisk - ) - _ <- F - .raiseError[Unit](ForbiddenError(userInfo.userEmail)) - .whenA(!hasDeletePermission) _ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done auth call for delete azure disk permission"))) // check that workspaceId is not null workspaceId <- F.fromOption(disk.workspaceId, DiskWithoutWorkspaceException(diskId, ctx.traceId)) - hasWorkspacePermission <- authProvider.isUserWorkspaceReader( - WorkspaceResourceSamResourceId(workspaceId), - userInfo - ) - _ <- F.raiseUnless(hasWorkspacePermission)(ForbiddenError(userInfo.userEmail)) - // check if disk resource is deletable in WSM wsmDiskResourceId <- disk.wsmResourceId match { case Some(wsmResourceId) => diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 7d64ef2810b..9f73d511b0a 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -249,7 +249,6 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, userEmail, petSA, appTypeToFormattedByType(req.appType), - authProvider, samService, config.leoKubernetesConfig.diskConfig, parentWorkspaceId @@ -827,7 +826,6 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, userEmail, petSA, appTypeToFormattedByType(req.appType), - authProvider, samService, config.leoKubernetesConfig.diskConfig ) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/ProxyService.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/ProxyService.scala index 65573e74be7..a232d291c69 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/ProxyService.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/ProxyService.scala @@ -93,15 +93,14 @@ class ProxyService( samDAO: SamDAO[IO], googleTokenCache: Cache[IO, String, (UserInfo, Instant)], samResourceCache: Cache[IO, SamResourceCacheKey, (Option[String], Option[AppAccessScope])], - val samService: SamService[IO] + samService: SamService[IO] )(implicit val system: ActorSystem, executionContext: ExecutionContext, dbRef: DbReference[IO], loggerIO: StructuredLogger[IO], metrics: OpenTelemetryMetrics[IO] -) extends LazyLogging - with SamUtils[IO] { +) extends LazyLogging { val httpsConnectionContext = ConnectionContext.httpsClient(sslContext) val clientConnectionSettings = ClientConnectionSettings(system).withTransport(ClientTransport.withCustomResolver(proxyResolver.resolveAkka)) @@ -271,7 +270,13 @@ class ProxyService( ctx <- ev.ask[AppContext] samResource <- getCachedRuntimeSamResource(RuntimeCacheKey(cloudContext, runtimeName)) - _ <- checkRuntimeAction(userInfo, cloudContext, runtimeName, samResource, RuntimeAction.ConnectToRuntime) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtimeName, + samResource, + RuntimeAction.ConnectToRuntime + ) hostStatus <- getRuntimeTargetHost(cloudContext, runtimeName) _ <- hostStatus match { diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeService.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeService.scala index 897564b50e3..396a5b33003 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeService.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeService.scala @@ -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 @@ -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]], @@ -85,7 +83,6 @@ object RuntimeService { new RuntimeServiceInterp( config, diskConfig, - authProvider, dockerDAO, googleStorageService, googleComputeService, diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala index 6df5e789be1..e1a665bb903 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala @@ -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.{ @@ -61,20 +56,18 @@ 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]], publisherQueue: Queue[F, LeoPubsubMessage], - val samService: SamService[F] + samService: SamService[F] )(implicit F: Async[F], log: StructuredLogger[F], dbReference: DbReference[F], ec: ExecutionContext, metrics: OpenTelemetryMetrics[F] -) extends RuntimeService[F] - with SamUtils[F] { +) extends RuntimeService[F] { override def createRuntime( userInfo: UserInfo, @@ -163,7 +156,6 @@ class RuntimeServiceInterp[F[_]: Parallel]( userEmail, petSA, FormattedBy.GCE, - authProvider, samService, diskConfig, parentWorkspaceId @@ -240,7 +232,13 @@ class RuntimeServiceInterp[F[_]: Parallel]( for { // throws 404 if not existent resp <- RuntimeServiceDbQueries.getRuntime(cloudContext, runtimeName).transaction - _ <- checkRuntimeAction(userInfo, cloudContext, runtimeName, resp.samResource, RuntimeAction.GetRuntimeStatus) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtimeName, + resp.samResource, + RuntimeAction.GetRuntimeStatus + ) } yield resp override def listRuntimes(userInfo: UserInfo, cloudContext: Option[CloudContext], params: Map[String, String])( @@ -381,11 +379,12 @@ class RuntimeServiceInterp[F[_]: Parallel]( ): F[Unit] = for { ctx <- as.ask - _ <- checkRuntimeAction(userInfo, - cloudContext, - runtime.clusterName, - runtime.samResource, - RuntimeAction.DeleteRuntime + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtime.clusterName, + runtime.samResource, + RuntimeAction.DeleteRuntime ) // Mark the resource as deleted in Leo's DB @@ -457,11 +456,12 @@ class RuntimeServiceInterp[F[_]: Parallel]( F.raiseError[ClusterRecord](RuntimeNotFoundException(cloudContext, runtimeName, "no record in database")) )(F.pure) - _ <- checkRuntimeAction(userInfo, - cloudContext, - runtimeName, - RuntimeSamResourceId(runtime.internalId), - RuntimeAction.ModifyRuntime + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtimeName, + RuntimeSamResourceId(runtime.internalId), + RuntimeAction.ModifyRuntime ) // throw 409 if the cluster is not updatable _ <- @@ -821,7 +821,14 @@ class RuntimeServiceInterp[F[_]: Parallel]( F.raiseError[Runtime](RuntimeNotFoundException(cloudContext, runtimeName, "Not found in database")) )(F.pure) - _ <- checkRuntimeAction(userInfo, cloudContext, runtimeName, runtime.samResource, action, userEmail) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtimeName, + runtime.samResource, + action, + userEmail + ) } yield runtime } @@ -926,7 +933,6 @@ object RuntimeServiceInterp { userEmail: WorkbenchEmail, serviceAccount: WorkbenchEmail, willBeUsedBy: FormattedBy, - authProvider: LeoAuthProvider[F], samService: SamService[F], diskConfig: PersistentDiskConfig, workspaceId: Option[WorkspaceId] @@ -943,6 +949,14 @@ object RuntimeServiceInterp { disk <- diskOpt match { case Some(pd) => for { + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + pd.name, + pd.samResource, + PersistentDiskAction.AttachPersistentDisk, + ctx.traceId + ) _ <- if (pd.zone == targetZone) F.unit else @@ -977,38 +991,31 @@ 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)) - samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString)) - diskBeforeSave <- F.fromEither( - DiskServiceInterp.convertToDisk( - userEmail, - serviceAccount, - cloudContext, - req.name, - samResource, - diskConfig, - CreateDiskRequest.fromDiskConfigRequest(req, Some(targetZone)), - ctx.now, - willBeUsedBy == FormattedBy.Galaxy, - None, - workspaceId + _ <- 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 = DiskServiceInterp.convertToDisk( + userEmail, + serviceAccount, + cloudContext, + req.name, + samResource, + diskConfig, + CreateDiskRequest.fromDiskConfigRequest(req, Some(targetZone)), + ctx.now, + willBeUsedBy == FormattedBy.Galaxy, + None, + workspaceId ) // Create a persistent-disk Sam resource with a creator policy and the google project as the parent _ <- samService.createResource( @@ -1032,7 +1039,6 @@ object RuntimeServiceInterp { userEmail: WorkbenchEmail, serviceAccount: WorkbenchEmail, willBeUsedBy: FormattedBy, - authProvider: LeoAuthProvider[F], samService: SamService[F], diskConfig: PersistentDiskConfig )(implicit @@ -1047,6 +1053,14 @@ object RuntimeServiceInterp { disk <- diskOpt match { case Some(pd) => for { + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + pd.name, + pd.samResource, + PersistentDiskAction.AttachPersistentDisk, + ctx.traceId + ) _ <- if (pd.zone == targetZone) F.unit else @@ -1081,25 +1095,21 @@ 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( + diskBeforeSave = DiskServiceInterp.convertToDisk( userEmail, serviceAccount, @@ -1113,7 +1123,6 @@ object RuntimeServiceInterp { None, Some(workspaceId) ) - ) // Create a persistent-disk Sam resource with a creator policy and the workspace as the parent _ <- samService.createResource(userInfo.accessToken.token, samResource, diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterp.scala index e2896512968..76c25be9527 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterp.scala @@ -42,10 +42,9 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( publisherQueue: Queue[F, LeoPubsubMessage], dateAccessUpdaterQueue: Queue[F, UpdateDateAccessedMessage], wsmClientProvider: WsmApiClientProvider[F], - val samService: SamService[F] + samService: SamService[F] )(implicit F: Async[F], dbReference: DbReference[F], ec: ExecutionContext, log: StructuredLogger[F]) - extends RuntimeV2Service[F] - with SamUtils[F] { + extends RuntimeV2Service[F] { override def createRuntime( userInfo: UserInfo, @@ -137,7 +136,6 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( disks <- DiskServiceDbQueries .listDisks( Map.empty, - includeDeleted = false, Some(userEmail), Some(cloudContext), Some(workspaceId) @@ -164,7 +162,7 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( case false => for { samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString)) - pd <- F.fromEither( + pd = convertToDisk( userEmail, cloudContext, @@ -175,7 +173,6 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( workspaceId, ctx.now ) - ) // Create a persistent-disk Sam resource with a creator policy and the workspace as the parent _ <- samService.createResource(userInfo.accessToken.token, samResource, @@ -238,7 +235,13 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( ctx <- as.ask runtime <- RuntimeServiceDbQueries.getRuntimeByWorkspaceId(workspaceId, runtimeName).transaction - _ <- checkRuntimeAction(userInfo, workspaceId, runtimeName, runtime.samResource, RuntimeAction.GetRuntimeStatus) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + workspaceId, + runtimeName, + runtime.samResource, + RuntimeAction.GetRuntimeStatus + ) _ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done auth call for get azure runtime permission"))) } yield runtime @@ -367,7 +370,8 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( ctx <- as.ask runtime <- RuntimeServiceDbQueries.getRuntimeByWorkspaceId(workspaceId, runtimeName).transaction - _ <- checkRuntimeAction( + _ <- SamUtils.checkRuntimeAction( + samService, userInfo, workspaceId, runtimeName, @@ -446,7 +450,7 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( req: CreateAzureRuntimeRequest, workspaceId: WorkspaceId, now: Instant - ): Either[Throwable, PersistentDisk] = { + ): PersistentDisk = { // create a LabelMap of default labels val defaultLabelMap: LabelMap = Map( @@ -458,14 +462,7 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( // combine default and given labels val allLabels = req.azureDiskConfig.labels ++ defaultLabelMap - for { - // check the labels do not contain forbidden keys - labels <- - if (allLabels.contains(includeDeletedKey)) - Left(IllegalLabelKeyException(includeDeletedKey)) - else - Right(allLabels) - } yield PersistentDisk( + PersistentDisk( DiskId(0), cloudContext, ZoneName("unset"), @@ -494,7 +491,13 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( )(implicit as: Ask[F, AppContext]): F[ClusterRecord] = for { runtime <- RuntimeServiceDbQueries.getActiveRuntimeRecord(workspaceId, runtimeName).transaction - _ <- checkRuntimeAction(userInfo, workspaceId, runtimeName, RuntimeSamResourceId(runtime.internalId), action) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + workspaceId, + runtimeName, + RuntimeSamResourceId(runtime.internalId), + action + ) } yield runtime private def errorHandler(runtimeId: Long, ctx: AppContext): Throwable => F[Unit] = diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/HttpRoutesSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/HttpRoutesSpec.scala index 3cd05336896..44941c71f23 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/HttpRoutesSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/HttpRoutesSpec.scala @@ -532,7 +532,7 @@ class HttpRoutesSpec // Tests only parameter parsing, not service logic. it should "list runtimes v2 with labels" in isolatedDbTest { Get( - s"/api/v2/runtimes/${workspaceId.value.toString}/azure?foo=bar&includeDeleted=true" + s"/api/v2/runtimes/${workspaceId.value.toString}/azure?foo=bar" ) ~> routes.route ~> check { status shouldEqual StatusCodes.OK diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala index 8031d97abca..d73ba194a14 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala @@ -187,7 +187,6 @@ trait TestLeoRoutes { val runtimeService = RuntimeService( serviceConfig, ConfigReader.appConfig.persistentDisk, - allowListAuthProvider, new MockDockerDAO, Some(FakeGoogleStorageInterpreter), Some(FakeGoogleComputeService), diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index 84f17055227..5720723e2de 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -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) diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterpSpec.scala index d4537cefd90..f69d0a7e066 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterpSpec.scala @@ -2,6 +2,7 @@ package org.broadinstitute.dsde.workbench.leonardo package http package service +import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.model.headers.OAuth2BearerToken import cats.effect.IO import cats.mtl.Ask @@ -25,16 +26,16 @@ import org.broadinstitute.dsde.workbench.model import org.broadinstitute.dsde.workbench.model.google.GoogleProject import org.broadinstitute.dsde.workbench.model.{TraceId, UserInfo, WorkbenchEmail, WorkbenchUserId} import org.mockito.ArgumentMatchers -import org.mockito.ArgumentMatchers.any +import org.mockito.ArgumentMatchers.{any, eq => isEq} import org.mockito.Mockito._ import org.scalatest.flatspec.AnyFlatSpec import org.scalatestplus.mockito.MockitoSugar import java.time.Instant import java.util.UUID -import scala.collection.immutable.List import scala.concurrent.ExecutionContext.Implicits.global import scala.concurrent.Future +import org.broadinstitute.dsde.workbench.leonardo.dao.sam.{SamException, SamService} trait DiskServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with TestComponent with MockitoSugar { val emptyCreateDiskReq = CreateDiskRequest( @@ -52,16 +53,15 @@ trait DiskServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Test def makeDiskService(dontCloneFromTheseGoogleFolders: Vector[String] = Vector.empty, googleProjectDAO: GoogleProjectDAO = new MockGoogleProjectDAO, - allowListAuthProvider: AllowlistAuthProvider = allowListAuthProvider + samService: SamService[IO] = MockSamService ) = { val publisherQueue = QueueFactory.makePublisherQueue() val diskService = new DiskServiceInterp( ConfigReader.appConfig.persistentDisk.copy(dontCloneFromTheseGoogleFolders = dontCloneFromTheseGoogleFolders), - allowListAuthProvider, publisherQueue, Some(MockGoogleDiskService), Some(googleProjectDAO), - MockSamService + samService ) (diskService, publisherQueue) } @@ -74,8 +74,18 @@ class DiskServiceInterpTest with MockitoSugar { "DiskService" should "fail with AuthorizationError if user doesn't have project level permission" in { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val googleProject = GoogleProject("googleProject") + when(samService.getUserEmail(isEq(userInfo4.accessToken.token))(any())).thenReturn(IO.pure(userInfo4.userEmail)) + when( + samService.checkAuthorized(any(), + isEq(ProjectSamResourceId(googleProject)), + isEq(ProjectAction.CreatePersistentDisk) + )( + any() + ) + ).thenReturn(IO.raiseError(SamException.create("forbidden", 403, TraceId("")))) val res = for { d <- diskService @@ -150,7 +160,6 @@ class DiskServiceInterpTest val publisherQueue = QueueFactory.makePublisherQueue() val diskService = new DiskServiceInterp( ConfigReader.appConfig.persistentDisk.copy(dontCloneFromTheseGoogleFolders = forbiddenFolders), - allowListAuthProvider, publisherQueue, Some(new MockGoogleDiskService { override def getDisk(project: GoogleProject, zone: ZoneName, diskName: DiskName)(implicit @@ -269,17 +278,15 @@ class DiskServiceInterpTest it should "fail with BadRequestException if user doesn't have permission to source disk" in isolatedDbTest { val dummyDiskLink = "dummyDiskLink" - val authProviderMock = mock[LeoAuthProvider[IO]](defaultMockitoAnswer[IO]) val googleDiskServiceMock = mock[GoogleDiskService[IO]](defaultMockitoAnswer[IO]) - + val samService = mock[SamService[IO]] val publisherQueue = QueueFactory.makePublisherQueue() val diskService = new DiskServiceInterp( ConfigReader.appConfig.persistentDisk, - authProviderMock, publisherQueue, Some(googleDiskServiceMock), Some(new MockGoogleProjectDAO), - MockSamService + samService = samService ) val userInfoCreator = UserInfo(OAuth2BearerToken(""), WorkbenchUserId("creator"), WorkbenchEmail("creator@example.com"), 0) @@ -289,26 +296,18 @@ class DiskServiceInterpTest val googleProject = GoogleProject("project1") val diskName = DiskName("diskName1") val workspaceId = WorkspaceId(UUID.randomUUID()) - when( - authProviderMock.hasPermission(ArgumentMatchers.eq(ProjectSamResourceId(googleProject)), - ArgumentMatchers.eq(ProjectAction.CreatePersistentDisk), - ArgumentMatchers.eq(userInfoCreator) - )(any(), any()) - ).thenReturn(IO.pure(true)) - - when( - authProviderMock.hasPermission(ArgumentMatchers.eq(ProjectSamResourceId(googleProject)), - ArgumentMatchers.eq(ProjectAction.CreatePersistentDisk), - ArgumentMatchers.eq(userInfoCloner) - )(any(), any()) - ).thenReturn(IO.pure(true)) - - when( - authProviderMock.isUserProjectReader(ArgumentMatchers.eq(CloudContext.Gcp(googleProject)), - ArgumentMatchers.eq(userInfoCloner) - )(any()) - ).thenReturn(IO.pure(true)) - + when(samService.getUserEmail(isEq(userInfoCreator.accessToken.token))(any())) + .thenReturn(IO.pure(userInfoCreator.userEmail)) + when(samService.getPetServiceAccount(isEq(userInfoCreator.accessToken.token), isEq(googleProject))(any())) + .thenReturn(IO.pure(WorkbenchEmail("creatorPet@example.com"))) + when(samService.getUserEmail(isEq(userInfoCloner.accessToken.token))(any())) + .thenReturn(IO.pure(userInfoCloner.userEmail)) + when(samService.getPetServiceAccount(isEq(userInfoCloner.accessToken.token), isEq(googleProject))(any())) + .thenReturn(IO.pure(WorkbenchEmail("clonerPet@example.com"))) + when(samService.checkAuthorized(isEq(userInfoCreator.accessToken.token), any(), any())(any())).thenReturn(IO.unit) + when(samService.lookupWorkspaceParentForGoogleProject(isEq(userInfoCreator.accessToken.token), any())(any())) + .thenReturn(IO.pure(Option(workspaceId))) + when(samService.createResource(any(), any(), any(), any(), any())(any())).thenReturn(IO.unit) when( googleDiskServiceMock.getDisk(googleProject, ConfigReader.appConfig.persistentDisk.defaultZone, diskName) ).thenReturn(IO.pure(Some(Disk.newBuilder().setSelfLink(dummyDiskLink).build()))) @@ -328,14 +327,12 @@ class DiskServiceInterpTest .transaction .map { r => when( - authProviderMock.hasPermissionWithProjectFallback( - ArgumentMatchers.eq(r.samResource), - ArgumentMatchers.eq(PersistentDiskAction.ReadPersistentDisk), - ArgumentMatchers.eq(ProjectAction.ReadPersistentDisk), - ArgumentMatchers.eq(userInfoCloner), - ArgumentMatchers.eq(googleProject) - )(any[SamResourceAction[PersistentDiskSamResourceId, ReadPersistentDisk.type]], any[Ask[IO, TraceId]]) - ).thenReturn(IO.pure(false)) + samService.checkAuthorized(isEq(userInfoCloner.accessToken.token), + isEq(r.samResource), + isEq(PersistentDiskAction.ReadPersistentDisk) + )(any()) + ) + .thenReturn(IO.raiseError(SamException.create("forbidden", 403, TraceId("")))) } cloneAttempt <- diskService @@ -366,26 +363,36 @@ class DiskServiceInterpTest res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } - it should "fail to get a disk when user doesn't have project access" in isolatedDbTest { - val (diskService, _) = makeDiskService() + it should "fail to get a disk when user doesn't have access" in isolatedDbTest { + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { samResource <- IO(PersistentDiskSamResourceId(UUID.randomUUID.toString)) disk <- makePersistentDisk(None).copy(samResource = samResource).save() + _ = when( + samService.checkAuthorized(isEq(unauthorizedUserInfo.accessToken.token), + isEq(disk.samResource), + isEq(PersistentDiskAction.ReadPersistentDisk) + )(any()) + ).thenReturn(IO.raiseError(SamException.create("forbidden", 403, TraceId("")))) getResponse <- diskService.getDisk(unauthorizedUserInfo, disk.cloudContext, disk.name) } yield getResponse - a[ForbiddenError] should be thrownBy { + a[DiskNotFoundException] should be thrownBy { res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } } it should "list disks" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() disk2 <- makePersistentDisk(Some(DiskName("d2"))).save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map("includeLabels" -> "key1,key2,key4")) } yield { listResponse.map(_.id).toSet shouldBe Set(disk1.id, disk2.id) @@ -396,11 +403,14 @@ class DiskServiceInterpTest } it should "list azure and gcp disks" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1")), cloudContextOpt = Some(cloudContextGcp)).save() disk2 <- makePersistentDisk(Some(DiskName("d2")), cloudContextOpt = Some(cloudContextAzure)).save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map("includeLabels" -> "key1,key2,key4")) } yield { listResponse.map(_.id).toSet shouldBe Set(disk1.id, disk2.id) @@ -411,37 +421,32 @@ class DiskServiceInterpTest } it should "list disks with a project" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1")), cloudContextOpt = Some(cloudContextGcp)).save() disk2 <- makePersistentDisk(Some(DiskName("d2")), cloudContextOpt = Some(cloudContextGcp)).save() - _ <- makePersistentDisk(None, cloudContextOpt = Some(CloudContext.Gcp(GoogleProject("non-default")))).save() + disk3 <- makePersistentDisk(Some(DiskName("d3")), cloudContextOpt = Some(CloudContext.Gcp(project2))).save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn( + IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId, disk3.samResource.resourceId)) + ) listResponse <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map.empty) } yield listResponse.map(_.id).toSet shouldBe Set(disk1.id, disk2.id) res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } - it should "list disks with project access" in isolatedDbTest { - val (diskService, _) = makeDiskService() - - val res = for { - disk1 <- makePersistentDisk(Some(DiskName("d1")), cloudContextOpt = Some(cloudContextGcp)).save() - disk2 <- makePersistentDisk(Some(DiskName("d2")), cloudContextOpt = Some(CloudContext.Gcp(project2))).save() - _ <- makePersistentDisk(None, cloudContextOpt = Some(CloudContext.Gcp(GoogleProject("non-default")))).save() - listResponse <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map.empty) - } yield listResponse.map(_.id).toSet shouldBe Set(disk1.id) - - res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) - } - it should "list disks with parameters" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() - _ <- makePersistentDisk(Some(DiskName("d2"))).save() + disk2 <- makePersistentDisk(Some(DiskName("d2"))).save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) _ <- labelQuery.save(disk1.id.value, LabelResourceType.PersistentDisk, "foo", "bar").transaction listResponse <- diskService.listDisks(userInfo, None, Map("foo" -> "bar")) } yield listResponse.map(_.id).toSet shouldBe Set(disk1.id) @@ -450,9 +455,10 @@ class DiskServiceInterpTest } it should "list disks belonging to other users" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) - // Make disks belonging to different users than the calling user + // Make disks belonging to different users than the calling user that the calling user has access to val res = for { disk1 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))( @@ -462,17 +468,19 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user2@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map.empty) } yield - // Since the calling user is allow-listed in the auth provider, it should return - // the disks belonging to other users. + // Since the calling user has access to the disks, should see both when not filtering by role=creator listResponse.map(_.id).toSet shouldBe Set(disk1.id, disk2.id) res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } it should "list disks belonging to self and others, if not filtered by role=creator" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() @@ -480,6 +488,8 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map.empty) } yield // Since the calling user has access to both disks, should see both @@ -489,7 +499,8 @@ class DiskServiceInterpTest } it should "list disks belonging to self only, if filtered by role=creator" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() @@ -497,6 +508,8 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map("role" -> "creator")) } yield // Since the calling user created disk1 only, only disk1 is visible when filtered by role=creator @@ -506,7 +519,8 @@ class DiskServiceInterpTest } it should "fail to list disks if filtered by role=not_creator" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() @@ -514,6 +528,8 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map("role" -> "manager")) } yield listResponse @@ -523,7 +539,10 @@ class DiskServiceInterpTest } it should "delete a disk" in isolatedDbTest { - val (diskService, publisherQueue) = makeDiskService() + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + val (diskService, publisherQueue) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -546,7 +565,10 @@ class DiskServiceInterpTest } it should "fail to delete a disk if it is attached to a runtime" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + val (diskService, _) = makeDiskService(samService = samService) val res = for { t <- appContext.ask[AppContext] @@ -568,8 +590,13 @@ class DiskServiceInterpTest res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } - it should "fail to delete a disk if user lost project access" in isolatedDbTest { - val (diskService, _) = makeDiskService(allowListAuthProvider = allowListAuthProvider2) + it should "fail to delete a disk if user can view the disk but not delete it" in isolatedDbTest { + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, TraceId("")))) + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.ReadPersistentDisk))(any())) + .thenReturn(IO.unit) + val (diskService, _) = makeDiskService(samService = samService) val res = for { t <- appContext.ask[AppContext] @@ -585,14 +612,21 @@ class DiskServiceInterpTest ) ) ) - err <- diskService.deleteDisk(userInfo, GoogleProject(disk.cloudContext.asString), disk.name).attempt - } yield err shouldBe Left(ForbiddenError(userInfo.userEmail, Some(t.traceId))) + deleteResp <- diskService.deleteDisk(userInfo, GoogleProject(disk.cloudContext.asString), disk.name) + } yield deleteResp - res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + a[ForbiddenError] should be thrownBy { + res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } } it should "delete a disk records but not queue delete disk message" in isolatedDbTest { - val (diskService, publisherQueue) = makeDiskService() + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + when(samService.deleteResource(any(), any())(any())) + .thenReturn(IO.unit) + val (diskService, publisherQueue) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -600,7 +634,8 @@ class DiskServiceInterpTest disk <- makePersistentDisk(Some(DiskName("d1")), cloudContextOpt = Some(cloudContextGcp)) .copy(samResource = diskSamResource) .save() - + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map.empty) _ <- diskService.deleteDiskRecords(userInfo, cloudContextGcp, listResponse.head) @@ -619,8 +654,11 @@ class DiskServiceInterpTest } it should "fail to delete a disk records if the user does not have permission" in isolatedDbTest { - val (diskService1, _) = makeDiskService() - val (diskService2, _) = makeDiskService(allowListAuthProvider = allowListAuthProvider2) + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), any())(any())) + .thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, TraceId("")))) + when(samService.deleteResource(any(), any())(any())).thenReturn(IO.unit) + val (diskService, _) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -628,10 +666,11 @@ class DiskServiceInterpTest disk <- makePersistentDisk(Some(DiskName("d1")), cloudContextOpt = Some(cloudContextGcp)) .copy(samResource = diskSamResource) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk.samResource.resourceId))) + listResponse <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map.empty) - listResponse <- diskService1.listDisks(userInfo, Some(cloudContextGcp), Map.empty) - - err <- diskService2.deleteDiskRecords(userInfo, cloudContextGcp, listResponse.head).attempt + err <- diskService.deleteDiskRecords(userInfo, cloudContextGcp, listResponse.head).attempt status <- persistentDiskQuery .getStatus(disk.id) .transaction @@ -645,7 +684,12 @@ class DiskServiceInterpTest } it should "delete all disks records but not queue delete disk messages" in isolatedDbTest { - val (diskService, publisherQueue) = makeDiskService() + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + when(samService.deleteResource(any(), any())(any())) + .thenReturn(IO.unit) + val (diskService, publisherQueue) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -658,15 +702,16 @@ class DiskServiceInterpTest disk2 <- makePersistentDisk(Some(DiskName("d2")), cloudContextOpt = Some(cloudContextGcp)) .copy(samResource = diskSamResource2) .save() - + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) _ <- diskService.deleteAllDisksRecords(userInfo, cloudContextGcp) - disks <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map("includeDeleted" -> "true")) + disks <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map.empty) message <- publisherQueue.tryTake } yield { - disks.map(_.status) shouldEqual List(DiskStatus.Deleted, DiskStatus.Deleted) + disks shouldEqual List.empty message shouldBe None } @@ -674,7 +719,8 @@ class DiskServiceInterpTest } it should "delete all orphaned disks" in isolatedDbTest { - val (diskService, publisherQueue) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, publisherQueue) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -689,16 +735,28 @@ class DiskServiceInterpTest .save() diskSamResource3 <- IO(PersistentDiskSamResourceId(UUID.randomUUID.toString)) disk3 <- makePersistentDisk(Some(DiskName("d3")), cloudContextOpt = Some(cloudContextGcp)) - .copy(samResource = diskSamResource1) + .copy(samResource = diskSamResource3) .save() diskSamResource4 <- IO(PersistentDiskSamResourceId(UUID.randomUUID.toString)) disk4 <- makePersistentDisk(Some(DiskName("d4")), cloudContextOpt = Some(cloudContextGcp)) .copy(samResource = diskSamResource4) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn( + IO.pure( + List(disk1.samResource.resourceId, + disk2.samResource.resourceId, + disk3.samResource.resourceId, + disk4.samResource.resourceId + ) + ) + ) + _ = when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) _ <- diskService.deleteAllOrphanedDisks(userInfo, cloudContextGcp, Vector(disk1.id), Vector(disk2.name)) - disks <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map("includeDeleted" -> "true")) + disks <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map.empty) messages <- publisherQueue.tryTakeN(Some(2)) @@ -717,7 +775,8 @@ class DiskServiceInterpTest } it should "fail to delete all orphaned disks if a disk is not deletable" in isolatedDbTest { - val (diskService, publisherQueue) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, publisherQueue) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -730,10 +789,11 @@ class DiskServiceInterpTest disk2 <- makePersistentDisk(Some(DiskName("d2")), cloudContextOpt = Some(cloudContextGcp)) .copy(samResource = diskSamResource2, status = DiskStatus.Deleting) .save() - + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) _ <- diskService.deleteAllOrphanedDisks(userInfo, cloudContextGcp, Vector.empty, Vector.empty) - disks <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map("includeDeleted" -> "true")) + disks <- diskService.listDisks(userInfo, Some(cloudContextGcp), Map.empty) messages <- publisherQueue.tryTakeN(Some(2)) @@ -784,6 +844,29 @@ class DiskServiceInterpTest res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } + it should "fail to update a disk if the user does not have permission" in isolatedDbTest { + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.ModifyPersistentDisk))(any())) + .thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, TraceId("")))) + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.ReadPersistentDisk))(any())) + .thenReturn(IO.unit) + + val (diskService, _) = makeDiskService(samService = samService) + + val res = for { + t <- appContext.ask[AppContext] + diskSamResource <- IO(PersistentDiskSamResourceId(UUID.randomUUID.toString)) + disk <- makePersistentDisk(None).copy(samResource = diskSamResource).save() + req = UpdateDiskRequest(Map.empty, DiskSize(600)) + fail <- diskService + .updateDisk(userInfo, GoogleProject(disk.cloudContext.asString), disk.name, req) + } yield fail + + a[ForbiddenError] should be thrownBy { + res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } + } + it should "get a correct sam policy map for disks" in { val map = DiskServiceInterp.getDiskSamPolicyMap(userEmail) map should have size 1 diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterpSpec.scala index 3f56099cf68..47cfaf14e25 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterpSpec.scala @@ -2,6 +2,7 @@ package org.broadinstitute.dsde.workbench.leonardo package http package service +import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.model.headers.OAuth2BearerToken import cats.effect.IO import cats.effect.std.Queue @@ -10,7 +11,7 @@ import org.broadinstitute.dsde.workbench.google2.{MachineTypeName, ZoneName} import org.broadinstitute.dsde.workbench.leonardo.CommonTestData._ import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.PersistentDiskSamResourceId import org.broadinstitute.dsde.workbench.leonardo.TestUtils.appContext -import org.broadinstitute.dsde.workbench.leonardo.auth.AllowlistAuthProvider +import org.broadinstitute.dsde.workbench.leonardo.dao.sam.{SamException, SamService} import org.broadinstitute.dsde.workbench.leonardo.dao.{MockWsmClientProvider, MockWsmDAO, WsmApiClientProvider, WsmDao} import org.broadinstitute.dsde.workbench.leonardo.db._ import org.broadinstitute.dsde.workbench.leonardo.model.ForbiddenError @@ -18,7 +19,10 @@ import org.broadinstitute.dsde.workbench.leonardo.monitor.LeoPubsubMessage import org.broadinstitute.dsde.workbench.leonardo.monitor.LeoPubsubMessage.DeleteDiskV2Message import org.broadinstitute.dsde.workbench.leonardo.util.QueueFactory import org.broadinstitute.dsde.workbench.model.{UserInfo, WorkbenchEmail, WorkbenchUserId} +import org.mockito.Mockito.when +import org.mockito.ArgumentMatchers.{any, eq => isEq} import org.scalatest.flatspec.AnyFlatSpec +import org.scalatestplus.mockito.MockitoSugar.mock import org.typelevel.log4cats.StructuredLogger import java.util.UUID @@ -29,14 +33,14 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te val wsmClientProvider = new MockWsmClientProvider private def makeDiskV2Service(queue: Queue[IO, LeoPubsubMessage], - allowlistAuthProvider: AllowlistAuthProvider = allowListAuthProvider, wsmDao: WsmDao[IO] = wsmDao, - wsmClientProvider: WsmApiClientProvider[IO] = wsmClientProvider + wsmClientProvider: WsmApiClientProvider[IO] = wsmClientProvider, + samService: SamService[IO] = MockSamService ) = new DiskV2ServiceInterp[IO]( - allowlistAuthProvider, queue, - wsmClientProvider + wsmClientProvider, + samService ) val diskV2Service = makeDiskV2Service(QueueFactory.makePublisherQueue(), wsmDao = new MockWsmDAO) @@ -48,11 +52,15 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te 0 ) // this email is allowlisted val publisherQueue = QueueFactory.makePublisherQueue() - val diskV2Service = makeDiskV2Service(publisherQueue) + val samService = mock[SamService[IO]] + val diskV2Service = makeDiskV2Service(publisherQueue, samService = samService) val res = for { _ <- publisherQueue.tryTake // just to make sure there's no messages in the queue to start with pd <- makePersistentDisk().copy(status = DiskStatus.Ready).save() + _ = when( + samService.checkAuthorized(any(), isEq(pd.samResource), isEq(PersistentDiskAction.ReadPersistentDisk))(any()) + ).thenReturn(IO.unit) getResponse <- diskV2Service .getDisk( @@ -90,15 +98,19 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } - it should "fail with ForbiddenError if user doesn't have permission" in isolatedDbTest { + it should "fail with DiskNotFound if user doesn't have permission" in isolatedDbTest { val publisherQueue = QueueFactory.makePublisherQueue() - val diskV2Service = makeDiskV2Service(publisherQueue) + val samService = mock[SamService[IO]] + val diskV2Service = makeDiskV2Service(publisherQueue, samService = samService) val userInfo = UserInfo(OAuth2BearerToken(""), WorkbenchUserId("stranger"), WorkbenchEmail("stranger@example.com"), 0) val res = for { ctx <- appContext.ask[AppContext] pd <- makePersistentDisk().copy(status = DiskStatus.Ready).save() + _ = when( + samService.checkAuthorized(any(), isEq(pd.samResource), isEq(PersistentDiskAction.ReadPersistentDisk))(any()) + ).thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, ctx.traceId))) getResponse <- diskV2Service .getDisk( @@ -106,35 +118,16 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te pd.id ) .attempt - } yield getResponse shouldBe Left(ForbiddenError(WorkbenchEmail("stranger@example.com"))) - res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) - } - - it should "fail with DiskNotFound if creator loses workspace access" in isolatedDbTest { - val publisherQueue = QueueFactory.makePublisherQueue() - val diskV2service2 = makeDiskV2Service(publisherQueue, allowListAuthProvider2) - val userInfo = UserInfo(OAuth2BearerToken(""), - WorkbenchUserId("userId"), - WorkbenchEmail("user1@example.com"), - 0 - ) // this email is the disk creator, but NOT allow-listed in allowListAuthProvider2 - val res = for { - ctx <- appContext.ask[AppContext] - pd <- makePersistentDisk().copy(status = DiskStatus.Ready).save() - - getResponse <- diskV2service2 - .getDisk( - userInfo, - pd.id - ) - .attempt - } yield getResponse shouldBe Left(ForbiddenError(WorkbenchEmail("user1@example.com"))) + } yield getResponse shouldBe Left(DiskNotFoundByIdException(pd.id, ctx.traceId)) res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } it should "delete a disk" in isolatedDbTest { val publisherQueue = QueueFactory.makePublisherQueue() - val diskV2Service = makeDiskV2Service(publisherQueue) + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + val diskV2Service = makeDiskV2Service(publisherQueue, samService = samService) val res = for { ctx <- appContext.ask[AppContext] @@ -174,12 +167,11 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te IO.pure(WsmState(Some("CREATING"))) } - val diskV2Service = makeDiskV2Service(publisherQueue, wsmClientProvider = wsmClientProvider) - val userInfo = UserInfo(OAuth2BearerToken(""), - WorkbenchUserId("userId"), - WorkbenchEmail("user1@example.com"), - 0 - ) // this email is allow-listed + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + val diskV2Service = + makeDiskV2Service(publisherQueue, wsmClientProvider = wsmClientProvider, samService = samService) val res = for { ctx <- appContext.ask[AppContext] @@ -205,12 +197,11 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te IO.pure(WsmState(None)) } - val diskV2Service = makeDiskV2Service(publisherQueue, wsmClientProvider = wsmClientProvider) - val userInfo = UserInfo(OAuth2BearerToken(""), - WorkbenchUserId("userId"), - WorkbenchEmail("user1@example.com"), - 0 - ) // this email is allow-listed + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + val diskV2Service = + makeDiskV2Service(publisherQueue, wsmClientProvider = wsmClientProvider, samService = samService) val res = for { ctx <- appContext.ask[AppContext] @@ -232,12 +223,10 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te it should "fail to delete a disk if it is attached to a runtime" in isolatedDbTest { val publisherQueue = QueueFactory.makePublisherQueue() - val diskV2Service = makeDiskV2Service(publisherQueue) - val userInfo = UserInfo(OAuth2BearerToken(""), - WorkbenchUserId("userId"), - WorkbenchEmail("user1@example.com"), - 0 - ) // this email is allow-listed + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + val diskV2Service = makeDiskV2Service(publisherQueue, samService = samService) val res = for { ctx <- appContext.ask[AppContext] @@ -262,12 +251,10 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te it should "fail to delete a disk if it has no workspaceId" in isolatedDbTest { val publisherQueue = QueueFactory.makePublisherQueue() - val diskV2Service = makeDiskV2Service(publisherQueue) - val userInfo = UserInfo(OAuth2BearerToken(""), - WorkbenchUserId("userId"), - WorkbenchEmail("user1@example.com"), - 0 - ) // this email is allow-listed + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) + val diskV2Service = makeDiskV2Service(publisherQueue, samService = samService) val res = for { ctx <- appContext.ask[AppContext] @@ -287,18 +274,26 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } - it should "fail to delete a disk if its creator lost access to the workspace" in isolatedDbTest { + it should "ffail to delete a disk if the user does not have delete access and not reveal its existence if the user cannot read it" in isolatedDbTest { val publisherQueue = QueueFactory.makePublisherQueue() - val diskV2service2 = makeDiskV2Service(publisherQueue, allowListAuthProvider2) - val userInfo = UserInfo(OAuth2BearerToken(""), - WorkbenchUserId("userId"), - WorkbenchEmail("user1@example.com"), - 0 - ) // this email is the disk creator, but NOT allow-listed in allowListAuthProvider2 + val samService = mock[SamService[IO]] + val diskV2service2 = makeDiskV2Service(publisherQueue, samService = samService) + val res = for { ctx <- appContext.ask[AppContext] disk <- makePersistentDisk().copy(status = DiskStatus.Ready).save() - + _ = when( + samService.checkAuthorized(isEq(userInfo.accessToken.token), + isEq(disk.samResource), + isEq(PersistentDiskAction.DeletePersistentDisk) + )(any()) + ).thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, ctx.traceId))) + _ = when( + samService.checkAuthorized(isEq(userInfo.accessToken.token), + isEq(disk.samResource), + isEq(PersistentDiskAction.ReadPersistentDisk) + )(any()) + ).thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, ctx.traceId))) _ <- IO( makeCluster(1).saveWithRuntimeConfig( RuntimeConfig.AzureConfig(MachineTypeName("n1-standard-4"), Some(disk.id), None) @@ -311,4 +306,37 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } + + it should "fail to delete a disk if the user does not have delete access" in isolatedDbTest { + val publisherQueue = QueueFactory.makePublisherQueue() + val samService = mock[SamService[IO]] + val diskV2service2 = makeDiskV2Service(publisherQueue, samService = samService) + + val res = for { + ctx <- appContext.ask[AppContext] + disk <- makePersistentDisk().copy(status = DiskStatus.Ready).save() + _ = when( + samService.checkAuthorized(isEq(userInfo.accessToken.token), + isEq(disk.samResource), + isEq(PersistentDiskAction.DeletePersistentDisk) + )(any()) + ).thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, ctx.traceId))) + _ = when( + samService.checkAuthorized(isEq(userInfo.accessToken.token), + isEq(disk.samResource), + isEq(PersistentDiskAction.ReadPersistentDisk) + )(any()) + ).thenReturn(IO.unit) + _ <- IO( + makeCluster(1).saveWithRuntimeConfig( + RuntimeConfig.AzureConfig(MachineTypeName("n1-standard-4"), Some(disk.id), None) + ) + ) + err <- diskV2service2.deleteDisk(userInfo, disk.id).attempt + } yield err shouldBe Left( + ForbiddenError(userInfo.userEmail) + ) + + res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } } diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala index 00b2d96c3fe..9f41bf38f71 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala @@ -96,7 +96,6 @@ trait RuntimeServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with T azureServiceConfig ), ConfigReader.appConfig.persistentDisk, - authProvider, new MockDockerDAO, Some(FakeGoogleStorageInterpreter), Some(computeService), @@ -2307,7 +2306,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2361,7 +2359,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2391,7 +2388,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2418,7 +2414,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2442,7 +2437,6 @@ class RuntimeServiceInterpTest unauthorizedEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2478,7 +2472,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2503,7 +2496,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.Galaxy, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2520,7 +2512,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -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) @@ -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) diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterpSpec.scala index 90f76be3a6e..950ddd4d9f6 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterpSpec.scala @@ -289,7 +289,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with .createRuntime(userInfo, name0, workspaceId, false, defaultCreateAzureRuntimeReq) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk = disks.head now <- IO.realTimeInstant @@ -318,7 +318,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with .createRuntime(userInfo, name0, workspaceId, false, defaultCreateAzureRuntimeReq) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk = disks.head now <- IO.realTimeInstant @@ -350,7 +350,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with .createRuntime(userInfo, name0, workspaceId, false, defaultCreateAzureRuntimeReq) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk = disks.head now <- IO.realTimeInstant @@ -792,7 +792,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with defaultCreateAzureRuntimeReq ) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk = disks.head _ <- persistentDiskQuery.updateWSMResourceId(disk.id, wsmResourceId, context.now).transaction @@ -856,7 +856,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with defaultCreateAzureRuntimeReq ) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk = disks.head _ <- persistentDiskQuery.updateWSMResourceId(disk.id, wsmResourceId, context.now).transaction @@ -1020,7 +1020,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with defaultCreateAzureRuntimeReq ) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk = disks.head _ <- persistentDiskQuery.updateWSMResourceId(disk.id, wsmResourceId, context.now).transaction @@ -1062,7 +1062,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with defaultCreateAzureRuntimeReq ) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk = disks.head _ <- persistentDiskQuery.updateWSMResourceId(disk.id, wsmResourceId, context.now).transaction @@ -1300,7 +1300,7 @@ class RuntimeV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with ) disks <- DiskServiceDbQueries - .listDisks(Map.empty, includeDeleted = false, Some(userInfo.userEmail), None, Some(workspaceId)) + .listDisks(Map.empty, Some(userInfo.userEmail), None, Some(workspaceId)) .transaction disk1 <- persistentDiskQuery.getActiveByName(azureCloudContext, DiskName("diskName1")).transaction