From dcecab1befefa24562d333dca603c07d690bb781 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Wed, 31 Jan 2024 13:27:19 -0500 Subject: [PATCH] Address comments --- .../sam/dataAccess/DirectoryDAO.scala | 6 +-- .../sam/dataAccess/PostgresDirectoryDAO.scala | 29 +++++++------- .../dsde/workbench/sam/model/SamModel.scala | 5 --- .../model/api/AdminUpdateUserRequest.scala | 38 +++++++++++++++++++ .../sam/model/api/SamJsonSupport.scala | 2 - .../workbench/sam/model/api/SamModelApi.scala | 6 --- .../workbench/sam/service/UserService.scala | 38 ++++++------------- .../sam/dataAccess/MockDirectoryDAO.scala | 14 +++---- .../dataAccess/PostgresDirectoryDAOSpec.scala | 8 ++-- .../StatefulMockDirectoryDaoBuilder.scala | 6 +-- 10 files changed, 81 insertions(+), 71 deletions(-) create mode 100644 src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/AdminUpdateUserRequest.scala 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 6ad97e379..e25b6c20f 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 @@ -4,8 +4,8 @@ import cats.effect.IO import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.model.google.ServiceAccountSubjectId import org.broadinstitute.dsde.workbench.sam.azure.{ManagedIdentityObjectId, PetManagedIdentity, PetManagedIdentityId} -import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, SamUserAttributes} -import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUserTos, UserUpdate} +import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes} +import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUserTos} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.time.Instant @@ -53,7 +53,7 @@ trait DirectoryDAO { def setUserAzureB2CId(userId: WorkbenchUserId, b2cId: AzureB2CId, samRequestContext: SamRequestContext): IO[Unit] def updateUserEmail(userId: WorkbenchUserId, email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Unit] def deleteUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Unit] - def updateUser(samUser: SamUser, userUpdate: UserUpdate, samRequestContext: SamRequestContext): IO[Option[SamUser]] + def updateUser(samUser: SamUser, userUpdate: AdminUpdateUserRequest, samRequestContext: SamRequestContext): IO[Option[SamUser]] def listUsersGroups(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Set[WorkbenchGroupIdentity]] def listUserDirectMemberships(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[LazyList[WorkbenchGroupIdentity]] 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 e07e53978..46deeafe3 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 @@ -12,7 +12,7 @@ import org.broadinstitute.dsde.workbench.sam.db.SamTypeBinders._ import org.broadinstitute.dsde.workbench.sam.db._ import org.broadinstitute.dsde.workbench.sam.db.tables._ import org.broadinstitute.dsde.workbench.sam.model._ -import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, SamUserAttributes} +import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes} import org.broadinstitute.dsde.workbench.sam.util.{DatabaseSupport, SamRequestContext} import org.postgresql.util.PSQLException import scalikejdbc._ @@ -469,41 +469,42 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val override def updateUserEmail(userId: WorkbenchUserId, email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Unit] = IO.unit - override def updateUser(samUser: SamUser, userUpdate: UserUpdate, samRequestContext: SamRequestContext): IO[Option[SamUser]] = + override def updateUser(samUser: SamUser, userUpdate: AdminUpdateUserRequest, samRequestContext: SamRequestContext): IO[Option[SamUser]] = serializableWriteTransaction("updateUser", samRequestContext) { implicit session => val u = UserTable.column + // NOTE updating emails and 'enabled' status is currently not supported by this method val setColumnsClause = userUpdate match { - case UserUpdate(None, None) => throw new WorkbenchException("Cannot update user with no values.") - case UserUpdate(Some(newAzureB2CId), None) => + case AdminUpdateUserRequest(None, None, _, _) => throw new WorkbenchException("Cannot update user with no values.") + case AdminUpdateUserRequest(Some(newAzureB2CId), None, _, _) => samsqls"(${u.azureB2cId}, ${u.updatedAt})" - case UserUpdate(None, Some(newGoogleSubjectId)) => + case AdminUpdateUserRequest(None, Some(newGoogleSubjectId), _, _) => samsqls"(${u.googleSubjectId}, ${u.updatedAt})" - case UserUpdate(Some(newGoogleSubjectId), Some(newAzureB2CId)) => + case AdminUpdateUserRequest(Some(newGoogleSubjectId), Some(newAzureB2CId), _, _) => samsqls"(${u.googleSubjectId}, ${u.azureB2cId}, ${u.updatedAt})" } var updatedUser = samUser.copy( - googleSubjectId = userUpdate.newGoogleSubjectId.map(GoogleSubjectId).orElse(samUser.googleSubjectId), - azureB2CId = userUpdate.newAzureB2CId.map(AzureB2CId).orElse(samUser.azureB2CId), + googleSubjectId = userUpdate.googleSubjectId.orElse(samUser.googleSubjectId), + azureB2CId = userUpdate.azureB2CId.orElse(samUser.azureB2CId), updatedAt = Instant.now() ) val setColumnValuesClause = userUpdate match { - case UserUpdate(None, None) => throw new WorkbenchException("Cannot update user with no values.") - case UserUpdate(Some("null"), None) => + case AdminUpdateUserRequest(None, None, _, _) => throw new WorkbenchException("Cannot update user with no values.") + case AdminUpdateUserRequest(Some(AzureB2CId("null")), None, _, _) => updatedUser = updatedUser.copy(azureB2CId = None) // string interpolation adds quotes around null so we have to special case it here samsqls"(null, ${Instant.now()})" - case UserUpdate(None, Some("null")) => + case AdminUpdateUserRequest(None, Some(GoogleSubjectId("null")), _, _) => updatedUser = updatedUser.copy(googleSubjectId = None) // string interpolation adds quotes around null so we have to special case it here samsqls"(null, ${Instant.now()})" - case UserUpdate(Some(newGoogleSubjectId), None) => + case AdminUpdateUserRequest(Some(newGoogleSubjectId), None, _, _) => samsqls"($newGoogleSubjectId, ${Instant.now()})" - case UserUpdate(None, Some(newAzureB2CId)) => + case AdminUpdateUserRequest(None, Some(newAzureB2CId), _, _) => samsqls"($newAzureB2CId, ${Instant.now()})" - case UserUpdate(Some(newGoogleSubjectId), Some(newAzureB2CId)) => + case AdminUpdateUserRequest(Some(newGoogleSubjectId), Some(newAzureB2CId), _, _) => samsqls"($newGoogleSubjectId, $newAzureB2CId, ${Instant.now()})" } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala index a89b7b1e6..2fbf34844 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala @@ -72,11 +72,6 @@ object UserStatusDetails { tosAccepted: Boolean, adminEnabled: Boolean ) - -@Lenses final case class UserUpdate( - newAzureB2CId: Option[String], - newGoogleSubjectId: Option[String] -) @Lenses final case class TermsOfServiceAcceptance(value: String) extends ValueObject @Lenses final case class TermsOfServiceComplianceStatus(userId: WorkbenchUserId, userHasAcceptedLatestTos: Boolean, permitsSystemUsage: Boolean) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/AdminUpdateUserRequest.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/AdminUpdateUserRequest.scala new file mode 100644 index 000000000..b4dd55207 --- /dev/null +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/AdminUpdateUserRequest.scala @@ -0,0 +1,38 @@ +package org.broadinstitute.dsde.workbench.sam.model.api + +import akka.http.scaladsl.model.StatusCodes +import monocle.macros.Lenses +import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport._ +import org.broadinstitute.dsde.workbench.model.{AzureB2CId, ErrorReport, GoogleSubjectId, WorkbenchEmail} +import spray.json.DefaultJsonProtocol._ +import spray.json.RootJsonFormat + +object AdminUpdateUserRequest { + implicit val UserUpdateRequestFormat: RootJsonFormat[AdminUpdateUserRequest] = + jsonFormat(AdminUpdateUserRequest.apply, "azureB2CId", "googleSubjectId", "email", "enabled") + + def apply( + azureB2CId: Option[AzureB2CId], + googleSubjectId: Option[GoogleSubjectId], + email: Option[WorkbenchEmail], + enabled: Option[Boolean] + ): AdminUpdateUserRequest = + AdminUpdateUserRequest(azureB2CId, googleSubjectId, email, enabled) +} +@Lenses final case class AdminUpdateUserRequest( + azureB2CId: Option[AzureB2CId], + googleSubjectId: Option[GoogleSubjectId], + email: Option[WorkbenchEmail], + enabled: Option[Boolean] +) { + def isValid(user: SamUser): Seq[ErrorReport] = { + import org.broadinstitute.dsde.workbench.google.errorReportSource + + var errorReports = Seq[ErrorReport]() + if (azureB2CId.contains(AzureB2CId("null")) && user.googleSubjectId.isEmpty) + errorReports = errorReports ++ Seq(ErrorReport(StatusCodes.BadRequest, "Unable to null azureB2CId when the user's googleSubjectId is already null")) + else if (googleSubjectId.contains(GoogleSubjectId("null")) && user.azureB2CId.isEmpty) + errorReports = errorReports ++ Seq(ErrorReport(StatusCodes.BadRequest, "Unable to null googleSubjectId when the user's azureB2CId is already null")) + errorReports + } +} diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala index d0d6a64a0..45178a65a 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala @@ -101,8 +101,6 @@ object SamJsonSupport { implicit val UserPolicyResponseFormat = jsonFormat5(UserPolicyResponse.apply) - implicit val UserUpdateRequestFormat = jsonFormat4(AdminUpdateUserRequest.apply) - implicit val RolesAndActionsFormat = jsonFormat2(RolesAndActions.apply) implicit val UserResourcesResponseFormat = jsonFormat6(UserResourcesResponse.apply) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamModelApi.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamModelApi.scala index ba85df73e..5e7fe7124 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamModelApi.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamModelApi.scala @@ -4,12 +4,6 @@ import monocle.macros.Lenses import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.sam.model._ import spray.json.{DefaultJsonProtocol, JsObject, JsString, JsValue, RootJsonFormat, deserializationError} -@Lenses final case class AdminUpdateUserRequest( - azureB2CId: Option[AzureB2CId], - googleSubjectId: Option[GoogleSubjectId], - email: Option[WorkbenchEmail], - enabled: Option[Boolean] -) // AccessPolicyMembership.memberPolicies is logically read-only; at some point in the future it could be lazy-loaded // (via extra queries) based on the contents of memberEmails. 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 24ac85649..6006a32bc 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 @@ -208,36 +208,20 @@ class UserService( var errorReports = Seq[ErrorReport]() request.email.foreach(email => errorReports = errorReports ++ validateEmail(email, blockedEmailDomains)) request.azureB2CId.foreach(azureB2CId => errorReports = errorReports ++ validateAzureB2CId(azureB2CId)) + errorReports = errorReports ++ request.isValid(user) if (errorReports.nonEmpty) { IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, "invalid user update", errorReports))) } else { // apply all updates - val userUpdate = UserUpdate(request.azureB2CId.map(_.value), request.googleSubjectId.map(_.value)) - - userUpdate match { - case UserUpdate(Some("null"), None) if user.googleSubjectId.isEmpty => - IO.raiseError( - new WorkbenchExceptionWithErrorReport( - ErrorReport(StatusCodes.BadRequest, "Cannot null a user's azureB2CId when the user has no googleSubjectId") - ) - ) - case UserUpdate(None, Some("null")) if user.azureB2CId.isEmpty => - IO.raiseError( - new WorkbenchExceptionWithErrorReport( - ErrorReport(StatusCodes.BadRequest, "Cannot null a user's googleSubjectId when the user has no azureB2CId") - ) - ) - case _ => - val updatedUser = if (request.azureB2CId.isDefined || request.googleSubjectId.isDefined) { - directoryDAO.updateUser(user, userUpdate, samRequestContext) - } else IO(Option(user)) - - if (request.email.isDefined) { - updatedUser.map { u => - directoryDAO.updateUserEmail(userId, request.email.get, samRequestContext) - u.map(_.copy(email = request.email.get)) - } - } else updatedUser - } + val updatedUser = if (request.azureB2CId.isDefined || request.googleSubjectId.isDefined) { + directoryDAO.updateUser(user, request, samRequestContext) + } else IO(Option(user)) + + if (request.email.isDefined) { + updatedUser.map { u => + directoryDAO.updateUserEmail(userId, request.email.get, samRequestContext) + u.map(_.copy(email = request.email.get)) + } + } else updatedUser } case None => IO(None) } 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 4e217e13b..2ebef8165 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 @@ -8,8 +8,8 @@ import org.broadinstitute.dsde.workbench.model.google.ServiceAccountSubjectId import org.broadinstitute.dsde.workbench.sam._ import org.broadinstitute.dsde.workbench.sam.azure.{ManagedIdentityObjectId, PetManagedIdentity, PetManagedIdentityId} import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable -import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, SamUserAttributes} -import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbenchGroup, SamUserTos, UserUpdate} +import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes} +import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbenchGroup, SamUserTos} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.time.Instant @@ -129,17 +129,17 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench users -= userId } - override def updateUser(samUser: SamUser, userUpdate: UserUpdate, samRequestContext: SamRequestContext): IO[Option[SamUser]] = { + override def updateUser(samUser: SamUser, userUpdate: AdminUpdateUserRequest, samRequestContext: SamRequestContext): IO[Option[SamUser]] = { val updatedUser = for { user <- users.get(samUser.id) updatedUser = user.copy( googleSubjectId = - if (userUpdate.newGoogleSubjectId.contains("null")) None - else if (userUpdate.newGoogleSubjectId.isDefined) userUpdate.newGoogleSubjectId.map(GoogleSubjectId) + if (userUpdate.googleSubjectId.contains(GoogleSubjectId("null"))) None + else if (userUpdate.googleSubjectId.isDefined) userUpdate.googleSubjectId else user.googleSubjectId, azureB2CId = - if (userUpdate.newAzureB2CId.contains("null")) None - else if (userUpdate.newAzureB2CId.isDefined) userUpdate.newAzureB2CId.map(AzureB2CId) + if (userUpdate.azureB2CId.contains(AzureB2CId("null"))) None + else if (userUpdate.azureB2CId.isDefined) userUpdate.azureB2CId else user.azureB2CId, updatedAt = Instant.now() ) 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 5dd640b42..c55060b4c 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 @@ -12,7 +12,7 @@ import org.broadinstitute.dsde.workbench.sam.db.TestDbReference import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable import org.broadinstitute.dsde.workbench.sam.matchers.TimeMatchers import org.broadinstitute.dsde.workbench.sam.model._ -import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, SamUserAttributes} +import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes} import org.broadinstitute.dsde.workbench.sam.{Generator, RetryableAnyFreeSpec, TestSupport} import org.scalatest.Inside.inside import org.scalatest.matchers.should.Matchers @@ -1324,7 +1324,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.createUser(user, samRequestContext).unsafeRunSync() dao.loadUser(user.id, samRequestContext).unsafeRunSync().flatMap(_.googleSubjectId) shouldBe user.googleSubjectId - dao.updateUser(user, UserUpdate(None, Option("null")), samRequestContext).unsafeRunSync() + dao.updateUser(user, AdminUpdateUserRequest(None, Option(GoogleSubjectId("null")), None, None), samRequestContext).unsafeRunSync() dao.loadUser(user.id, samRequestContext).unsafeRunSync().flatMap(_.googleSubjectId) shouldBe None } @@ -1335,7 +1335,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.createUser(user, samRequestContext).unsafeRunSync() dao.loadUser(user.id, samRequestContext).unsafeRunSync().flatMap(_.azureB2CId) shouldBe user.azureB2CId - dao.updateUser(user, UserUpdate(Option("null"), None), samRequestContext).unsafeRunSync() + dao.updateUser(user, AdminUpdateUserRequest(Option(AzureB2CId("null")), None, None, None), samRequestContext).unsafeRunSync() dao.loadUser(user.id, samRequestContext).unsafeRunSync().flatMap(_.azureB2CId) shouldBe None } @@ -1360,7 +1360,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.createUser(user, samRequestContext).unsafeRunSync() dao.loadUser(user.id, samRequestContext).unsafeRunSync().flatMap(_.googleSubjectId) shouldBe user.googleSubjectId - dao.updateUser(user, UserUpdate(None, Option("null")), samRequestContext).unsafeRunSync() + dao.updateUser(user, AdminUpdateUserRequest(None, Option(GoogleSubjectId("null")), None, None), samRequestContext).unsafeRunSync() dao.loadUser(user.id, samRequestContext).unsafeRunSync().flatMap(_.googleSubjectId) shouldBe None } 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 ae0349d1a..7500d9d84 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 @@ -3,8 +3,8 @@ package org.broadinstitute.dsde.workbench.sam.dataAccess import cats.effect.IO import cats.effect.unsafe.implicits.global import org.broadinstitute.dsde.workbench.model._ -import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, UserUpdate} -import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, SamUserAttributes} +import org.broadinstitute.dsde.workbench.sam.model.BasicWorkbenchGroup +import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.mockito.ArgumentMatchers import org.mockito.IdiomaticMockito.StubbingOps @@ -233,7 +233,7 @@ case class StatefulMockDirectoryDaoBuilder() extends MockitoSugar { lenient() .doReturn(IO(Option(samUser))) .when(mockedDirectoryDAO) - .updateUser(ArgumentMatchers.eq(samUser), any[UserUpdate], any[SamRequestContext]) + .updateUser(ArgumentMatchers.eq(samUser), any[AdminUpdateUserRequest], any[SamRequestContext]) }