[IA-5066] DO NOT MERGE getRuntime v1 checks notebook-cluster-status action assuming hiera… #4785

mlilynolting · 2024-09-18T18:05:00Z

Assumes Sam model hierarchy of runtime under project parent, uses checkPermission as best practice.

Jira ticket: https://broadworkbench.atlassian.net/browse/IA-5066

https://broadworkbench.atlassian.net/browse/IA-5063

Summary of changes

getRuntime now checks permission once, for the relevant runtime action, using the preferred SamService checkPermission.

…rchy under project parent, uses checkPermission as best practice

codecov · 2024-09-18T18:25:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.78%. Comparing base (4ad7058) to head (e66ffe4).
Report is 9 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4785      +/-   ##
===========================================
- Coverage    74.79%   74.78%   -0.02%     
===========================================
  Files          165      165              
  Lines        14946    14940       -6     
  Branches      1178     1144      -34     
===========================================
- Hits         11179    11173       -6     
  Misses        3767     3767

Flag	Coverage Δ
	`74.78% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...h/leonardo/http/service/RuntimeServiceInterp.scala	`88.73% <100.00%> (+0.19%)`	⬆️

... and 1 file with indirect coverage changes

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 4ad7058...e66ffe4. Read the comment docs.

rtitle · 2024-09-19T16:50:25Z

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

-            RuntimeNotFoundException(cloudContext, runtimeName, "permission denied")
-          )
-    } yield resp
+      .adaptError { case _: SamException =>


Do we need to adapt the error? SamServiceInterp already throws a friendly LeoException with status 403 -- can we just throw that?

My understanding is that the best practice is to mask 403/forbidden style errors with 404 messages, since users without access rights shouldn't be able to verify that a runtime with the given ID even exists. This is how getRuntime worked before, and I think it's standard across Leo (and a pretty common pattern elsewhere)

I feel like it's reasonable to respond 404 if the runtime doesn't exist and 403 if the runtime exists but the caller doesn't have permission.

I'll ask AppSec what their preferred behavior is.

For context I think you wrote this comment ~4ya

I think our comments crossed.. yeah, just thinking more about the best practice.

For instance, I was trying google apis and they seem to respond 403 in both cases:

$ gcloud projects describe asdfasdffoobar --verbosity debug User [[email protected]] does not have permission to access projects instance [asdfasdffoobar] (or it may not exist): … "error": { "code": 403, "message": "The caller does not have permission", "status": "PERMISSION_DENIED" }

Maybe we should respond 403 with a message "Caller does not have permission or it doesn't exist" 🤔

rtitle · 2024-09-19T16:52:46Z

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


-      // throws 404 if not existent


Would be nice to keep this comment -- RuntimeServiceDbQueries.getRuntime will throw 404 if the runtime doesn't exist.

I got tripped up on "throw" and was frustrated trying to test it until I realized that it wasn't throwing the error, it was returning it in the usual Scala way. Wasn't sure of the right verb and wasn't sure that it needed saying

Yeah, that's an annoyance I have with this Scala style: the exceptions aren't annotated. Any IO can throw any Throwable at any time. At least in Java you can specify throws IOException, for example (although you still have unchecked exceptions so it's not perfect).

Anyway that's just my thinking about why I like to leave comments about what certain things can throw. :)

But it isn't throwing, is it? Like these come back as return values in Eithers, like Left(SamException...))

It throws the exception when you run the IO, and it short-circuits the for-comprehension. So close enough to throwing, IMO.

Technically they are not Eithers -- there is no Either in the type. An IO[A] can either yield a successful A or raise an error, which short-circuits subsequent flatMap chains. But they are not represented as Either, the success-or-failure behavior is baked into the IO data type.

rtitle · 2024-09-19T16:55:08Z

...scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala

@@ -819,34 +821,65 @@ class RuntimeServiceInterpTest
  }

  it should "get a runtime" in isolatedDbTest {
+    val mockSamService: SamService[IO] = mock[SamService[IO]](defaultMockitoAnswer[IO])


This should be the default response of MockSamService, so not sure you need to override the mock here.

MockSamService isn't a Mockito mock, though, and I wanted to have access to the method stubbing/mocking for my tests.

rtitle · 2024-09-19T17:01:03Z

...scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala

+    val service = makeRuntimeService(samService = mockSamService)
+
+    val exc = service
+      .getRuntime(unauthorizedUserInfo, cloudContextGcp, RuntimeName("cluster"))


Not sure this call is hitting samService.checkAuthorized. Seems like we want to be getting an existing runtime, to verify the access control check fails as expected? Right now it looks like the runtime doesn't exist, so it would return 404.

rtitle · 2024-09-19T17:01:56Z

...scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterpSpec.scala

+        isEq(RuntimeAction.GetRuntimeStatus)
+      )(any(Ask[IO, AppContext].getClass))
+    ).thenReturn(
+      IO.pure(


Don't think IO[Either[..]] is the correct return type. Maybe .thenReturn(IO.raiseException(SamException.create…))?

[] getRuntime v1 checks notebook-cluster-status action assuming hiera…

2588861

…rchy under project parent, uses checkPermission as best practice

mlilynolting changed the title ~~[IA-5066] getRuntime v1 checks notebook-cluster-status action assuming hiera…~~ [IA-5066] DO NOT MERGE getRuntime v1 checks notebook-cluster-status action assuming hiera… Sep 18, 2024

mlilynolting marked this pull request as draft September 18, 2024 18:07

mlilynolting requested review from lucymcnatt, rtitle and LizBaldo September 19, 2024 15:52

rtitle reviewed Sep 19, 2024

View reviewed changes

mlilynolting added 3 commits September 19, 2024 14:32

[IA-5066] fix tests of getRuntime

39dabbc

[IA-5066] comment

836c286

[IA-5066] Render 404 and 403 without obfuscation

e66ffe4

LizBaldo closed this Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IA-5066] DO NOT MERGE getRuntime v1 checks notebook-cluster-status action assuming hiera… #4785

[IA-5066] DO NOT MERGE getRuntime v1 checks notebook-cluster-status action assuming hiera… #4785

mlilynolting commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

rtitle Sep 19, 2024

mlilynolting Sep 19, 2024

rtitle Sep 19, 2024

mlilynolting Sep 19, 2024

rtitle Sep 19, 2024 •

edited

Loading

rtitle Sep 19, 2024

mlilynolting Sep 19, 2024

rtitle Sep 19, 2024

mlilynolting Sep 19, 2024

rtitle Sep 19, 2024 •

edited

Loading

rtitle Sep 19, 2024

mlilynolting Sep 20, 2024

rtitle Sep 19, 2024

mlilynolting Sep 20, 2024

rtitle Sep 19, 2024

mlilynolting Sep 20, 2024

[IA-5066] DO NOT MERGE getRuntime v1 checks notebook-cluster-status action assuming hiera… #4785

[IA-5066] DO NOT MERGE getRuntime v1 checks notebook-cluster-status action assuming hiera… #4785

Conversation

mlilynolting commented Sep 18, 2024 • edited Loading

Summary of changes

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

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

rtitle Sep 19, 2024 • edited Loading

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

rtitle Sep 19, 2024 • edited Loading

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

mlilynolting commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

rtitle Sep 19, 2024 •

edited

Loading

rtitle Sep 19, 2024 •

edited

Loading