Merge 38bc991 into 6a3f760

src/main/scala/org/broadinstitute/dsde/workbench/sam/directory/LdapDirectoryDAO.scala

@@ -202,17 +202,8 @@ class LdapDirectoryDAO(
       unmarshalUser(results).toOption
     }
 
-  private def unmarshalUser(results: Entry): Either[String, WorkbenchUser] =
-    for {
-      uid <- getAttribute(results, Attr.uid).toRight(s"${Attr.uid} attribute missing")
-      email <- getAttribute(results, Attr.email).toRight(s"${Attr.email} attribute missing")
-    } yield WorkbenchUser(WorkbenchUserId(uid), getAttribute(results, Attr.googleSubjectId).map(GoogleSubjectId), WorkbenchEmail(email))
-
-  private def unmarshalUserThrow(results: Entry): WorkbenchUser = unmarshalUser(results).fold(s => throw new WorkbenchException(s), identity)
-
   override def loadUsers(userIds: Set[WorkbenchUserId]): IO[Stream[WorkbenchUser]] = {
-    val filters = userIds.grouped(batchSize).map(batch => Filter.createORFilter(batch.map(g => Filter.createEqualityFilter(Attr.uid, g.value)).asJava)).toSeq
-    executeLdap(IO(ldapSearchStream(peopleOu, SearchScope.ONE, filters: _*)(unmarshalUserThrow)))
+    loadUsersInternal(userIds)
   }
 
   override def deleteUser(userId: WorkbenchUserId): IO[Unit] =
@@ -251,24 +242,6 @@ class LdapDirectoryDAO(
     }
   }
 
-  def listFlattenedMembers(groupId: WorkbenchGroupIdentity, visitedGroupIds: Set[WorkbenchGroupIdentity] = Set.empty): IO[Set[WorkbenchUserId]] = {
-    for {
-      directMembers <- listDirectMembers(groupId)
-      users = directMembers.collect { case subject: WorkbenchUserId => subject }
-      subGroups = directMembers.collect { case subject: WorkbenchGroupIdentity => subject }
-      updatedVisitedGroupIds = visitedGroupIds ++ subGroups
-      nestedUsers <- (subGroups -- visitedGroupIds).toList.traverse(subGroupId => listFlattenedMembers(subGroupId, updatedVisitedGroupIds))
-    } yield {
-      users ++ nestedUsers.flatten
-    }
-  }
-
-  def listDirectMembers(groupId: WorkbenchGroupIdentity): IO[Set[WorkbenchSubject]] = {
-    executeLdap(
-      IO(getAttributes(ldapConnectionPool.getEntry(groupDn(groupId), Attr.uniqueMember), Attr.uniqueMember).map(dnToSubject))
-    )
-  }
-
   override def listAncestorGroups(groupId: WorkbenchGroupIdentity): IO[Set[WorkbenchGroupIdentity]] = listMemberOfGroups(groupId)
 
   override def enableIdentity(subject: WorkbenchSubject): IO[Unit] =

src/main/scala/org/broadinstitute/dsde/workbench/sam/openam/LdapAccessPolicyDAO.scala

@@ -262,23 +262,11 @@ class LdapAccessPolicyDAO(
       userPolicies <- allPolicies.filterA(policy => isGroupMember(policy.id, userId))
     } yield userPolicies.toSet
 
-  override def listFlattenedPolicyMembers(policyId: FullyQualifiedPolicyId): IO[Set[WorkbenchUser]] = executeLdap(
-    // we only care about entries in ou=people and only 1 level down but searching the whole directory is MUCH faster
-    // for some reason (a couple seconds vs hundreds). So search everything and ignore anything that is not workbenchPerson
-    IO(ldapSearchStream(directoryConfig.baseDn, SearchScope.SUB, Filter.createEqualityFilter(Attr.memberOf, policyDn(policyId))) { entry =>
-      if (entry.getObjectClassValues.map(_.toLowerCase).contains(ObjectClass.workbenchPerson.toLowerCase)) {
-        Option(unmarshalUser(entry))
-      } else {
-        None
-      }
-    }.flatten.toSet)
-  )
-
-  private def unmarshalUser(entry: Entry): WorkbenchUser = {
-    val uid = getAttribute(entry, Attr.uid).getOrElse(throw new WorkbenchException(s"${Attr.uid} attribute missing"))
-    val email = getAttribute(entry, Attr.email).getOrElse(throw new WorkbenchException(s"${Attr.email} attribute missing"))
-
-    WorkbenchUser(WorkbenchUserId(uid), getAttribute(entry, Attr.googleSubjectId).map(GoogleSubjectId), WorkbenchEmail(email))
+  override def listFlattenedPolicyMembers(policyId: FullyQualifiedPolicyId): IO[Set[WorkbenchUser]] = {
+    for {
+      memberUserIds <- listFlattenedMembers(policyId)
+      users <- loadUsersInternal(memberUserIds)
+    } yield users.toSet
   }
 
   override def setPolicyIsPublic(policyId: FullyQualifiedPolicyId, public: Boolean): IO[Unit] = {

src/main/scala/org/broadinstitute/dsde/workbench/sam/util/LdapSupport.scala

@@ -4,7 +4,7 @@ import cats.data.OptionT
 import cats.effect.{ContextShift, IO}
 import cats.implicits._
 import com.unboundid.ldap.sdk._
-import org.broadinstitute.dsde.workbench.model.{WorkbenchEmail, WorkbenchException, WorkbenchGroupIdentity, WorkbenchGroupName, WorkbenchSubject}
+import org.broadinstitute.dsde.workbench.model._
 import org.broadinstitute.dsde.workbench.sam.directory.DirectorySubjectNameSupport
 import org.broadinstitute.dsde.workbench.sam.model.BasicWorkbenchGroup
 import org.broadinstitute.dsde.workbench.sam.schema.JndiSchemaDAO.Attr
@@ -136,6 +136,37 @@ trait LdapSupport extends DirectorySubjectNameSupport {
     }
   }
 
+  def listFlattenedMembers(groupId: WorkbenchGroupIdentity, visitedGroupIds: Set[WorkbenchGroupIdentity] = Set.empty): IO[Set[WorkbenchUserId]] = {
+    for {
+      directMembers <- listDirectMembers(groupId)
+      users = directMembers.collect { case subject: WorkbenchUserId => subject }
+      subGroups = directMembers.collect { case subject: WorkbenchGroupIdentity => subject }
+      updatedVisitedGroupIds = visitedGroupIds ++ subGroups
+      nestedUsers <- (subGroups -- visitedGroupIds).toList.traverse(subGroupId => listFlattenedMembers(subGroupId, updatedVisitedGroupIds))
+    } yield {
+      users ++ nestedUsers.flatten
+    }
+  }
+
+  private def listDirectMembers(groupId: WorkbenchGroupIdentity): IO[Set[WorkbenchSubject]] = {
+    executeLdap(
+      IO(getAttributes(ldapConnectionPool.getEntry(groupDn(groupId), Attr.uniqueMember), Attr.uniqueMember).map(dnToSubject))
+    )
+  }
+
+  protected def loadUsersInternal(userIds: Set[WorkbenchUserId]): IO[Stream[WorkbenchUser]] = {
+    val filters = userIds.grouped(batchSize).map(batch => Filter.createORFilter(batch.map(g => Filter.createEqualityFilter(Attr.uid, g.value)).asJava)).toSeq
+    executeLdap(IO(ldapSearchStream(peopleOu, SearchScope.ONE, filters: _*)(unmarshalUserThrow)))
+  }
+
+  protected def unmarshalUser(results: Entry): Either[String, WorkbenchUser] =
+    for {
+      uid <- getAttribute(results, Attr.uid).toRight(s"${Attr.uid} attribute missing")
+      email <- getAttribute(results, Attr.email).toRight(s"${Attr.email} attribute missing")
+    } yield WorkbenchUser(WorkbenchUserId(uid), getAttribute(results, Attr.googleSubjectId).map(GoogleSubjectId), WorkbenchEmail(email))
+
+  private def unmarshalUserThrow(results: Entry): WorkbenchUser = unmarshalUser(results).fold(s => throw new WorkbenchException(s), identity)
+
   private def unmarshalGroup(results: Entry): Either[String, BasicWorkbenchGroup] =
     for {
       cn <- getAttribute(results, Attr.cn).toRight(s"${Attr.cn} attribute missing: ${results.getDN}")

src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala

@@ -835,6 +835,38 @@ class ResourceServiceSpec extends FlatSpec with Matchers with ScalaFutures with
     service.listAllFlattenedResourceUsers(resource).unsafeRunSync() should contain theSameElementsAs Set(dummyUserIdInfo, user1, user2, user3, user4, user5, user6)
   }
 
+  it should "return a flattened list of all of the users in any of a resource's policies even if the users are in a managed group" in {
+    val resource = FullyQualifiedResourceId(defaultResourceType.name, ResourceId(UUID.randomUUID().toString))
+    val managedGroupName = "foo"
+    val resourceOwnerUserInfo = UserInfo(OAuth2BearerToken("token"), WorkbenchUserId("user1"), WorkbenchEmail("user1@company.com"), 0)
+    val managedGroupOwnerUserInfo = UserInfo(OAuth2BearerToken("token"), WorkbenchUserId("user2"), WorkbenchEmail("user2@company.com"), 0)
+
+    dirDAO.createUser(WorkbenchUser(resourceOwnerUserInfo.userId, Some(TestSupport.genGoogleSubjectId()), resourceOwnerUserInfo.userEmail)).unsafeRunSync()
+    dirDAO.createUser(WorkbenchUser(managedGroupOwnerUserInfo.userId, Some(TestSupport.genGoogleSubjectId()), managedGroupOwnerUserInfo.userEmail)).unsafeRunSync()
+
+    val resourceOwner = dirDAO.loadUser(resourceOwnerUserInfo.userId).unsafeRunSync().map { user => UserIdInfo(user.id, user.email, user.googleSubjectId) }.get
+    val managedGroupOwner = dirDAO.loadUser(managedGroupOwnerUserInfo.userId).unsafeRunSync().map { user => UserIdInfo(user.id, user.email, user.googleSubjectId) }.get
+
+    val directPolicyMember = UserIdInfo(WorkbenchUserId("directMember"), WorkbenchEmail("directMember@fake.com"), None)
+    val user3 = UserIdInfo(WorkbenchUserId("user3"), WorkbenchEmail("user3@fake.com"), None)
+    val user4 = UserIdInfo(WorkbenchUserId("user4"), WorkbenchEmail("user4@fake.com"), None)
+
+    dirDAO.createUser(WorkbenchUser(directPolicyMember.userSubjectId, None, directPolicyMember.userEmail)).unsafeRunSync()
+    dirDAO.createUser(WorkbenchUser(user3.userSubjectId, None, user3.userEmail)).unsafeRunSync()
+    dirDAO.createUser(WorkbenchUser(user4.userSubjectId, None, user4.userEmail)).unsafeRunSync()
+
+    service.createResourceType(defaultResourceType).unsafeRunSync()
+    service.createResourceType(managedGroupResourceType).unsafeRunSync()
+    runAndWait(service.createResource(defaultResourceType, resource.resourceId, resourceOwnerUserInfo))
+    runAndWait(managedGroupService.createManagedGroup(ResourceId(managedGroupName), managedGroupOwnerUserInfo))
+    runAndWait(service.createPolicy(FullyQualifiedPolicyId(resource, AccessPolicyName("can-catalog")), Set(directPolicyMember.userSubjectId, WorkbenchGroupName(managedGroupName)), Set(ResourceRoleName("other")), Set.empty))
+
+    runAndWait(managedGroupService.addSubjectToPolicy(ResourceId(managedGroupName), ManagedGroupService.memberPolicyName, user3.userSubjectId))
+    runAndWait(managedGroupService.addSubjectToPolicy(ResourceId(managedGroupName), ManagedGroupService.memberPolicyName, user4.userSubjectId))
+
+    service.listAllFlattenedResourceUsers(resource).unsafeRunSync() should contain theSameElementsAs Set(resourceOwner, managedGroupOwner, directPolicyMember, user3, user4)
+  }
+
   "loadAccessPolicyWithEmails" should "get emails for users, groups and policies" in {
     val testResult = for {
       _ <- service.createResourceType(defaultResourceType)