[AN-385] Move listRuntimes over to new Sam permissions model - take 2 (…

…#4827)

automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/Leonardo.scala

@@ -64,11 +64,11 @@ object Leonardo extends RestClient with LazyLogging {
       s"api/google${versionPath}/runtimes/${googleProject.value}/${runtimeName.asString}"
     }
 
-    def listIncludingDeletedRuntime(
+    def listRuntime(
       googleProject: GoogleProject
     )(implicit token: AuthToken): Seq[ListRuntimeResponseCopy] = {
-      val path = s"api/google/v1/runtimes/${googleProject.value}?includeDeleted=true"
-      logger.info(s"Listing runtimes including deleted in project: GET /$path")
+      val path = s"api/google/v1/runtimes/${googleProject.value}"
+      logger.info(s"Listing runtimes in project: GET /$path")
       val parsedRequest = parseResponse(getRequest(s"$url/$path"))
       handleListRuntimeResponse(parsedRequest)
     }

automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/LeonardoApiClient.scala

@@ -458,24 +458,19 @@ 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
       r <- client.expectOr[List[ListPersistentDiskResponse]](
         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

automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/LeonardoTestUtils.scala

@@ -404,7 +404,7 @@ trait LeonardoTestUtils
     implicit val patienceConfig: PatienceConfig = clusterPatience
     eventually {
       val allStatus: Set[ClusterStatus] = Leonardo.cluster
-        .listIncludingDeletedRuntime(googleProject)
+        .listRuntime(googleProject)
         .filter(c => c.runtimeName == runtimeName && c.googleProject == googleProject)
         .map(_.status)
         .toSet

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)
   }

http/src/main/resources/swagger/api-docs.yaml

@@ -170,13 +170,6 @@ paths:
           required: false
           schema:
             type: string
-        - in: query
-          name: includeDeleted
-          description: Optional filter that includes any runtimes with a Deleted status.
-          required: false
-          schema:
-            type: boolean
-            default: false
       responses:
         "200":
           description: List of runtimes
@@ -225,13 +218,6 @@ paths:
           required: false
           schema:
             type: string
-        - in: query
-          name: includeDeleted
-          description: Optional filter that includes any runtimes with a Deleted status.
-          required: false
-          schema:
-            type: boolean
-            default: false
       responses:
         "200":
           description: List of runtimes
@@ -538,13 +524,6 @@ paths:
           required: false
           schema:
             type: string
-        - in: query
-          name: includeDeleted
-          description: Optional filter that includes any runtimes with a Deleted status.
-          required: false
-          schema:
-            type: boolean
-            default: false
         - in: query
           name: includeLabels
           description: >
@@ -602,13 +581,6 @@ paths:
           required: false
           schema:
             type: string
-        - in: query
-          name: includeDeleted
-          description: Optional filter that includes any runtimes with a Deleted status.
-          required: false
-          schema:
-            type: boolean
-            default: false
         - in: query
           name: includeLabels
           description: >
@@ -666,13 +638,6 @@ paths:
           required: false
           schema:
             type: string
-        - in: query
-          name: includeDeleted
-          description: Optional filter that includes any runtimes with a Deleted status.
-          required: false
-          schema:
-            type: boolean
-            default: false
         - in: query
           name: includeLabels
           description: >
@@ -1023,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: >
@@ -1093,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: >
@@ -1392,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: >

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)
             }
       }

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 =>