fix: User can be project admin without being project member (DEV-1383) (

#2248) Co-authored-by: Marcin Procyk <marcin.procyk@dasch.swiss>
dasch-swiss · Oct 18, 2022 · c1aa8f0 · c1aa8f0
1 parent 3f40fe3
commit c1aa8f0
Show file tree

Hide file tree

Showing 10 changed files with 231 additions and 56 deletions.
diff --git a/Makefile b/Makefile
@@ -331,8 +331,10 @@ clean-metals: ## clean SBT and Metals related stuff
 	@rm -rf .bsp
 	@rm -rf .metals
 	@rm -rf target
+	@sbt "clean"
 
-clean: docs-clean clean-local-tmp clean-docker clean-sipi-tmp clean-sipi-projects ## clean build artifacts
+
+clean: docs-clean clean-local-tmp clean-docker clean-sipi-tmp ## clean build artifacts
 	@rm -rf .env
 
 .PHONY: clean-sipi-tmp

diff --git a/docs/03-apis/api-admin/index.md b/docs/03-apis/api-admin/index.md
@@ -3,9 +3,9 @@
  * SPDX-License-Identifier: Apache-2.0
 -->
 
-# Knora Admin API
+# DSP Admin API
 
-The Knora admin API makes it possible to administer Knora projects, users, user groups, permissions, and hierarchical lists.
+The DSP Admin API makes it possible to administer projects, users, user groups, permissions, and hierarchical lists.
 
 - [Introduction](introduction.md)
 - [Overview](overview.md)

diff --git a/docs/03-apis/api-admin/users.md b/docs/03-apis/api-admin/users.md
@@ -130,8 +130,7 @@ specified by the `id` in the request body as below:
 ### Delete user
 
   - Required permission: SystemAdmin / self
-  - Remark: The same as updating a user and changing `status` to
-    `false`. To un-delete, set `status` to `true`.
+  - Remark: The same as updating a user and changing `status` to `false`. To un-delete, set `status` to `true`.
   - PUT: `/admin/users/iri/<userIri>/Status`
   - BODY:
 
@@ -144,8 +143,7 @@ specified by the `id` in the request body as below:
 ### Delete user (-\update user)**
 
   - Required permission: SystemAdmin / self
-  - Remark: The same as updating a user and changing `status` to
-    `false`. To un-delete, set `status` to `true`.
+  - Remark: The same as updating a user and changing `status` to `false`. To un-delete, set `status` to `true`.
   - DELETE: `/admin/users/iri/<userIri>`
   - BODY: empty
 
@@ -158,13 +156,14 @@ specified by the `id` in the request body as below:
 
 ### Add/remove user to/from project
 
-  - Required permission: SystemAdmin / ProjectAdmin / self (if
-    project self-assignment is enabled)
+  - Required permission: SystemAdmin / ProjectAdmin / self (if project self-assignment is enabled)
   - Required information: project IRI, user IRI
   - Effects: `knora-base:isInProject` user property
   - POST / DELETE: `/admin/users/iri/<userIri>/project-memberships/<projectIri>`
   - BODY: empty
 
+Note: When a user is project admin in the same project, his project admin membership will be removed as well.
+
 ## User's group membership operations
 
 ### Get user's project admin memberships
@@ -179,6 +178,8 @@ specified by the `id` in the request body as below:
   - POST / DELETE: `/admin/users/iri/<userIri>/project-admin-memberships/<projectIri>`
   - BODY: empty
 
+Note: In order to add a user to a project admin group, the user needs to be member of that project.
+
 ### Get user's group memberships**
 
   - GET: `/admin/users/iri/<userIri>/group-memberships`

diff --git a/docs/05-internals/design/principles/design-overview.md b/docs/05-internals/design/principles/design-overview.md
@@ -29,7 +29,7 @@ DSP-API supports different versions of its API for working with humanities data:
 - [DSP-API v1](../../../03-apis/api-v1/index.md), legacy API compatibile with applications 
   that used the prototype software.
 
-There is also a [Knora admin API](../../../03-apis/api-admin/index.md) for
+There is also an [Admin API](../../../03-apis/api-admin/index.md) for
 administering DSP projects.
 
 The DSP-API code base includes some functionality that is shared by these different

diff --git a/...main/scala/org/knora/webapi/messages/admin/responder/usersmessages/UsersMessagesADM.scala b/...main/scala/org/knora/webapi/messages/admin/responder/usersmessages/UsersMessagesADM.scala
@@ -837,13 +837,16 @@ case class UserChangeRequestADM(
     throw BadRequestException("Too many parameters sent for system admin membership change.")
   }
 
-  // change project memberships
-  if (projects.isDefined && parametersCount > 1) {
+  // change project memberships (could also involve changing projectAdmin memberships)
+  if (
+    projects.isDefined && projectsAdmin.isDefined && parametersCount > 2 ||
+    projects.isDefined && !projectsAdmin.isDefined && parametersCount > 1
+  ) {
     throw BadRequestException("Too many parameters sent for project membership change.")
   }
 
-  // change projectAdmin memberships
-  if (projectsAdmin.isDefined && parametersCount > 1) {
+  // change projectAdmin memberships only (without changing project memberships)
+  if (projectsAdmin.isDefined && !projects.isDefined && parametersCount > 1) {
     throw BadRequestException("Too many parameters sent for projectAdmin membership change.")
   }
 

diff --git a/webapi/src/main/scala/org/knora/webapi/responders/admin/UsersResponderADM.scala b/webapi/src/main/scala/org/knora/webapi/responders/admin/UsersResponderADM.scala
@@ -694,7 +694,6 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *
    * @param userIri              the user's IRI.
    * @param projectIri           the project's IRI.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
    * @return
@@ -779,7 +778,6 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *
    * @param userIri              the user's IRI.
    * @param projectIri           the project's IRI.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
    * @return
@@ -826,7 +824,7 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
                                      )
         currentProjectMembershipIris = currentProjectMemberships.map(_.id)
 
-        // check if user is not already a member and if he is then remove the project from to list
+        // check if user is a member and if he is then remove the project from to list
         updatedProjectMembershipIris =
           if (currentProjectMembershipIris.contains(projectIri)) {
             currentProjectMembershipIris diff Seq(projectIri)
@@ -836,10 +834,29 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
             )
           }
 
+        // get users current project admin membership list
+        currentProjectAdminMemberships <- userProjectAdminMembershipsGetADM(
+                                            userIri = userIri,
+                                            requestingUser = KnoraSystemInstances.Users.SystemUser,
+                                            apiRequestID = apiRequestID
+                                          )
+
+        currentProjectAdminMembershipIris: Seq[IRI] = currentProjectAdminMemberships.map(_.id)
+
+        // in case the user has an admin membership for that project, remove it as well
+        maybeUpdatedProjectAdminMembershipIris = if (currentProjectAdminMembershipIris.contains(projectIri)) {
+                                                   Some(
+                                                     currentProjectAdminMembershipIris.filterNot(p => p == projectIri)
+                                                   )
+                                                 } else None
+
         // create the update request by using the SystemUser
         result <- updateUserADM(
                     userIri = userIri,
-                    userUpdatePayload = UserChangeRequestADM(projects = Some(updatedProjectMembershipIris)),
+                    userUpdatePayload = UserChangeRequestADM(
+                      projects = Some(updatedProjectMembershipIris),
+                      projectsAdmin = maybeUpdatedProjectAdminMembershipIris
+                    ),
                     requestingUser = KnoraSystemInstances.Users.SystemUser,
                     apiRequestID = apiRequestID
                   )
@@ -859,7 +876,6 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    * Returns the user's project admin group memberships as a sequence of [[IRI]]
    *
    * @param userIri              the user's IRI.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
    * @return a [[UserProjectMembershipsGetResponseV1]].
@@ -912,7 +928,6 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    * is a member of the project admin group.
    *
    * @param userIri              the user's IRI.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
    * @return a [[UserProjectMembershipsGetResponseV1]].
@@ -942,10 +957,9 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *
    * @param userIri              the user's IRI.
    * @param projectIri           the project's IRI.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
-   * @return
+   * @return a [[UserOperationResponseADM]].
    */
   private def userProjectAdminMembershipAddRequestADM(
     userIri: IRI,
@@ -983,6 +997,22 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
         _              = if (!projectExists) throw NotFoundException(s"The project $projectIri does not exist.")
 
         // get users current project membership list
+        currentProjectMemberships <- userProjectMembershipsGetADM(
+                                       userIri = userIri,
+                                       requestingUser = KnoraSystemInstances.Users.SystemUser
+                                     )
+
+        currentProjectMembershipIris = currentProjectMemberships.map(_.id)
+
+        // check if user is already project member and if not throw exception
+
+        _ = if (!currentProjectMembershipIris.contains(projectIri)) {
+              throw BadRequestException(
+                s"User $userIri is not a member of project $projectIri. A user needs to be a member of the project to be added as project admin."
+              )
+            }
+
+        // get users current project admin membership list
         currentProjectAdminMemberships <- userProjectAdminMembershipsGetADM(
                                             userIri = userIri,
                                             requestingUser = KnoraSystemInstances.Users.SystemUser,
@@ -991,7 +1021,7 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
 
         currentProjectAdminMembershipIris: Seq[IRI] = currentProjectAdminMemberships.map(_.id)
 
-        // check if user is already member and if not then append to list
+        // check if user is already project admin and if not then append to list
         updatedProjectAdminMembershipIris =
           if (!currentProjectAdminMembershipIris.contains(projectIri)) {
             currentProjectAdminMembershipIris :+ projectIri
@@ -1026,10 +1056,9 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *
    * @param userIri              the user's IRI.
    * @param projectIri           the project's IRI.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
-   * @return
+   * @return a [[UserOperationResponseADM]]
    */
   private def userProjectAdminMembershipRemoveRequestADM(
     userIri: IRI,
@@ -1109,7 +1138,6 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    * Returns the user's group memberships as a sequence of [[GroupADM]]
    *
    * @param userIri              the IRI of the user.
-   *
    * @param requestingUser       the requesting user.
    * @return a sequence of [[GroupADM]].
    */
@@ -1162,7 +1190,6 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *
    * @param userIri              the user's IRI.
    * @param groupIri             the group IRI.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
    * @return a [[UserOperationResponseADM]].
@@ -1255,6 +1282,15 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
 
   }
 
+  /**
+   * Removes a user from a group.
+   *
+   * @param userIri              the user's IRI.
+   * @param groupIri             the group IRI.
+   * @param requestingUser       the requesting user.
+   * @param apiRequestID         the unique api request ID.
+   * @return a [[UserOperationResponseADM]].
+   */
   private def userGroupMembershipRemoveRequestADM(
     userIri: IRI,
     groupIri: IRI,
@@ -1343,10 +1379,9 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *
    * @param userIri              the IRI of the existing user that we want to update.
    * @param userUpdatePayload    the updated information.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
-   * @return a future containing a [[UserOperationResponseADM]].
+   * @return a [[UserOperationResponseADM]].
    * @throws BadRequestException         if necessary parameters are not supplied.
    * @throws UpdateNotPerformedException if the update was not performed.
    */
@@ -1558,10 +1593,9 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *
    * @param userIri              the IRI of the existing user that we want to update.
    * @param password             the new password.
-   *
    * @param requestingUser       the requesting user.
    * @param apiRequestID         the unique api request ID.
-   * @return a future containing a [[UserOperationResponseADM]].
+   * @return a [[UserOperationResponseADM]].
    * @throws BadRequestException         if necessary parameters are not supplied.
    * @throws UpdateNotPerformedException if the update was not performed.
    */
@@ -1636,9 +1670,9 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    *                     - http://blog.ircmaxell.com/2012/12/seven-ways-to-screw-up-bcrypt.html
    *
    * @param userCreatePayloadADM    a [[UserCreatePayloadADM]] object containing information about the new user to be created.
-   *
-   * @param requestingUser       a [[UserADM]] object containing information about the requesting user.
-   * @return a future containing the [[UserOperationResponseADM]].
+   * @param requestingUser          a [[UserADM]] object containing information about the requesting user.
+   * @param apiRequestID         the unique api request ID.
+   * @return a [[UserOperationResponseADM]].
    */
   private def createNewUserADM(
     userCreatePayloadADM: UserCreatePayloadADM,
@@ -1761,10 +1795,13 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
   /**
    * Tries to retrieve a [[UserADM]] either from triplestore or cache if caching is enabled.
    * If user is not found in cache but in triplestore, then user is written to cache.
+   *
+   * @param identifier The identifier of the user (can be IRI, e-mail or username)
+   * @return a [[Option[UserADM]]]
    */
   private def getUserFromCacheOrTriplestore(
     identifier: UserIdentifierADM
-  ): Future[Option[UserADM]] = tracedFuture("admin-user-get-user-from-cache-or-triplestore") {
+  ): Future[Option[UserADM]] = // tracedFuture("admin-user-get-user-from-cache-or-triplestore") {
     if (cacheServiceSettings.cacheServiceEnabled) {
       // caching enabled
       getUserFromCache(identifier).flatMap {
@@ -1793,10 +1830,13 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
       log.debug("getUserFromCacheOrTriplestore - caching disabled. getting from triplestore.")
       getUserFromTriplestore(identifier = identifier)
     }
-  }
+  // }
 
   /**
    * Tries to retrieve a [[UserADM]] from the triplestore.
+   *
+   * @param identifier The identifier of the user (can be IRI, e-mail or username)
+   * @return a [[Option[UserADM]]]
    */
   private def getUserFromTriplestore(
     identifier: UserIdentifierADM
@@ -1836,8 +1876,7 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    * Helper method used to create a [[UserADM]] from the [[SparqlExtendedConstructResponse]] containing user data.
    *
    * @param statements           result from the SPARQL query containing user data.
-   *
-   * @return a [[UserADM]] containing the user's data.
+   * @return a [[Option[UserADM]]]
    */
   private def statements2UserADM(
     statements: (SubjectV2, Map[SmartIri, Seq[LiteralV2]])
@@ -2114,6 +2153,9 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
 
   /**
    * Tries to retrieve a [[UserADM]] from the cache.
+   *
+   * @param identifier the user's identifier (could be IRI, e-mail or username)
+   * @return a [[Option[UserADM]]]
    */
   private def getUserFromCache(identifier: UserIdentifierADM): Future[Option[UserADM]] =
     tracedFuture("admin-user-get-user-from-cache") {
@@ -2132,7 +2174,7 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
    * Writes the user profile to cache.
    *
    * @param user a [[UserADM]].
-   * @return true if writing was successful.
+   * @return Unit
    * @throws ApplicationCacheException when there is a problem with writing the user's profile to cache.
    */
   private def writeUserADMToCache(user: UserADM): Future[Unit] = for {
@@ -2142,6 +2184,9 @@ class UsersResponderADM(responderData: ResponderData) extends Responder(responde
 
   /**
    * Removes the user from cache.
+   *
+   * @param maybeUser the optional user which is removed from the cache
+   * @return a [[Unit]]
    */
   private def invalidateCachedUserADM(maybeUser: Option[UserADM]): Future[Unit] =
     if (cacheServiceSettings.cacheServiceEnabled) {

diff --git a/webapi/src/main/scala/org/knora/webapi/routing/HealthRoute.scala b/webapi/src/main/scala/org/knora/webapi/routing/HealthRoute.scala
@@ -113,7 +113,7 @@ final case class HealthRoute(routeData: KnoraRouteData, runtime: Runtime[State])
       get { requestContext =>
         val res: ZIO[State, Nothing, HttpResponse] = {
           for {
-            _     <- ZIO.logInfo("health route start")
+            _     <- ZIO.logDebug("health route start")
             ec    <- ZIO.executor.map(_.asExecutionContext)
             state <- ZIO.service[State]
             requestingUser <-
@@ -123,7 +123,7 @@ final case class HealthRoute(routeData: KnoraRouteData, runtime: Runtime[State])
                 )
                 .orElse(ZIO.succeed(KnoraSystemInstances.Users.AnonymousUser))
             result <- healthCheck(state)
-            _      <- ZIO.logInfo("health route finished") @@ ZIOAspect.annotated("user-id", requestingUser.id.toString())
+            _      <- ZIO.logDebug("health route finished") @@ ZIOAspect.annotated("user-id", requestingUser.id.toString())
           } yield result
         } @@ LogAspect.logSpan("health-request") @@ LogAspect.logAnnotateCorrelationId(requestContext.request)