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..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,7 +130,11 @@ trait AccessPolicyDAO { samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] - def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] + def findPolicyGroupsInUse( + 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 23aa83d48..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 @@ -951,6 +951,50 @@ class PostgresAccessPolicyDAO( } } + // Return value: [(policyToUpdate, policyToRemove)] + override def findPolicyGroupsInUse( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = + readOnlyTransaction("findAffectedPolicyGroups", samRequestContext) { implicit session => + 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 ${policyTable.groupId} as childGroupId, ${policyTable.name} as policyName + FROM ${PolicyTable as policyTable} + WHERE ${policyTable.resourceId} = (${loadResourcePKSubQuery(resourceId)}) + ) + 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 parentPolicyResourceType = rs.get[ResourceTypeName]("parentPolicyResourceType") + val parentPolicyResourceId = rs.get[ResourceId]("parentPolicyResourceName") + val parentPolciyAccessName = rs.get[AccessPolicyName]("parentPolicyName") + val memberPolicyAccessName = rs.get[AccessPolicyName]("childPolicyName") + + val parentPolicyFullResourceId = + FullyQualifiedResourceId(parentPolicyResourceType, parentPolicyResourceId) + val parentPolicyFullId = FullyQualifiedPolicyId(parentPolicyFullResourceId, parentPolciyAccessName) + val memberPolicyFullId = FullyQualifiedPolicyId(resourceId, memberPolicyAccessName) + + (parentPolicyFullId, memberPolicyFullId) + } + .list() + .apply() + + } + private def deleteAllResourcePolicies(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext)(implicit session: DBSession ): Unit = { @@ -975,37 +1019,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/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala index c7adfe304..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 @@ -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) @@ -440,22 +439,13 @@ class ResourceService( // leave a tomb stone if the resource type does not allow reuse leaveTombStone = !resourceTypes(resource.resourceTypeName).reuseIds + affectedPolicies <- accessPolicyDAO.findPolicyGroupsInUse(resource, samRequestContext) // New method + _ <- affectedPolicies.traverse { case (policyToUpdate, policyToRemove) => removeSubjectFromPolicy(policyToUpdate, policyToRemove, samRequestContext) } _ <- 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) - 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..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,8 +123,10 @@ 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 findPolicyGroupsInUse( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = IO.pure(List.empty) override def listAccessPolicies( resourceTypeName: ResourceTypeName, 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..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 @@ -3553,6 +3553,99 @@ 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 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 = + Resource( + childResourceFullyQualifiedId.resourceTypeName, + childResourceFullyQualifiedId.resourceId, + Set.empty, + 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.findPolicyGroupsInUse(childResourceFullyQualifiedId, samRequestContext).unsafeRunSync() + + policyGroups should contain theSameElementsAs List((parentPolicy.id, childPolicy.id), (otherParentPolicy.id, otherChildPolicy.id)) + } + } } private def uuid: String = 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..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 @@ -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( + defaultResourceType.name, + 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(resourceType.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())