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 index a13877849..b1a3ab6b6 100644 --- 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 @@ -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) 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 10200d1a4..f6170b315 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 @@ -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 diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminUserRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminUserRoutesSpec.scala index 0c0992942..feca4bbe0 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminUserRoutesSpec.scala +++ b/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) 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 1baea6ef9..40514713e 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 @@ -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 } }