From 9e8a803fac60a2c9b39a451a5ad1d4a50a39f54b Mon Sep 17 00:00:00 2001 From: tlangs Date: Mon, 22 Jul 2024 12:04:25 -0400 Subject: [PATCH 1/4] ID-1300 Admin Get Users by ID --- .../sam/api/ServiceAdminRoutes.scala | 8 +++ .../sam/dataAccess/DirectoryDAO.scala | 5 ++ .../sam/dataAccess/PostgresDirectoryDAO.scala | 19 +++++++ .../workbench/sam/service/UserService.scala | 3 ++ .../sam/dataAccess/MockDirectoryDAO.scala | 5 ++ .../dataAccess/PostgresDirectoryDAOSpec.scala | 10 ++++ .../StatefulMockDirectoryDaoBuilder.scala | 3 ++ .../UserServiceSpecs/GetUsersByIdsSpec.scala | 50 +++++++++++++++++++ 8 files changed, 103 insertions(+) create mode 100644 src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/GetUsersByIdsSpec.scala 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..f3a9a7702 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 @@ -8,6 +8,7 @@ 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 spray.json.DefaultJsonProtocol._ trait ServiceAdminRoutes extends SecurityDirectives with SamRequestContextDirectives with SamUserDirectives with SamModelDirectives { @@ -53,6 +54,13 @@ trait ServiceAdminRoutes extends SecurityDirectives with SamRequestContextDirect .map(users => (if (users.nonEmpty) OK else NotFound) -> users) } } + } ~ + postWithTelemetry(samRequestContext) { + entity(as[Seq[WorkbenchUserId]]) { samUserIds => + complete { + userService.getUsersByIds(samUserIds, 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/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/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") + } + } + +} From 8919986e2912bb8a12d67cf3dcda63ff91d97f43 Mon Sep 17 00:00:00 2001 From: tlangs Date: Tue, 23 Jul 2024 11:09:51 -0400 Subject: [PATCH 2/4] rename test and add api docs --- src/main/resources/swagger/api-docs.yaml | 30 +++++++++++++++++++ ...pec.scala => ServiceAdminRoutesSpec.scala} | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) rename src/test/scala/org/broadinstitute/dsde/workbench/sam/api/{AdminServiceUserRoutesSpec.scala => ServiceAdminRoutesSpec.scala} (97%) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index d58d335b8..322138b4f 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -168,6 +168,36 @@ 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' + 403: + description: You do not have service admin privileges + content: { } + 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/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminServiceUserRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutesSpec.scala similarity index 97% 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..7db034654 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 @@ -14,7 +14,7 @@ import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import spray.json.DefaultJsonProtocol.immSetFormat -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 From f28783d52dedc35e5f0b1296d46a220bd8fd54ea Mon Sep 17 00:00:00 2001 From: tlangs Date: Tue, 23 Jul 2024 11:58:41 -0400 Subject: [PATCH 3/4] pr comments --- src/main/resources/swagger/api-docs.yaml | 2 +- .../sam/api/ServiceAdminRoutes.scala | 20 ++++++++---- .../sam/api/ServiceAdminRoutesSpec.scala | 32 ++++++++++++++++++- .../sam/service/MockUserServiceBuilder.scala | 4 +++ 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 322138b4f..4394b25cc 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 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 f3a9a7702..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._ @@ -9,6 +11,7 @@ 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 { @@ -56,12 +59,17 @@ trait ServiceAdminRoutes extends SecurityDirectives with SamRequestContextDirect } } ~ postWithTelemetry(samRequestContext) { - entity(as[Seq[WorkbenchUserId]]) { samUserIds => - complete { - userService.getUsersByIds(samUserIds, 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/test/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutesSpec.scala index 7db034654..1c02a2faf 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ServiceAdminRoutesSpec.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,7 +13,8 @@ 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 ServiceAdminRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteTest with MockitoSugar with TestSupport { val defaultUser: SamUser = Generator.genWorkbenchUserBoth.sample.get @@ -127,4 +129,32 @@ class ServiceAdminRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRou } } + "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/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 = { From 7eeefa0bde95cf74436aee1c1fecf82950beba17 Mon Sep 17 00:00:00 2001 From: tlangs Date: Wed, 24 Jul 2024 09:53:59 -0400 Subject: [PATCH 4/4] pr comments --- src/main/resources/swagger/api-docs.yaml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 4394b25cc..bd74d445f 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -189,9 +189,18 @@ paths: 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: { } + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' 500: description: Internal Server Error content: