[CORE-58] Cascade policy deletion #1564

calypsomatic · 2024-10-15T17:16:39Z

Ticket: https://broadworkbench.atlassian.net/browse/CORE-58

This PR now cascade deletes policies when deleting a resource. Instead of throwing an error that some policy members are in use like deleteResource previous did, it simply removes the problematic rows from the table.

I've demonstrated on my BEE that with this code, when a workspace has policies attached to it due to an imported TDR snapshot, it can be successfully deleted without any errors or orphaned policy references.

Why:

Users were unable to delete any workspace that had had a TDR snapshot with a policy imported into it, as those policies had entries in Sam's database that were not being deleted.

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

davidangb

Some comments inline about separate APIs/functions vs adding to the existing ones; otherwise, looks like a good approach to me. I'd love a knowledgable Sam person to review the SQL queries but it seems like a good high-level approach.

Can you write tests for this?

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala

src/main/resources/swagger/api-docs.yaml

dvoet · 2024-10-17T14:04:04Z

I think this is fine and should just happen all the time. No new api, not even an option on the existing api. Just do it.

One other change that needs to happen is that any groups that are modified need to be synchronized by calling cloudExtensions.onGroupUpdate.

So, I would update ResourceService.deleteResource directly to first figure out any groups that are changing and sync them instead of calling checkNoPoliciesInUse. Then I would modify accessPolicyDAO.deleteResource to include your 2 new delete statements in the same transaction as the other deletes.

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala

dvoet · 2024-10-22T18:47:11Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala

@@ -441,21 +440,11 @@ class ResourceService(
      // leave a tomb stone if the resource type does not allow reuse
      leaveTombStone = !resourceTypes(resource.resourceTypeName).reuseIds
      _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext)
+      _ <- cloudSyncPolicies(resource, samRequestContext)


The resource was deleted in the line above, the database calls in cloudSyncPolicies won't find anything. I think you need to get the list of groups that will be changed by having the resource removed before deleting the resource then sync them afterwards.

And something else I think I missed: PostgresGroupDAO.updateGroupUpdatedDateAndVersion needs to be called for each group updated.

Just to make sure I understand what you're asking, I should do something like:

affectedPolicies <- accessPolicyDAO.findAffectedPolicies(resource, samRequestContext) //New method _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) _ <- cloudSyncPolicies(affectedPolicies, samRequestContext) //Update this method to take in policies instead of finding them _ <- updateGroupUpdatedDateAndVersion(affectedPolicies, samRequestContext)

Is that correct?

yes but updateGroupUpdatedDateAndVersion should come before cloudSyncPolicies. The sync process keeps track of the last synchronized version and won't do anything if that version is the same as the current version. So the order of operations should be change the policy/group in the sam db, bump the version, cloud sync.

And here is yet another idea (sorry if I am jerking you around): instead of your new database queries to bulk delete entries, you can list the policies that need to be updated along with which policy is being removed. For each of those pairs (policy changing, policy removed), call removeSubjectFromPolicy (in the same class, ResourceService). That takes care of the database calls, syncing, everything. The code would look something like

affectedPolicies <- accessPolicyDAO.findAffectedPolicies(resource, samRequestContext) //New method _ <- affectedPolicies.traverse { case (policyToUpdate, policyToRemove) => removeSubjectFromPolicy(policyToUpdate, policyToRemove, samRequestContext) }

where findAffectedPolicies (or maybe there is a better name) returns a IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]]

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala

dvoet · 2024-10-24T20:28:24Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala

+        .map { rs =>
+          val resourceTypeName1 = rs.get[ResourceTypeName](rt.resultName.name)
+          val resourceId1 = rs.get[ResourceId](r.resultName.name)
+          val accessPolicyName1 = rs.get[AccessPolicyName](pg.resultName.name)


pg is an alias to the group table so its name will not be an access policy name

dvoet · 2024-10-24T20:31:45Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala

+      val g = GroupTable.syntax("g")
+      val pg = GroupTable.syntax("pg")
+      val gm = GroupMemberTable.syntax("gm")
+      val p = PolicyTable.syntax("p")
+      val rt = ResourceTypeTable.syntax("rt")
+      val r = ResourceTable.syntax("r")


I suggest naming these something more readable and include whether they are representing rows from the container or member policy. Same with fullyQualifiedPolicyId1 vs fullyQualifiedPolicyId2. One is the container and one is the member. Naming them better will help keep things straight.

davidangb · 2024-10-29T19:17:54Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala


      // remove from cloud first so a failure there does not leave sam in a bad state
      _ <- cloudDeletePolicies(resource, samRequestContext)
      _ <- deleteActionManagedIdentitiesForResource(resource, samRequestContext)

      // leave a tomb stone if the resource type does not allow reuse
      leaveTombStone = !resourceTypes(resource.resourceTypeName).reuseIds
+      affectedPolicies <- accessPolicyDAO.findAffectedPolicyGroups(resource, samRequestContext) // New method
+      _ <- affectedPolicies.traverse { case (policyToUpdate, policyToRemove) => removeSubjectFromPolicy(policyToUpdate, policyToRemove, samRequestContext) }
+      _ <- cloudSyncPolicies(affectedPolicies, samRequestContext) // TODO: use this on affected policies


if you are using removeSubjectFromPolicy, do you still need the call to cloudSyncPolicies? Does removeSubjectFromPolicy handle the syncing internally?

Not sure, actually. Maybe @dvoet knows?

you don't need to call cloudSyncPolicies

davidangb · 2024-10-29T19:21:10Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala

+      resourceId: FullyQualifiedResourceId,
+      samRequestContext: SamRequestContext
+  ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]]
+


findPolicyGroupMemberhips? findPolicyGroupParticipation? maybe findPolicyGroupsInUse to reflect the method it is supplanting?

davidangb · 2024-10-29T19:28:44Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala

@@ -569,6 +569,7 @@ class PostgresAccessPolicyDAO(

  override def deleteResource(resource: FullyQualifiedResourceId, leaveTombStone: Boolean, samRequestContext: SamRequestContext): IO[Unit] =
    serializableWriteTransaction("deleteResource", samRequestContext) { implicit session =>
+      deletePolicyMembersFromGroups(resource)


is this still necessary here, given that ResourceService calls removeSubjectFromPolicy?

I guess not!

davidangb · 2024-10-29T19:32:23Z

...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala

+        policyGroups should not be empty
+        policyGroups.head._1 shouldEqual parentPolicy.id
+        policyGroups.head._2 shouldEqual childPolicy.id


I think you could also write this as:

policyGroups should contain theSameElementsAs List(parentPolicy.id, childPolicy.id)

optional; pick whatever approach is more consistent with Sam code. I do see theSameElementsAs used elsewhere in Sam.

need an extra set of parens in there
policyGroups should contain theSameElementsAs List((parentPolicy.id, childPolicy.id))
and that 1 line can replace your 3

dvoet

I think this looks right. Approved assuming the cleanup David pointed and my test comments.

dvoet · 2024-10-30T15:57:45Z

...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala

+        policyGroups should not be empty
+        policyGroups.head._1 shouldEqual parentPolicy.id
+        policyGroups.head._2 shouldEqual childPolicy.id


need an extra set of parens in there
policyGroups should contain theSameElementsAs List((parentPolicy.id, childPolicy.id))
and that 1 line can replace your 3

dvoet · 2024-10-30T16:02:57Z

...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala

+            childResourceFullyQualifiedId.resourceTypeName,
+            childResourceFullyQualifiedId.resourceId,
+            Set.empty,
+            Set(childPolicy)


I would add 2 more policies to childResource, one that is not a member of a parentResource policy and on that is a member of a different policy. Maybe even add another parent resource.

sonarqubecloud · 2024-10-30T20:21:48Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

davidangb

thanks for sticking with this one!

calypsomatic added 8 commits October 15, 2024 13:12

try new api

3616bde

wrong tabbing?

abdf118

fix api

511f6cf

update delete query

5c2c881

fix query

4a80260

one more try

08abd65

split into two

ef2be68

single transaction

fc94de3

calypsomatic changed the title ~~try new api~~ [CORE-58] Cascade policy deletion Oct 15, 2024

davidangb reviewed Oct 16, 2024

View reviewed changes

Merge branch 'develop' into core-58-cascade-policy

e50536f

calypsomatic and others added 7 commits October 17, 2024 14:45

test out sync and delete policies

4455509

update order of operations

841c2c8

remove deletecascade

2ad6f23

whoops

a9a3083

cleanup and add test

5f1ac33

removed commented out code and fix test

a6bcf06

Merge branch 'develop' into core-58-cascade-policy

8ba9c4e

calypsomatic marked this pull request as ready for review October 21, 2024 19:45

calypsomatic requested review from dvoet and davidangb October 22, 2024 18:50

dvoet requested changes Oct 22, 2024

View reviewed changes

calypsomatic added 3 commits October 24, 2024 14:24

first pass updated delete resource

8c0ad7d

scalafmt

f78fb5a

remove commented out code

8b4fa1b

dvoet reviewed Oct 24, 2024

View reviewed changes

calypsomatic added 2 commits October 29, 2024 13:46

fix affected policy groups and add test

bf02fc0

sonar findings

34d18e8

calypsomatic requested a review from dvoet October 29, 2024 18:41

davidangb reviewed Oct 29, 2024

View reviewed changes

dvoet approved these changes Oct 30, 2024

View reviewed changes

calypsomatic added 3 commits October 30, 2024 14:36

expand test

7428caf

remove unnecessary method

825b766

don't sync policies

e4f2975

calypsomatic requested a review from davidangb October 30, 2024 19:32

remove unused method

71b8dd5

davidangb approved these changes Oct 31, 2024

View reviewed changes

calypsomatic merged commit 58be30b into develop Oct 31, 2024
25 checks passed

calypsomatic deleted the core-58-cascade-policy branch October 31, 2024 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-58] Cascade policy deletion #1564

[CORE-58] Cascade policy deletion #1564

calypsomatic commented Oct 15, 2024 •

edited

Loading

davidangb left a comment

dvoet commented Oct 17, 2024

dvoet Oct 22, 2024

calypsomatic Oct 23, 2024 •

edited

Loading

dvoet Oct 23, 2024

dvoet Oct 24, 2024

dvoet Oct 24, 2024

davidangb Oct 29, 2024

calypsomatic Oct 30, 2024

dvoet Oct 30, 2024

davidangb Oct 29, 2024

davidangb Oct 29, 2024

calypsomatic Oct 30, 2024

davidangb Oct 29, 2024

dvoet Oct 30, 2024

dvoet left a comment

dvoet Oct 30, 2024

dvoet Oct 30, 2024

sonarqubecloud bot commented Oct 30, 2024

davidangb left a comment

[CORE-58] Cascade policy deletion #1564

[CORE-58] Cascade policy deletion #1564

Conversation

calypsomatic commented Oct 15, 2024 • edited Loading

davidangb left a comment

Choose a reason for hiding this comment

dvoet commented Oct 17, 2024

Choose a reason for hiding this comment

calypsomatic Oct 23, 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

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

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

sonarqubecloud bot commented Oct 30, 2024

Quality Gate passed

davidangb left a comment

Choose a reason for hiding this comment

calypsomatic commented Oct 15, 2024 •

edited

Loading

calypsomatic Oct 23, 2024 •

edited

Loading