refactor: Prevent illegal updates with KnoraUserService (#3098)

dasch-swiss · Mar 6, 2024 · 4111312 · 4111312
1 parent 12db892
commit 4111312
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 27 deletions.
diff --git a/webapi/src/main/scala/org/knora/webapi/slice/admin/api/service/UsersRestService.scala b/webapi/src/main/scala/org/knora/webapi/slice/admin/api/service/UsersRestService.scala
@@ -36,7 +36,6 @@ import org.knora.webapi.slice.admin.domain.service.KnoraUserService
 import org.knora.webapi.slice.admin.domain.service.KnoraUserToUserConverter
 import org.knora.webapi.slice.admin.domain.service.PasswordService
 import org.knora.webapi.slice.admin.domain.service.ProjectADMService
-import org.knora.webapi.slice.admin.domain.service.UserChangeRequest
 import org.knora.webapi.slice.admin.domain.service.UserService
 import org.knora.webapi.slice.common.api.AuthorizationRestService
 import org.knora.webapi.slice.common.api.KnoraResponseRenderer
@@ -135,14 +134,14 @@ final case class UsersRestService(
     _    <- ensureNotABuiltInUser(userIri)
     _    <- ensureSelfUpdateOrSystemAdmin(userIri, requestingUser)
     user <- getKnoraUserOrNotFound(userIri)
-    theChange = UserChangeRequest(
-                  username = changeRequest.username,
-                  email = changeRequest.email,
-                  givenName = changeRequest.givenName,
-                  familyName = changeRequest.familyName,
-                  lang = changeRequest.lang,
-                )
-    updated  <- knoraUserService.updateUser(user, theChange)
+    updated <- knoraUserService.updateUser(
+                 user,
+                 changeRequest.username,
+                 changeRequest.email,
+                 changeRequest.givenName,
+                 changeRequest.familyName,
+                 changeRequest.lang,
+               )
     response <- asExternalUserResponseADM(requestingUser, updated)
   } yield response
 
@@ -177,7 +176,7 @@ final case class UsersRestService(
       _        <- ensureNotABuiltInUser(userIri)
       _        <- ensureSelfUpdateOrSystemAdmin(userIri, requestingUser)
       user     <- getKnoraUserOrNotFound(userIri)
-      updated  <- knoraUserService.updateUser(user, UserChangeRequest(status = Some(changeRequest.status)))
+      updated  <- knoraUserService.updateUserStatus(user, changeRequest.status)
       response <- asExternalUserResponseADM(requestingUser, updated)
     } yield response
 
@@ -190,8 +189,7 @@ final case class UsersRestService(
       _        <- ensureNotABuiltInUser(userIri)
       _        <- auth.ensureSystemAdmin(requestingUser)
       user     <- getKnoraUserOrNotFound(userIri)
-      theUpdate = UserChangeRequest(systemAdmin = Some(changeRequest.systemAdmin))
-      updated  <- knoraUserService.updateUser(user, theUpdate)
+      updated  <- knoraUserService.updateSystemAdminStatus(user, changeRequest.systemAdmin)
       response <- asExternalUserResponseADM(requestingUser, updated)
     } yield response
 

diff --git a/webapi/src/main/scala/org/knora/webapi/slice/admin/domain/service/KnoraUserService.scala b/webapi/src/main/scala/org/knora/webapi/slice/admin/domain/service/KnoraUserService.scala
@@ -31,30 +31,36 @@ import org.knora.webapi.slice.admin.domain.model.User
 import org.knora.webapi.slice.admin.domain.model.UserStatus
 import org.knora.webapi.slice.admin.domain.model.Username
 import org.knora.webapi.slice.admin.domain.service.KnoraUserService.Errors.UserServiceError
+import org.knora.webapi.slice.admin.domain.service.KnoraUserService.UserChangeRequest
 import org.knora.webapi.store.cache.CacheService
 
-final case class UserChangeRequest(
-  username: Option[Username] = None,
-  email: Option[Email] = None,
-  givenName: Option[GivenName] = None,
-  familyName: Option[FamilyName] = None,
-  status: Option[UserStatus] = None,
-  lang: Option[LanguageCode] = None,
-  passwordHash: Option[PasswordHash] = None,
-  projects: Option[Chunk[ProjectIri]] = None,
-  projectsAdmin: Option[Chunk[ProjectIri]] = None,
-  groups: Option[Chunk[GroupIri]] = None,
-  systemAdmin: Option[SystemAdmin] = None,
-)
-
 case class KnoraUserService(
   private val userRepo: KnoraUserRepo,
   private val iriService: IriService,
   private val passwordService: PasswordService,
   private val cacheService: CacheService,
 ) {
 
-  def updateUser(kUser: KnoraUser, update: UserChangeRequest): Task[KnoraUser] = {
+  def updateSystemAdminStatus(knoraUser: KnoraUser, status: SystemAdmin): Task[KnoraUser] =
+    updateUser(knoraUser, UserChangeRequest(systemAdmin = Some(status)))
+
+  def updateUserStatus(knoraUser: KnoraUser, status: UserStatus): Task[KnoraUser] =
+    updateUser(knoraUser, UserChangeRequest(status = Some(status)))
+
+  def updateUser(
+    knoraUser: KnoraUser,
+    username: Option[Username] = None,
+    email: Option[Email],
+    givenName: Option[GivenName] = None,
+    familyName: Option[FamilyName] = None,
+    lang: Option[LanguageCode] = None,
+  ): Task[KnoraUser] = {
+    val theUpdate =
+      UserChangeRequest(username = username, email = email, givenName = givenName, familyName = familyName, lang = lang)
+    updateUser(knoraUser, theUpdate)
+  }
+
+  private def updateUser(kUser: KnoraUser, update: UserChangeRequest): Task[KnoraUser] = {
     val updatedUser = kUser.copy(
       username = update.username.getOrElse(kUser.username),
       email = update.email.getOrElse(kUser.email),
@@ -210,6 +216,20 @@ case class KnoraUserService(
 }
 
 object KnoraUserService {
+  private final case class UserChangeRequest(
+    username: Option[Username] = None,
+    email: Option[Email] = None,
+    givenName: Option[GivenName] = None,
+    familyName: Option[FamilyName] = None,
+    status: Option[UserStatus] = None,
+    lang: Option[LanguageCode] = None,
+    passwordHash: Option[PasswordHash] = None,
+    projects: Option[Chunk[ProjectIri]] = None,
+    projectsAdmin: Option[Chunk[ProjectIri]] = None,
+    groups: Option[Chunk[GroupIri]] = None,
+    systemAdmin: Option[SystemAdmin] = None,
+  )
+
   object Errors {
     final case class UserServiceError(message: String)
   }