fix and test requestAccess

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

@@ -26,6 +26,6 @@ trait AccessPolicyDAO {
   def listAccessPolicies(resourceTypeName: ResourceTypeName, userId: WorkbenchUserId): IO[Set[ResourceIdAndPolicyName]]
   def listAccessPolicies(resource: FullyQualifiedResourceId): IO[Set[AccessPolicy]]
   def listAccessPoliciesForUser(resource: FullyQualifiedResourceId, user: WorkbenchUserId): IO[Set[AccessPolicy]]
-  def listFlattenedPolicyMembers(policyId: FullyQualifiedPolicyId): IO[Set[WorkbenchUserId]]
+  def listFlattenedPolicyMembers(policyId: FullyQualifiedPolicyId): IO[Set[WorkbenchUser]]
   def setPolicyIsPublic(policyId: FullyQualifiedPolicyId, isPublic: Boolean): IO[Unit]
 }

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

@@ -210,8 +210,8 @@ class LdapAccessPolicyDAO(protected val ldapConnectionPool: LDAPConnectionPool,
     }
   } yield accessPolicies.getOrElse(Set.empty)
 
-  override def listFlattenedPolicyMembers(policyId: FullyQualifiedPolicyId): IO[Set[WorkbenchUserId]] = cs.evalOn(ecForLdapBlockingIO)(
-    IO(ldapSearchStream(peopleOu, SearchScope.ONE, Filter.createEqualityFilter(Attr.memberOf, policyDn(policyId)))(unmarshalUser).map(_.id).toSet)
+  override def listFlattenedPolicyMembers(policyId: FullyQualifiedPolicyId): IO[Set[WorkbenchUser]] = cs.evalOn(ecForLdapBlockingIO)(
+    IO(ldapSearchStream(peopleOu, SearchScope.ONE, Filter.createEqualityFilter(Attr.memberOf, policyDn(policyId)))(unmarshalUser).toSet)
   )
 
   private def unmarshalUser(entry: Entry): WorkbenchUser = {

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupService.scala

@@ -136,18 +136,37 @@ class ManagedGroupService(private val resourceService: ResourceService, private
   }
 
   def requestAccess(resourceId: ResourceId, requesterUserId: WorkbenchUserId): Future[Unit] = {
+    def extractGoogleSubjectId(requesterUser: Option[WorkbenchUser]) = {
+      (for { u <- requesterUser; s <- u.googleSubjectId } yield s) match {
+        case Some(subjectId) => IO.pure(WorkbenchUserId(subjectId.value))
+        // don't know how a user would get this far without getting a subject id
+        case None => IO.raiseError(new WorkbenchException(s"unable to find subject id for $requesterUserId"))
+      }
+    }
+
     getAccessInstructions(resourceId).flatMap {
       case Some(accessInstructions) =>
         Future.failed(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, s"Please follow special access instructions: $accessInstructions")))
       case None =>
-        val resourceAndPolicyName = FullyQualifiedPolicyId(
+        // Thurloe is the thing that sends the emails and it knows only about google subject ids, not internal sam user ids
+        // so we have to do some conversion here which makes the code look less straight forward
+        val resourceAndAdminPolicyName = FullyQualifiedPolicyId(
           FullyQualifiedResourceId(ManagedGroupService.managedGroupTypeName, resourceId), ManagedGroupService.adminPolicyName)
-        accessPolicyDAO.listFlattenedPolicyMembers(resourceAndPolicyName).unsafeToFuture().map { users =>
-          val notifications = users.map { recipientUserId =>
-            Notifications.GroupAccessRequestNotification(recipientUserId, WorkbenchGroupName(resourceId.value).value, users, requesterUserId)
+
+        val notificationIO = for {
+          requesterUser <- directoryDAO.loadUser(requesterUserId)
+          requesterSubjectId <- extractGoogleSubjectId(requesterUser)
+          admins <- accessPolicyDAO.listFlattenedPolicyMembers(resourceAndAdminPolicyName)
+          // ignore any admin that does not have a google subject id (they have not registered yet anyway)
+          adminUserIds = admins.flatMap { admin => admin.googleSubjectId.map(id => WorkbenchUserId(id.value)) }
+        } yield {
+          val notifications = adminUserIds.map { recipientUserId =>
+            Notifications.GroupAccessRequestNotification(recipientUserId, WorkbenchGroupName(resourceId.value).value, adminUserIds, requesterSubjectId)
           }
           cloudExtensions.fireAndForgetNotifications(notifications)
         }
+
+        notificationIO.unsafeToFuture()
     }
   }
 

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala

@@ -399,7 +399,7 @@ class ResourceService(private val resourceTypes: Map[ResourceTypeName, ResourceT
     for {
       accessPolicies <- accessPolicyDAO.listAccessPolicies(resourceId)
       members <- accessPolicies.toList.parTraverse(accessPolicy => accessPolicyDAO.listFlattenedPolicyMembers(accessPolicy.id))
-      workbenchUsers <- IO.fromFuture(IO(directoryDAO.loadUsers(members.toSet.flatten)))
+      workbenchUsers = members.flatten.toSet
     } yield {
       workbenchUsers.map(user => UserIdInfo(user.id, user.email, user.googleSubjectId)).toSet
     }

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV1Spec.scala

@@ -1174,7 +1174,7 @@ class ResourceRoutesV1Spec extends FlatSpec with Matchers with ScalatestRouteTes
 
     Get(s"/api/resources/v1/${resourceType.name}/${resourceId.value}/allUsers") ~> samRoutes.route ~> check {
       status shouldEqual StatusCodes.OK
-      responseAs[Set[UserIdInfo]] shouldEqual Set(userIdInfo)
+      responseAs[Set[UserIdInfo]].map(_.userSubjectId) shouldEqual Set(userIdInfo.userSubjectId)
     }
   }
 
@@ -1239,7 +1239,7 @@ class ResourceRoutesV1Spec extends FlatSpec with Matchers with ScalatestRouteTes
 
     Get(s"/api/resources/v1/${resourceType.name}/${resourceId.value}/allUsers") ~> samRoutes.route ~> check {
       status shouldEqual StatusCodes.OK
-      responseAs[Set[UserIdInfo]] shouldEqual Set(userIdInfo)
+      responseAs[Set[UserIdInfo]].map(_.userSubjectId) shouldEqual Set(userIdInfo.userSubjectId)
     }
 
     val otherUserSamRoutes = TestSamRoutes(Map(resourceType.name -> resourceType), UserInfo(OAuth2BearerToken("accessToken"), WorkbenchUserId("user2"), WorkbenchEmail("user2@example.com"), 0))

src/test/scala/org/broadinstitute/dsde/workbench/sam/openam/MockAccessPolicyDAO.scala

@@ -111,12 +111,12 @@ class MockAccessPolicyDAO(private val policies: mutable.Map[WorkbenchGroupIdenti
 
   // current implementation returns only the WorkbenchUserIds in this policy. it does not fully mock the behavior of LdapAccessPolicyDAO.
   // this function previously just returned an empty set and this is sufficient for the test I'm using it in (as of 10/25/18), so good enough for now
-  override def listFlattenedPolicyMembers(policyIdentity: FullyQualifiedPolicyId): IO[Set[WorkbenchUserId]] = IO {
+  override def listFlattenedPolicyMembers(policyIdentity: FullyQualifiedPolicyId): IO[Set[WorkbenchUser]] = IO {
     val members = policies.collect {
       case (`policyIdentity`, policy: AccessPolicy) => policy.members
     }
     members.flatten.collect {
-      case u: WorkbenchUserId => u
+      case u: WorkbenchUserId => WorkbenchUser(u, Some(GoogleSubjectId(u.value)), WorkbenchEmail("dummy"))
     }.toSet
   }
 

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

@@ -3,6 +3,7 @@ package org.broadinstitute.dsde.workbench.sam.service
 import java.net.URI
 import java.util.UUID
 
+import akka.http.scaladsl.model.StatusCodes
 import akka.http.scaladsl.model.headers.OAuth2BearerToken
 import com.unboundid.ldap.sdk.{LDAPConnection, LDAPConnectionPool}
 import org.broadinstitute.dsde.workbench.model._
@@ -12,6 +13,7 @@ import org.broadinstitute.dsde.workbench.sam.google.GoogleExtensions
 import org.broadinstitute.dsde.workbench.sam.model._
 import org.broadinstitute.dsde.workbench.sam.openam.{AccessPolicyDAO, LdapAccessPolicyDAO}
 import org.broadinstitute.dsde.workbench.sam.schema.JndiSchemaDAO
+import org.mockito.ArgumentMatchers
 import org.mockito.Mockito.{verify, when}
 import org.scalatest._
 import org.scalatest.concurrent.ScalaFutures
@@ -408,4 +410,36 @@ class ManagedGroupServiceSpec extends FlatSpec with Matchers with TestSupport wi
     managedGroupService.setAccessInstructions(managedGroup.resourceId, newInstructions).unsafeRunSync()
     runAndWait(managedGroupService.getAccessInstructions(managedGroup.resourceId)).getOrElse(None) shouldEqual newInstructions
   }
+
+  "ManagedGroupService requestAccess" should "send notifications" in {
+    val mockCloudExtension = mock[CloudExtensions]
+    when(mockCloudExtension.publishGroup(ArgumentMatchers.any[WorkbenchGroupName])).thenReturn(Future.successful(()))
+    val testManagedGroupService = new ManagedGroupService(resourceService, policyEvaluatorService, resourceTypeMap, policyDAO, dirDAO, mockCloudExtension, testDomain)
+
+    assertMakeGroup(groupId = resourceId.value, managedGroupService = testManagedGroupService)
+
+    val requester = dirDAO.createUser(WorkbenchUser(WorkbenchUserId("userId1"), Some(GoogleSubjectId("this subject id is different that the user id")), WorkbenchEmail("user1@company.com"))).futureValue
+    val adminGoogleSubjectId = WorkbenchUserId(dirDAO.loadUser(dummyUserInfo.userId).unsafeRunSync().flatMap(_.googleSubjectId).getOrElse(fail("could not find admin google subject id")).value)
+
+    val expectedNotificationMessages = Set(
+      Notifications.GroupAccessRequestNotification(
+        adminGoogleSubjectId,
+        WorkbenchGroupName(resourceId.value).value,
+        Set(adminGoogleSubjectId),
+        requester.googleSubjectId.map(id => WorkbenchUserId(id.value)).getOrElse(fail("no requester google subject id"))
+      ))
+
+    testManagedGroupService.requestAccess(resourceId, requester.id).futureValue
+
+    verify(mockCloudExtension).fireAndForgetNotifications(expectedNotificationMessages)
+  }
+
+  it should "throw an error if access instructions exist" in {
+    assertMakeGroup(groupId = resourceId.value)
+    managedGroupService.setAccessInstructions(resourceId, "instructions").unsafeRunSync()
+    val error = intercept[WorkbenchExceptionWithErrorReport] {
+      runAndWait(managedGroupService.requestAccess(resourceId, dummyUserInfo.userId))
+    }
+    error.errorReport.statusCode should be(Some(StatusCodes.BadRequest))
+  }
 }