diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index d58d335b8..bd74d445f 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -70,7 +70,7 @@ paths: get: tags: - Admin - summary: Retrieves a user record, by user id + summary: Retrieves a user record, by user id. Limited to 1000 user IDs. operationId: adminGetUser parameters: - name: userId @@ -168,6 +168,45 @@ paths: application/json: schema: $ref: '#/components/schemas/ErrorReport' + post: + tags: + - Admin + summary: Gets a list of users for a list of Sam User IDs + operationId: adminGetUsersByIDs + requestBody: + content: + application/json: + schema: + type: array + items: + type: string + responses: + 200: + description: list of matching users + content: + application/json: + schema: + type: array + items: + $ref: '#/components/schemas/User' + 400: + description: Request invalid. Request body must be a list of less than 1000 Sam User IDs. + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' + 403: + description: You do not have service admin privileges + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' + 500: + description: Internal Server Error + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' /api/admin/v1/user/{userId}/disable: put: tags: diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutes.scala index 950d4be79..b82c6ad7d 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutes.scala @@ -1,6 +1,8 @@ -package org.broadinstitute.dsde.workbench.sam.api +package org.broadinstitute.dsde.workbench.sam +package api import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ +import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.model.StatusCodes.{NotFound, OK} import akka.http.scaladsl.server import akka.http.scaladsl.server.Directives._ @@ -8,6 +10,8 @@ import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._ import org.broadinstitute.dsde.workbench.sam.service.ResourceService import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext +import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport._ +import org.broadinstitute.dsde.workbench.sam.model.api.SamUser import spray.json.DefaultJsonProtocol._ trait ServiceAdminRoutes extends SecurityDirectives with SamRequestContextDirectives with SamUserDirectives with SamModelDirectives { @@ -53,7 +57,19 @@ trait ServiceAdminRoutes extends SecurityDirectives with SamRequestContextDirect .map(users => (if (users.nonEmpty) OK else NotFound) -> users) } } + } ~ + postWithTelemetry(samRequestContext) { + entity(as[Seq[WorkbenchUserId]]) { + case Seq() => complete(OK -> Seq.empty[SamUser]) + case userIds: Seq[WorkbenchUserId] if userIds.length > 1000 => + throw new WorkbenchExceptionWithErrorReport( + ErrorReport(StatusCodes.BadRequest, "Batch request too large. Batch request too large, must be less than 1000") + ) + case userIds: Seq[WorkbenchUserId] => + complete { + userService.getUsersByIds(userIds, samRequestContext) + } + } } - } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala index 1be569418..3d03f77da 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala @@ -63,6 +63,11 @@ trait DirectoryDAO { def loadUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUser]] + def batchLoadUsers( + samUserIds: Set[WorkbenchUserId], + samRequestContext: SamRequestContext + ): IO[Seq[SamUser]] + def loadUsersByQuery( userId: Option[WorkbenchUserId], googleSubjectId: Option[GoogleSubjectId], diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala index cd9be4e70..140d2a501 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala @@ -421,6 +421,25 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val .map(UserTable.unmarshalUserRecord) } + override def batchLoadUsers( + samUserIds: Set[WorkbenchUserId], + samRequestContext: SamRequestContext + ): IO[Seq[SamUser]] = + if (samUserIds.isEmpty) { + IO.pure(Seq.empty) + } else { + readOnlyTransaction("batchLoadUsers", samRequestContext) { implicit session => + val userTable = UserTable.syntax + val loadUserQuery = samsql"select ${userTable.resultAll} from ${UserTable as userTable} where ${userTable.id} in (${samUserIds})" + + loadUserQuery + .map(UserTable(userTable)) + .list() + .apply() + .map(UserTable.unmarshalUserRecord) + } + } + override def loadUsersByQuery( userId: Option[WorkbenchUserId], googleSubjectId: Option[GoogleSubjectId], diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala index 98d29c576..e16b15bf5 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala @@ -173,6 +173,9 @@ class UserService( def getUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUser]] = directoryDAO.loadUser(userId, samRequestContext) + def getUsersByIds(samUserIds: Seq[WorkbenchUserId], samRequestContext: SamRequestContext): IO[Seq[SamUser]] = + directoryDAO.batchLoadUsers(samUserIds.toSet, samRequestContext) + def getUsersByQuery( userId: Option[WorkbenchUserId], googleSubjectId: Option[GoogleSubjectId], diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminServiceUserRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutesSpec.scala similarity index 79% rename from src/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminServiceUserRoutesSpec.scala rename to src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutesSpec.scala index a21c32533..1c02a2faf 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminServiceUserRoutesSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutesSpec.scala @@ -4,6 +4,7 @@ import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.testkit.ScalatestRouteTest import org.broadinstitute.dsde.workbench.model.{WorkbenchEmail, WorkbenchUserId} +import org.broadinstitute.dsde.workbench.sam.Generator.genWorkbenchUserBoth import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._ import org.broadinstitute.dsde.workbench.sam.model._ import org.broadinstitute.dsde.workbench.sam.model.api.SamUser @@ -12,9 +13,10 @@ import org.broadinstitute.dsde.workbench.sam.{Generator, TestSupport} import org.mockito.scalatest.MockitoSugar import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -import spray.json.DefaultJsonProtocol.immSetFormat +import spray.json.DefaultJsonProtocol._ +import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport._ -class AdminServiceUserRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteTest with MockitoSugar with TestSupport { +class ServiceAdminRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteTest with MockitoSugar with TestSupport { val defaultUser: SamUser = Generator.genWorkbenchUserBoth.sample.get val defaultUserId: WorkbenchUserId = defaultUser.id val defaultUserEmail: WorkbenchEmail = defaultUser.email @@ -127,4 +129,32 @@ class AdminServiceUserRoutesSpec extends AnyFlatSpec with Matchers with Scalates } } + "POST /admin/v2/users" should "get the matching user records when provided a list of user IDs when called as a service admin" in { + // Arrange + val users = Seq.range(0, 10).map(_ => genWorkbenchUserBoth.sample.get) + val samRoutes = new MockSamRoutesBuilder(allUsersGroup) + .callAsAdminServiceUser() // enabled "admin" user who is making the http request + .withEnabledUsers(users) + .build + + // Act and Assert + Post(s"/api/admin/v2/users", users.map(_.id)) ~> samRoutes.route ~> check { + status shouldEqual StatusCodes.OK + responseAs[Seq[SamUser]] should contain theSameElementsAs users + } + } + + it should "reject a request for more than 1000 users" in { + // Arrange + val users = Seq.range(0, 1001).map(_ => genWorkbenchUserBoth.sample.get) + val samRoutes = new MockSamRoutesBuilder(allUsersGroup) + .callAsAdminServiceUser() // enabled "admin" user who is making the http request + .withEnabledUsers(users) + .build + + // Act and Assert + Post(s"/api/admin/v2/users", users.map(_.id)) ~> samRoutes.route ~> check { + status shouldEqual StatusCodes.BadRequest + } + } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala index 2bc11b176..835bdc6fb 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala @@ -115,6 +115,11 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench users.get(userId) } + override def batchLoadUsers( + samUserIds: Set[WorkbenchUserId], + samRequestContext: SamRequestContext + ): IO[Seq[SamUser]] = IO(samUserIds.flatMap(users.get).toSeq) + override def loadUsersByQuery( userId: Option[WorkbenchUserId], googleSubjectId: Option[GoogleSubjectId], diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala index 9d4ba0e84..7c04091fa 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala @@ -625,6 +625,16 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B } } + "batchLoadUsers" - { + "loads a list of users" in { + assume(databaseEnabled, databaseEnabledClue) + val users = Seq.range(0, 10).map(_ => Generator.genWorkbenchUserBoth.sample.get) + users.foreach(user => dao.createUser(user, samRequestContext).unsafeRunSync()) + val loadedUsers = dao.batchLoadUsers(users.map(_.id).toSet, samRequestContext).unsafeRunSync() + loadedUsers should contain theSameElementsAs users + } + } + "deleteUser" - { "delete users" in { assume(databaseEnabled, databaseEnabledClue) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockDirectoryDaoBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockDirectoryDaoBuilder.scala index 7500d9d84..c4ce74a19 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockDirectoryDaoBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockDirectoryDaoBuilder.scala @@ -162,6 +162,9 @@ case class StatefulMockDirectoryDaoBuilder() extends MockitoSugar { .toSet ) ) + mockedDirectoryDAO.batchLoadUsers(any[Set[WorkbenchUserId]], any[SamRequestContext]) answers ((samUserIds: Set[WorkbenchUserId], _: SamRequestContext) => + IO(samUsers.filter(user => samUserIds.contains(user.id)).toSeq) + ) this } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserServiceBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserServiceBuilder.scala index 066bd4c4e..96122389b 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserServiceBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserServiceBuilder.scala @@ -174,6 +174,10 @@ case class MockUserServiceBuilder() extends IdiomaticMockito { .toSet ) ) + + mockUserService.getUsersByIds(any[Seq[WorkbenchUserId]], any[SamRequestContext]) answers ((userIds: Seq[WorkbenchUserId]) => + IO(samUsers.filter(user => userIds.contains(user.id)).toSeq) + ) } private def makeUserAppearEnabled(samUser: SamUser, mockUserService: UserService): Unit = { diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/GetUsersByIdsSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/GetUsersByIdsSpec.scala new file mode 100644 index 000000000..6414122ed --- /dev/null +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/GetUsersByIdsSpec.scala @@ -0,0 +1,50 @@ +package org.broadinstitute.dsde.workbench.sam.service.UserServiceSpecs + +import org.broadinstitute.dsde.workbench.model.WorkbenchEmail +import org.broadinstitute.dsde.workbench.sam.Generator.genWorkbenchUserBoth +import org.broadinstitute.dsde.workbench.sam.model.BasicWorkbenchGroup +import org.broadinstitute.dsde.workbench.sam.service.{CloudExtensions, TestUserServiceBuilder} + +import scala.concurrent.ExecutionContextExecutor + +class GetUsersByIdsSpec extends UserServiceTestTraits { + implicit val ec: ExecutionContextExecutor = scala.concurrent.ExecutionContext.global + + val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("all_users@fake.com")) + + describe("When getting") { + describe("users by IDs") { + it("should be successful") { + // Arrange + val users = Seq.range(0, 10).map(_ => genWorkbenchUserBoth.sample.get) + val userService = TestUserServiceBuilder() + .withAllUsersGroup(allUsersGroup) + .withExistingUsers(users) + .withEnabledUsers(users) + .build + // Act + val response = runAndWait(userService.getUsersByIds(users.map(_.id), samRequestContext)) + + // Assert + assert(response.nonEmpty, "Getting users by ID should return a list of users") + response should contain theSameElementsAs users + } + } + + } + describe("a user that does not exist") { + it("should be unsuccessful") { + // Arrange + val userWithBothIds = genWorkbenchUserBoth.sample.get + val userService = TestUserServiceBuilder() + .withAllUsersGroup(allUsersGroup) + .build + // Act + val response = runAndWait(userService.getUser(userWithBothIds.id, samRequestContext)) + + // Assert + assert(response.isEmpty, "Getting a nonexistent user should not find a user") + } + } + +}