Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[IA-5060] Inherit manager roles to notebook-cluster and persistent-disk #1535

Merged
merged 7 commits into from
Sep 10, 2024

Conversation

rtitle
Copy link
Contributor

@rtitle rtitle commented Sep 6, 2024

Ticket: https://broadworkbench.atlassian.net/browse/IA-5060

What:

Phase 1 of the Simplify Leo Access Control epic.

This PR:

  • Allows notebook-cluster and perstistent-disk resource types to inherit from google-project or workspace.
  • Makes notebook-cluster/connect and persistent-disk/attach actions auth domain constrainable.

Why:

We are trying to clean up/simplify Leonardo access control logic. Part of this is standardizing on consistent Sam resource types and using inheritance to propagate permissions. This PR sets up the Sam model; subsequent Leonardo changes will be made to migrate to it.


PR checklist

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've filled out the Security Risk Assessment (requires Broad Internal network access) and attached the result to the JIRA ticket

@rtitle rtitle changed the title Fix Leo access control [DRAFT] Fix Leo access control Sep 6, 2024
}
persistent-disk = {
actionPatterns = {
read = {
description = "read metadata and contents of persistent disk"
description = "read metadata of persistent disk"
Copy link
Contributor Author

@rtitle rtitle Sep 6, 2024

Choose a reason for hiding this comment

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

read is just used for listing disks; attach is needed to actually view the contents of a disk

@rtitle rtitle changed the title [DRAFT] Fix Leo access control [DRAFT] Fix Leo access control (phase 1) Sep 6, 2024
@@ -790,6 +800,7 @@ resourceTypes = {
}
connect = {
description = "connect to the Jupyter notebook running on the notebook cluster"
authDomainConstrainable = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused about how granular the authDomainConstrainable is here. I thought that every resources / actions would inherit that configuration from the workspace after Doug's PR

Copy link
Contributor Author

@rtitle rtitle Sep 6, 2024

Choose a reason for hiding this comment

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

My understanding was you still have to annotate which actions on the child are authDomainConstrainable (like connect). @dvoet can you confirm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rtitle is correct. The auth domain is inherited but which actions have the auth domain applied is controlled by the authDomainConstrainable setting.

@rtitle
Copy link
Contributor Author

rtitle commented Sep 6, 2024

VERY draft Leo PR to start creating Sam resources with parents (phase 2): DataBiosphere/leonardo#4781

I'll try to test these together next week.

@rtitle rtitle changed the title [DRAFT] Fix Leo access control (phase 1) [IA-4295] Fix Leo access control (phase 1) Sep 9, 2024
@rtitle rtitle marked this pull request as ready for review September 9, 2024 19:35
@rtitle rtitle changed the title [IA-4295] Fix Leo access control (phase 1) [IA-5060] Inherit manager roles to notebook-cluster and persistent-disk Sep 10, 2024
Copy link

sonarcloud bot commented Sep 10, 2024

@rtitle
Copy link
Contributor Author

rtitle commented Sep 10, 2024

@LizBaldo requested review on this PR. I tested on a BEE and verified that all the resources (runtime, disk, app) continue to work normally.

@rtitle rtitle merged commit ee698da into develop Sep 10, 2024
24 of 25 checks passed
@rtitle rtitle deleted the rt-leo-access-control branch September 10, 2024 15:50
@ungwudik
Copy link

@rtitle or anyone here - Is the failure in orch-swat-test-job (dev, qa) a known issue or flakiness?

@rtitle
Copy link
Contributor Author

rtitle commented Sep 11, 2024

@ungwudik I noticed that, the Orch test failed with:

[info] - should link an eRA Commons account with access to none of the supported closed-access datasets *** FAILED *** (4 seconds, 225 milliseconds)
[info]   org.broadinstitute.dsde.workbench.service.RestException: {"causes":[],"message":"<!DOCTYPE html>\n<html lang=\"en\">\n<head>\n<meta charset=\"utf-8\">\n<title>Error</title>\n</head>\n<body>\n<pre>Cannot GET /dev/public-key.pem</pre>\n</body>\n</html>\n","source":"shibboleth","stackTrace":[],"statusCode":404}

Looks like the Orch swat test failed to get a dev shibboleth endpoint. This PR didn't touch anything related to orch/shibboleth. I'm not sure if this is a known dev infra issue? (cc @tlangs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants