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 b1aa946d7..707d63cff 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 @@ -553,14 +553,13 @@ class ResourceService( _ <- onPolicyUpdate(policyIdentity, originalPolicies, samRequestContext) } yield result case Some(existingAccessPolicy) => - val newAccessPolicy = AccessPolicy( - policyIdentity, - workbenchSubjects, - existingAccessPolicy.email, - policy.roles, - policy.actions, - policy.descendantPermissions, - existingAccessPolicy.public + // this function updates only the members, roles, actions, and descendantPermissions of the policy + // so the new policy is a copy of the existing policy with the updated fields + val newAccessPolicy = existingAccessPolicy.copy( + members = workbenchSubjects, + roles = policy.roles, + actions = policy.actions, + descendantPermissions = policy.descendantPermissions ) if (newAccessPolicy == existingAccessPolicy) { // short cut if access policy is unchanged 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 2e97201a1..f7536dd7b 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 @@ -54,6 +54,7 @@ import java.util.UUID import scala.concurrent.ExecutionContext.Implicits.global import scala.concurrent.duration._ import scala.jdk.CollectionConverters._ +import scala.util.Random /** Created by dvoet on 6/27/17. */ @@ -1474,7 +1475,7 @@ class ResourceServiceSpec .onGroupUpdate(ArgumentMatchers.eq(Seq(policyId)), ArgumentMatchers.eq(Set(member)), any[SamRequestContext]) } - it should "not call CloudExtensions.onGroupUpdate when members don't change" in { + it should "not do anything when policy is unchanged" in { val mockCloudExtensions: CloudExtensions = mock[CloudExtensions](RETURNS_SMART_NULLS) val mockDirectoryDAO: DirectoryDAO = mock[DirectoryDAO](RETURNS_SMART_NULLS) val mockAccessPolicyDAO = mock[AccessPolicyDAO](RETURNS_SMART_NULLS) @@ -1489,13 +1490,12 @@ class ResourceServiceSpec ) val policyId = FullyQualifiedPolicyId(FullyQualifiedResourceId(defaultResourceType.name, ResourceId("testR")), AccessPolicyName("testA")) - val accessPolicy = AccessPolicy(policyId, Set.empty, WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false) + val policyVersion = Random.between(10, 100) // random version to ensure it is neither the default nor fixed + val accessPolicy = AccessPolicy(policyId, Set.empty, WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false, version = policyVersion) - // setup existing policy with no members when(mockAccessPolicyDAO.listAccessPolicies(ArgumentMatchers.eq(policyId.resource), any[SamRequestContext])).thenReturn(IO.pure(LazyList(accessPolicy))) - // overwrite policy with no members - runAndWait( + val updatedPolicy = runAndWait( resourceService.overwritePolicy( defaultResourceType, policyId.accessPolicyName, @@ -1505,7 +1505,9 @@ class ResourceServiceSpec ) ) - verify(mockCloudExtensions, Mockito.after(500).never).onGroupUpdate(ArgumentMatchers.eq(Seq(policyId)), any[Set[WorkbenchSubject]], any[SamRequestContext]) + // no changes to policy, so no calls to overwritePolicy and version should not change + updatedPolicy.version shouldEqual policyVersion + verify(mockAccessPolicyDAO, Mockito.never).overwritePolicy(any[AccessPolicy], any[SamRequestContext]) } "overwriteAdminPolicy" should "succeed with a valid request" in {