Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Feb 14, 2025
Merged

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

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b8264a0
add support for making datasnapshot a child resource of dataset
pshapiro4broad Feb 11, 2025
a6ce39f
Add the descendent role mapping
pshapiro4broad Feb 11, 2025
55e4e0f
Merge branch 'develop' into ps/dt-1202-add-snapshot-parent
pshapiro4broad Feb 11, 2025
153210d
add actions and role mappings for `list_children` and `get_parent` so…
pshapiro4broad Feb 11, 2025
353ab99
try adding create_with_parent back as an custodian action
pshapiro4broad Feb 12, 2025
ab13315
move action directly to the owner role
pshapiro4broad Feb 12, 2025
afb49c0
only allow dataset custodian or steward to `list_children`
pshapiro4broad Feb 14, 2025
7604e4d
Merge branch 'develop' into ps/dt-1202-add-snapshot-parent
pshapiro4broad Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions src/main/resources/reference.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1335,6 +1335,12 @@ resourceTypes = {
"unlock_resource" = {
description = "Can unlock a resource"
}
add_child = {
description = "add a child resource"
}
list_children = {
description = "list child resources"
}
}
ownerRoleName = "steward"
roles = {
Expand All @@ -1343,14 +1349,17 @@ resourceTypes = {
includedRoles = ["custodian"]
}
custodian = {
roleActions = ["manage_schema", "create_datasnapshot", "ingest_data", "soft_delete", "hard_delete", "unlink_snapshot", "list_snapshots", "lock_resource", "unlock_resource", "share_policy::custodian", "share_policy::ingester", "share_policy::snapshot_creator"]
roleActions = ["manage_schema", "create_datasnapshot", "ingest_data", "soft_delete", "hard_delete", "unlink_snapshot", "list_snapshots", "lock_resource", "unlock_resource", "share_policy::custodian", "share_policy::ingester", "share_policy::snapshot_creator", "list_children"]
includedRoles = ["snapshot_creator"]
descendantRoles = {
datasnapshot = ["steward"]
}
}
ingester = {
roleActions = ["read_dataset", "read_data", "ingest_data"]
}
snapshot_creator = {
roleActions = ["read_dataset", "read_data", "read_policies", "link_snapshot"]
roleActions = ["read_dataset", "read_data", "read_policies", "link_snapshot", "add_child"]
}
admin = {
roleActions = ["share_policy::steward", "read_policies", "alter_policies", "unlock_resource"]
Expand Down Expand Up @@ -1437,18 +1446,24 @@ resourceTypes = {
add_child = {
description = "add a child resource"
}
create_with_parent = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@pshapiro4broad pshapiro4broad Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

description = "Enables creating a snapshot with a parent dataset"
}
get_parent = {
description = "get parent of snapshot"
}
}
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"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need "get_parent"?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

includedRoles = ["reader"]
}
reader = {
Expand Down
Loading