DT-1202: Allow creating datasnapshot as a child resource of dataset #1640

pshapiro4broad · 2025-02-11T18:01:41Z

Jira ticket: https://broadworkbench.atlassian.net/browse/DT-1202

What:

To support inheriting dataset custodian role as steward on a snapshot, we need to allow

creating the datasnapshot with a parent resource (create_with_parent)
adding a child resource to a dataset (add_child)

Also, we need to specify that dataset custodian or steward are steward on datasnapshot, if the parent resource is set.

Why:

This is support the goal of avoiding adding dataset custodians to every snapshot, to prevent hitting GCP quota limits.

How:

With this change

users with snapshot_creator or greater role on dataset can add_child to dataset
users with custodian or greater role on dataset can list_children
users with steward role can create_with_parent to datasnapshot, and get_parent
dataset custodian or greater role are steward on child datasnapshots.

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

dvoet · 2025-02-11T18:09:31Z

src/main/resources/reference.conf

@@ -1435,11 +1438,14 @@ resourceTypes = {
      add_child = {
        description = "add a child resource"
      }
+      create_with_parent = {


What is this action for? I think you probably want set_parent. https://github.com/broadinstitute/sam/blob/develop/README.md#resource-and-policy-management

I got it from https://github.com/broadinstitute/sam/blob/develop/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala#L34
which is used by requireCreateWithOptionalParent:

// parents are allowed for a resource type if the owner role contains the SamResourceActions.setParent or SamResourceActions.createWithParent action val parentAllowed = resourceType.roles .find(_.roleName == resourceType.ownerRoleName) .exists(role => role.actions.contains(SamResourceActions.setParent) || role.actions.contains(SamResourceActions.createWithParent))

It looks like when this was added in #1441, the README wasn't updated. I can switch to using set_parent if that's preferred. Relevant slack thread: https://broadinstitute.slack.com/archives/C03GMG4DUSE/p1715026844061349

Interesting. I guess you can keep your create_with_parent. If I had been paying attention, I might have suggested that they use set_parent but not grant remove_child on the parent.

I might have suggested that they use set_parent but not grant remove_child on the parent.

Well, TDR is the only user of create_with_parent so we could stop doing it if you want us to. I don't have a strong opinion one way or the other.

# Conflicts: # src/main/resources/reference.conf

src/main/resources/reference.conf

dvoet

I don't know if you intend to add parents after creating snapshots or not. If you do intend to add parents after the fact, then you will need set_parent

dvoet · 2025-02-11T19:23:08Z

src/main/resources/reference.conf

    }
    ownerRoleName = "steward"
    roles = {
      steward = {
-        roleActions = ["share_policy::steward", "share_policy::custodian", "update_passport_identifier", "view_journal"]
+        roleActions = ["share_policy::steward", "share_policy::custodian", "update_passport_identifier", "view_journal", "create_with_parent"]


You don't actually need to grant create_with_parent to anyone. The action's existence on the resource type is used only as a flag to allow resources to be created with a parent

When I tested this change in a bee, I found that it didn't work until I added create_with_parent as an action on the steward (owner) role.

I also tried adding create_with_parent as an action on custodian, which is included by steward, but that didn't work.

Looking at the Scala code, in SecurityDirectives.requireCreateWithOptionalParent():

// parents are allowed for a resource type if the owner role contains the SamResourceActions.setParent or SamResourceActions.createWithParent action val parentAllowed = resourceType.roles .find(_.roleName == resourceType.ownerRoleName) .exists(role => role.actions.contains(SamResourceActions.setParent) || role.actions.contains(SamResourceActions.createWithParent))

It appears that this is what the code is requiring -- the action must directly be on the owner role.

I'm wondering if the failure of this to work with included roles is a bug. Looking at the code path for the set_parent operation (ResourceRoutes.setResourceParent()), the code uses SecurityDirectives.requireAction() which looks like it does support included roles.

src/main/resources/reference.conf

… the parent/child state of resources can be interrogated by steward and custodian

src/main/resources/reference.conf

snf2ye · 2025-02-14T19:20:43Z

src/main/resources/reference.conf

        includedRoles = ["custodian"]
        descendantRoles = {
          snapshot-builder-request = ["approver"]
        }
      }
      custodian = {
-        roleActions = ["delete", "edit_datasnapshot", "update_snapshot", "share_policy::reader", "share_policy::aggregate_data_reader", "share_policy::discoverer", "read_policies", "set_public", "update_auth_domain", "lock_resource", "unlock_resource"]
+        roleActions = ["delete", "edit_datasnapshot", "update_snapshot", "share_policy::reader", "share_policy::aggregate_data_reader", "share_policy::discoverer", "read_policies", "set_public", "update_auth_domain", "lock_resource", "unlock_resource", "get_parent"]


Why do we need "get_parent"?

I think we'll need it in the UI, to find out of a snapshot has a parent dataset which is providing steward users to the snapshot. The UI can use this to fetch and show the dataset custodian users as "inherited" stewards in the Roles & Memberships tab.

sonarqubecloud · 2025-02-14T21:08:25Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

add support for making datasnapshot a child resource of dataset

b8264a0

pshapiro4broad requested a review from a team as a code owner February 11, 2025 18:01

pshapiro4broad requested review from dvoet and samanehsan February 11, 2025 18:01

pshapiro4broad changed the title ~~DT:1202: Add support for making datasnapshot a child resource of dataset~~ DT-1202: Add support for making datasnapshot a child resource of dataset Feb 11, 2025

pshapiro4broad changed the title ~~DT-1202: Add support for making datasnapshot a child resource of dataset~~ DT-1202: Allow creating datasnapshot as a child resource of dataset Feb 11, 2025

Add the descendent role mapping

a6ce39f

dvoet reviewed Feb 11, 2025

View reviewed changes

Merge branch 'develop' into ps/dt-1202-add-snapshot-parent

55e4e0f

# Conflicts: # src/main/resources/reference.conf

pshapiro4broad commented Feb 11, 2025

View reviewed changes

src/main/resources/reference.conf Show resolved Hide resolved

pshapiro4broad requested a review from dvoet February 11, 2025 19:14

dvoet approved these changes Feb 11, 2025

View reviewed changes

pshapiro4broad added 3 commits February 11, 2025 14:52

add actions and role mappings for list_children and get_parent so…

153210d

… the parent/child state of resources can be interrogated by steward and custodian

try adding create_with_parent back as an custodian action

353ab99

move action directly to the owner role

ab13315

snf2ye reviewed Feb 13, 2025

View reviewed changes

src/main/resources/reference.conf Outdated Show resolved Hide resolved

snf2ye reviewed Feb 14, 2025

View reviewed changes

only allow dataset custodian or steward to list_children

afb49c0

snf2ye approved these changes Feb 14, 2025

View reviewed changes

Merge branch 'develop' into ps/dt-1202-add-snapshot-parent

7604e4d

pshapiro4broad enabled auto-merge (squash) February 14, 2025 21:08

pshapiro4broad merged commit c24b03f into develop Feb 14, 2025
20 checks passed

pshapiro4broad deleted the ps/dt-1202-add-snapshot-parent branch February 14, 2025 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DT-1202: Allow creating datasnapshot as a child resource of dataset #1640

DT-1202: Allow creating datasnapshot as a child resource of dataset #1640

pshapiro4broad commented Feb 11, 2025 •

edited

Loading

dvoet Feb 11, 2025

pshapiro4broad Feb 11, 2025

pshapiro4broad Feb 11, 2025 •

edited

Loading

dvoet Feb 11, 2025

pshapiro4broad Feb 11, 2025

dvoet left a comment

dvoet Feb 11, 2025

pshapiro4broad Feb 12, 2025

pshapiro4broad Feb 13, 2025

snf2ye Feb 14, 2025

pshapiro4broad Feb 14, 2025

sonarqubecloud bot commented Feb 14, 2025

DT-1202: Allow creating datasnapshot as a child resource of dataset #1640

DT-1202: Allow creating datasnapshot as a child resource of dataset #1640

Conversation

pshapiro4broad commented Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshapiro4broad Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoet 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

sonarqubecloud bot commented Feb 14, 2025

Quality Gate passed

pshapiro4broad commented Feb 11, 2025 •

edited

Loading

pshapiro4broad Feb 11, 2025 •

edited

Loading