Address comments

broadinstitute · Jan 31, 2024 · dcecab1 · dcecab1
1 parent 7cbc359
commit dcecab1
Show file tree

Hide file tree

Showing 10 changed files with 81 additions and 71 deletions.
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/...est/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/...la/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockDirectoryDaoBuilder.scala b/...la/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])
 
   }