Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
08950a1
listDisks
marctalbott Jan 8, 2025
9194755
createDisk
marctalbott Jan 8, 2025
d7c68d9
getDiskv1
marctalbott Jan 10, 2025
03e9d12
getDiskv2
marctalbott Jan 10, 2025
9c5918e
deleteDiskv2
marctalbott Jan 10, 2025
c61d5a6
deleteDiskv1
marctalbott Jan 10, 2025
154ffcd
updateDisk
marctalbott Jan 10, 2025
06bd6ae
deleteDiskRecords
marctalbott Jan 10, 2025
9a7e442
attachDisk
marctalbott Jan 13, 2025
814a783
switch to object instead of trait
marctalbott Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 40 additions & 7 deletions http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/DiskServiceDbQueries.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 =
Expand Down
96 changes: 27 additions & 69 deletions ...ain/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@ 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
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.SamService
import org.broadinstitute.dsde.workbench.leonardo.db._
import org.broadinstitute.dsde.workbench.leonardo.http.service.DiskServiceInterp._
import org.broadinstitute.dsde.workbench.leonardo.model.SamResourceAction._
Expand All @@ -29,9 +29,6 @@ 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,
Expand Down Expand Up @@ -207,72 +204,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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, so the new behavior is that is a user does not have permission (either on the project or list disk action), sam will return an empty list of IDs, and we won't be leaking any info to the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty much -- Sam will return all of the disks that the user has access to and then the query will filter out any disks that are in a different project if cloudContext is defined. Sam doesn't do an explicit permissions check on the project, but it will only return disks that the user has permission to see.

paramMap <- F.fromEither(processListParameters(params))
creatorOnly <- F.fromEither(processCreatorOnlyParameter(userInfo.userEmail, params, ctx.traceId))
disks <- DiskServiceDbQueries.listDisks(paramMap._1, paramMap._2, creatorOnly, cloudContext).transaction
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is so nice to see this code being deleted, thank you!

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]
Expand Down
Loading