From 3616bded520c0b912b9b790edb1cc0114bcd6a94 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 13:12:56 -0400 Subject: [PATCH 01/23] try new api --- src/main/resources/swagger/api-docs.yaml | 33 +++++++++++++++++++ .../workbench/sam/api/ResourceRoutes.scala | 16 +++++++++ .../sam/dataAccess/AccessPolicyDAO.scala | 2 ++ .../dataAccess/PostgresAccessPolicyDAO.scala | 29 ++++++++++++++++ .../sam/service/ResourceService.scala | 17 ++++++++++ .../sam/dataAccess/MockAccessPolicyDAO.scala | 3 ++ 6 files changed, 100 insertions(+) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index d5430a19f..16f6a3e65 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -2123,6 +2123,39 @@ paths: description: Resource type does not exist or you are not a member of any policy on the resource content: { } + /api/resources/v2/{resourceTypeName}/{resourceId}/cascade: + delete: + tags: + - Resources + summary: Delete a resource + operationId: deleteResourceV2 + parameters: + - name: resourceTypeName + in: path + description: Type of the resource + required: true + schema: + type: string + - name: resourceId + in: path + description: Id of the resource + required: true + schema: + type: string + responses: + 204: + description: Successfully deleted resource + content: { } + 400: + description: Cannot delete a resource with children. Delete the children first then try again. + content: { } + 403: + description: You do not have permission to perform this action on the resource or permissions on the resource's parent + content: { } + 404: + description: Resource type does not exist or you are not a member of any + policy on the resource + content: { } /api/resources/v2/{resourceTypeName}/{resourceId}/actions: get: tags: 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 index 5f17cf02e..64b26df91 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala @@ -189,6 +189,12 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM listActionsForUser(resource, samUser, samRequestContext) } } ~ + pathPrefix("cascade") { + pathEndOrSingleSlash { + deleteResourceCascade(resource, samUser, samRequestContext) ~ + postDefaultResource(resourceType, resource, samUser, samRequestContext) + } + } ~ pathPrefix("allUsers") { pathEndOrSingleSlash { getAllResourceUsers(resource, samUser, samRequestContext) @@ -332,6 +338,16 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM } } + def deleteResourceCascade(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.deleteResourceCascade(resource, samRequestContext).map(_ => StatusCodes.NoContent)) + } + } + def postDefaultResource( resourceType: ResourceType, resource: FullyQualifiedResourceId, 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 index bb3397830..3f89068d8 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala @@ -131,6 +131,8 @@ trait AccessPolicyDAO { ): IO[Seq[FilterResourcesResult]] 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/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 23aa83d48..690d93b5c 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -1006,6 +1006,35 @@ class PostgresAccessPolicyDAO( } } + override def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { + + logger.warn("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.warn(s"deleteQuery: ${deleteQuery.statement}") + deleteQuery.update().apply() + } + + 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() + } + } 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/ResourceService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala index c7adfe304..939821cfc 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -445,6 +445,23 @@ class ResourceService( _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) } yield () + @throws(classOf[WorkbenchExceptionWithErrorReport]) // Necessary to make Mockito happy + def deleteResourceCascade(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = + for { + _ <- checkNoChildren(resource, samRequestContext) + _ <- accessPolicyDAO.removePolicyGroupsInUse(resource, samRequestContext) + + // 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 + _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) + + _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) + } yield () + private def checkNoPoliciesInUse(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = accessPolicyDAO.checkPolicyGroupsInUse(resource, samRequestContext).flatMap { problematicGroups => if (problematicGroups.nonEmpty) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index 41c1f7a38..7f1b4943b 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -126,6 +126,9 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = IO.pure(List.empty) + override def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = + IO.pure() + override def listAccessPolicies( resourceTypeName: ResourceTypeName, user: WorkbenchUserId, From abdf118209cb1f7bced75bb2d0f66051fac1215b Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 13:27:06 -0400 Subject: [PATCH 02/23] wrong tabbing? --- src/main/resources/swagger/api-docs.yaml | 64 ++++++++++++------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 16f6a3e65..778dfea9c 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -2124,38 +2124,38 @@ paths: policy on the resource content: { } /api/resources/v2/{resourceTypeName}/{resourceId}/cascade: - delete: - tags: - - Resources - summary: Delete a resource - operationId: deleteResourceV2 - parameters: - - name: resourceTypeName - in: path - description: Type of the resource - required: true - schema: - type: string - - name: resourceId - in: path - description: Id of the resource - required: true - schema: - type: string - responses: - 204: - description: Successfully deleted resource - content: { } - 400: - description: Cannot delete a resource with children. Delete the children first then try again. - content: { } - 403: - description: You do not have permission to perform this action on the resource or permissions on the resource's parent - content: { } - 404: - description: Resource type does not exist or you are not a member of any - policy on the resource - content: { } + delete: + tags: + - Resources + summary: Delete a resource + operationId: deleteResourceV2 + parameters: + - name: resourceTypeName + in: path + description: Type of the resource + required: true + schema: + type: string + - name: resourceId + in: path + description: Id of the resource + required: true + schema: + type: string + responses: + 204: + description: Successfully deleted resource + content: { } + 400: + description: Cannot delete a resource with children. Delete the children first then try again. + content: { } + 403: + description: You do not have permission to perform this action on the resource or permissions on the resource's parent + content: { } + 404: + description: Resource type does not exist or you are not a member of any + policy on the resource + content: { } /api/resources/v2/{resourceTypeName}/{resourceId}/actions: get: tags: From 511f6cfbda30826e38f50cf7ef11b287281a7764 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 13:38:28 -0400 Subject: [PATCH 03/23] fix api --- src/main/resources/swagger/api-docs.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 778dfea9c..7a680b2bd 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -2128,7 +2128,7 @@ paths: tags: - Resources summary: Delete a resource - operationId: deleteResourceV2 + operationId: deleteResourceCascadeV2 parameters: - name: resourceTypeName in: path From 5c2c881b6b93a73b2a105656588048f8bb4e120c Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 14:32:14 -0400 Subject: [PATCH 04/23] update delete query --- .../dataAccess/PostgresAccessPolicyDAO.scala | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 690d93b5c..b831a3573 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -1014,25 +1014,25 @@ class PostgresAccessPolicyDAO( 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)}) - """ + val deleteQuery = samsql"""delete from ${GroupMemberTable as gm} where ${gm.memberGroupId} in + (select distinct ${gm.result.memberGroupId} + from ${GroupMemberTable as gm} + join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} + where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})) + """ logger.warn(s"deleteQuery: ${deleteQuery.statement}") deleteQuery.update().apply() } 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() + val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gm} where ${gm.memberGroupId} in + (select distinct ${gm.result.memberGroupId} + from ${GroupMemberTable as gm} + join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} + where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})) + """ + logger.info(s"deleteFlatQuery: ${deleteFlatQuery.statement}") + deleteFlatQuery.update().apply() } } override def loadPolicy(resourceAndPolicyName: FullyQualifiedPolicyId, samRequestContext: SamRequestContext): IO[Option[AccessPolicy]] = From 4a80260cb5b0a59baf4c573fa438e5da6dfc5572 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 14:38:09 -0400 Subject: [PATCH 05/23] fix query --- .../dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index b831a3573..2f10870c1 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -1025,7 +1025,7 @@ class PostgresAccessPolicyDAO( } serializableWriteTransaction("removePolicyGroupsInUse", samRequestContext) { implicit session => - val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gm} where ${gm.memberGroupId} in + val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} where ${gmf.memberGroupId} in (select distinct ${gm.result.memberGroupId} from ${GroupMemberTable as gm} join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} From 08abd65499ac8f43d685832105333982e9dc9d95 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 14:55:47 -0400 Subject: [PATCH 06/23] one more try --- .../workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 2f10870c1..1ac2e94bc 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -1026,9 +1026,9 @@ class PostgresAccessPolicyDAO( serializableWriteTransaction("removePolicyGroupsInUse", samRequestContext) { implicit session => val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} where ${gmf.memberGroupId} in - (select distinct ${gm.result.memberGroupId} - from ${GroupMemberTable as gm} - join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} + (select distinct ${gmf.result.memberGroupId} + from ${GroupMemberFlatTable as gmf} + join ${PolicyTable as p} on ${gmf.memberGroupId} = ${p.groupId} where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})) """ logger.info(s"deleteFlatQuery: ${deleteFlatQuery.statement}") From ef2be686d96bb86ce67dc2b139ea7691074a3b76 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 15:17:36 -0400 Subject: [PATCH 07/23] split into two --- .../dataAccess/PostgresAccessPolicyDAO.scala | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 1ac2e94bc..5db115379 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -1007,14 +1007,18 @@ class PostgresAccessPolicyDAO( } override def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { + removeFromGroupTable(resourceId, samRequestContext) + removeFromGroupTableFlat(resourceId, samRequestContext) + } + private def removeFromGroupTable(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { logger.warn("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} where ${gm.memberGroupId} in + serializableWriteTransaction("removeFromGroupTable", samRequestContext) { implicit session => + val deleteQuery = + samsql"""delete from ${GroupMemberTable as gm} where ${gm.memberGroupId} in (select distinct ${gm.result.memberGroupId} from ${GroupMemberTable as gm} join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} @@ -1023,8 +1027,14 @@ class PostgresAccessPolicyDAO( logger.warn(s"deleteQuery: ${deleteQuery.statement}") deleteQuery.update().apply() } + } + + private def removeFromGroupTableFlat(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { + logger.warn("deleting from group_member_flat tables") + val p = PolicyTable.syntax("p") + val gmf = GroupMemberFlatTable.syntax("gmf") - serializableWriteTransaction("removePolicyGroupsInUse", samRequestContext) { implicit session => + serializableWriteTransaction("removeFromGroupTableFlat", samRequestContext) { implicit session => val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} where ${gmf.memberGroupId} in (select distinct ${gmf.result.memberGroupId} from ${GroupMemberFlatTable as gmf} From fc94de393e108bde95b07cd77b64913ecd950c84 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 15 Oct 2024 15:44:19 -0400 Subject: [PATCH 08/23] single transaction --- .../sam/dataAccess/PostgresAccessPolicyDAO.scala | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 5db115379..96a0ce620 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -1007,14 +1007,10 @@ class PostgresAccessPolicyDAO( } override def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { - removeFromGroupTable(resourceId, samRequestContext) - removeFromGroupTableFlat(resourceId, samRequestContext) - } - - private def removeFromGroupTable(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { logger.warn("deleting from group_member tables") val gm = GroupMemberTable.syntax("gm") val p = PolicyTable.syntax("p") + val gmf = GroupMemberFlatTable.syntax("gmf") serializableWriteTransaction("removeFromGroupTable", samRequestContext) { implicit session => val deleteQuery = @@ -1026,15 +1022,7 @@ class PostgresAccessPolicyDAO( """ logger.warn(s"deleteQuery: ${deleteQuery.statement}") deleteQuery.update().apply() - } - } - private def removeFromGroupTableFlat(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { - logger.warn("deleting from group_member_flat tables") - val p = PolicyTable.syntax("p") - val gmf = GroupMemberFlatTable.syntax("gmf") - - serializableWriteTransaction("removeFromGroupTableFlat", samRequestContext) { implicit session => val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} where ${gmf.memberGroupId} in (select distinct ${gmf.result.memberGroupId} from ${GroupMemberFlatTable as gmf} @@ -1045,6 +1033,7 @@ class PostgresAccessPolicyDAO( deleteFlatQuery.update().apply() } } + override def loadPolicy(resourceAndPolicyName: FullyQualifiedPolicyId, samRequestContext: SamRequestContext): IO[Option[AccessPolicy]] = listPolicies(resourceAndPolicyName.resource, limitOnePolicy = Option(resourceAndPolicyName.accessPolicyName), samRequestContext).map(_.headOption) From 4455509505fdcefc31e6c6edb1d9d3c19d0e3db6 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Thu, 17 Oct 2024 14:45:31 -0400 Subject: [PATCH 09/23] test out sync and delete policies --- .../dataAccess/PostgresAccessPolicyDAO.scala | 23 +++++++++++++++++++ .../sam/service/ResourceService.scala | 16 ++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 96a0ce620..c5714450c 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -573,6 +573,29 @@ class PostgresAccessPolicyDAO( deleteEffectivePolicies(resource, resourceTypePKsByName) removeAuthDomainFromResource(resource, samRequestContext) + val gm = GroupMemberTable.syntax("gm") + val p = PolicyTable.syntax("p") + val gmf = GroupMemberFlatTable.syntax("gmf") + + val deleteQuery = + samsql"""delete from ${GroupMemberTable as gm} where ${gm.memberGroupId} in + (select distinct ${gm.result.memberGroupId} + from ${GroupMemberTable as gm} + join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} + where ${p.resourceId} = (${loadResourcePKSubQuery(resource)})) + """ + logger.warn(s"deleteQuery: ${deleteQuery.statement}") + deleteQuery.update().apply() + + val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} where ${gmf.memberGroupId} in + (select distinct ${gmf.result.memberGroupId} + from ${GroupMemberFlatTable as gmf} + join ${PolicyTable as p} on ${gmf.memberGroupId} = ${p.groupId} + where ${p.resourceId} = (${loadResourcePKSubQuery(resource)})) + """ + logger.info(s"deleteFlatQuery: ${deleteFlatQuery.statement}") + deleteFlatQuery.update().apply() + val r = ResourceTable.syntax("r") if (leaveTombStone) { // if leaving a tombstone, we need to orphan the resource 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 index 939821cfc..ecd7a1010 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -432,7 +432,8 @@ class ResourceService( def deleteResource(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = for { _ <- checkNoChildren(resource, samRequestContext) - _ <- checkNoPoliciesInUse(resource, samRequestContext) +// _ <- checkNoPoliciesInUse(resource, samRequestContext) + _ <- cloudSyncPolicies(resource, samRequestContext) // remove from cloud first so a failure there does not leave sam in a bad state _ <- cloudDeletePolicies(resource, samRequestContext) @@ -514,6 +515,19 @@ class ResourceService( } } yield policiesToDelete + def cloudSyncPolicies(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[LazyList[AccessPolicy]] = + for { +// updatedPolicies <- accessPolicyDAO.listAccessPolicies(resource, samRequestContext) +// removedMembers = originalPolicies.flatMap(_.members).toSet -- updatedPolicies.flatMap(_.members).toSet +// addedMembers = updatedPolicies.flatMap(_.members).toSet -- originalPolicies.flatMap(_.members).toSet +// addedMembers = updatedPolicies.flatMap(_.members).toSet + + policiesToUpdate <- accessPolicyDAO.listAccessPolicies(resource, samRequestContext) + _ <- policiesToUpdate.traverse { policy => + cloudExtensions.onGroupUpdate(Seq(policy.id), Set.empty, samRequestContext) + } + } yield policiesToUpdate + def listUserResourceRoles(resource: FullyQualifiedResourceId, samUser: SamUser, samRequestContext: SamRequestContext): IO[Set[ResourceRoleName]] = accessPolicyDAO.listUserResourceRoles(resource, samUser.id, samRequestContext) From 841c2c8ec75461be54641df8e2af67479f2c6358 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Thu, 17 Oct 2024 15:52:54 -0400 Subject: [PATCH 10/23] update order of operations --- .../sam/dataAccess/PostgresAccessPolicyDAO.scala | 8 ++++---- .../dsde/workbench/sam/service/ResourceService.scala | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index c5714450c..2c8cd28ae 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -569,10 +569,6 @@ class PostgresAccessPolicyDAO( override def deleteResource(resource: FullyQualifiedResourceId, leaveTombStone: Boolean, samRequestContext: SamRequestContext): IO[Unit] = serializableWriteTransaction("deleteResource", samRequestContext) { implicit session => - deleteAllResourcePolicies(resource, samRequestContext) - deleteEffectivePolicies(resource, resourceTypePKsByName) - removeAuthDomainFromResource(resource, samRequestContext) - val gm = GroupMemberTable.syntax("gm") val p = PolicyTable.syntax("p") val gmf = GroupMemberFlatTable.syntax("gmf") @@ -596,6 +592,10 @@ class PostgresAccessPolicyDAO( logger.info(s"deleteFlatQuery: ${deleteFlatQuery.statement}") deleteFlatQuery.update().apply() + deleteAllResourcePolicies(resource, samRequestContext) + deleteEffectivePolicies(resource, resourceTypePKsByName) + removeAuthDomainFromResource(resource, samRequestContext) + val r = ResourceTable.syntax("r") if (leaveTombStone) { // if leaving a tombstone, we need to orphan the resource 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 index ecd7a1010..1f80f4d51 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -433,7 +433,6 @@ class ResourceService( for { _ <- checkNoChildren(resource, samRequestContext) // _ <- checkNoPoliciesInUse(resource, samRequestContext) - _ <- cloudSyncPolicies(resource, samRequestContext) // remove from cloud first so a failure there does not leave sam in a bad state _ <- cloudDeletePolicies(resource, samRequestContext) @@ -442,6 +441,7 @@ 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) _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) } yield () From 2ad6f23a6d1d2c17c11c6ebb02afdad6f64e2d29 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Thu, 17 Oct 2024 16:09:14 -0400 Subject: [PATCH 11/23] remove deletecascade --- src/main/resources/swagger/api-docs.yaml | 33 ----------------- .../workbench/sam/api/ResourceRoutes.scala | 16 -------- .../sam/dataAccess/AccessPolicyDAO.scala | 2 - .../dataAccess/PostgresAccessPolicyDAO.scala | 37 ++----------------- .../sam/service/ResourceService.scala | 29 ++------------- .../sam/dataAccess/MockAccessPolicyDAO.scala | 3 -- 6 files changed, 7 insertions(+), 113 deletions(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 7a680b2bd..d5430a19f 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -2123,39 +2123,6 @@ paths: description: Resource type does not exist or you are not a member of any policy on the resource content: { } - /api/resources/v2/{resourceTypeName}/{resourceId}/cascade: - delete: - tags: - - Resources - summary: Delete a resource - operationId: deleteResourceCascadeV2 - parameters: - - name: resourceTypeName - in: path - description: Type of the resource - required: true - schema: - type: string - - name: resourceId - in: path - description: Id of the resource - required: true - schema: - type: string - responses: - 204: - description: Successfully deleted resource - content: { } - 400: - description: Cannot delete a resource with children. Delete the children first then try again. - content: { } - 403: - description: You do not have permission to perform this action on the resource or permissions on the resource's parent - content: { } - 404: - description: Resource type does not exist or you are not a member of any - policy on the resource - content: { } /api/resources/v2/{resourceTypeName}/{resourceId}/actions: get: tags: 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 index 64b26df91..5f17cf02e 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala @@ -189,12 +189,6 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM listActionsForUser(resource, samUser, samRequestContext) } } ~ - pathPrefix("cascade") { - pathEndOrSingleSlash { - deleteResourceCascade(resource, samUser, samRequestContext) ~ - postDefaultResource(resourceType, resource, samUser, samRequestContext) - } - } ~ pathPrefix("allUsers") { pathEndOrSingleSlash { getAllResourceUsers(resource, samUser, samRequestContext) @@ -338,16 +332,6 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM } } - def deleteResourceCascade(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.deleteResourceCascade(resource, samRequestContext).map(_ => StatusCodes.NoContent)) - } - } - def postDefaultResource( resourceType: ResourceType, resource: FullyQualifiedResourceId, 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 index 3f89068d8..bb3397830 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala @@ -131,8 +131,6 @@ trait AccessPolicyDAO { ): IO[Seq[FilterResourcesResult]] 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/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 2c8cd28ae..73122dd44 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -574,11 +574,10 @@ class PostgresAccessPolicyDAO( val gmf = GroupMemberFlatTable.syntax("gmf") val deleteQuery = - samsql"""delete from ${GroupMemberTable as gm} where ${gm.memberGroupId} in - (select distinct ${gm.result.memberGroupId} - from ${GroupMemberTable as gm} - join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} - where ${p.resourceId} = (${loadResourcePKSubQuery(resource)})) + samsql"""delete from ${GroupMemberTable as gm} + using ${PolicyTable as p} + where ${gm.memberGroupId} = ${p.groupId} + and ${p.resourceId} = (${loadResourcePKSubQuery(resource)} """ logger.warn(s"deleteQuery: ${deleteQuery.statement}") deleteQuery.update().apply() @@ -1029,34 +1028,6 @@ class PostgresAccessPolicyDAO( } } - override def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = { - logger.warn("deleting from group_member tables") - val gm = GroupMemberTable.syntax("gm") - val p = PolicyTable.syntax("p") - val gmf = GroupMemberFlatTable.syntax("gmf") - - serializableWriteTransaction("removeFromGroupTable", samRequestContext) { implicit session => - val deleteQuery = - samsql"""delete from ${GroupMemberTable as gm} where ${gm.memberGroupId} in - (select distinct ${gm.result.memberGroupId} - from ${GroupMemberTable as gm} - join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} - where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})) - """ - logger.warn(s"deleteQuery: ${deleteQuery.statement}") - deleteQuery.update().apply() - - val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} where ${gmf.memberGroupId} in - (select distinct ${gmf.result.memberGroupId} - from ${GroupMemberFlatTable as gmf} - join ${PolicyTable as p} on ${gmf.memberGroupId} = ${p.groupId} - where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})) - """ - logger.info(s"deleteFlatQuery: ${deleteFlatQuery.statement}") - deleteFlatQuery.update().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/ResourceService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala index 1f80f4d51..7342df0f9 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -432,7 +432,6 @@ class ResourceService( def deleteResource(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = for { _ <- checkNoChildren(resource, samRequestContext) -// _ <- checkNoPoliciesInUse(resource, samRequestContext) // remove from cloud first so a failure there does not leave sam in a bad state _ <- cloudDeletePolicies(resource, samRequestContext) @@ -446,23 +445,6 @@ class ResourceService( _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) } yield () - @throws(classOf[WorkbenchExceptionWithErrorReport]) // Necessary to make Mockito happy - def deleteResourceCascade(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = - for { - _ <- checkNoChildren(resource, samRequestContext) - _ <- accessPolicyDAO.removePolicyGroupsInUse(resource, samRequestContext) - - // 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 - _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) - - _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) - } yield () - private def checkNoPoliciesInUse(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = accessPolicyDAO.checkPolicyGroupsInUse(resource, samRequestContext).flatMap { problematicGroups => if (problematicGroups.nonEmpty) @@ -517,16 +499,11 @@ class ResourceService( def cloudSyncPolicies(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[LazyList[AccessPolicy]] = for { -// updatedPolicies <- accessPolicyDAO.listAccessPolicies(resource, samRequestContext) -// removedMembers = originalPolicies.flatMap(_.members).toSet -- updatedPolicies.flatMap(_.members).toSet -// addedMembers = updatedPolicies.flatMap(_.members).toSet -- originalPolicies.flatMap(_.members).toSet -// addedMembers = updatedPolicies.flatMap(_.members).toSet - - policiesToUpdate <- accessPolicyDAO.listAccessPolicies(resource, samRequestContext) - _ <- policiesToUpdate.traverse { policy => + policiesToSync <- accessPolicyDAO.listAccessPolicies(resource, samRequestContext) + _ <- policiesToSync.traverse { policy => cloudExtensions.onGroupUpdate(Seq(policy.id), Set.empty, samRequestContext) } - } yield policiesToUpdate + } yield policiesToSync def listUserResourceRoles(resource: FullyQualifiedResourceId, samUser: SamUser, samRequestContext: SamRequestContext): IO[Set[ResourceRoleName]] = accessPolicyDAO.listUserResourceRoles(resource, samUser.id, samRequestContext) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index 7f1b4943b..41c1f7a38 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -126,9 +126,6 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = IO.pure(List.empty) - override def removePolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = - IO.pure() - override def listAccessPolicies( resourceTypeName: ResourceTypeName, user: WorkbenchUserId, From a9a3083f4a56273df4213de5407bf85c4cf67ba9 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Thu, 17 Oct 2024 16:24:01 -0400 Subject: [PATCH 12/23] whoops --- .../dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 73122dd44..ca65163e5 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -577,7 +577,7 @@ class PostgresAccessPolicyDAO( samsql"""delete from ${GroupMemberTable as gm} using ${PolicyTable as p} where ${gm.memberGroupId} = ${p.groupId} - and ${p.resourceId} = (${loadResourcePKSubQuery(resource)} + and ${p.resourceId} = (${loadResourcePKSubQuery(resource)}) """ logger.warn(s"deleteQuery: ${deleteQuery.statement}") deleteQuery.update().apply() From 5f1ac335e1972ce08754036f0fb0f6190871200c Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Mon, 21 Oct 2024 12:47:48 -0400 Subject: [PATCH 13/23] cleanup and add test --- .../sam/dataAccess/AccessPolicyDAO.scala | 2 +- .../dataAccess/PostgresAccessPolicyDAO.scala | 70 +++++++++---------- .../sam/service/ResourceService.scala | 11 --- .../sam/dataAccess/MockAccessPolicyDAO.scala | 4 +- .../sam/service/ResourceServiceSpec.scala | 60 +++++++++++++++- 5 files changed, 97 insertions(+), 50 deletions(-) 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 index bb3397830..57c42f1d1 100644 --- 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,7 @@ trait AccessPolicyDAO { samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] - def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] +// def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] } sealed abstract class LoadResourceAuthDomainResult diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index ca65163e5..80126cccf 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -573,6 +573,7 @@ class PostgresAccessPolicyDAO( val p = PolicyTable.syntax("p") val gmf = GroupMemberFlatTable.syntax("gmf") + // Remove policy members from group tables to prevent FK Violations val deleteQuery = samsql"""delete from ${GroupMemberTable as gm} using ${PolicyTable as p} @@ -582,11 +583,10 @@ class PostgresAccessPolicyDAO( logger.warn(s"deleteQuery: ${deleteQuery.statement}") deleteQuery.update().apply() - val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} where ${gmf.memberGroupId} in - (select distinct ${gmf.result.memberGroupId} - from ${GroupMemberFlatTable as gmf} - join ${PolicyTable as p} on ${gmf.memberGroupId} = ${p.groupId} - where ${p.resourceId} = (${loadResourcePKSubQuery(resource)})) + val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} + using ${PolicyTable as p} + where ${gmf.memberGroupId} = ${p.groupId} + and ${p.resourceId} = (${loadResourcePKSubQuery(resource)}) """ logger.info(s"deleteFlatQuery: ${deleteFlatQuery.statement}") deleteFlatQuery.update().apply() @@ -997,36 +997,36 @@ class PostgresAccessPolicyDAO( } } - 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/ResourceService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala index 7342df0f9..cdeb2d767 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -445,17 +445,6 @@ class ResourceService( _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) } yield () - private def checkNoPoliciesInUse(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = - 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 .map { service => diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index 41c1f7a38..dcabbb3bd 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -123,8 +123,8 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam policies -= policy } - override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = - IO.pure(List.empty) +// override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = +// IO.pure(List.empty) override def listAccessPolicies( resourceTypeName: ResourceTypeName, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala index 081d1c80e..7f2746157 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala @@ -2278,7 +2278,7 @@ class ResourceServiceSpec testDeleteResource(managedGroupResourceType) } - it should "delete any action managed identites for the resource while it deletes the resource" in { + it should "delete any action managed identities for the resource while it deletes the resource" in { assume(databaseEnabled, databaseEnabledClue) val resource = FullyQualifiedResourceId(defaultResourceType.name, ResourceId("my-resource")) @@ -2378,6 +2378,64 @@ class ResourceServiceSpec assert(policyDAO.listAccessPolicies(parentResource, samRequestContext).unsafeRunSync().nonEmpty) } + it should "delete any policy members from groups before deleting resource" in { + assume(databaseEnabled, databaseEnabledClue) + + // Create a resource with a policy + val ownerRoleName = ResourceRoleName("owner") + val resourceType = ResourceType( + ResourceTypeName(UUID.randomUUID().toString), + Set(SamResourceActionPatterns.delete, ResourceActionPattern("view", "", false)), + Set(ResourceRole(ownerRoleName, Set(ResourceAction("delete"), ResourceAction("view")))), + ownerRoleName + ) + val resourceName = ResourceId("resource") + + runAndWait(service.createResourceType(resourceType, samRequestContext)) + + // creating policy and to refer to by policy identifiers + val policyMembership = AccessPolicyMembershipRequest(Set(dummyUser.email), Set(ResourceAction("view")), Set(ownerRoleName), Option(Set.empty)) + val policyName = AccessPolicyName("foo") + val resource = + runAndWait(service.createResource(resourceType, resourceName, Map(policyName -> policyMembership), Set.empty, None, dummyUser.id, samRequestContext)) + val policies: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName), samRequestContext).unsafeRunSync() + + // creating policy to refer to by policy email + val resourceName2 = ResourceId("resource2") + val policyMembership2 = AccessPolicyMembershipRequest(Set(dummyUser.email), Set(ResourceAction("view")), Set(ownerRoleName), None, None) + val policyName2 = AccessPolicyName("foo2") + val resource2 = + runAndWait( + service.createResource(resourceType, resourceName2, Map(policyName2 -> policyMembership2), Set.empty, None, dummyUser.id, samRequestContext) + ) + val policies2: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName2), samRequestContext).unsafeRunSync() + + // Add second resource to first policy + policies2.foreach { policy => + runAndWait( + service.addSubjectToPolicy( + FullyQualifiedPolicyId(FullyQualifiedResourceId(resourceType.name, resourceName), policyName), + FullyQualifiedPolicyId(FullyQualifiedResourceId(resourceType.name, resourceName2), policyName2), + samRequestContext + ) + ) + } + val policy2Emails = policies2.map(p => p.email).toSet + // Verify that policies are in group + val updatedPolicies: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName), samRequestContext).unsafeRunSync() + policy2Emails.subsetOf(updatedPolicies.head.policy.memberEmails) shouldBe true + // Delete resource; should not throw an error + runAndWait(service.deleteResource(FullyQualifiedResourceId(defaultResourceType.name, resourceName2), samRequestContext)) + + // Verify that policies are no longer in group + val updatedPolicies2: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName), samRequestContext).unsafeRunSync() + policy2Emails.subsetOf(updatedPolicies2.head.policy.memberEmails) shouldBe false + } + "validatePolicy" should "succeed with a correct policy" in { val emailToMaybeSubject = Map(dummyUser.email -> Option(dummyUser.id.asInstanceOf[WorkbenchSubject])) val policy = service.ValidatableAccessPolicy(AccessPolicyName("a"), emailToMaybeSubject, Set(ownerRoleName), Set(ResourceAction("alter_policies")), Set()) From a6bcf06364bd9a88b17ab41a7a7f0b8c6ac9acdd Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Mon, 21 Oct 2024 15:39:55 -0400 Subject: [PATCH 14/23] removed commented out code and fix test --- .../sam/dataAccess/AccessPolicyDAO.scala | 1 - .../dataAccess/PostgresAccessPolicyDAO.scala | 31 ------------------- .../sam/dataAccess/MockAccessPolicyDAO.scala | 3 -- .../sam/service/ResourceServiceSpec.scala | 4 +-- 4 files changed, 2 insertions(+), 37 deletions(-) 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 index 57c42f1d1..c59ec84f4 100644 --- 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,6 @@ trait AccessPolicyDAO { samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] -// def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] } sealed abstract class LoadResourceAuthDomainResult diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 80126cccf..8b0fd10fa 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -997,37 +997,6 @@ class PostgresAccessPolicyDAO( } } -// 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/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index dcabbb3bd..73f95d880 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -123,9 +123,6 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam policies -= policy } -// override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = -// IO.pure(List.empty) - override def listAccessPolicies( resourceTypeName: ResourceTypeName, user: WorkbenchUserId, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala index 7f2746157..5e85e19e3 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala @@ -2384,7 +2384,7 @@ class ResourceServiceSpec // Create a resource with a policy val ownerRoleName = ResourceRoleName("owner") val resourceType = ResourceType( - ResourceTypeName(UUID.randomUUID().toString), + defaultResourceType.name, Set(SamResourceActionPatterns.delete, ResourceActionPattern("view", "", false)), Set(ResourceRole(ownerRoleName, Set(ResourceAction("delete"), ResourceAction("view")))), ownerRoleName @@ -2428,7 +2428,7 @@ class ResourceServiceSpec service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName), samRequestContext).unsafeRunSync() policy2Emails.subsetOf(updatedPolicies.head.policy.memberEmails) shouldBe true // Delete resource; should not throw an error - runAndWait(service.deleteResource(FullyQualifiedResourceId(defaultResourceType.name, resourceName2), samRequestContext)) + runAndWait(service.deleteResource(FullyQualifiedResourceId(resourceType.name, resourceName2), samRequestContext)) // Verify that policies are no longer in group val updatedPolicies2: Seq[AccessPolicyResponseEntry] = From 8c0ad7d60c5505867893ceff5c616f7fefe949bd Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Thu, 24 Oct 2024 14:24:19 -0400 Subject: [PATCH 15/23] first pass updated delete resource --- .../sam/dataAccess/AccessPolicyDAO.scala | 6 + .../dataAccess/PostgresAccessPolicyDAO.scala | 142 +++++++++++++++--- .../sam/service/ResourceService.scala | 14 +- .../sam/dataAccess/MockAccessPolicyDAO.scala | 5 + 4 files changed, 141 insertions(+), 26 deletions(-) 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 index c59ec84f4..a64ab184b 100644 --- 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,6 +130,12 @@ trait AccessPolicyDAO { samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] + // TODO: better name? + def findAffectedPolicyGroups( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] + } sealed abstract class LoadResourceAuthDomainResult diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 8b0fd10fa..878a533d3 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -569,28 +569,7 @@ class PostgresAccessPolicyDAO( override def deleteResource(resource: FullyQualifiedResourceId, leaveTombStone: Boolean, samRequestContext: SamRequestContext): IO[Unit] = serializableWriteTransaction("deleteResource", samRequestContext) { implicit session => - val gm = GroupMemberTable.syntax("gm") - val p = PolicyTable.syntax("p") - val gmf = GroupMemberFlatTable.syntax("gmf") - - // Remove policy members from group tables to prevent FK Violations - val deleteQuery = - samsql"""delete from ${GroupMemberTable as gm} - using ${PolicyTable as p} - where ${gm.memberGroupId} = ${p.groupId} - and ${p.resourceId} = (${loadResourcePKSubQuery(resource)}) - """ - logger.warn(s"deleteQuery: ${deleteQuery.statement}") - deleteQuery.update().apply() - - val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} - using ${PolicyTable as p} - where ${gmf.memberGroupId} = ${p.groupId} - and ${p.resourceId} = (${loadResourcePKSubQuery(resource)}) - """ - logger.info(s"deleteFlatQuery: ${deleteFlatQuery.statement}") - deleteFlatQuery.update().apply() - + deletePolicyMembersFromGroups(resource, samRequestContext) deleteAllResourcePolicies(resource, samRequestContext) deleteEffectivePolicies(resource, resourceTypePKsByName) removeAuthDomainFromResource(resource, samRequestContext) @@ -973,6 +952,125 @@ class PostgresAccessPolicyDAO( } } + // TODO write the correct query + // Return value: [(policyToUpdate, policyToRemove)] + override def findAffectedPolicyGroups( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = +// val g = GroupTable.syntax("g") +// val pg = GroupTable.syntax("pg") // problematic group +// val gm = GroupMemberTable.syntax("gm") +// val p = PolicyTable.syntax("p") + + readOnlyTransaction("findAffectedPolicyGroups", samRequestContext) { implicit session => +// samsql"""select * from ${GroupMemberTable as gm} +// using ${PolicyTable as p} +// where ${gm.memberGroupId} = ${p.groupId} +// and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})""" +// 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() + +// val query = samsql""" +// select ${pg.result.name}, ${g.result.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)}) +// ) +//""" + +// val result: List[(String, String)] = query +// .map { rs => +// (rs.string(pg.resultName.name), rs.string(g.resultName.name)) +// } +// .list() +// .apply() + 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") + + val query = samsql""" + select ${pg.result.name}, ${rt.result.name}, ${r.result.name}, ${g.result.name} + from ${GroupTable as g} + join ${GroupMemberTable as gm} on ${g.id} = ${gm.memberGroupId} + join ${GroupTable as pg} on ${gm.groupId} = ${pg.id} + join ${PolicyTable as p} on ${pg.id} = ${p.groupId} + join ${ResourceTable as r} on ${p.resourceId} = ${r.id} + join ${ResourceTypeTable as rt} on ${r.resourceTypeId} = ${rt.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)}) + ) + """ + + query.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) + val accessPolicyName2 = rs.get[AccessPolicyName](g.resultName.name) + + val fullyQualifiedResourceId1 = FullyQualifiedResourceId(resourceTypeName1, resourceId1) + val fullyQualifiedPolicyId1 = FullyQualifiedPolicyId(fullyQualifiedResourceId1, accessPolicyName1) + val fullyQualifiedPolicyId2 = FullyQualifiedPolicyId(resourceId, accessPolicyName2) + + (fullyQualifiedPolicyId1, fullyQualifiedPolicyId2) + }.list().apply() + + } + + private def deletePolicyMembersFromGroups(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext)(implicit + session: DBSession + ): Unit = { + val gm = GroupMemberTable.syntax("gm") + val p = PolicyTable.syntax("p") + val gmf = GroupMemberFlatTable.syntax("gmf") + + // Remove policy members from group tables to prevent FK Violations + val deleteQuery = + samsql"""delete from ${GroupMemberTable as gm} + using ${PolicyTable as p} + where ${gm.memberGroupId} = ${p.groupId} + and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)}) + """ + deleteQuery.update().apply() + + val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} + using ${PolicyTable as p} + where ${gmf.memberGroupId} = ${p.groupId} + and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)}) + """ + deleteFlatQuery.update().apply() + } + private def deleteAllResourcePolicies(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext)(implicit session: DBSession ): Unit = { 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 index cdeb2d767..8591ad18f 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -439,8 +439,10 @@ class ResourceService( // 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 _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) - _ <- cloudSyncPolicies(resource, samRequestContext) _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) } yield () @@ -486,13 +488,17 @@ class ResourceService( } } yield policiesToDelete - def cloudSyncPolicies(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[LazyList[AccessPolicy]] = + def cloudSyncPolicies( + policiesList: List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)], + samRequestContext: SamRequestContext + ): IO[List[FullyQualifiedPolicyId]] = { + val policiesToSync: List[FullyQualifiedPolicyId] = policiesList.map { case (firstPolicy, _) => firstPolicy }.toSet.toList for { - policiesToSync <- accessPolicyDAO.listAccessPolicies(resource, samRequestContext) _ <- policiesToSync.traverse { policy => - cloudExtensions.onGroupUpdate(Seq(policy.id), Set.empty, samRequestContext) + cloudExtensions.onGroupUpdate(Seq(policy), Set.empty, samRequestContext) } } yield policiesToSync + } def listUserResourceRoles(resource: FullyQualifiedResourceId, samUser: SamUser, samRequestContext: SamRequestContext): IO[Set[ResourceRoleName]] = accessPolicyDAO.listUserResourceRoles(resource, samUser.id, samRequestContext) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index 73f95d880..1565c7941 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -123,6 +123,11 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam policies -= policy } + override def findAffectedPolicyGroups( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = IO.pure(List.empty) + override def listAccessPolicies( resourceTypeName: ResourceTypeName, user: WorkbenchUserId, From f78fb5a1bcdb4732652e3b8a873fae91b1790aba Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Thu, 24 Oct 2024 14:42:57 -0400 Subject: [PATCH 16/23] scalafmt --- .../dataAccess/PostgresAccessPolicyDAO.scala | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 878a533d3..7c53dbb8b 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -1009,14 +1009,14 @@ class PostgresAccessPolicyDAO( // } // .list() // .apply() - 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") - - val query = samsql""" + 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") + + val query = samsql""" select ${pg.result.name}, ${rt.result.name}, ${r.result.name}, ${g.result.name} from ${GroupTable as g} join ${GroupMemberTable as gm} on ${g.id} = ${gm.memberGroupId} @@ -1032,18 +1032,21 @@ class PostgresAccessPolicyDAO( ) """ - query.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) - val accessPolicyName2 = rs.get[AccessPolicyName](g.resultName.name) + query + .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) + val accessPolicyName2 = rs.get[AccessPolicyName](g.resultName.name) - val fullyQualifiedResourceId1 = FullyQualifiedResourceId(resourceTypeName1, resourceId1) - val fullyQualifiedPolicyId1 = FullyQualifiedPolicyId(fullyQualifiedResourceId1, accessPolicyName1) - val fullyQualifiedPolicyId2 = FullyQualifiedPolicyId(resourceId, accessPolicyName2) + val fullyQualifiedResourceId1 = FullyQualifiedResourceId(resourceTypeName1, resourceId1) + val fullyQualifiedPolicyId1 = FullyQualifiedPolicyId(fullyQualifiedResourceId1, accessPolicyName1) + val fullyQualifiedPolicyId2 = FullyQualifiedPolicyId(resourceId, accessPolicyName2) - (fullyQualifiedPolicyId1, fullyQualifiedPolicyId2) - }.list().apply() + (fullyQualifiedPolicyId1, fullyQualifiedPolicyId2) + } + .list() + .apply() } From 8b4fa1bc1bf38dfad446e4c8abea0b58be5dcbf5 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Thu, 24 Oct 2024 16:12:23 -0400 Subject: [PATCH 17/23] remove commented out code --- .../dataAccess/PostgresAccessPolicyDAO.scala | 59 ++----------------- 1 file changed, 6 insertions(+), 53 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 7c53dbb8b..e2d057d51 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -958,57 +958,7 @@ class PostgresAccessPolicyDAO( resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = -// val g = GroupTable.syntax("g") -// val pg = GroupTable.syntax("pg") // problematic group -// val gm = GroupMemberTable.syntax("gm") -// val p = PolicyTable.syntax("p") - readOnlyTransaction("findAffectedPolicyGroups", samRequestContext) { implicit session => -// samsql"""select * from ${GroupMemberTable as gm} -// using ${PolicyTable as p} -// where ${gm.memberGroupId} = ${p.groupId} -// and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})""" -// 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() - -// val query = samsql""" -// select ${pg.result.name}, ${g.result.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)}) -// ) -//""" - -// val result: List[(String, String)] = query -// .map { rs => -// (rs.string(pg.resultName.name), rs.string(g.resultName.name)) -// } -// .list() -// .apply() val g = GroupTable.syntax("g") val pg = GroupTable.syntax("pg") val gm = GroupMemberTable.syntax("gm") @@ -1017,7 +967,7 @@ class PostgresAccessPolicyDAO( val r = ResourceTable.syntax("r") val query = samsql""" - select ${pg.result.name}, ${rt.result.name}, ${r.result.name}, ${g.result.name} + select ${pg.result.name}, ${rt.result.name}, ${r.result.name}, ${p.result.name} from ${GroupTable as g} join ${GroupMemberTable as gm} on ${g.id} = ${gm.memberGroupId} join ${GroupTable as pg} on ${gm.groupId} = ${pg.id} @@ -1031,13 +981,16 @@ class PostgresAccessPolicyDAO( where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)}) ) """ - + query.foreach { rs => + println(s"pg.name: ${rs.string(pg.resultName.name)}, rt.name: ${rs.string(rt.resultName.name)}, r.name: ${rs.string(r.resultName.name)}, g.name: ${rs + .string(p.resultName.name)}") + } query .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) - val accessPolicyName2 = rs.get[AccessPolicyName](g.resultName.name) + val accessPolicyName2 = rs.get[AccessPolicyName](p.resultName.name) val fullyQualifiedResourceId1 = FullyQualifiedResourceId(resourceTypeName1, resourceId1) val fullyQualifiedPolicyId1 = FullyQualifiedPolicyId(fullyQualifiedResourceId1, accessPolicyName1) From bf02fc0d5059fbb549332e73bfa546d883ec09a2 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 29 Oct 2024 13:46:52 -0400 Subject: [PATCH 18/23] fix affected policy groups and add test --- .../dataAccess/PostgresAccessPolicyDAO.scala | 59 ++++++++----------- .../PostgresAccessPolicyDAOSpec.scala | 55 +++++++++++++++++ 2 files changed, 81 insertions(+), 33 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index e2d057d51..4fd3b1b9f 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -952,51 +952,44 @@ class PostgresAccessPolicyDAO( } } - // TODO write the correct query // Return value: [(policyToUpdate, policyToRemove)] override def findAffectedPolicyGroups( resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = readOnlyTransaction("findAffectedPolicyGroups", samRequestContext) { implicit session => - 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") + val group_member_table = GroupMemberTable.syntax("group_member_table") + val policy_table = PolicyTable.syntax("policy_table") + val parent_policy_table = PolicyTable.syntax("parent_policy_table") + val resource_type = ResourceTypeTable.syntax("resource_type") + val resource_table = ResourceTable.syntax("resource_table") val query = samsql""" - select ${pg.result.name}, ${rt.result.name}, ${r.result.name}, ${p.result.name} - from ${GroupTable as g} - join ${GroupMemberTable as gm} on ${g.id} = ${gm.memberGroupId} - join ${GroupTable as pg} on ${gm.groupId} = ${pg.id} - join ${PolicyTable as p} on ${pg.id} = ${p.groupId} - join ${ResourceTable as r} on ${p.resourceId} = ${r.id} - join ${ResourceTypeTable as rt} on ${r.resourceTypeId} = ${rt.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)}) - ) - """ - query.foreach { rs => - println(s"pg.name: ${rs.string(pg.resultName.name)}, rt.name: ${rs.string(rt.resultName.name)}, r.name: ${rs.string(r.resultName.name)}, g.name: ${rs - .string(p.resultName.name)}") - } + WITH resourcePolicies as ( + SELECT ${policy_table.groupId} as childGroupId, ${policy_table.name} as policyName + FROM ${PolicyTable as policy_table} + WHERE ${policy_table.resourceId} = (${loadResourcePKSubQuery(resourceId)}) + ) + SELECT resourcePolicies.policyName as childPolicyName, ${parent_policy_table.name} as parentPolicyName, ${resource_table.name} as parentPolicyResourceName, ${resource_type.name} as parentPolicyResourceType +from ${GroupMemberTable as group_member_table} + JOIN resourcePolicies ON ${group_member_table.memberGroupId} = resourcePolicies.childGroupId + JOIN ${PolicyTable as parent_policy_table} ON ${parent_policy_table.groupId} = ${group_member_table.groupId} + JOIN ${ResourceTable as resource_table} ON ${resource_table.id} = ${parent_policy_table.resourceId} + JOIN ${ResourceTypeTable as resource_type} ON ${resource_type.id} = ${resource_table.resourceTypeId} +""" query .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) - val accessPolicyName2 = rs.get[AccessPolicyName](p.resultName.name) + val parent_policy_resource_type = rs.get[ResourceTypeName]("parentPolicyResourceType") + val parent_policy_resource_id = rs.get[ResourceId]("parentPolicyResourceName") + val parent_policy_access_name = rs.get[AccessPolicyName]("parentPolicyName") + val member_policy_access_name = rs.get[AccessPolicyName]("childPolicyName") - val fullyQualifiedResourceId1 = FullyQualifiedResourceId(resourceTypeName1, resourceId1) - val fullyQualifiedPolicyId1 = FullyQualifiedPolicyId(fullyQualifiedResourceId1, accessPolicyName1) - val fullyQualifiedPolicyId2 = FullyQualifiedPolicyId(resourceId, accessPolicyName2) + val parent_policy_full_resource_id = + FullyQualifiedResourceId(parent_policy_resource_type, parent_policy_resource_id) + val parent_policy_full_id = FullyQualifiedPolicyId(parent_policy_full_resource_id, parent_policy_access_name) + val member_policy_full_id = FullyQualifiedPolicyId(resourceId, member_policy_access_name) - (fullyQualifiedPolicyId1, fullyQualifiedPolicyId2) + (parent_policy_full_id, member_policy_full_id) } .list() .apply() diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala index 178c99b22..166525639 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala @@ -3553,6 +3553,61 @@ class PostgresAccessPolicyDAOSpec extends AnyFreeSpec with Matchers with BeforeA dao.listResourcesUsingAuthDomain(authDomainGroupName, samRequestContext).unsafeRunSync() shouldEqual Set.empty } } + + "findAffectedPolicyGroups" - { + "returns parent and member policy groups" in { + assume(databaseEnabled, databaseEnabledClue) + + // Create a resource with a policy + dao.createResourceType(resourceType, samRequestContext).unsafeRunSync() + + val user = Generator.genWorkbenchUserBoth.sample.get + dirDao.createUser(user, samRequestContext).unsafeRunSync() + + val parentResourceFullyQualifiedId = FullyQualifiedResourceId(resourceType.name, ResourceId("parent_resource")) + val childResourceFullyQualifiedId = FullyQualifiedResourceId(resourceType.name, ResourceId("child_resource")) + + val parentPolicy = AccessPolicy( + FullyQualifiedPolicyId(parentResourceFullyQualifiedId, AccessPolicyName("parentPolicyName")), + Set(user.id), + WorkbenchEmail("parentPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + val childPolicy = AccessPolicy( + FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("childPolicyName")), + Set(user.id), + WorkbenchEmail("childPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + + val parentResource = + Resource(parentResourceFullyQualifiedId.resourceTypeName, parentResourceFullyQualifiedId.resourceId, Set.empty, Set(parentPolicy)) + val childResource = + Resource( + childResourceFullyQualifiedId.resourceTypeName, + childResourceFullyQualifiedId.resourceId, + Set.empty, + Set(childPolicy) + ) + dao.createResource(parentResource, samRequestContext).unsafeRunSync() + dao.createResource(childResource, samRequestContext).unsafeRunSync() + + // Add child policy to parent policy + dirDao.addGroupMember(parentPolicy.id, childPolicy.id, samRequestContext).unsafeRunSync() + + val policyGroups = dao.findAffectedPolicyGroups(childResourceFullyQualifiedId, samRequestContext).unsafeRunSync() + + policyGroups should not be empty + policyGroups.head._1 shouldEqual parentPolicy.id + policyGroups.head._2 shouldEqual childPolicy.id + } + } } private def uuid: String = From 34d18e8b168686e1f00f8fb4156c61a987a45d67 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Tue, 29 Oct 2024 14:11:07 -0400 Subject: [PATCH 19/23] sonar findings --- .../dataAccess/PostgresAccessPolicyDAO.scala | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 4fd3b1b9f..b5e168a57 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -569,7 +569,7 @@ class PostgresAccessPolicyDAO( override def deleteResource(resource: FullyQualifiedResourceId, leaveTombStone: Boolean, samRequestContext: SamRequestContext): IO[Unit] = serializableWriteTransaction("deleteResource", samRequestContext) { implicit session => - deletePolicyMembersFromGroups(resource, samRequestContext) + deletePolicyMembersFromGroups(resource) deleteAllResourcePolicies(resource, samRequestContext) deleteEffectivePolicies(resource, resourceTypePKsByName) removeAuthDomainFromResource(resource, samRequestContext) @@ -958,45 +958,45 @@ class PostgresAccessPolicyDAO( samRequestContext: SamRequestContext ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = readOnlyTransaction("findAffectedPolicyGroups", samRequestContext) { implicit session => - val group_member_table = GroupMemberTable.syntax("group_member_table") - val policy_table = PolicyTable.syntax("policy_table") - val parent_policy_table = PolicyTable.syntax("parent_policy_table") - val resource_type = ResourceTypeTable.syntax("resource_type") - val resource_table = ResourceTable.syntax("resource_table") + val groupMemberTable = GroupMemberTable.syntax("group_member_table") + val policyTable = PolicyTable.syntax("policy_table") + val parentPolicyTable = PolicyTable.syntax("parent_policy_table") + val resourceType = ResourceTypeTable.syntax("resource_type") + val resourceTable = ResourceTable.syntax("resource_table") val query = samsql""" WITH resourcePolicies as ( - SELECT ${policy_table.groupId} as childGroupId, ${policy_table.name} as policyName - FROM ${PolicyTable as policy_table} - WHERE ${policy_table.resourceId} = (${loadResourcePKSubQuery(resourceId)}) + SELECT ${policyTable.groupId} as childGroupId, ${policyTable.name} as policyName + FROM ${PolicyTable as policyTable} + WHERE ${policyTable.resourceId} = (${loadResourcePKSubQuery(resourceId)}) ) - SELECT resourcePolicies.policyName as childPolicyName, ${parent_policy_table.name} as parentPolicyName, ${resource_table.name} as parentPolicyResourceName, ${resource_type.name} as parentPolicyResourceType -from ${GroupMemberTable as group_member_table} - JOIN resourcePolicies ON ${group_member_table.memberGroupId} = resourcePolicies.childGroupId - JOIN ${PolicyTable as parent_policy_table} ON ${parent_policy_table.groupId} = ${group_member_table.groupId} - JOIN ${ResourceTable as resource_table} ON ${resource_table.id} = ${parent_policy_table.resourceId} - JOIN ${ResourceTypeTable as resource_type} ON ${resource_type.id} = ${resource_table.resourceTypeId} + SELECT resourcePolicies.policyName as childPolicyName, ${parentPolicyTable.name} as parentPolicyName, ${resourceTable.name} as parentPolicyResourceName, ${resourceType.name} as parentPolicyResourceType +from ${GroupMemberTable as groupMemberTable} + JOIN resourcePolicies ON ${groupMemberTable.memberGroupId} = resourcePolicies.childGroupId + JOIN ${PolicyTable as parentPolicyTable} ON ${parentPolicyTable.groupId} = ${groupMemberTable.groupId} + JOIN ${ResourceTable as resourceTable} ON ${resourceTable.id} = ${parentPolicyTable.resourceId} + JOIN ${ResourceTypeTable as resourceType} ON ${resourceType.id} = ${resourceTable.resourceTypeId} """ query .map { rs => - val parent_policy_resource_type = rs.get[ResourceTypeName]("parentPolicyResourceType") - val parent_policy_resource_id = rs.get[ResourceId]("parentPolicyResourceName") - val parent_policy_access_name = rs.get[AccessPolicyName]("parentPolicyName") - val member_policy_access_name = rs.get[AccessPolicyName]("childPolicyName") + val parentPolicyResourceType = rs.get[ResourceTypeName]("parentPolicyResourceType") + val parentPolicyResourceId = rs.get[ResourceId]("parentPolicyResourceName") + val parentPolciyAccessName = rs.get[AccessPolicyName]("parentPolicyName") + val memberPolicyAccessName = rs.get[AccessPolicyName]("childPolicyName") - val parent_policy_full_resource_id = - FullyQualifiedResourceId(parent_policy_resource_type, parent_policy_resource_id) - val parent_policy_full_id = FullyQualifiedPolicyId(parent_policy_full_resource_id, parent_policy_access_name) - val member_policy_full_id = FullyQualifiedPolicyId(resourceId, member_policy_access_name) + val parentPolicyFullResourceId = + FullyQualifiedResourceId(parentPolicyResourceType, parentPolicyResourceId) + val parentPolicyFullId = FullyQualifiedPolicyId(parentPolicyFullResourceId, parentPolciyAccessName) + val memberPolicyFullId = FullyQualifiedPolicyId(resourceId, memberPolicyAccessName) - (parent_policy_full_id, member_policy_full_id) + (parentPolicyFullId, memberPolicyFullId) } .list() .apply() } - private def deletePolicyMembersFromGroups(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext)(implicit + private def deletePolicyMembersFromGroups(resourceId: FullyQualifiedResourceId)(implicit session: DBSession ): Unit = { val gm = GroupMemberTable.syntax("gm") From 7428caf27efe0a4af930a4b02b6fda501c65567f Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Wed, 30 Oct 2024 14:36:32 -0400 Subject: [PATCH 20/23] expand test --- .../sam/dataAccess/AccessPolicyDAO.scala | 3 +- .../dataAccess/PostgresAccessPolicyDAO.scala | 4 +- .../sam/service/ResourceService.scala | 2 +- .../sam/dataAccess/MockAccessPolicyDAO.scala | 2 +- .../PostgresAccessPolicyDAOSpec.scala | 48 +++++++++++++++++-- 5 files changed, 48 insertions(+), 11 deletions(-) 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 index a64ab184b..755e027cd 100644 --- 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,8 +130,7 @@ trait AccessPolicyDAO { samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] - // TODO: better name? - def findAffectedPolicyGroups( + def findPolicyGroupsInUse( resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index b5e168a57..741973c7e 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -569,7 +569,7 @@ class PostgresAccessPolicyDAO( override def deleteResource(resource: FullyQualifiedResourceId, leaveTombStone: Boolean, samRequestContext: SamRequestContext): IO[Unit] = serializableWriteTransaction("deleteResource", samRequestContext) { implicit session => - deletePolicyMembersFromGroups(resource) +// deletePolicyMembersFromGroups(resource) deleteAllResourcePolicies(resource, samRequestContext) deleteEffectivePolicies(resource, resourceTypePKsByName) removeAuthDomainFromResource(resource, samRequestContext) @@ -953,7 +953,7 @@ class PostgresAccessPolicyDAO( } // Return value: [(policyToUpdate, policyToRemove)] - override def findAffectedPolicyGroups( + override def findPolicyGroupsInUse( resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = 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 index 8591ad18f..7540e1e1c 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -439,7 +439,7 @@ class ResourceService( // 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 <- accessPolicyDAO.findPolicyGroupsInUse(resource, samRequestContext) // New method _ <- affectedPolicies.traverse { case (policyToUpdate, policyToRemove) => removeSubjectFromPolicy(policyToUpdate, policyToRemove, samRequestContext) } _ <- cloudSyncPolicies(affectedPolicies, samRequestContext) // TODO: use this on affected policies _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index 1565c7941..b9a00c3e4 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -123,7 +123,7 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam policies -= policy } - override def findAffectedPolicyGroups( + override def findPolicyGroupsInUse( resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = IO.pure(List.empty) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala index 166525639..6c85d9c52 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala @@ -3586,6 +3586,37 @@ class PostgresAccessPolicyDAOSpec extends AnyFreeSpec with Matchers with BeforeA false ) + val otherParentResourceFullyQualifiedId = FullyQualifiedResourceId(resourceType.name, ResourceId("other_parent_resource")) + val otherParentPolicy = AccessPolicy( + FullyQualifiedPolicyId(otherParentResourceFullyQualifiedId, AccessPolicyName("otherParentPolicyName")), + Set(user.id), + WorkbenchEmail("otherParentPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + + val otherChildPolicy = AccessPolicy( + FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("otherChildPolicyName")), + Set(user.id), + WorkbenchEmail("otherChildPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + + val thirdPolicy = AccessPolicy( + FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("thirdPolicyName")), + Set(user.id), + WorkbenchEmail("thirdPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + val parentResource = Resource(parentResourceFullyQualifiedId.resourceTypeName, parentResourceFullyQualifiedId.resourceId, Set.empty, Set(parentPolicy)) val childResource = @@ -3593,19 +3624,26 @@ class PostgresAccessPolicyDAOSpec extends AnyFreeSpec with Matchers with BeforeA childResourceFullyQualifiedId.resourceTypeName, childResourceFullyQualifiedId.resourceId, Set.empty, - Set(childPolicy) + Set(childPolicy, otherChildPolicy, thirdPolicy) ) + val otherResource = Resource( + otherParentResourceFullyQualifiedId.resourceTypeName, + otherParentResourceFullyQualifiedId.resourceId, + Set.empty, + Set(otherParentPolicy) + ) dao.createResource(parentResource, samRequestContext).unsafeRunSync() dao.createResource(childResource, samRequestContext).unsafeRunSync() + dao.createResource(otherResource, samRequestContext).unsafeRunSync() + + dirDao.addGroupMember(otherParentPolicy.id, otherChildPolicy.id, samRequestContext).unsafeRunSync() // Add child policy to parent policy dirDao.addGroupMember(parentPolicy.id, childPolicy.id, samRequestContext).unsafeRunSync() - val policyGroups = dao.findAffectedPolicyGroups(childResourceFullyQualifiedId, samRequestContext).unsafeRunSync() + val policyGroups = dao.findPolicyGroupsInUse(childResourceFullyQualifiedId, samRequestContext).unsafeRunSync() - policyGroups should not be empty - policyGroups.head._1 shouldEqual parentPolicy.id - policyGroups.head._2 shouldEqual childPolicy.id + policyGroups should contain theSameElementsAs List((parentPolicy.id, childPolicy.id), (otherParentPolicy.id, otherChildPolicy.id)) } } } From 825b7665d28804937ee42ec2d29c8e4349da76fc Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Wed, 30 Oct 2024 14:54:45 -0400 Subject: [PATCH 21/23] remove unnecessary method --- .../dataAccess/PostgresAccessPolicyDAO.scala | 25 ------------------- .../sam/service/ResourceService.scala | 2 +- 2 files changed, 1 insertion(+), 26 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 741973c7e..68d7e508d 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -569,7 +569,6 @@ class PostgresAccessPolicyDAO( override def deleteResource(resource: FullyQualifiedResourceId, leaveTombStone: Boolean, samRequestContext: SamRequestContext): IO[Unit] = serializableWriteTransaction("deleteResource", samRequestContext) { implicit session => -// deletePolicyMembersFromGroups(resource) deleteAllResourcePolicies(resource, samRequestContext) deleteEffectivePolicies(resource, resourceTypePKsByName) removeAuthDomainFromResource(resource, samRequestContext) @@ -996,30 +995,6 @@ from ${GroupMemberTable as groupMemberTable} } - private def deletePolicyMembersFromGroups(resourceId: FullyQualifiedResourceId)(implicit - session: DBSession - ): Unit = { - val gm = GroupMemberTable.syntax("gm") - val p = PolicyTable.syntax("p") - val gmf = GroupMemberFlatTable.syntax("gmf") - - // Remove policy members from group tables to prevent FK Violations - val deleteQuery = - samsql"""delete from ${GroupMemberTable as gm} - using ${PolicyTable as p} - where ${gm.memberGroupId} = ${p.groupId} - and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)}) - """ - deleteQuery.update().apply() - - val deleteFlatQuery = samsql"""delete from ${GroupMemberFlatTable as gmf} - using ${PolicyTable as p} - where ${gmf.memberGroupId} = ${p.groupId} - and ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)}) - """ - deleteFlatQuery.update().apply() - } - private def deleteAllResourcePolicies(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext)(implicit session: DBSession ): Unit = { 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 index 7540e1e1c..2d7686727 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -441,7 +441,7 @@ class ResourceService( leaveTombStone = !resourceTypes(resource.resourceTypeName).reuseIds affectedPolicies <- accessPolicyDAO.findPolicyGroupsInUse(resource, samRequestContext) // New method _ <- affectedPolicies.traverse { case (policyToUpdate, policyToRemove) => removeSubjectFromPolicy(policyToUpdate, policyToRemove, samRequestContext) } - _ <- cloudSyncPolicies(affectedPolicies, samRequestContext) // TODO: use this on affected policies + _ <- cloudSyncPolicies(affectedPolicies, samRequestContext) _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) From e4f2975f0f7d4b1c0c07e4a3a634f69818b8bf8c Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Wed, 30 Oct 2024 14:58:37 -0400 Subject: [PATCH 22/23] don't sync policies --- .../dsde/workbench/sam/service/ResourceService.scala | 1 - 1 file changed, 1 deletion(-) 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 index 2d7686727..9a8041cc2 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -441,7 +441,6 @@ class ResourceService( leaveTombStone = !resourceTypes(resource.resourceTypeName).reuseIds affectedPolicies <- accessPolicyDAO.findPolicyGroupsInUse(resource, samRequestContext) // New method _ <- affectedPolicies.traverse { case (policyToUpdate, policyToRemove) => removeSubjectFromPolicy(policyToUpdate, policyToRemove, samRequestContext) } - _ <- cloudSyncPolicies(affectedPolicies, samRequestContext) _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) From 71b8dd571809e36fc65f1f5f946333584bdec5e5 Mon Sep 17 00:00:00 2001 From: Bria Morgan Date: Wed, 30 Oct 2024 16:21:13 -0400 Subject: [PATCH 23/23] remove unused method --- .../dsde/workbench/sam/service/ResourceService.scala | 12 ------------ 1 file changed, 12 deletions(-) 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 index 9a8041cc2..5ba6767af 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -487,18 +487,6 @@ class ResourceService( } } yield policiesToDelete - def cloudSyncPolicies( - policiesList: List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)], - samRequestContext: SamRequestContext - ): IO[List[FullyQualifiedPolicyId]] = { - val policiesToSync: List[FullyQualifiedPolicyId] = policiesList.map { case (firstPolicy, _) => firstPolicy }.toSet.toList - for { - _ <- policiesToSync.traverse { policy => - cloudExtensions.onGroupUpdate(Seq(policy), Set.empty, samRequestContext) - } - } yield policiesToSync - } - def listUserResourceRoles(resource: FullyQualifiedResourceId, samUser: SamUser, samRequestContext: SamRequestContext): IO[Set[ResourceRoleName]] = accessPolicyDAO.listUserResourceRoles(resource, samUser.id, samRequestContext)