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

ID-1333 Add group version increment to onPolicyUpdate #1492

Merged
merged 7 commits into from
Jul 16, 2024
Merged

ID-1333 Add group version increment to onPolicyUpdate #1492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e9694ee
PROD-972 Add group version increment to onPolicyUpdate
Ghost-in-a-Jar Jul 15, 2024
e90559b
PROD-972 Add group version increment to onPolicyUpdate
Ghost-in-a-Jar Jul 15, 2024
e7b6492
add logging on group sync skip
Ghost-in-a-Jar Jul 15, 2024
de9bcb1
Add ancestor policy group version update
Ghost-in-a-Jar Jul 15, 2024
df10db6
fix GoogleExtensionSpec
Ghost-in-a-Jar Jul 15, 2024
4715f7f
Add ResourceServiceSpec test case
Ghost-in-a-Jar Jul 15, 2024
f425110
Add test for constrained policy group version update
Ghost-in-a-Jar Jul 15, 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
3 changes: 3 additions & 0 deletions src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,9 @@ class GoogleExtensions(
accessPolicyDAO.listSyncedAccessPolicyIdsOnResourcesConstrainedByGroup(_, relevantMembers, samRequestContext)
)

// Update group versions for all the groups that are ancestors of the managed group so that they can be synced
_ <- constrainedResourceAccessPolicyIds.flatten.traverse(p => directoryDAO.updateGroupUpdatedDateAndVersionWithSession(p, samRequestContext))

// return messages for all the affected access policies and the original group we started with
} yield constrainedResourceAccessPolicyIds.flatten.map(accessPolicyId => accessPolicyId.toJson.compactPrint)

Expand Down
4 changes: 3 additions & 1 deletion src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ class GoogleGroupSynchronizer(
IO.pure(Map.empty)
} else {
loadSamGroupForSynchronization(groupId, samRequestContext).flatMap {
case Left(group) => IO.pure(Map(group.email -> Seq.empty))
case Left(group) =>
logger.info(s"Group ${group.id}:${group.email} does not need synchronization, skipping.")
IO.pure(Map(group.email -> Seq.empty))
case Right(group) =>
for {
members <- calculateAuthDomainIntersectionIfRequired(group, samRequestContext)
Expand Down
8 changes: 4 additions & 4 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 @@ -717,6 +717,10 @@ class ResourceService(
changeEvents = createAccessChangeEvents(policyId.resource, originalPolicies, updatedPolicies)
_ <- AuditLogger.logAuditEventIO(samRequestContext, changeEvents.toSeq: _*)

_ <- directoryDAO.updateGroupUpdatedDateAndVersionWithSession(
FullyQualifiedPolicyId(policyId.resource, policyId.accessPolicyName),
samRequestContext
)
_ <- cloudExtensions.onGroupUpdate(Seq(policyId), removedMembers ++ addedMembers, samRequestContext).attempt.flatMap {
case Left(regrets) => IO(logger.error(s"error calling cloudExtensions.onGroupUpdate for $policyId", regrets))
case Right(_) => IO.unit
Expand Down Expand Up @@ -885,10 +889,6 @@ class ResourceService(
for {
originalPolicies <- accessPolicyDAO.listAccessPolicies(policyId.resource, samRequestContext)
policyChanged <- accessPolicyDAO.setPolicyIsPublic(policyId, public, samRequestContext)
_ <- directoryDAO.updateGroupUpdatedDateAndVersionWithSession(
FullyQualifiedPolicyId(policyId.resource, policyId.accessPolicyName),
samRequestContext
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the next thing that happens is onPolicyUpdateIfChanged which calls onPolicyUpdate if the policy actually changed.

_ <- onPolicyUpdateIfChanged(policyId, originalPolicies, samRequestContext)(policyChanged)
} yield ()
}
Expand Down
20 changes: 20 additions & 0 deletions src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,6 +1080,12 @@ class GoogleExtensionSpec(_system: ActorSystem)
when(mockDirectoryDAO.getSynchronizedDate(any[FullyQualifiedPolicyId], any[SamRequestContext]))
.thenReturn(IO.pure(Some(new GregorianCalendar(2018, 8, 26).getTime())))
when(mockGoogleGroupSyncPubSubDAO.publishMessages(any[String], any[Seq[MessageRequest]])).thenReturn(Future.successful(()))
when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

// mock responses for onManagedGroupUpdate
when(mockAccessPolicyDAO.listSyncedAccessPolicyIdsOnResourcesConstrainedByGroup(WorkbenchGroupName(managedGroupId), Set.empty, samRequestContext))
Expand All @@ -1088,6 +1094,8 @@ class GoogleExtensionSpec(_system: ActorSystem)
runAndWait(googleExtensions.onGroupUpdate(Seq(managedGroupRPN), Set.empty, samRequestContext))

verify(mockGoogleGroupSyncPubSubDAO, times(1)).publishMessages(any[String], any[Seq[MessageRequest]])

verify(mockDirectoryDAO, times(2)).updateGroupUpdatedDateAndVersionWithSession(any[WorkbenchGroupIdentity], any[SamRequestContext])
}

it should "trigger updates to constrained policies when updating a group that is a part of a managed group" in {
Expand Down Expand Up @@ -1130,6 +1138,12 @@ class GoogleExtensionSpec(_system: ActorSystem)
when(mockDirectoryDAO.getSynchronizedDate(any[FullyQualifiedPolicyId], any[SamRequestContext]))
.thenReturn(IO.pure(Some(new GregorianCalendar(2018, 8, 26).getTime())))
when(mockGoogleGroupSyncPubSubDAO.publishMessages(any[String], any[Seq[MessageRequest]])).thenReturn(Future.successful(()))
when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

// mock ancestor call to establish subgroup relationship to managed group
when(mockDirectoryDAO.listAncestorGroups(WorkbenchGroupName(subGroupId), samRequestContext))
Expand Down Expand Up @@ -1184,6 +1198,12 @@ class GoogleExtensionSpec(_system: ActorSystem)
when(mockDirectoryDAO.getSynchronizedDate(any[FullyQualifiedPolicyId], any[SamRequestContext]))
.thenReturn(IO.pure(Some(new GregorianCalendar(2018, 8, 26).getTime())))
when(mockGoogleGroupSyncPubSubDAO.publishMessages(any[String], any[Seq[MessageRequest]])).thenReturn(Future.successful(()))
when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

// mock ancestor call to establish nested group structure for owner policy and subgroup in managed group
when(mockDirectoryDAO.listAncestorGroups(WorkbenchGroupName(subGroupId), samRequestContext))
Expand Down
47 changes: 42 additions & 5 deletions 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 @@ -286,6 +286,8 @@ class ResourceServiceSpec
service.setPublic(policyToUpdate, false, samRequestContext).unsafeRunSync()
service.isPublic(policyToUpdate, samRequestContext).unsafeRunSync() should equal(false)

service.loadPolicy(policyToUpdate, samRequestContext).unsafeRunSync().get.version shouldEqual 3

// cleanup
runAndWait(service.deleteResource(resource, samRequestContext))
}
Expand Down Expand Up @@ -1164,7 +1166,7 @@ class ResourceServiceSpec
val policies =
policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("[email protected]")))

assert(policies.contains(newPolicy))
assert(policies.contains(newPolicy.copy(version = 2)))
}

it should "should add a memberPolicy as a member when specified through policy identifiers" in {
Expand Down Expand Up @@ -1285,6 +1287,13 @@ class ResourceServiceSpec
)
).thenReturn(IO.unit)

when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

runAndWait(
resourceService.overwritePolicy(
defaultResourceType,
Expand Down Expand Up @@ -1325,6 +1334,12 @@ class ResourceServiceSpec
// function calls that should pass but what they return does not matter
when(mockAccessPolicyDAO.overwritePolicy(ArgumentMatchers.eq(accessPolicy), any[SamRequestContext])).thenReturn(IO.pure(accessPolicy))
when(mockCloudExtensions.onGroupUpdate(ArgumentMatchers.eq(Seq(policyId)), ArgumentMatchers.eq(Set(member)), any[SamRequestContext])).thenReturn(IO.unit)
when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

// overwrite policy with no members
runAndWait(
Expand Down Expand Up @@ -1412,7 +1427,7 @@ class ResourceServiceSpec
val policies =
policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("[email protected]")))

assert(policies.contains(newPolicy))
assert(policies.contains(newPolicy.copy(version = 2)))
}

it should "fail if any members are not test.firecloud.org accounts" in {
Expand Down Expand Up @@ -1486,7 +1501,7 @@ class ResourceServiceSpec
val policies =
policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("[email protected]")))

assert(policies.contains(newPolicy))
assert(policies.contains(newPolicy.copy(version = 2)))
}

it should "call CloudExtensions.onGroupUpdate when members change" in {
Expand Down Expand Up @@ -1516,6 +1531,12 @@ class ResourceServiceSpec
// function calls that should pass but what they return does not matter
when(mockAccessPolicyDAO.overwritePolicyMembers(ArgumentMatchers.eq(policyId), ArgumentMatchers.eq(Set.empty), any[SamRequestContext])).thenReturn(IO.unit)
when(mockCloudExtensions.onGroupUpdate(ArgumentMatchers.eq(Seq(policyId)), ArgumentMatchers.eq(Set(member)), any[SamRequestContext])).thenReturn(IO.unit)
when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

// overwrite policy members with empty set
runAndWait(resourceService.overwritePolicyMembers(policyId, Set.empty, samRequestContext))
Expand Down Expand Up @@ -1576,7 +1597,7 @@ class ResourceServiceSpec

val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync()

assert(policies.contains(newPolicy))
assert(policies.contains(newPolicy.copy(version = 2)))
}

it should "fail when given an invalid action" in {
Expand Down Expand Up @@ -2107,6 +2128,13 @@ class ResourceServiceSpec
IO.pure(LazyList(AccessPolicy(policyId, Set.empty, WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false))),
IO.pure(LazyList(AccessPolicy(policyId, Set(member), WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false)))
)
when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

runAndWait(resourceService.addSubjectToPolicy(policyId, member, samRequestContext))

verify(mockCloudExtensions, Mockito.timeout(500))
Expand Down Expand Up @@ -2164,6 +2192,13 @@ class ResourceServiceSpec
IO.pure(LazyList(AccessPolicy(policyId, Set.empty, WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false))),
IO.pure(LazyList(AccessPolicy(policyId, Set(member), WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false)))
)
when(
mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession(
any[WorkbenchGroupIdentity],
any[SamRequestContext]
)
).thenReturn(IO.unit)

runAndWait(resourceService.removeSubjectFromPolicy(policyId, member, samRequestContext))

verify(mockCloudExtensions, Mockito.timeout(1000))
Expand Down Expand Up @@ -3122,7 +3157,9 @@ class ResourceServiceSpec

returnedPolicies should contain theSameElementsAs Set(expectedPolicy)

policyDAO.loadPolicy(testPolicyId, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail(""))) shouldBe Some(expectedPolicy)
policyDAO.loadPolicy(testPolicyId, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail(""))) shouldBe Some(
expectedPolicy.copy(version = 2)
)
}

it should "validate admin policies" in {
Expand Down
Loading