Address comments

src/main/resources/swagger/api-docs.yaml

@@ -43,7 +43,8 @@ paths:
     patch:
       tags:
         - Admin
-      summary: Updates given fields on a user, by user id
+      summary: Updates given fields on a user record in the database. Ids can be nulled by sending 'null' as the value of the id in the request.
+        An azureB2CId cannot be set to null if the user already has a null googleSubjectId and vice versa.
       operationId: adminUpdateUser
       parameters:
         - name: userId
@@ -4334,16 +4335,12 @@ components:
     UpdateUserRequest:
       type: object
       properties:
-        googleSubjectId:
+        azureB2CId:
           type: string
-          description: User's Google subject Id
-        email:
+          description: User's Azure B2C Id, can be set to null
+        googleSubjectId:
           type: string
-          description: User's email address
-          format: email
-        enabled:
-          type: boolean
-          description: Whether or not the user is enabled
+          description: User's Google subject Id, can be set to null
       description: specification of a UpdateUserRequest
     FilteredResourceResponse:
       type: object

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]]

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()})"
       }
 

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)

src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/AdminUpdateUserRequest.scala

@@ -0,0 +1,34 @@
+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}
+import spray.json.DefaultJsonProtocol._
+import spray.json.RootJsonFormat
+
+object AdminUpdateUserRequest {
+  implicit val UserUpdateRequestFormat: RootJsonFormat[AdminUpdateUserRequest] =
+    jsonFormat(AdminUpdateUserRequest.apply, "azureB2CId", "googleSubjectId")
+
+  def apply(
+      azureB2CId: Option[AzureB2CId],
+      googleSubjectId: Option[GoogleSubjectId]
+  ): AdminUpdateUserRequest =
+    AdminUpdateUserRequest(azureB2CId, googleSubjectId)
+}
+@Lenses final case class AdminUpdateUserRequest(
+    azureB2CId: Option[AzureB2CId],
+    googleSubjectId: Option[GoogleSubjectId]
+) {
+  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
+  }
+}

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)

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.

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

@@ -206,38 +206,14 @@ class UserService(
       case Some(user) =>
         // validate all fields to be updated
         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
-          }
+          if (request.azureB2CId.isDefined || request.googleSubjectId.isDefined) {
+            directoryDAO.updateUser(user, request, samRequestContext)
+          } else IO(Option(user))
         }
       case None => IO(None)
     }

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

@@ -67,97 +67,14 @@ class AdminUserRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteT
     }
   }
 
-  "PATCH /admin/v1/user/{userSubjectId}" should "update a user if the requesting user is an admin" in {
-    // Arrange
-    val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
-      .callAsAdminUser() // enabled "admin" user who is making the http request
-      .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
-      .withAllowedUser(defaultUser)
-      .build
-    val requestBody = AdminUpdateUserRequest(None, None, Some(newUserEmail), None)
-    // Act
-    Patch(s"/api/admin/v1/user/$defaultUserId", requestBody) ~> samRoutes.route ~> check {
-      // Assert
-      withClue(s"Response Body: ${responseAs[String]}")(status shouldEqual StatusCodes.OK)
-      // Enabled in particular since we cant directly extract the user from the builder
-      responseAs[SamUser] shouldEqual defaultUser.copy(email = newUserEmail)
-    }
-  }
-
-  it should "report an error when updating a user if the request email is invalid and the requesting user is an admin" in {
-    // Arrange
-    val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
-      .callAsAdminUser()
-      .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
-      .withAllowedUser(defaultUser)
-      .withBadEmail()
-      .build
-    val requestBody = AdminUpdateUserRequest(None, None, Some(newUserEmail), None)
-
-    // Act
-    Patch(s"/api/admin/v1/user/$defaultUserId", requestBody) ~> samRoutes.route ~> check {
-      // Assert
-      withClue(s"Response Body: ${responseAs[String]}")(status shouldEqual StatusCodes.BadRequest)
-    }
-  }
-
-  it should "not find a user when trying to update a user if the user does not exist and the requesting user is an admin" in {
-    // Arrange
-    val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
-      .callAsAdminUser()
-      .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
-      .withAllowedUser(defaultUser)
-      .build
-    val requestBody = AdminUpdateUserRequest(None, None, Some(newUserEmail), None)
-
-    // Act
-    Patch(s"/api/admin/v1/user/$badUserId", requestBody) ~> samRoutes.route ~> check {
-      // Assert
-      withClue(s"Response Body: ${responseAs[String]}")(status shouldEqual StatusCodes.NotFound)
-    }
-  }
-
-  it should "forbid updating a user if the requesting user is a non admin" in {
-    // Arrange
-    val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
-      .callAsNonAdminUser()
-      .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
-      .withAllowedUser(defaultUser)
-      .build
-    val requestBody = AdminUpdateUserRequest(None, None, Some(newUserEmail), None)
-
-    // Act
-    Patch(s"/api/admin/v1/user/$defaultUserId", requestBody) ~> samRoutes.route ~> check {
-      // Assert
-      withClue(s"Response Body: ${responseAs[String]}")(status shouldEqual StatusCodes.Forbidden)
-    }
-  }
-
-  it should "forbid updating a user for a user if the user does not exist and the requesting user is a non admin" in {
-    // Arrange
-    val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
-      .callAsNonAdminUser()
-      .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
-      .withAllowedUser(defaultUser)
-      .build
-    val requestBody = AdminUpdateUserRequest(None, None, Some(newUserEmail), None)
-
-    // Act
-    Patch(s"/api/admin/v1/user/$badUserId", requestBody) ~> samRoutes.route ~> check {
-      // Assert
-      withClue(s"Response Body: ${responseAs[String]}")(status shouldEqual StatusCodes.Forbidden)
-    }
-  }
-
   "PATCH /admin/v1/user/{userSubjectId}" should "update a user's googleSubjectId" in {
     // Arrange
     val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
       .callAsAdminUser() // enabled "admin" user who is making the http request
       .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
-      .withAllowedUser(defaultUser)
       .build
     val newGoogleSubjectId = Some(GoogleSubjectId("newGoogleSubjectId"))
-    val requestBody = AdminUpdateUserRequest(None, newGoogleSubjectId, None, None)
+    val requestBody = AdminUpdateUserRequest(None, newGoogleSubjectId)
     // Act
     Patch(s"/api/admin/v1/user/$defaultUserId", requestBody) ~> samRoutes.route ~> check {
       // Assert
@@ -174,7 +91,7 @@ class AdminUserRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteT
       .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
       .withAllowedUser(defaultUser)
       .build
-    val requestBody = AdminUpdateUserRequest(None, Some(GoogleSubjectId("null")), None, None)
+    val requestBody = AdminUpdateUserRequest(None, Some(GoogleSubjectId("null")))
     // Act
     Patch(s"/api/admin/v1/user/$defaultUserId", requestBody) ~> samRoutes.route ~> check {
       // Assert
@@ -192,7 +109,7 @@ class AdminUserRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteT
       .withAllowedUser(defaultUser)
       .build
     val newAzureB2CId = Some(AzureB2CId("0000-0000-0000-0000"))
-    val requestBody = AdminUpdateUserRequest(newAzureB2CId, None, None, None)
+    val requestBody = AdminUpdateUserRequest(newAzureB2CId, None)
     // Act
     Patch(s"/api/admin/v1/user/$defaultUserId", requestBody) ~> samRoutes.route ~> check {
       // Assert
@@ -209,7 +126,7 @@ class AdminUserRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteT
       .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
       .withAllowedUser(defaultUser)
       .build
-    val requestBody = AdminUpdateUserRequest(Some(AzureB2CId("null")), None, None, None)
+    val requestBody = AdminUpdateUserRequest(Some(AzureB2CId("null")), None)
     // Act
     Patch(s"/api/admin/v1/user/$defaultUserId", requestBody) ~> samRoutes.route ~> check {
       // Assert