From 38bc991cebaf1e692a5293b98ef3c331bb46c35a Mon Sep 17 00:00:00 2001 From: Marcus Talbott Date: Tue, 26 Nov 2019 16:04:27 -0500 Subject: [PATCH] unroll policy membership on our side to fix bug --- .../sam/directory/LdapDirectoryDAO.scala | 29 +--------------- .../sam/openam/LdapAccessPolicyDAO.scala | 22 +++---------- .../dsde/workbench/sam/util/LdapSupport.scala | 33 ++++++++++++++++++- .../sam/service/ResourceServiceSpec.scala | 32 ++++++++++++++++++ 4 files changed, 70 insertions(+), 46 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/directory/LdapDirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/directory/LdapDirectoryDAO.scala index efbcf92fbf..6df9b09ec9 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/directory/LdapDirectoryDAO.scala +++ b/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] = diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/openam/LdapAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/openam/LdapAccessPolicyDAO.scala index 937905078c..4982fdccb0 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/openam/LdapAccessPolicyDAO.scala +++ b/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] = { diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/LdapSupport.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/LdapSupport.scala index ebbea334c7..d4a7f46228 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/LdapSupport.scala +++ b/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}") 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 e0fd26d470..34b008c43e 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 @@ -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)