fix: add more access control to message conversations (#19139)

dhis2 · Nov 14, 2024 · c31ce96 · c31ce96
1 parent 5cb8e3b
commit c31ce96
Show file tree

Hide file tree

Showing 6 changed files with 430 additions and 115 deletions.
diff --git a/...s-service-core/src/main/java/org/hisp/dhis/configuration/DefaultConfigurationService.java b/...s-service-core/src/main/java/org/hisp/dhis/configuration/DefaultConfigurationService.java
@@ -92,7 +92,8 @@ public boolean isCorsWhitelisted(String origin) {
   @Transactional(readOnly = true)
   public boolean isUserInFeedbackRecipientUserGroup(UserDetails user) {
     UserGroup feedbackRecipients = getConfiguration().getFeedbackRecipients();
-    return feedbackRecipients != null
-        && user.getUserGroupIds().contains(feedbackRecipients.getUid());
+    if (feedbackRecipients == null) return false;
+    return feedbackRecipients.getMembers().stream()
+        .anyMatch(member -> member.getUid().equals(user.getUid()));
   }
 }
diff --git a/...services/dhis-service-core/src/main/java/org/hisp/dhis/message/DefaultMessageService.java b/...services/dhis-service-core/src/main/java/org/hisp/dhis/message/DefaultMessageService.java
@@ -450,9 +450,8 @@ public List<UserMessage> getLastRecipients(int first, int max) {
   @Override
   @Transactional(readOnly = true)
   public boolean hasAccessToManageFeedbackMessages(UserDetails userDetails) {
-    userDetails = (userDetails != null ? userDetails : CurrentUserUtil.getCurrentUserDetails());
-    return configurationService.isUserInFeedbackRecipientUserGroup(userDetails)
-        || userDetails.isAuthorized("ALL");
+    boolean isInGroup = configurationService.isUserInFeedbackRecipientUserGroup(userDetails);
+    return isInGroup || userDetails.isAuthorized("ALL");
   }
 
   // -------------------------------------------------------------------------

diff --git a/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/DhisConvenienceTest.java b/dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/DhisConvenienceTest.java
@@ -191,7 +191,6 @@
 import org.hisp.dhis.trackedentityfilter.EntityQueryCriteria;
 import org.hisp.dhis.trackedentityfilter.TrackedEntityFilter;
 import org.hisp.dhis.trackerdataview.TrackerDataView;
-import org.hisp.dhis.user.CurrentUserGroupInfo;
 import org.hisp.dhis.user.CurrentUserUtil;
 import org.hisp.dhis.user.User;
 import org.hisp.dhis.user.UserDetails;
@@ -2639,21 +2638,6 @@ protected void injectSecurityContextUser(User user) {
     }
 
     user = userService.getUser(user.getUid());
-
-    CurrentUserGroupInfo currentUserGroupInfo = userService.getCurrentUserGroupInfo(user.getUid());
-    if (user.getGroups().size() != currentUserGroupInfo.getUserGroupUIDs().size()) {
-      String msg =
-          String.format(
-              "User '%s' getGroups().size() has %d groups, but  getUserGroupUIDs() returns %d groups!",
-              user.getUsername(),
-              user.getGroups().size(),
-              currentUserGroupInfo.getUserGroupUIDs().size());
-
-      log.error(msg);
-
-      throw new RuntimeException(msg);
-    }
-
     UserDetails userDetails = userService.createUserDetails(user);
 
     injectSecurityContext(userDetails);

diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/DhisControllerTestBase.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/DhisControllerTestBase.java
@@ -72,6 +72,10 @@ public User getSuperUser() {
     return superUser;
   }
 
+  protected final String getAdminUid() {
+    return superUser.getUid();
+  }
+
   protected final User switchToSuperuser() {
     switchContextToUser(userService.getUser(superUser.getUid()));
     return superUser;

diff --git a/...-api/src/test/java/org/hisp/dhis/webapi/controller/MessageConversationControllerTest.java b/...-api/src/test/java/org/hisp/dhis/webapi/controller/MessageConversationControllerTest.java
@@ -27,11 +27,19 @@
  */
 package org.hisp.dhis.webapi.controller;
 
+import static com.google.common.collect.Sets.newHashSet;
 import static org.hisp.dhis.web.WebClientUtils.assertStatus;
 
+import org.hisp.dhis.common.IdentifiableObjectManager;
+import org.hisp.dhis.configuration.Configuration;
+import org.hisp.dhis.configuration.ConfigurationService;
+import org.hisp.dhis.user.User;
+import org.hisp.dhis.user.UserGroup;
 import org.hisp.dhis.web.HttpStatus;
 import org.hisp.dhis.webapi.DhisControllerConvenienceTest;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
 
 /**
  * Tests the {@link MessageConversationController} using (mocked) REST requests.
@@ -40,6 +48,9 @@
  */
 class MessageConversationControllerTest extends DhisControllerConvenienceTest {
 
+  @Autowired private ConfigurationService configurationService;
+  @Autowired private IdentifiableObjectManager manager;
+
   @Test
   void testPostJsonObject() {
     assertWebMessage(
@@ -135,4 +146,289 @@ void testPostMessageConversationFeedback() {
         POST("/messageConversations/feedback?subject=test", "The message")
             .content(HttpStatus.CREATED));
   }
+
+  @Test
+  @DisplayName("Change feedback message when in the recipient group")
+  void testChangeFeedbackMessage() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED, POST("/messageConversations/feedback?subject=test", "The message"));
+
+    User ringo = createUserWithAuth("ringo");
+    UserGroup ugA = createUserGroup('A', newHashSet(ringo));
+    manager.save(ugA);
+    Configuration config = configurationService.getConfiguration();
+    config.setFeedbackRecipients(ugA);
+    configurationService.setConfiguration(config);
+
+    switchContextToUser(ringo);
+
+    POST("/messageConversations/read?user=%s".formatted(ringo.getUid()), "['%s']".formatted(uid))
+        .content(HttpStatus.OK);
+  }
+
+  @Test
+  @DisplayName("Change feedback message when not in the recipient group")
+  void testChangeFeedbackMessageWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED, POST("/messageConversations/feedback?subject=test", "The message"));
+
+    User ringo = switchToNewUser("ringo");
+
+    POST("/messageConversations/read?user=%s".formatted(ringo.getUid()), "['%s']".formatted(uid))
+        .content(HttpStatus.FORBIDDEN);
+  }
+
+  @Test
+  @DisplayName("Post message to conversation without permission")
+  void testPostToConversationWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to send messages to this conversation.",
+        POST("/messageConversations/%s".formatted(uid), "{'text':'text'}")
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Add recipient without permission")
+  void testAddRecipientWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to change recipients in this conversation.",
+        POST(
+                "/messageConversations/%s/recipients".formatted(uid),
+                "{'users':[{'id':'%s'}],'userGroups':[],'organisationUnits':[]}"
+                    .formatted(ringo.getUid()))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Change priority without permission")
+  void testChangePriorityWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to change priority in this conversation.",
+        POST(
+                "/messageConversations/%s/priority?messageConversationPriority=HIGH".formatted(uid),
+                "{'users':[{'id':'%s'}],'userGroups':[],'organisationUnits':[]}"
+                    .formatted(ringo.getUid()))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Change status without permission")
+  void testChangeStatusWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to change status in this conversation.",
+        POST(
+                "/messageConversations/%s/status?messageConversationStatus=SOLVED".formatted(uid),
+                "{'users':[{'id':'%s'}],'userGroups':[],'organisationUnits':[]}"
+                    .formatted(ringo.getUid()))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Assign user without permission")
+  void testAssignUsersWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to assign a user to this conversation.",
+        POST(
+                "/messageConversations/%s/assign?userId=%s".formatted(uid, ringo.getUid()),
+                "{'users':[{'id':'%s'}],'userGroups':[],'organisationUnits':[]}"
+                    .formatted(ringo.getUid()))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Delete assigned user without permission")
+  void testDeleteAssignUsersWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to remove the assigned user to this conversation.",
+        DELETE(
+                "/messageConversations/%s/assign".formatted(uid),
+                "{'users':[{'id':'%s'}],'userGroups':[],'organisationUnits':[]}"
+                    .formatted(ringo.getUid()))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Change followup without permission")
+  void testChangeFollowupWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to change followups in this conversation.",
+        POST(
+                "/messageConversations/followup?userUid=%s".formatted(ringo.getUid()),
+                "['%s']".formatted(uid))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Change unfollowup without permission")
+  void testChangeUnFollowupWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to change unfollowups in this conversation.",
+        POST(
+                "/messageConversations/unfollowup?userUid=%s".formatted(ringo.getUid()),
+                "['%s']".formatted(uid))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Remove user from conversation without permission")
+  void testRemoveUserWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to remove assigned users to this conversation.",
+        DELETE("/messageConversations/%s/%s".formatted(uid, getAdminUid()))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Remove user from conversation without permission")
+  void testRemoveUsersBatchWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to remove assigned users to this conversation.",
+        DELETE("/messageConversations?mc=%s&user=%s".formatted(uid, getAdminUid()))
+            .content(HttpStatus.FORBIDDEN));
+  }
+
+  @Test
+  @DisplayName("Change read status without permission")
+  void testChangeReadStatusWithoutPermission() {
+    String uid =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/messageConversations/",
+                "{'subject':'Subject','text':'Text','users':[{'id':'" + getAdminUid() + "'}]}"));
+
+    User ringo = switchToNewUser("ringo");
+
+    assertWebMessage(
+        "Forbidden",
+        403,
+        "ERROR",
+        "Not authorized to change read property of this conversation.",
+        POST(
+                "/messageConversations/read?user=%s".formatted(ringo.getUid()),
+                "['%s']".formatted(uid))
+            .content(HttpStatus.FORBIDDEN));
+  }
 }