Move all validation into request class

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

@@ -2,11 +2,14 @@ package org.broadinstitute.dsde.workbench.sam.model.api
 
 import akka.http.scaladsl.model.StatusCodes
 import monocle.macros.Lenses
+import org.broadinstitute.dsde.workbench.google.errorReportSource
 import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport._
 import org.broadinstitute.dsde.workbench.model.{AzureB2CId, ErrorReport, GoogleSubjectId}
 import spray.json.DefaultJsonProtocol._
 import spray.json.RootJsonFormat
 
+import java.util.regex.Pattern
+
 object AdminUpdateUserRequest {
   implicit val UserUpdateRequestFormat: RootJsonFormat[AdminUpdateUserRequest] =
     jsonFormat(AdminUpdateUserRequest.apply, "azureB2CId", "googleSubjectId")
@@ -21,10 +24,16 @@ object AdminUpdateUserRequest {
     azureB2CId: Option[AzureB2CId],
     googleSubjectId: Option[GoogleSubjectId]
 ) {
+  val UUID_REGEX =
+    Pattern.compile("^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$")
+  private def validateAzureB2CId(azureB2CId: AzureB2CId): Option[ErrorReport] =
+    if (!UUID_REGEX.matcher(azureB2CId.value).matches() && !(azureB2CId.value == "null")) {
+      Option(ErrorReport(s"invalid azureB2CId [${azureB2CId.value}]"))
+    } else None
   def isValid(user: SamUser): Seq[ErrorReport] = {
-    import org.broadinstitute.dsde.workbench.google.errorReportSource
 
     var errorReports = Seq[ErrorReport]()
+    azureB2CId.foreach(azureB2CId => errorReports = errorReports ++ validateAzureB2CId(azureB2CId))
     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)

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

@@ -11,21 +11,13 @@ import org.broadinstitute.dsde.workbench.sam.azure.ManagedIdentityObjectId
 import org.broadinstitute.dsde.workbench.sam.config.AzureServicesConfig
 import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO
 import org.broadinstitute.dsde.workbench.sam.model._
-import org.broadinstitute.dsde.workbench.sam.model.api.{
-  AdminUpdateUserRequest,
-  SamUser,
-  SamUserAllowances,
-  SamUserAttributes,
-  SamUserAttributesRequest,
-  SamUserRegistrationRequest
-}
+import org.broadinstitute.dsde.workbench.sam.model.api._
 import org.broadinstitute.dsde.workbench.sam.service.UserService.genWorkbenchUserId
 import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.IOWithLogging
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 
 import java.security.SecureRandom
 import java.time.Instant
-import java.util.regex.Pattern
 import javax.naming.NameNotFoundException
 import scala.concurrent.ExecutionContext
 import scala.util.matching.Regex
@@ -41,8 +33,6 @@ class UserService(
 )(implicit
     val executionContext: ExecutionContext
 ) extends LazyLogging {
-  val UUID_REGEX =
-    Pattern.compile("^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$");
   // this is what's currently called
   def createUser(possibleNewUser: SamUser, samRequestContext: SamRequestContext): IO[UserStatus] =
     createUser(possibleNewUser, None, samRequestContext)
@@ -154,11 +144,6 @@ class UserService(
       Option(ErrorReport("cannot create user when neither google subject id nor azure b2c id exists"))
     } else None
 
-  private def validateAzureB2CId(azureB2CId: AzureB2CId): Option[ErrorReport] =
-    if (!UUID_REGEX.matcher(azureB2CId.value).matches() && !(azureB2CId.value == "null")) {
-      Option(ErrorReport(s"invalid azureB2CId [${azureB2CId.value}]"))
-    } else None
-
   private def validateEmail(email: WorkbenchEmail, blockedEmailDomains: Seq[String]): Option[ErrorReport] =
     if (!UserService.emailRegex.matches(email.value)) {
       Option(ErrorReport(StatusCodes.BadRequest, s"invalid email address [${email.value}]"))
@@ -205,9 +190,7 @@ class UserService(
     directoryDAO.loadUser(userId, samRequestContext).flatMap {
       case Some(user) =>
         // validate all fields to be updated
-        var errorReports = Seq[ErrorReport]()
-        request.azureB2CId.foreach(azureB2CId => errorReports = errorReports ++ validateAzureB2CId(azureB2CId))
-        errorReports = errorReports ++ request.isValid(user)
+        val errorReports = request.isValid(user)
         if (errorReports.nonEmpty) {
           IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, "invalid user update", errorReports)))
         } else { // apply all updates

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

@@ -72,6 +72,7 @@ class AdminUserRoutesSpec extends AnyFlatSpec with Matchers with ScalatestRouteT
     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)

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

@@ -1,18 +1,9 @@
 package org.broadinstitute.dsde.workbench.sam.service
 
-import akka.http.scaladsl.model.StatusCodes
 import cats.effect.IO
-import org.broadinstitute.dsde.workbench.google.errorReportSource
 import org.broadinstitute.dsde.workbench.model._
 import org.broadinstitute.dsde.workbench.sam.TestSupport.enabledMapNoTosAccepted
-import org.broadinstitute.dsde.workbench.sam.model.api.{
-  AdminUpdateUserRequest,
-  SamUser,
-  SamUserAllowances,
-  SamUserAttributes,
-  SamUserAttributesRequest,
-  SamUserRegistrationRequest
-}
+import org.broadinstitute.dsde.workbench.sam.model.api._
 import org.broadinstitute.dsde.workbench.sam.model.{UserStatus, UserStatusDetails}
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 import org.mockito.ArgumentMatchersSugar.{any, argThat, eqTo}
@@ -107,17 +98,17 @@ case class MockUserServiceBuilder() extends IdiomaticMockito {
     mockUserService.getUser(eqTo(samUser.id), any[SamRequestContext]) answers ((_: WorkbenchUserId) => IO(Option(samUser)))
     mockUserService.deleteUser(eqTo(samUser.id), any[SamRequestContext]) returns IO(())
     mockUserService.updateUserCrud(eqTo(samUser.id), any[AdminUpdateUserRequest], any[SamRequestContext]) answers (
-      (_: WorkbenchUserId, r: AdminUpdateUserRequest) => {
-        val newAzureB2CId = if (r.azureB2CId.contains(AzureB2CId("null"))) None else if (r.azureB2CId.isDefined) r.azureB2CId else samUser.azureB2CId
-        val newGoogleSubjectId =
-          if (r.googleSubjectId.contains(GoogleSubjectId("null"))) None else if (r.googleSubjectId.isDefined) r.googleSubjectId else samUser.googleSubjectId
-        val newUser = samUser.copy(
-          azureB2CId = newAzureB2CId,
-          googleSubjectId = newGoogleSubjectId
-        )
-        IO(Option(newUser))
-      }
-    )
+        (_: WorkbenchUserId, r: AdminUpdateUserRequest, _: SamRequestContext) => {
+          val newAzureB2CId = if (r.azureB2CId.contains(AzureB2CId("null"))) None else if (r.azureB2CId.isDefined) r.azureB2CId else samUser.azureB2CId
+          val newGoogleSubjectId =
+            if (r.googleSubjectId.contains(GoogleSubjectId("null"))) None else if (r.googleSubjectId.isDefined) r.googleSubjectId else samUser.googleSubjectId
+          val newUser = samUser.copy(
+            azureB2CId = newAzureB2CId,
+            googleSubjectId = newGoogleSubjectId
+          )
+          IO(Option(newUser))
+        }
+      )
     mockUserService.enableUser(any[WorkbenchUserId], any[SamRequestContext]) returns {
       IO(None)
     }
@@ -222,13 +213,6 @@ case class MockUserServiceBuilder() extends IdiomaticMockito {
     ) returns IO(userAttributes)
   }
 
-  private def handleMalformedEmail(mockUserService: UserService): Unit =
-    if (isBadEmail) {
-      mockUserService.updateUserCrud(any[WorkbenchUserId], any[AdminUpdateUserRequest], any[SamRequestContext]) returns {
-        IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, "invalid user update", Seq())))
-      }
-    } else ()
-
   // The order that dictates the priority of the mocks should be handled in this build method
   // so that individual tests do not need to be concerned about what order the builder methods are called
   // the more specific the matcher, the later it should be defined as the priority of mock invokes are last in first out
@@ -241,7 +225,6 @@ case class MockUserServiceBuilder() extends IdiomaticMockito {
     allowedUsers.foreach(u => makeUserAppearAllowed(u, mockUserService))
     disallowedUsers.foreach(u => makeUserAppearDisallowed(u, mockUserService))
     userAttributesSet.foreach(tup => makeUserAttributesAppear(tup._1, tup._2, mockUserService))
-    handleMalformedEmail(mockUserService)
     mockUserService
   }
 }