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

marctalbott · 2025-01-13T22:12:47Z

Jira ticket: https://broadworkbench.atlassian.net/browse/CORE-211

Summary of changes

This PR may be easiest to review by stepping through the commits in order. Each commit contains a single operation authz conversion.

What

Rely on new hierarchical permissions model for Leo resources in Sam and only check one permission for each disk operation.

Why

Simplify Leo authz process

Testing these changes

What to test

Who tested and where

This change is covered by automated tests
- NB: Rerun automation tests on this PR by commenting jenkins retest or jenkins multi-test.
I validated this change
Primary reviewer validated this change
I validated this change in the dev environment

marctalbott · 2025-01-13T22:18:05Z

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamUtils.scala

-                         samResourceId: SamResourceId,
-                         action: RuntimeAction,
-                         userEmail: Option[WorkbenchEmail] = None
+object SamUtils {


I tried out switching this over to an object instead of a trait to reuse the code in the RuntimeServiceInterp object and found that I slightly prefer it to the trait, but I'm curious to hear what others think! The switchover is in the last commit if you'd like to compare

I like it better too, it makes the code more readable IMO

codecov · 2025-01-13T22:33:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.62%. Comparing base (dce08ef) to head (814a783).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4822      +/-   ##
===========================================
- Coverage    74.62%   74.62%   -0.01%     
===========================================
  Files          166      166              
  Lines        14692    14632      -60     
  Branches      1135     1170      +35     
===========================================
- Hits         10964    10919      -45     
+ Misses        3728     3713      -15

Files with missing lines	Coverage Δ
...ute/dsde/workbench/leonardo/dao/sam/SamUtils.scala	`100.00% <100.00%> (ø)`
...e/workbench/leonardo/db/DiskServiceDbQueries.scala	`100.00% <100.00%> (ø)`
...rkbench/leonardo/http/AppDependenciesBuilder.scala	`97.87% <100.00%> (ø)`
...bench/leonardo/http/AzureDependenciesBuilder.scala	`97.14% <ø> (-0.16%)`	⬇️
...rkbench/leonardo/http/GcpDependenciesBuilder.scala	`73.14% <ø> (-0.31%)`	⬇️
...ench/leonardo/http/service/DiskServiceInterp.scala	`94.83% <100.00%> (+3.08%)`	⬆️
...ch/leonardo/http/service/DiskV2ServiceInterp.scala	`97.22% <100.00%> (+4.76%)`	⬆️
...ch/leonardo/http/service/LeoAppServiceInterp.scala	`87.34% <ø> (-0.04%)`	⬇️
...workbench/leonardo/http/service/ProxyService.scala	`73.55% <100.00%> (+0.12%)`	⬆️
...rkbench/leonardo/http/service/RuntimeService.scala	`100.00% <ø> (ø)`
... and 2 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dce08ef...814a783. Read the comment docs.

LizBaldo

Looks amazing! I only have a few comments / nits that should not be a blocker for merging. I really appreciate the logical organization by commit, it made it very easy to review, thanks a bunch!

LizBaldo · 2025-01-15T16:59:11Z

...c/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala

-      )
-      _ <- F.raiseWhen(!hasProjectPermission.getOrElse(true))(ForbiddenError(userInfo.userEmail, Some(ctx.traceId)))
-
+      samDiskIds <- samService.listResources(userInfo.accessToken.token, SamResourceType.PersistentDisk)


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

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

LizBaldo · 2025-01-15T16:59:35Z

...c/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala

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


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

LizBaldo · 2025-01-15T17:01:08Z

...st/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterpSpec.scala

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


Ouch, good catch, thanks!

LizBaldo · 2025-01-15T18:21:59Z

...ain/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala

@@ -1126,6 +1110,32 @@ object RuntimeServiceInterp {
      }
    } yield disk

+  private def checkAttachAction[F[_]](userInfo: UserInfo,


Nit, would it make sense to move this with the other checkAction function in SamUtils? Might not be in the scope for this PR, but disks can also be attached to apps and so it could make sense to move this logic out of the RuntimeServiceInterp file

LizBaldo · 2025-01-15T18:25:07Z

...ain/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala

@@ -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,


Nice, ignore my last comment then (I was reviewing commit by commit and did not see it yet sorry!

LizBaldo · 2025-01-15T18:25:31Z

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamUtils.scala

-                         samResourceId: SamResourceId,
-                         action: RuntimeAction,
-                         userEmail: Option[WorkbenchEmail] = None
+object SamUtils {


I like it better too, it makes the code more readable IMO

marctalbott added 10 commits January 10, 2025 13:51

listDisks

08950a1

createDisk

9194755

getDiskv1

d7c68d9

getDiskv2

03e9d12

deleteDiskv2

9c5918e

deleteDiskv1

c61d5a6

updateDisk

154ffcd

deleteDiskRecords

06bd6ae

attachDisk

9a7e442

switch to object instead of trait

814a783

marctalbott commented Jan 13, 2025

View reviewed changes

marctalbott changed the title ~~Mtalbott sam disks~~ [CORE-211] Convert disk operations to new Sam permissions model Jan 13, 2025

marctalbott marked this pull request as ready for review January 14, 2025 16:03

marctalbott requested a review from a team as a code owner January 14, 2025 16:03

dvoet approved these changes Jan 15, 2025

View reviewed changes

LizBaldo approved these changes Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marctalbott commented Jan 13, 2025 •

edited

Loading

marctalbott Jan 13, 2025

LizBaldo Jan 15, 2025

codecov bot commented Jan 13, 2025

LizBaldo left a comment

LizBaldo Jan 15, 2025

marctalbott Jan 15, 2025

LizBaldo Jan 15, 2025

LizBaldo Jan 15, 2025

LizBaldo Jan 15, 2025

LizBaldo Jan 15, 2025

LizBaldo Jan 15, 2025

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

Are you sure you want to change the base?

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

Conversation

marctalbott commented Jan 13, 2025 • edited Loading

Summary of changes

What

Why

Testing these changes

What to test

Who tested and where

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 13, 2025

Codecov Report

LizBaldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marctalbott commented Jan 13, 2025 •

edited

Loading