try new parameter

broadinstitute · Oct 7, 2024 · 6230914 · 6230914
1 parent 24e220a
commit 6230914
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 70 deletions.
diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml
@@ -2109,6 +2109,12 @@ paths:
           required: true
           schema:
             type: string
+        - name: cascadePolicies
+          in: query
+          description: Cascade delete of policies associated with the resource
+          required: false
+          schema:
+            type: boolean
       responses:
         204:
           description: Successfully deleted resource

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala
@@ -324,11 +324,13 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM
 
   def deleteResource(resource: FullyQualifiedResourceId, samUser: SamUser, samRequestContext: SamRequestContext): server.Route =
     deleteWithTelemetry(samRequestContext, resourceParams(resource): _*) {
-      // Note that this does not require remove_child on the parent if it exists. remove_child is meant to prevent
-      // users from removing a child only to add it to a different parent and thus circumvent any permissions
-      // a parent may be enforcing. Deleting a child does not allow this situation.
-      requireAction(resource, SamResourceActions.delete, samUser.id, samRequestContext) {
-        complete(resourceService.deleteResource(resource, samRequestContext).map(_ => StatusCodes.NoContent))
+      parameter("cascadePolicies".as[Boolean].?) { cascadePolicies =>
+        // Note that this does not require remove_child on the parent if it exists. remove_child is meant to prevent
+        // users from removing a child only to add it to a different parent and thus circumvent any permissions
+        // a parent may be enforcing. Deleting a child does not allow this situation.
+        requireAction(resource, SamResourceActions.delete, samUser.id, samRequestContext) {
+          complete(resourceService.deleteResource(resource, cascadePolicies, samRequestContext).map(_ => StatusCodes.NoContent))
+        }
       }
     }
 

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala
@@ -130,7 +130,10 @@ trait AccessPolicyDAO {
       samRequestContext: SamRequestContext
   ): IO[Seq[FilterResourcesResult]]
 
-  def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Int]
+  def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]]
+
+  def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit]
+
 }
 
 sealed abstract class LoadResourceAuthDomainResult

diff --git a/...main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/...main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala
@@ -975,71 +975,66 @@ class PostgresAccessPolicyDAO(
     }
   }
 
-  override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Int] = {
-    logger.info(s"checkPolicyGroupsInUse: ${resourceId}")
-    if (resourceId.resourceTypeName == SamResourceTypes.workspaceName) {
-      logger.info("deleting from group_member tables")
-      val gm = GroupMemberTable.syntax("gm")
-      val p = PolicyTable.syntax("p")
-      val gmf = GroupMemberFlatTable.syntax("gmf")
+  override def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = {
 
-      serializableWriteTransaction("checkPolicyGroupsInUse", samRequestContext) { implicit session =>
-        val deleteQuery = samsql"""
+    logger.info("deleting from group_member tables")
+    val gm = GroupMemberTable.syntax("gm")
+    val p = PolicyTable.syntax("p")
+    val gmf = GroupMemberFlatTable.syntax("gmf")
+
+    serializableWriteTransaction("removePolicyGroupsInUse", samRequestContext) { implicit session =>
+      val deleteQuery = samsql"""
       delete from ${GroupMemberTable as gm}
       using ${PolicyTable as p}
       where ${gm.memberGroupId} = ${p.groupId}
       and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})
     """
-        logger.info(s"deleteQuery: ${deleteQuery.statement}")
-        deleteQuery.update().apply()
-      }
+      logger.info(s"deleteQuery: ${deleteQuery.statement}")
+      deleteQuery.update().apply()
+    }
 
-      serializableWriteTransaction("checkPolicyGroupsInUse", samRequestContext) { implicit session =>
-        val deleteQuery = samsql"""
+    serializableWriteTransaction("removePolicyGroupsInUse", samRequestContext) { implicit session =>
+      val deleteQuery = samsql"""
       delete from ${GroupMemberFlatTable as gmf}
       using ${PolicyTable as p}
       where ${gmf.memberGroupId} = ${p.groupId}
       and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})
     """
-        logger.info(s"deleteQuery: ${deleteQuery.statement}")
-        deleteQuery.update().apply()
-      }
-    } else {
-      logger.info("Not deleting")
-      IO.pure(0)
+      logger.info(s"deleteQuery: ${deleteQuery.statement}")
+      deleteQuery.update().apply()
     }
   }
 
-//  override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = {
-//    val g = GroupTable.syntax("g")
-//    val pg = GroupTable.syntax("pg") // problematic group
-//    val gm = GroupMemberTable.syntax("gm")
-//    val p = PolicyTable.syntax("p")
-//
-//    readOnlyTransaction("checkPolicyGroupsInUse", samRequestContext) { implicit session =>
-//      val problematicGroupsQuery =
-//        samsql"""select ${g.result.id}, ${g.result.name}, array_agg(${pg.name}) as ${pg.resultName.name}
-//                     from ${GroupTable as g}
-//                     join ${GroupMemberTable as gm} on ${g.id} = ${gm.memberGroupId}
-//                     join ${GroupTable as pg} on ${gm.groupId} = ${pg.id}
-//                     where ${g.id} in
-//                         (select distinct ${gm.result.memberGroupId}
-//                          from ${GroupMemberTable as gm}
-//                          join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId}
-//                          where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)}))
-//                     group by ${g.id}, ${g.name}"""
-//      problematicGroupsQuery
-//        .map(rs =>
-//          Map(
-//            "groupId" -> rs.get[GroupPK](g.resultName.id).value.toString,
-//            "groupName" -> rs.get[String](g.resultName.name),
-//            "still used in group(s):" -> rs.get[String](pg.resultName.name)
-//          )
-//        )
-//        .list()
-//        .apply()
-//    }
-//  }
+  override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = {
+    val g = GroupTable.syntax("g")
+    val pg = GroupTable.syntax("pg") // problematic group
+    val gm = GroupMemberTable.syntax("gm")
+    val p = PolicyTable.syntax("p")
+
+    readOnlyTransaction("checkPolicyGroupsInUse", samRequestContext) { implicit session =>
+      val problematicGroupsQuery =
+        samsql"""select ${g.result.id}, ${g.result.name}, array_agg(${pg.name}) as ${pg.resultName.name}
+                     from ${GroupTable as g}
+                     join ${GroupMemberTable as gm} on ${g.id} = ${gm.memberGroupId}
+                     join ${GroupTable as pg} on ${gm.groupId} = ${pg.id}
+                     where ${g.id} in
+                         (select distinct ${gm.result.memberGroupId}
+                          from ${GroupMemberTable as gm}
+                          join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId}
+                          where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)}))
+                     group by ${g.id}, ${g.name}"""
+      problematicGroupsQuery
+        .map(rs =>
+          Map(
+            "groupId" -> rs.get[GroupPK](g.resultName.id).value.toString,
+            "groupName" -> rs.get[String](g.resultName.name),
+            "still used in group(s):" -> rs.get[String](pg.resultName.name)
+          )
+        )
+        .list()
+        .apply()
+    }
+  }
 
   override def loadPolicy(resourceAndPolicyName: FullyQualifiedPolicyId, samRequestContext: SamRequestContext): IO[Option[AccessPolicy]] =
     listPolicies(resourceAndPolicyName.resource, limitOnePolicy = Option(resourceAndPolicyName.accessPolicyName), samRequestContext).map(_.headOption)

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupService.scala
@@ -141,7 +141,7 @@ class ManagedGroupService(
       managedGroupResourceId = FullyQualifiedResourceId(managedGroupType.name, groupId)
       _ <- resourceService.cloudDeletePolicies(managedGroupResourceId, samRequestContext)
       _ <- directoryDAO.deleteGroup(WorkbenchGroupName(groupId.value), samRequestContext)
-      _ <- resourceService.deleteResource(managedGroupResourceId, samRequestContext)
+      _ <- resourceService.deleteResource(managedGroupResourceId, Some(false), samRequestContext)
     } yield ()
 
   def listGroups(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Iterable[ManagedGroupMembershipEntry]] =

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala
@@ -429,12 +429,12 @@ class ResourceService(
   //      preventing a new Resource with the same ID from being created
   // Resources with children cannot be deleted and will throw a 400.
   @throws(classOf[WorkbenchExceptionWithErrorReport]) // Necessary to make Mockito happy
-  def deleteResource(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = {
+  def deleteResource(resource: FullyQualifiedResourceId, cascadePolicies: Option[Boolean], samRequestContext: SamRequestContext): IO[Unit] = {
     logger.info(s"Deleting resource ${resource.resourceId} of type ${resource.resourceTypeName}")
 
     for {
       _ <- checkNoChildren(resource, samRequestContext)
-      _ <- checkNoPoliciesInUse(resource, samRequestContext)
+      _ <- checkNoPoliciesInUse(resource, cascadePolicies, samRequestContext)
 
       // remove from cloud first so a failure there does not leave sam in a bad state
       _ <- cloudDeletePolicies(resource, samRequestContext)
@@ -448,17 +448,22 @@ class ResourceService(
     } yield ()
   }
 
-  private def checkNoPoliciesInUse(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Int] =
-    accessPolicyDAO.checkPolicyGroupsInUse(resource, samRequestContext)
-//      .flatMap { problematicGroups =>
-//      if (problematicGroups.nonEmpty)
-//        IO.raiseError(
-//          new WorkbenchExceptionWithErrorReport( // throws a 500 since that's the current behavior
-//            ErrorReport(StatusCodes.InternalServerError, s"Foreign Key Violation(s) while deleting group(s): ${problematicGroups}")
-//          )
-//        )
-//      else IO.unit
-//    }
+  private def checkNoPoliciesInUse(resource: FullyQualifiedResourceId, cascadePolicies: Option[Boolean], samRequestContext: SamRequestContext): IO[Unit] =
+    cascadePolicies match {
+      case Some(true) =>
+        accessPolicyDAO.removePolicyGroupsInUse(resource, samRequestContext)
+        IO.unit
+      case _ =>
+        accessPolicyDAO.checkPolicyGroupsInUse(resource, samRequestContext).flatMap { problematicGroups =>
+          if (problematicGroups.nonEmpty)
+            IO.raiseError(
+              new WorkbenchExceptionWithErrorReport( // throws a 500 since that's the current behavior
+                ErrorReport(StatusCodes.InternalServerError, s"Foreign Key Violation(s) while deleting group(s): ${problematicGroups}")
+              )
+            )
+          else IO.unit
+        }
+    }
 
   private def deleteActionManagedIdentitiesForResource(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] =
     azureService