Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CORE-58] Cascade policy deletion #1564

Merged
merged 25 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)]]

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

sealed abstract class LoadResourceAuthDomainResult
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -432,30 +432,20 @@ 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)
_ <- deleteActionManagedIdentitiesForResource(resource, samRequestContext)

// 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 =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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("[email protected]"),
resourceType.roles.map(_.roleName),
Set(readAction, writeAction),
Set.empty,
false
)
val childPolicy = AccessPolicy(
FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("childPolicyName")),
Set(user.id),
WorkbenchEmail("[email protected]"),
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("[email protected]"),
resourceType.roles.map(_.roleName),
Set(readAction, writeAction),
Set.empty,
false
)

val otherChildPolicy = AccessPolicy(
FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("otherChildPolicyName")),
Set(user.id),
WorkbenchEmail("[email protected]"),
resourceType.roles.map(_.roleName),
Set(readAction, writeAction),
Set.empty,
false
)

val thirdPolicy = AccessPolicy(
FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("thirdPolicyName")),
Set(user.id),
WorkbenchEmail("[email protected]"),
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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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())
Expand Down
Loading