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 ea8578f039..b6114da135 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 6ea66d1852..4350bf0184 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 @@ -3,6 +3,7 @@ package db import cats.syntax.all._ import org.broadinstitute.dsde.workbench.google2.DiskName +import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.PersistentDiskSamResourceId import org.broadinstitute.dsde.workbench.leonardo.db.LeoProfile.api._ import org.broadinstitute.dsde.workbench.leonardo.db.LeoProfile.mappedColumnImplicits._ import org.broadinstitute.dsde.workbench.leonardo.db.LeoProfile.unmarshalDestroyedDate @@ -15,19 +16,51 @@ 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, + includeDeleted: Boolean, + 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, includeDeleted, creatorOnly, cloudContextOpt, workspaceOpt) + } + + def listDisks( + labelMap: LabelMap, + includeDeleted: Boolean, + creatorOnly: Option[WorkbenchEmail], + cloudContextOpt: Option[CloudContext] = None, + workspaceOpt: Option[WorkspaceId] = None + )(implicit + ec: ExecutionContext + ): DBIO[List[PersistentDisk]] = + filterListDisks(persistentDiskQuery.tableQuery, + labelMap, + includeDeleted, + creatorOnly, + cloudContextOpt, + workspaceOpt + ) + + private def filterListDisks( + baseQuery: Query[PersistentDiskTable, PersistentDiskRecord, Seq], + labelMap: LabelMap, + includeDeleted: Boolean, + 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 = 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 e2f908a0a0..0d17fdecdd 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 5d2faeaf24..19038d6938 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 eacc768daf..ce501eb289 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 b46638dce3..9dfa7fdac8 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,11 @@ 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.SamResourceId._ import org.broadinstitute.dsde.workbench.leonardo.config.PersistentDiskConfig +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._ -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.{ @@ -29,13 +28,9 @@ 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 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 +53,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) @@ -179,26 +176,15 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, ): F[GetPersistentDiskResponse] = 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 override def listDisks(userInfo: UserInfo, cloudContext: Option[CloudContext], params: Map[String, String])(implicit @@ -207,72 +193,33 @@ 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, + paramMap._2, + 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 +228,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 +298,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 +336,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 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 2bc4345aa6..c92f24bb25 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,16 @@ 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.raiseUnless(hasWorkspacePermission)(ForbiddenError(userInfo.userEmail)) - - hasDiskPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction]( - diskResp.samResource, - PersistentDiskAction.ReadPersistentDisk, - 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 ) - - _ <- 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 +62,18 @@ 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 - ) - _ <- F - .raiseError[Unit](DiskNotFoundByIdException(diskId, ctx.traceId)) - .whenA(!hasReadPermission) - - // check delete permission - hasDeletePermission = listOfPermissions.toSet.contains( - PersistentDiskAction.DeletePersistentDisk + _ <- SamUtils.checkDiskAction(samService, + userInfo, + diskId, + disk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId ) - _ <- 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 7d64ef2810..9f73d511b0 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 65573e74be..a232d291c6 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 897564b50e..396a5b3300 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 2f7c92f1e2..6f1aeb7462 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 @@ -33,11 +33,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._ @@ -61,20 +58,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 +158,6 @@ class RuntimeServiceInterp[F[_]: Parallel]( userEmail, petSA, FormattedBy.GCE, - authProvider, samService, diskConfig, parentWorkspaceId @@ -240,7 +234,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])( @@ -398,11 +398,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 @@ -474,11 +475,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 _ <- @@ -838,7 +840,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 private[service] def getAuthorizedIds( @@ -992,7 +1001,6 @@ object RuntimeServiceInterp { userEmail: WorkbenchEmail, serviceAccount: WorkbenchEmail, willBeUsedBy: FormattedBy, - authProvider: LeoAuthProvider[F], samService: SamService[F], diskConfig: PersistentDiskConfig, workspaceId: Option[WorkspaceId] @@ -1009,6 +1017,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 @@ -1043,23 +1059,19 @@ object RuntimeServiceInterp { if (isAttached) F.raiseError[Unit](DiskAlreadyAttachedException(CloudContext.Gcp(googleProject), req.name, ctx.traceId)) else F.unit - hasPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction]( - pd.samResource, - PersistentDiskAction.AttachPersistentDisk, - userInfo - ) - - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) } yield PersistentDiskRequestResult(pd, false) case None => for { - hasPermission <- authProvider.hasPermission[ProjectSamResourceId, ProjectAction]( - ProjectSamResourceId(googleProject), - ProjectAction.CreatePersistentDisk, - userInfo - ) - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) + _ <- samService + .checkAuthorized(userInfo.accessToken.token, + ProjectSamResourceId(googleProject), + ProjectAction.CreatePersistentDisk + ) + .adaptError { + case e: SamException if e.statusCode == StatusCodes.Forbidden => ForbiddenError(userEmail) + } + samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString)) diskBeforeSave <- F.fromEither( DiskServiceInterp.convertToDisk( @@ -1098,7 +1110,6 @@ object RuntimeServiceInterp { userEmail: WorkbenchEmail, serviceAccount: WorkbenchEmail, willBeUsedBy: FormattedBy, - authProvider: LeoAuthProvider[F], samService: SamService[F], diskConfig: PersistentDiskConfig )(implicit @@ -1113,6 +1124,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 @@ -1147,23 +1166,18 @@ object RuntimeServiceInterp { if (isAttached) F.raiseError[Unit](DiskAlreadyAttachedException(cloudContext, req.name, ctx.traceId)) else F.unit - hasPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction]( - pd.samResource, - PersistentDiskAction.AttachPersistentDisk, - userInfo - ) - - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) } yield PersistentDiskRequestResult(pd, false) case None => for { - hasPermission <- authProvider.hasPermission[WorkspaceResourceSamResourceId, WorkspaceAction]( - WorkspaceResourceSamResourceId(workspaceId), - WorkspaceAction.CreateControlledApplicationResource, - userInfo - ) // TODO: Correct check? - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) + _ <- samService + .checkAuthorized(userInfo.accessToken.token, + WorkspaceResourceSamResourceId(workspaceId), + WorkspaceAction.Compute + ) + .adaptError { + case e: SamException if e.statusCode == StatusCodes.Forbidden => ForbiddenError(userEmail) + } samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString)) diskBeforeSave <- F.fromEither( DiskServiceInterp.convertToDisk( 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 a498c1e206..b41e70d9c5 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 @@ -52,10 +52,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, @@ -256,7 +255,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 @@ -400,7 +405,8 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( ctx <- as.ask runtime <- RuntimeServiceDbQueries.getRuntimeByWorkspaceId(workspaceId, runtimeName).transaction - _ <- checkRuntimeAction( + _ <- SamUtils.checkRuntimeAction( + samService, userInfo, workspaceId, runtimeName, @@ -534,7 +540,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/TestLeoRoutes.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala index 6820ee2b83..7d54524504 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 @@ -188,7 +188,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 84f1705522..5720723e2d 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 d4537cefd9..7cd660bc11 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 @@ -16,6 +17,7 @@ import org.broadinstitute.dsde.workbench.leonardo.PersistentDiskAction.ReadPersi import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.{PersistentDiskSamResourceId, ProjectSamResourceId} import org.broadinstitute.dsde.workbench.leonardo.TestUtils.defaultMockitoAnswer import org.broadinstitute.dsde.workbench.leonardo.auth.AllowlistAuthProvider +import org.broadinstitute.dsde.workbench.leonardo.dao.sam.{SamException, SamService} import org.broadinstitute.dsde.workbench.leonardo.db._ import org.broadinstitute.dsde.workbench.leonardo.model._ import org.broadinstitute.dsde.workbench.leonardo.monitor.LeoPubsubMessage._ @@ -25,7 +27,7 @@ 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 @@ -52,16 +54,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 +75,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 +161,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 +279,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 +297,19 @@ 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 +329,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 +365,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 +405,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 +423,34 @@ 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() - 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() + 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)) + ) - 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) + } yield listResponse.map(_.id).toSet shouldBe Set(disk1.id, disk2.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 +459,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 +472,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 +492,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 +503,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 +512,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 +523,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 +532,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 +543,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 +569,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 +594,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 +616,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,6 +638,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) @@ -619,8 +659,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 +671,12 @@ 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 <- diskService1.listDisks(userInfo, Some(cloudContextGcp), Map.empty) + listResponse <- diskService.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 +690,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,6 +708,8 @@ 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) @@ -674,7 +726,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,12 +742,24 @@ 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)) @@ -717,7 +782,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,6 +796,8 @@ 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) @@ -784,6 +852,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 3f56099cf6..282158dd73 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 "fail 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 c238ad837c..dce120b75f 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), @@ -2354,7 +2353,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2408,7 +2406,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2438,7 +2435,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2465,7 +2461,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2489,7 +2484,6 @@ class RuntimeServiceInterpTest unauthorizedEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2525,7 +2519,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2550,7 +2543,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.Galaxy, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2567,7 +2559,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2586,6 +2577,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) @@ -2597,8 +2593,7 @@ class RuntimeServiceInterpTest unauthorizedEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, - MockSamService, + samService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) )(implicitly, implicitly, implicitly, scala.concurrent.ExecutionContext.global)