diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index b8a51fcf9..3bc4232b5 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -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) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala index 31db7a17f..1b95e7964 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala @@ -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) 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 f5162c787..bcd2f46c0 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 @@ -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 @@ -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 - ) _ <- onPolicyUpdateIfChanged(policyId, originalPolicies, samRequestContext)(policyChanged) } yield () } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala index 9b53ae49f..3b8b9f13b 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala @@ -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)) @@ -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 { @@ -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)) @@ -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)) 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 a70f489fc..30f2ba53a 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 @@ -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)) } @@ -1164,7 +1166,7 @@ class ResourceServiceSpec val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("policy-randomuuid@example.com"))) - 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 { @@ -1285,6 +1287,13 @@ class ResourceServiceSpec ) ).thenReturn(IO.unit) + when( + mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession( + any[WorkbenchGroupIdentity], + any[SamRequestContext] + ) + ).thenReturn(IO.unit) + runAndWait( resourceService.overwritePolicy( defaultResourceType, @@ -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( @@ -1412,7 +1427,7 @@ class ResourceServiceSpec val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("policy-randomuuid@example.com"))) - assert(policies.contains(newPolicy)) + assert(policies.contains(newPolicy.copy(version = 2))) } it should "fail if any members are not test.firecloud.org accounts" in { @@ -1486,7 +1501,7 @@ class ResourceServiceSpec val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("policy-randomuuid@example.com"))) - assert(policies.contains(newPolicy)) + assert(policies.contains(newPolicy.copy(version = 2))) } it should "call CloudExtensions.onGroupUpdate when members change" in { @@ -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)) @@ -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 { @@ -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)) @@ -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)) @@ -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 {