From 08950a143e92b68dd3e2004cae5c219b5f27db43 Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Wed, 8 Jan 2025 12:10:36 -0500 Subject: [PATCH 01/10] listDisks --- .../leonardo/db/DiskServiceDbQueries.scala | 47 ++++++-- .../http/service/DiskServiceInterp.scala | 96 +++++---------- .../http/service/DiskServiceInterpSpec.scala | 112 ++++++++++++------ 3 files changed, 145 insertions(+), 110 deletions(-) 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/service/DiskServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala index b46638dce3..b611ad9a29 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,8 +11,9 @@ import cats.syntax.all._ import com.google.api.services.cloudresourcemanager.model.Ancestor import org.broadinstitute.dsde.workbench.google.GoogleProjectDAO import org.broadinstitute.dsde.workbench.google2.{DiskName, GoogleDiskService} -import org.broadinstitute.dsde.workbench.leonardo.JsonCodec._ +import org.broadinstitute.dsde.workbench.leonardo.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._ @@ -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, @@ -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) 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] 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..c951363790 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 @@ -16,6 +16,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.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 +26,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,6 +53,7 @@ trait DiskServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Test def makeDiskService(dontCloneFromTheseGoogleFolders: Vector[String] = Vector.empty, googleProjectDAO: GoogleProjectDAO = new MockGoogleProjectDAO, + samService: SamService[IO] = MockSamService, allowListAuthProvider: AllowlistAuthProvider = allowListAuthProvider ) = { val publisherQueue = QueueFactory.makePublisherQueue() @@ -61,7 +63,7 @@ trait DiskServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Test publisherQueue, Some(MockGoogleDiskService), Some(googleProjectDAO), - MockSamService + samService ) (diskService, publisherQueue) } @@ -381,11 +383,14 @@ class DiskServiceInterpTest } 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 +401,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 +419,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 +455,10 @@ class DiskServiceInterpTest } it should "list disks belonging to other users" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) - // Make disks belonging to different users than the calling user + // Make disks belonging to different users than the calling user that the calling user has access to val res = for { disk1 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))( @@ -462,17 +468,19 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user2@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map.empty) } yield - // Since the calling user is allow-listed in the auth provider, it should return - // the disks belonging to other users. + // Since the calling user has access to the disks, should see both when not filtering by role=creator listResponse.map(_.id).toSet shouldBe Set(disk1.id, disk2.id) res.unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } it should "list disks belonging to self and others, if not filtered by role=creator" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() @@ -480,6 +488,8 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map.empty) } yield // Since the calling user has access to both disks, should see both @@ -489,7 +499,8 @@ class DiskServiceInterpTest } it should "list disks belonging to self only, if filtered by role=creator" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() @@ -497,6 +508,8 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map("role" -> "creator")) } yield // Since the calling user created disk1 only, only disk1 is visible when filtered by role=creator @@ -506,7 +519,8 @@ class DiskServiceInterpTest } it should "fail to list disks if filtered by role=not_creator" in isolatedDbTest { - val (diskService, _) = makeDiskService() + val samService = mock[SamService[IO]] + val (diskService, _) = makeDiskService(samService = samService) val res = for { disk1 <- makePersistentDisk(Some(DiskName("d1"))).save() @@ -514,6 +528,8 @@ class DiskServiceInterpTest disk2 <- LeoLenses.diskToCreator .set(WorkbenchEmail("a_different_user@example.com"))(makePersistentDisk(Some(DiskName("d2")))) .save() + _ = when(samService.listResources(any(), isEq(SamResourceType.PersistentDisk))(any())) + .thenReturn(IO.pure(List(disk1.samResource.resourceId, disk2.samResource.resourceId))) listResponse <- diskService.listDisks(userInfo, None, Map("role" -> "manager")) } yield listResponse @@ -592,7 +608,10 @@ class DiskServiceInterpTest } 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.deleteResource(any(), any())(any())) + .thenReturn(IO.unit) + val (diskService, publisherQueue) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -600,6 +619,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 +640,10 @@ 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.deleteResource(any(), any())(any())).thenReturn(IO.unit) + val (diskService1, _) = makeDiskService(samService = samService) + val (diskService2, _) = makeDiskService(allowListAuthProvider = allowListAuthProvider2, samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -628,6 +651,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 <- diskService1.listDisks(userInfo, Some(cloudContextGcp), Map.empty) @@ -645,7 +670,10 @@ 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.deleteResource(any(), any())(any())) + .thenReturn(IO.unit) + val (diskService, publisherQueue) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -658,6 +686,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 +704,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 +720,22 @@ 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 + ) + ) + ) _ <- diskService.deleteAllOrphanedDisks(userInfo, cloudContextGcp, Vector(disk1.id), Vector(disk2.name)) @@ -717,7 +758,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 +772,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) From 91947555541367e54360d8e649225bda3339b3f1 Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Wed, 8 Jan 2025 17:30:01 -0500 Subject: [PATCH 02/10] createDisk --- .../http/service/DiskServiceInterp.scala | 21 +++++++++++-------- .../http/service/DiskServiceInterpSpec.scala | 14 +++++++++++-- 2 files changed, 24 insertions(+), 11 deletions(-) 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 b611ad9a29..fb89fc3a59 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 @@ -13,7 +13,7 @@ import org.broadinstitute.dsde.workbench.google.GoogleProjectDAO import org.broadinstitute.dsde.workbench.google2.{DiskName, GoogleDiskService} 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.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._ @@ -36,12 +36,13 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, publisherQueue: Queue[F, LeoPubsubMessage], googleDiskService: Option[GoogleDiskService[F]], googleProjectDAO: Option[GoogleProjectDAO], - samService: SamService[F] + val samService: SamService[F] )(implicit F: Async[F], dbReference: DbReference[F], ec: ExecutionContext -) extends DiskService[F] { +) extends DiskService[F] + with SamUtils[F] { override def createDisk( userInfo: UserInfo, @@ -55,12 +56,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) 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 c951363790..c34d057b3b 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 @@ -16,7 +16,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.SamService +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._ @@ -76,8 +76,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 From d7c68d9ce4c1507b58526224ceb10ff828361a46 Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Fri, 10 Jan 2025 11:22:58 -0500 Subject: [PATCH 03/10] getDiskv1 --- .../workbench/leonardo/dao/sam/SamUtils.scala | 53 +++++++++++----- .../http/service/DiskServiceInterp.scala | 26 +++----- .../http/service/DiskServiceInterpSpec.scala | 62 +++++++++---------- 3 files changed, 75 insertions(+), 66 deletions(-) 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..d6da9b8f20 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.DiskNotFoundException import org.broadinstitute.dsde.workbench.leonardo.model.{ ForbiddenError, LeoException, @@ -14,12 +16,14 @@ import org.broadinstitute.dsde.workbench.leonardo.model.{ import org.broadinstitute.dsde.workbench.leonardo.{ AppContext, CloudContext, + 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] @@ -31,11 +35,12 @@ trait SamUtils[F[_]] { action: RuntimeAction, userEmail: Option[WorkbenchEmail] = None )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = - checkRuntimeActionInternal( + checkActionInternal( userInfo.accessToken, userEmail.getOrElse(userInfo.userEmail), samResourceId, action, + RuntimeAction.GetRuntimeStatus, RuntimeNotFoundException(cloudContext, runtimeName, "Not found in database") ) @@ -45,35 +50,53 @@ trait SamUtils[F[_]] { samResourceId: SamResourceId, action: RuntimeAction )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = - checkRuntimeActionInternal( + checkActionInternal( 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(userInfo: UserInfo, + cloudContext: CloudContext, + diskName: DiskName, + samResourceId: SamResourceId, + action: SamResourceAction, + traceId: TraceId + )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = + checkActionInternal( + userInfo.accessToken, + userInfo.userEmail, + samResourceId, + action, + PersistentDiskAction.ReadPersistentDisk, + DiskNotFoundException(cloudContext, diskName, traceId) + ) + + private def checkActionInternal(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/http/service/DiskServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala index fb89fc3a59..6a27414576 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 @@ -179,26 +179,14 @@ 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)) - + _ <- checkDiskAction(userInfo, + cloudContext, + diskName, + resp.samResource, + PersistentDiskAction.ReadPersistentDisk, + ctx.traceId + ) } yield resp override def listDisks(userInfo: UserInfo, cloudContext: Option[CloudContext], params: Map[String, String])(implicit 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 c34d057b3b..dc6fa5b17f 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 @@ -283,7 +283,7 @@ class DiskServiceInterpTest 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, @@ -291,7 +291,7 @@ class DiskServiceInterpTest publisherQueue, Some(googleDiskServiceMock), Some(new MockGoogleProjectDAO), - MockSamService + samService = samService ) val userInfoCreator = UserInfo(OAuth2BearerToken(""), WorkbenchUserId("creator"), WorkbenchEmail("creator@example.com"), 0) @@ -301,26 +301,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()))) @@ -340,14 +333,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 @@ -378,16 +369,23 @@ 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) } } From 03e9d12f218ad0e3cc38ce3240086403a2f303f0 Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Fri, 10 Jan 2025 11:54:19 -0500 Subject: [PATCH 04/10] getDiskv2 --- .../workbench/leonardo/dao/sam/SamUtils.scala | 18 ++++++- .../http/AppDependenciesBuilder.scala | 3 +- .../http/service/DiskV2ServiceInterp.scala | 36 ++++---------- .../service/DiskV2ServiceInterpSpec.scala | 49 ++++++++----------- 4 files changed, 49 insertions(+), 57 deletions(-) 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 d6da9b8f20..202a5b233c 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 @@ -6,7 +6,7 @@ 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.DiskNotFoundException +import org.broadinstitute.dsde.workbench.leonardo.http.service.{DiskNotFoundByIdException, DiskNotFoundException} import org.broadinstitute.dsde.workbench.leonardo.model.{ ForbiddenError, LeoException, @@ -16,6 +16,7 @@ import org.broadinstitute.dsde.workbench.leonardo.model.{ import org.broadinstitute.dsde.workbench.leonardo.{ AppContext, CloudContext, + DiskId, PersistentDiskAction, RuntimeAction, RuntimeName, @@ -75,6 +76,21 @@ trait SamUtils[F[_]] { DiskNotFoundException(cloudContext, diskName, traceId) ) + def checkDiskAction(userInfo: UserInfo, + diskId: DiskId, + samResourceId: SamResourceId, + action: SamResourceAction, + traceId: TraceId + )(implicit F: Async[F], as: Ask[F, AppContext]): F[Unit] = + checkActionInternal( + userInfo.accessToken, + userInfo.userEmail, + samResourceId, + action, + PersistentDiskAction.ReadPersistentDisk, + DiskNotFoundByIdException(diskId, traceId) + ) + private def checkActionInternal(userToken: OAuth2BearerToken, userEmail: WorkbenchEmail, samResourceId: SamResourceId, 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 0b9c5d6756..eea4a80a6a 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 @@ -93,7 +93,8 @@ class AppDependenciesBuilder(baselineDependenciesBuilder: BaselineDependenciesBu 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/service/DiskV2ServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskV2ServiceInterp.scala index 2bc4345aa6..bf3eb4aa72 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,9 @@ 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.SamResourceId.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 @@ -25,13 +23,15 @@ import scala.concurrent.ExecutionContext class DiskV2ServiceInterp[F[_]: Parallel]( authProvider: LeoAuthProvider[F], publisherQueue: Queue[F, LeoPubsubMessage], - wsmClientProvider: WsmApiClientProvider[F] + wsmClientProvider: WsmApiClientProvider[F], + val samService: SamService[F] )(implicit F: Async[F], dbReference: DbReference[F], ec: ExecutionContext, log: StructuredLogger[F] -) extends DiskV2Service[F] { +) extends DiskV2Service[F] + with SamUtils[F] { // backwards compatible with v1 getDisk route override def getDisk(userInfo: UserInfo, diskId: DiskId)(implicit @@ -44,28 +44,10 @@ 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 - ) - - _ <- 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) + _ <- F.fromOption(diskResp.workspaceId, DiskWithoutWorkspaceException(diskId, ctx.traceId)) + // check that user has read action on disk + _ <- checkDiskAction(userInfo, diskId, diskResp.samResource, PersistentDiskAction.ReadPersistentDisk, ctx.traceId) } yield diskResp override def deleteDisk(userInfo: UserInfo, diskId: DiskId)(implicit 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..dd12711d28 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 @@ -11,6 +12,7 @@ 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 +20,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 @@ -31,12 +36,14 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te 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 +55,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 +101,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,29 +121,7 @@ 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) } From 9c5918e0777729436bf1f1a725f7bf0fd2dea85e Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Fri, 10 Jan 2025 14:54:31 -0500 Subject: [PATCH 05/10] deleteDiskv2 --- .../http/AppDependenciesBuilder.scala | 1 - .../http/service/DiskV2ServiceInterp.scala | 26 +---- .../service/DiskV2ServiceInterpSpec.scala | 107 ++++++++++++------ 3 files changed, 72 insertions(+), 62 deletions(-) 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 eea4a80a6a..8e78839e2f 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,7 +91,6 @@ 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.samService 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 bf3eb4aa72..7df1034bd8 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,7 +8,6 @@ import cats.effect.Async import cats.effect.std.Queue import cats.mtl.Ask import cats.syntax.all._ -import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.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._ @@ -21,7 +20,6 @@ import org.typelevel.log4cats.StructuredLogger import scala.concurrent.ExecutionContext class DiskV2ServiceInterp[F[_]: Parallel]( - authProvider: LeoAuthProvider[F], publisherQueue: Queue[F, LeoPubsubMessage], wsmClientProvider: WsmApiClientProvider[F], val samService: SamService[F] @@ -59,34 +57,12 @@ 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 - ) - _ <- F - .raiseError[Unit](ForbiddenError(userInfo.userEmail)) - .whenA(!hasDeletePermission) - + _ <- checkDiskAction(userInfo, diskId, disk.samResource, PersistentDiskAction.DeletePersistentDisk, ctx.traceId) _ <- 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/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 dd12711d28..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 @@ -11,7 +11,6 @@ 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._ @@ -34,13 +33,11 @@ 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, samService: SamService[IO] = MockSamService ) = new DiskV2ServiceInterp[IO]( - allowlistAuthProvider, queue, wsmClientProvider, samService @@ -127,7 +124,10 @@ class DiskV2ServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Te 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] @@ -167,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] @@ -198,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] @@ -225,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] @@ -255,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] @@ -280,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) @@ -304,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) + } } From c61d5a6ee723d95078bca01d4e63c3ea160dbc47 Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Fri, 10 Jan 2025 15:24:08 -0500 Subject: [PATCH 06/10] deleteDiskv1 --- .../http/service/DiskServiceInterp.scala | 29 +++++------------- .../http/service/DiskServiceInterpSpec.scala | 30 ++++++++++++++----- 2 files changed, 30 insertions(+), 29 deletions(-) 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 6a27414576..cda7e05020 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 @@ -230,33 +230,18 @@ 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)) + _ <- checkDiskAction(userInfo, + cloudContext, + diskName, + disk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId + ) // throw 409 if the disk is not deletable _ <- if (disk.status.isDeletable) F.unit 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 dc6fa5b17f..67ef883e46 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 @@ -547,7 +548,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] @@ -570,7 +574,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] @@ -592,8 +599,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] @@ -609,10 +621,12 @@ 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 { @@ -744,6 +758,8 @@ class DiskServiceInterpTest ) ) ) + _ = when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.DeletePersistentDisk))(any())) + .thenReturn(IO.unit) _ <- diskService.deleteAllOrphanedDisks(userInfo, cloudContextGcp, Vector(disk1.id), Vector(disk2.name)) From 154ffcdfb4ed7e07b4523b011452e183a1748e4b Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Fri, 10 Jan 2025 15:30:08 -0500 Subject: [PATCH 07/10] updateDisk --- .../http/service/DiskServiceInterp.scala | 21 +---------------- .../http/service/DiskServiceInterpSpec.scala | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 20 deletions(-) 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 cda7e05020..12ad1b4285 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 @@ -343,34 +343,15 @@ 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 ) + _ <- checkDiskAction(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/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 67ef883e46..a07927432b 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 @@ -852,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 From 06bd6aeec3275530c8a6560700ffb2cd0e680794 Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Fri, 10 Jan 2025 15:43:40 -0500 Subject: [PATCH 08/10] deleteDiskRecords --- .../http/AzureDependenciesBuilder.scala | 1 - .../http/GcpDependenciesBuilder.scala | 1 - .../http/service/DiskServiceInterp.scala | 31 +++++++++---------- .../http/service/DiskServiceInterpSpec.scala | 20 ++++++------ 4 files changed, 24 insertions(+), 29 deletions(-) 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..5c24aaede5 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, 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..4988e9530e 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), 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 12ad1b4285..249dc2dd1b 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 @@ -16,7 +16,6 @@ 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.{ @@ -32,7 +31,6 @@ import java.util.UUID import scala.concurrent.ExecutionContext class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, - authProvider: LeoAuthProvider[F], publisherQueue: Queue[F, LeoPubsubMessage], googleDiskService: Option[GoogleDiskService[F]], googleProjectDAO: Option[GoogleProjectDAO], @@ -299,20 +297,13 @@ 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)) + _ <- checkDiskAction(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)) @@ -348,7 +339,13 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, disk <- diskOpt.fold(F.raiseError[PersistentDisk](DiskNotFoundException(cloudContext, diskName, ctx.traceId)))( F.pure ) - _ <- checkDiskAction(userInfo, cloudContext, diskName, disk.samResource, PersistentDiskAction.ModifyPersistentDisk, ctx.traceId) + _ <- checkDiskAction(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 { 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 a07927432b..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 @@ -54,13 +54,11 @@ trait DiskServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with Test def makeDiskService(dontCloneFromTheseGoogleFolders: Vector[String] = Vector.empty, googleProjectDAO: GoogleProjectDAO = new MockGoogleProjectDAO, - samService: SamService[IO] = MockSamService, - 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), @@ -163,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 @@ -282,13 +279,11 @@ 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), @@ -631,6 +626,8 @@ class DiskServiceInterpTest it should "delete a disk records but not queue delete disk message" in isolatedDbTest { 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) @@ -663,9 +660,10 @@ class DiskServiceInterpTest it should "fail to delete a disk records if the user does not have permission" in isolatedDbTest { 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 (diskService1, _) = makeDiskService(samService = samService) - val (diskService2, _) = makeDiskService(allowListAuthProvider = allowListAuthProvider2, samService = samService) + val (diskService, _) = makeDiskService(samService = samService) val res = for { context <- appContext.ask[AppContext] @@ -676,9 +674,9 @@ class DiskServiceInterpTest _ = 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 @@ -693,6 +691,8 @@ class DiskServiceInterpTest it should "delete all disks records but not queue delete disk messages" in isolatedDbTest { 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) From 9a7e4424dcbb4dc6ad069a86e98fd768cb3c8e8d Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Mon, 13 Jan 2025 15:10:42 -0500 Subject: [PATCH 09/10] attachDisk --- .../http/AzureDependenciesBuilder.scala | 1 - .../http/GcpDependenciesBuilder.scala | 1 - .../http/service/LeoAppServiceInterp.scala | 2 - .../http/service/RuntimeService.scala | 3 - .../http/service/RuntimeServiceInterp.scala | 80 +++++++++++-------- .../leonardo/http/api/TestLeoRoutes.scala | 1 - .../http/service/AppServiceInterpSpec.scala | 1 + .../service/RuntimeServiceInterpSpec.scala | 17 ++-- 8 files changed, 52 insertions(+), 54 deletions(-) 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 5c24aaede5..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 @@ -102,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 4988e9530e..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 @@ -283,7 +283,6 @@ class GcpDependencyBuilder extends CloudDependenciesBuilder { val runtimeService = RuntimeService( baselineDependencies.runtimeServicesConfig, ConfigReader.appConfig.persistentDisk, - baselineDependencies.authProvider, baselineDependencies.dockerDAO, Some(gcpDependencies.googleStorageService), Some(gcpDependencies.googleComputeService), 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/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 5aa49e7b7f..3c3157ce28 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala @@ -32,13 +32,8 @@ import org.broadinstitute.dsde.workbench.leonardo.dao.DockerDAO import org.broadinstitute.dsde.workbench.leonardo.dao.sam.{SamException, SamService, SamUtils} import org.broadinstitute.dsde.workbench.leonardo.db._ import org.broadinstitute.dsde.workbench.leonardo.http.service.DiskServiceInterp.getDiskSamPolicyMap -import org.broadinstitute.dsde.workbench.leonardo.model.SamResourceAction.{ - projectSamResourceAction, - workspaceSamResourceAction -} import org.broadinstitute.dsde.workbench.leonardo.http.service.RuntimeServiceInterp._ import org.broadinstitute.dsde.workbench.leonardo.model.SamResource.RuntimeSamResource -import org.broadinstitute.dsde.workbench.leonardo.model.SamResourceAction._ import org.broadinstitute.dsde.workbench.leonardo.model._ import org.broadinstitute.dsde.workbench.leonardo.monitor.LeoPubsubMessage._ import org.broadinstitute.dsde.workbench.leonardo.monitor.{ @@ -61,7 +56,6 @@ import scala.concurrent.ExecutionContext class RuntimeServiceInterp[F[_]: Parallel]( config: RuntimeServiceConfig, diskConfig: PersistentDiskConfig, - authProvider: LeoAuthProvider[F], dockerDAO: DockerDAO[F], googleStorageService: Option[GoogleStorageService[F]], googleComputeService: Option[GoogleComputeService[F]], @@ -163,7 +157,6 @@ class RuntimeServiceInterp[F[_]: Parallel]( userEmail, petSA, FormattedBy.GCE, - authProvider, samService, diskConfig, parentWorkspaceId @@ -926,7 +919,6 @@ object RuntimeServiceInterp { userEmail: WorkbenchEmail, serviceAccount: WorkbenchEmail, willBeUsedBy: FormattedBy, - authProvider: LeoAuthProvider[F], samService: SamService[F], diskConfig: PersistentDiskConfig, workspaceId: Option[WorkspaceId] @@ -943,6 +935,7 @@ object RuntimeServiceInterp { disk <- diskOpt match { case Some(pd) => for { + _ <- checkAttachAction(userInfo, samService, pd, cloudContext, req.name, ctx.traceId) _ <- if (pd.zone == targetZone) F.unit else @@ -977,23 +970,19 @@ object RuntimeServiceInterp { if (isAttached) F.raiseError[Unit](DiskAlreadyAttachedException(CloudContext.Gcp(googleProject), req.name, ctx.traceId)) else F.unit - hasPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction]( - pd.samResource, - PersistentDiskAction.AttachPersistentDisk, - userInfo - ) - - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) } yield PersistentDiskRequestResult(pd, false) case None => for { - hasPermission <- authProvider.hasPermission[ProjectSamResourceId, ProjectAction]( - ProjectSamResourceId(googleProject), - ProjectAction.CreatePersistentDisk, - userInfo - ) - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) + _ <- samService + .checkAuthorized(userInfo.accessToken.token, + ProjectSamResourceId(googleProject), + ProjectAction.CreatePersistentDisk + ) + .adaptError { + case e: SamException if e.statusCode == StatusCodes.Forbidden => ForbiddenError(userEmail) + } + samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString)) diskBeforeSave <- F.fromEither( DiskServiceInterp.convertToDisk( @@ -1032,7 +1021,6 @@ object RuntimeServiceInterp { userEmail: WorkbenchEmail, serviceAccount: WorkbenchEmail, willBeUsedBy: FormattedBy, - authProvider: LeoAuthProvider[F], samService: SamService[F], diskConfig: PersistentDiskConfig )(implicit @@ -1047,6 +1035,7 @@ object RuntimeServiceInterp { disk <- diskOpt match { case Some(pd) => for { + _ <- checkAttachAction(userInfo, samService, pd, cloudContext, req.name, ctx.traceId) _ <- if (pd.zone == targetZone) F.unit else @@ -1081,23 +1070,18 @@ object RuntimeServiceInterp { if (isAttached) F.raiseError[Unit](DiskAlreadyAttachedException(cloudContext, req.name, ctx.traceId)) else F.unit - hasPermission <- authProvider.hasPermission[PersistentDiskSamResourceId, PersistentDiskAction]( - pd.samResource, - PersistentDiskAction.AttachPersistentDisk, - userInfo - ) - - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) } yield PersistentDiskRequestResult(pd, false) case None => for { - hasPermission <- authProvider.hasPermission[WorkspaceResourceSamResourceId, WorkspaceAction]( - WorkspaceResourceSamResourceId(workspaceId), - WorkspaceAction.CreateControlledApplicationResource, - userInfo - ) // TODO: Correct check? - _ <- if (hasPermission) F.unit else F.raiseError[Unit](ForbiddenError(userEmail)) + _ <- samService + .checkAuthorized(userInfo.accessToken.token, + WorkspaceResourceSamResourceId(workspaceId), + WorkspaceAction.Compute + ) + .adaptError { + case e: SamException if e.statusCode == StatusCodes.Forbidden => ForbiddenError(userEmail) + } samResource <- F.delay(PersistentDiskSamResourceId(UUID.randomUUID().toString)) diskBeforeSave <- F.fromEither( DiskServiceInterp.convertToDisk( @@ -1126,6 +1110,32 @@ object RuntimeServiceInterp { } } yield disk + private def checkAttachAction[F[_]](userInfo: UserInfo, + samService: SamService[F], + pd: PersistentDisk, + cloudContext: CloudContext, + diskName: DiskName, + traceId: TraceId + )(implicit + as: Ask[F, AppContext], + F: Async[F] + ): F[Unit] = + samService + .checkAuthorized(userInfo.accessToken.token, pd.samResource, PersistentDiskAction.AttachPersistentDisk) + .handleErrorWith { + case e: SamException if e.statusCode == StatusCodes.Forbidden => + samService + .checkAuthorized(userInfo.accessToken.token, pd.samResource, PersistentDiskAction.ReadPersistentDisk) + .attempt + .flatMap { + case Left(e: SamException) if e.statusCode == StatusCodes.Forbidden => + F.raiseError(DiskNotFoundException(cloudContext, diskName, traceId)) + case Right(_) => F.raiseError(ForbiddenError(userInfo.userEmail)) + case Left(e) => F.raiseError(e) + } + case e => F.raiseError(e) + } + private[service] def calculateAutopauseThreshold( autopause: Option[Boolean], autopauseThreshold: Option[Int], 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 8031d97abc..d73ba194a1 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/TestLeoRoutes.scala @@ -187,7 +187,6 @@ trait TestLeoRoutes { val runtimeService = RuntimeService( serviceConfig, ConfigReader.appConfig.persistentDisk, - allowListAuthProvider, new MockDockerDAO, Some(FakeGoogleStorageInterpreter), Some(FakeGoogleComputeService), diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index 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/RuntimeServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala index eec9e50bf4..82e83e5129 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala @@ -96,7 +96,6 @@ trait RuntimeServiceInterpSpec extends AnyFlatSpec with LeonardoTestSuite with T azureServiceConfig ), ConfigReader.appConfig.persistentDisk, - authProvider, new MockDockerDAO, Some(FakeGoogleStorageInterpreter), Some(computeService), @@ -2307,7 +2306,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2361,7 +2359,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2391,7 +2388,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2418,7 +2414,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2442,7 +2437,6 @@ class RuntimeServiceInterpTest unauthorizedEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2478,7 +2472,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2503,7 +2496,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.Galaxy, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2520,7 +2512,6 @@ class RuntimeServiceInterpTest userEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, MockSamService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) @@ -2539,6 +2530,11 @@ class RuntimeServiceInterpTest } it should "fail to attach a disk when caller has no attach permission" in isolatedDbTest { + val samService = mock[SamService[IO]] + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.AttachPersistentDisk))(any())) + .thenReturn(IO.raiseError(SamException.create("forbidden", StatusCodes.Forbidden.intValue, TraceId("")))) + when(samService.checkAuthorized(any(), any(), isEq(PersistentDiskAction.ReadPersistentDisk))(any())) + .thenReturn(IO.unit) val res = for { savedDisk <- makePersistentDisk(None).save() req = PersistentDiskRequest(savedDisk.name, Some(savedDisk.size), Some(savedDisk.diskType), savedDisk.labels) @@ -2550,8 +2546,7 @@ class RuntimeServiceInterpTest unauthorizedEmail, serviceAccount, FormattedBy.GCE, - allowListAuthProvider, - MockSamService, + samService, ConfigReader.appConfig.persistentDisk, Some(workspaceId) )(implicitly, implicitly, implicitly, scala.concurrent.ExecutionContext.global) From 814a7831758198945187c0565fbb3bda8c4ab711 Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Mon, 13 Jan 2025 17:12:01 -0500 Subject: [PATCH 10/10] switch to object instead of trait --- .../workbench/leonardo/dao/sam/SamUtils.scala | 69 ++++++++------- .../http/service/DiskServiceInterp.scala | 57 ++++++------ .../http/service/DiskV2ServiceInterp.scala | 21 +++-- .../leonardo/http/service/ProxyService.scala | 13 ++- .../http/service/RuntimeServiceInterp.scala | 88 ++++++++++--------- .../http/service/RuntimeV2ServiceInterp.scala | 24 +++-- 6 files changed, 156 insertions(+), 116 deletions(-) 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 202a5b233c..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 @@ -26,17 +26,17 @@ import org.broadinstitute.dsde.workbench.leonardo.{ } 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] = checkActionInternal( + samService, userInfo.accessToken, userEmail.getOrElse(userInfo.userEmail), samResourceId, @@ -45,13 +45,15 @@ trait SamUtils[F[_]] { 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] = checkActionInternal( + samService, userInfo.accessToken, userInfo.userEmail, samResourceId, @@ -60,14 +62,16 @@ trait SamUtils[F[_]] { RuntimeNotFoundByWorkspaceIdException(workspaceId, runtimeName, "Not found in database") ) - def checkDiskAction(userInfo: UserInfo, - cloudContext: CloudContext, - diskName: DiskName, - samResourceId: SamResourceId, - action: SamResourceAction, - traceId: TraceId + 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, @@ -76,13 +80,15 @@ trait SamUtils[F[_]] { DiskNotFoundException(cloudContext, diskName, traceId) ) - def checkDiskAction(userInfo: UserInfo, - diskId: DiskId, - samResourceId: SamResourceId, - action: SamResourceAction, - traceId: 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, @@ -91,12 +97,13 @@ trait SamUtils[F[_]] { DiskNotFoundByIdException(diskId, traceId) ) - private def checkActionInternal(userToken: OAuth2BearerToken, - userEmail: WorkbenchEmail, - samResourceId: SamResourceId, - actionToCheck: SamResourceAction, - resourceReadAction: SamResourceAction, - notFoundException: LeoException + 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, actionToCheck) 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 249dc2dd1b..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 @@ -34,13 +34,12 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, publisherQueue: Queue[F, LeoPubsubMessage], googleDiskService: Option[GoogleDiskService[F]], googleProjectDAO: Option[GoogleProjectDAO], - val samService: SamService[F] + samService: SamService[F] )(implicit F: Async[F], dbReference: DbReference[F], ec: ExecutionContext -) extends DiskService[F] - with SamUtils[F] { +) extends DiskService[F] { override def createDisk( userInfo: UserInfo, @@ -178,12 +177,13 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, for { ctx <- as.ask resp <- DiskServiceDbQueries.getGetPersistentDiskResponse(cloudContext, diskName, ctx.traceId).transaction - _ <- checkDiskAction(userInfo, - cloudContext, - diskName, - resp.samResource, - PersistentDiskAction.ReadPersistentDisk, - ctx.traceId + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + diskName, + resp.samResource, + PersistentDiskAction.ReadPersistentDisk, + ctx.traceId ) } yield resp @@ -233,12 +233,13 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, disk <- diskOpt.fold(F.raiseError[PersistentDisk](DiskNotFoundException(cloudContext, diskName, ctx.traceId)))( F.pure ) - _ <- checkDiskAction(userInfo, - cloudContext, - diskName, - disk.samResource, - PersistentDiskAction.DeletePersistentDisk, - ctx.traceId + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + diskName, + disk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId ) // throw 409 if the disk is not deletable _ <- @@ -297,12 +298,13 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, .getGetPersistentDiskResponse(cloudContext, disk.name, ctx.traceId) .transaction - _ <- checkDiskAction(userInfo, - cloudContext, - dbdisk.name, - dbdisk.samResource, - PersistentDiskAction.DeletePersistentDisk, - ctx.traceId + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + dbdisk.name, + dbdisk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId ) // Mark the resource as deleted in Leo's DB @@ -339,12 +341,13 @@ class DiskServiceInterp[F[_]: Parallel](config: PersistentDiskConfig, disk <- diskOpt.fold(F.raiseError[PersistentDisk](DiskNotFoundException(cloudContext, diskName, ctx.traceId)))( F.pure ) - _ <- checkDiskAction(userInfo, - cloudContext, - diskName, - disk.samResource, - PersistentDiskAction.ModifyPersistentDisk, - ctx.traceId + _ <- 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 _ <- 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 7df1034bd8..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 @@ -22,14 +22,13 @@ import scala.concurrent.ExecutionContext class DiskV2ServiceInterp[F[_]: Parallel]( publisherQueue: Queue[F, LeoPubsubMessage], wsmClientProvider: WsmApiClientProvider[F], - val samService: SamService[F] + samService: SamService[F] )(implicit F: Async[F], dbReference: DbReference[F], ec: ExecutionContext, log: StructuredLogger[F] -) extends DiskV2Service[F] - with SamUtils[F] { +) extends DiskV2Service[F] { // backwards compatible with v1 getDisk route override def getDisk(userInfo: UserInfo, diskId: DiskId)(implicit @@ -45,7 +44,13 @@ class DiskV2ServiceInterp[F[_]: Parallel]( _ <- F.fromOption(diskResp.workspaceId, DiskWithoutWorkspaceException(diskId, ctx.traceId)) // check that user has read action on disk - _ <- checkDiskAction(userInfo, diskId, diskResp.samResource, PersistentDiskAction.ReadPersistentDisk, ctx.traceId) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + diskId, + diskResp.samResource, + PersistentDiskAction.ReadPersistentDisk, + ctx.traceId + ) } yield diskResp override def deleteDisk(userInfo: UserInfo, diskId: DiskId)(implicit @@ -57,7 +62,13 @@ class DiskV2ServiceInterp[F[_]: Parallel]( disk <- F.fromOption(diskOpt, DiskNotFoundByIdException(diskId, ctx.traceId)) - _ <- checkDiskAction(userInfo, diskId, disk.samResource, PersistentDiskAction.DeletePersistentDisk, ctx.traceId) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + diskId, + disk.samResource, + PersistentDiskAction.DeletePersistentDisk, + ctx.traceId + ) _ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done auth call for delete azure disk permission"))) // check that workspaceId is not null 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/RuntimeServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala index 3c3157ce28..288aa819bf 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 @@ -60,15 +60,14 @@ class RuntimeServiceInterp[F[_]: Parallel]( 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, @@ -233,7 +232,13 @@ class RuntimeServiceInterp[F[_]: Parallel]( for { // throws 404 if not existent resp <- RuntimeServiceDbQueries.getRuntime(cloudContext, runtimeName).transaction - _ <- checkRuntimeAction(userInfo, cloudContext, runtimeName, resp.samResource, RuntimeAction.GetRuntimeStatus) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtimeName, + resp.samResource, + RuntimeAction.GetRuntimeStatus + ) } yield resp override def listRuntimes(userInfo: UserInfo, cloudContext: Option[CloudContext], params: Map[String, String])( @@ -374,11 +379,12 @@ class RuntimeServiceInterp[F[_]: Parallel]( ): F[Unit] = for { ctx <- as.ask - _ <- checkRuntimeAction(userInfo, - cloudContext, - runtime.clusterName, - runtime.samResource, - RuntimeAction.DeleteRuntime + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtime.clusterName, + runtime.samResource, + RuntimeAction.DeleteRuntime ) // Mark the resource as deleted in Leo's DB @@ -450,11 +456,12 @@ class RuntimeServiceInterp[F[_]: Parallel]( F.raiseError[ClusterRecord](RuntimeNotFoundException(cloudContext, runtimeName, "no record in database")) )(F.pure) - _ <- checkRuntimeAction(userInfo, - cloudContext, - runtimeName, - RuntimeSamResourceId(runtime.internalId), - RuntimeAction.ModifyRuntime + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtimeName, + RuntimeSamResourceId(runtime.internalId), + RuntimeAction.ModifyRuntime ) // throw 409 if the cluster is not updatable _ <- @@ -814,7 +821,14 @@ class RuntimeServiceInterp[F[_]: Parallel]( F.raiseError[Runtime](RuntimeNotFoundException(cloudContext, runtimeName, "Not found in database")) )(F.pure) - _ <- checkRuntimeAction(userInfo, cloudContext, runtimeName, runtime.samResource, action, userEmail) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + cloudContext, + runtimeName, + runtime.samResource, + action, + userEmail + ) } yield runtime } @@ -935,7 +949,14 @@ object RuntimeServiceInterp { disk <- diskOpt match { case Some(pd) => for { - _ <- checkAttachAction(userInfo, samService, pd, cloudContext, req.name, ctx.traceId) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + pd.name, + pd.samResource, + PersistentDiskAction.AttachPersistentDisk, + ctx.traceId + ) _ <- if (pd.zone == targetZone) F.unit else @@ -1035,7 +1056,14 @@ object RuntimeServiceInterp { disk <- diskOpt match { case Some(pd) => for { - _ <- checkAttachAction(userInfo, samService, pd, cloudContext, req.name, ctx.traceId) + _ <- SamUtils.checkDiskAction(samService, + userInfo, + cloudContext, + pd.name, + pd.samResource, + PersistentDiskAction.AttachPersistentDisk, + ctx.traceId + ) _ <- if (pd.zone == targetZone) F.unit else @@ -1110,32 +1138,6 @@ object RuntimeServiceInterp { } } yield disk - private def checkAttachAction[F[_]](userInfo: UserInfo, - samService: SamService[F], - pd: PersistentDisk, - cloudContext: CloudContext, - diskName: DiskName, - traceId: TraceId - )(implicit - as: Ask[F, AppContext], - F: Async[F] - ): F[Unit] = - samService - .checkAuthorized(userInfo.accessToken.token, pd.samResource, PersistentDiskAction.AttachPersistentDisk) - .handleErrorWith { - case e: SamException if e.statusCode == StatusCodes.Forbidden => - samService - .checkAuthorized(userInfo.accessToken.token, pd.samResource, PersistentDiskAction.ReadPersistentDisk) - .attempt - .flatMap { - case Left(e: SamException) if e.statusCode == StatusCodes.Forbidden => - F.raiseError(DiskNotFoundException(cloudContext, diskName, traceId)) - case Right(_) => F.raiseError(ForbiddenError(userInfo.userEmail)) - case Left(e) => F.raiseError(e) - } - case e => F.raiseError(e) - } - private[service] def calculateAutopauseThreshold( autopause: Option[Boolean], autopauseThreshold: Option[Int], 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 ff0dc77519..94e382cdc9 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterp.scala @@ -42,10 +42,9 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( publisherQueue: Queue[F, LeoPubsubMessage], dateAccessUpdaterQueue: Queue[F, UpdateDateAccessedMessage], wsmClientProvider: WsmApiClientProvider[F], - val samService: SamService[F] + samService: SamService[F] )(implicit F: Async[F], dbReference: DbReference[F], ec: ExecutionContext, log: StructuredLogger[F]) - extends RuntimeV2Service[F] - with SamUtils[F] { + extends RuntimeV2Service[F] { override def createRuntime( userInfo: UserInfo, @@ -238,7 +237,13 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( ctx <- as.ask runtime <- RuntimeServiceDbQueries.getRuntimeByWorkspaceId(workspaceId, runtimeName).transaction - _ <- checkRuntimeAction(userInfo, workspaceId, runtimeName, runtime.samResource, RuntimeAction.GetRuntimeStatus) + _ <- SamUtils.checkRuntimeAction(samService, + userInfo, + workspaceId, + runtimeName, + runtime.samResource, + RuntimeAction.GetRuntimeStatus + ) _ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done auth call for get azure runtime permission"))) } yield runtime @@ -367,7 +372,8 @@ class RuntimeV2ServiceInterp[F[_]: Parallel]( ctx <- as.ask runtime <- RuntimeServiceDbQueries.getRuntimeByWorkspaceId(workspaceId, runtimeName).transaction - _ <- checkRuntimeAction( + _ <- SamUtils.checkRuntimeAction( + samService, userInfo, workspaceId, runtimeName, @@ -494,7 +500,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] =