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.

Sign up for GitHub

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

[CORE-58] Cascade policy deletion #1564

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
3616bde
try new api
calypsomatic Oct 15, 2024
abdf118
wrong tabbing?
calypsomatic Oct 15, 2024
511f6cf
fix api
calypsomatic Oct 15, 2024
5c2c881
update delete query
calypsomatic Oct 15, 2024
4a80260
fix query
calypsomatic Oct 15, 2024
08abd65
one more try
calypsomatic Oct 15, 2024
ef2be68
split into two
calypsomatic Oct 15, 2024
fc94de3
single transaction
calypsomatic Oct 15, 2024
e50536f
Merge branch 'develop' into core-58-cascade-policy
calypsomatic Oct 17, 2024
4455509
test out sync and delete policies
calypsomatic Oct 17, 2024
841c2c8
update order of operations
calypsomatic Oct 17, 2024
2ad6f23
remove deletecascade
calypsomatic Oct 17, 2024
a9a3083
whoops
calypsomatic Oct 17, 2024
5f1ac33
cleanup and add test
calypsomatic Oct 21, 2024
a6bcf06
removed commented out code and fix test
calypsomatic Oct 21, 2024
8ba9c4e
Merge branch 'develop' into core-58-cascade-policy
calypsomatic Oct 21, 2024
8c0ad7d
first pass updated delete resource
calypsomatic Oct 24, 2024
f78fb5a
scalafmt
calypsomatic Oct 24, 2024
8b4fa1b
remove commented out code
calypsomatic Oct 24, 2024
bf02fc0
fix affected policy groups and add test
calypsomatic Oct 29, 2024
34d18e8
sonar findings
calypsomatic Oct 29, 2024
7428caf
expand test
calypsomatic Oct 30, 2024
825b766
remove unnecessary method
calypsomatic Oct 30, 2024
e4f2975
don't sync policies
calypsomatic Oct 30, 2024
71b8dd5
remove unused method
calypsomatic Oct 30, 2024
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
1 change: 0 additions & 1 deletion src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
53 changes: 22 additions & 31 deletions ...main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,28 @@ 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}")
calypsomatic marked this conversation as resolved.
Show resolved Hide resolved
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}")
calypsomatic marked this conversation as resolved.
Show resolved Hide resolved
deleteFlatQuery.update().apply()
calypsomatic marked this conversation as resolved.
Show resolved Hide resolved

deleteAllResourcePolicies(resource, samRequestContext)
deleteEffectivePolicies(resource, resourceTypePKsByName)
removeAuthDomainFromResource(resource, samRequestContext)
Expand Down Expand Up @@ -975,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)

Expand Down
21 changes: 9 additions & 12 deletions src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -441,21 +440,11 @@ class ResourceService(
// leave a tomb stone if the resource type does not allow reuse
leaveTombStone = !resourceTypes(resource.resourceTypeName).reuseIds
_ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext)
_ <- cloudSyncPolicies(resource, samRequestContext)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@calypsomatic calypsomatic Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Is that correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

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

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


_ <- 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 Expand Up @@ -497,6 +486,14 @@ class ResourceService(
}
} yield policiesToDelete

def cloudSyncPolicies(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[LazyList[AccessPolicy]] =
for {
policiesToSync <- accessPolicyDAO.listAccessPolicies(resource, samRequestContext)
_ <- policiesToSync.traverse { policy =>
cloudExtensions.onGroupUpdate(Seq(policy.id), Set.empty, samRequestContext)
}
} yield policiesToSync

def listUserResourceRoles(resource: FullyQualifiedResourceId, samUser: SamUser, samRequestContext: SamRequestContext): IO[Set[ResourceRoleName]] =
accessPolicyDAO.listUserResourceRoles(resource, samUser.id, samRequestContext)

Expand Down
3 changes: 0 additions & 3 deletions src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
60 changes: 59 additions & 1 deletion src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala
View file
Open in desktop
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