Merge pull request #15715 from dashevchenko/alarmCommentPermissionBug

Fixed permission check on alarm comment edit

application/src/main/java/org/thingsboard/server/controller/AlarmCommentController.java

@@ -31,6 +31,7 @@ import org.thingsboard.server.common.data.alarm.Alarm;
 import org.thingsboard.server.common.data.alarm.AlarmComment;
 import org.thingsboard.server.common.data.alarm.AlarmCommentInfo;
 import org.thingsboard.server.common.data.alarm.AlarmCommentType;
+import org.thingsboard.server.common.data.exception.ThingsboardErrorCode;
 import org.thingsboard.server.common.data.exception.ThingsboardException;
 import org.thingsboard.server.common.data.id.AlarmCommentId;
 import org.thingsboard.server.common.data.id.AlarmId;
@@ -39,6 +40,7 @@ import org.thingsboard.server.common.data.page.PageLink;
 import org.thingsboard.server.config.annotations.ApiOperation;
 import org.thingsboard.server.queue.util.TbCoreComponent;
 import org.thingsboard.server.service.entitiy.alarm.TbAlarmCommentService;
+import org.thingsboard.server.service.security.model.SecurityUser;
 import org.thingsboard.server.service.security.permission.Operation;
 
 import static org.thingsboard.server.controller.ControllerConstants.ALARM_COMMENT_ID_PARAM_DESCRIPTION;
@@ -77,9 +79,13 @@ public class AlarmCommentController extends BaseController {
         checkParameter(ALARM_ID, strAlarmId);
         AlarmId alarmId = new AlarmId(toUUID(strAlarmId));
         Alarm alarm = checkAlarmInfoId(alarmId, Operation.WRITE);
+        SecurityUser currentUser = getCurrentUser();
+        if (alarmComment.getId() != null) {
+            checkUserPermission(alarmComment, alarmId, "edit", currentUser);
+        }
         alarmComment.setAlarmId(alarmId);
         alarmComment.setType(AlarmCommentType.OTHER);
-        return tbAlarmCommentService.saveAlarmComment(alarm, alarmComment, getCurrentUser());
+        return tbAlarmCommentService.saveAlarmComment(alarm, alarmComment, currentUser);
     }
 
     @ApiOperation(value = "Delete Alarm comment (deleteAlarmComment)",
@@ -93,7 +99,11 @@ public class AlarmCommentController extends BaseController {
 
         AlarmCommentId alarmCommentId = new AlarmCommentId(toUUID(strCommentId));
         AlarmComment alarmComment = checkAlarmCommentId(alarmCommentId, alarmId);
-        tbAlarmCommentService.deleteAlarmComment(alarm, alarmComment, getCurrentUser());
+        SecurityUser currentUser = getCurrentUser();
+        if (!currentUser.isTenantAdmin()) {
+            checkUserPermission(alarmComment, alarmId, "delete", currentUser);
+        }
+        tbAlarmCommentService.deleteAlarmComment(alarm, alarmComment, currentUser);
     }
 
     @ApiOperation(value = "Get Alarm comments (getAlarmComments)",
@@ -120,4 +130,12 @@ public class AlarmCommentController extends BaseController {
         return checkNotNull(alarmCommentService.findAlarmComments(alarm.getTenantId(), alarmId, pageLink));
     }
 
+    private void checkUserPermission(AlarmComment alarmComment, AlarmId alarmId, String operation, SecurityUser currentUser) throws ThingsboardException {
+        AlarmComment existingAlarmComment = checkAlarmCommentId(alarmComment.getId(), alarmId);
+        if (existingAlarmComment.getUserId() != null && !existingAlarmComment.getUserId().equals(currentUser.getId())) {
+            throw new ThingsboardException("User is not allowed to " + operation + " other user's comment",
+                    ThingsboardErrorCode.PERMISSION_DENIED);
+        }
+    }
+
 }

application/src/main/java/org/thingsboard/server/service/entitiy/alarm/DefaultTbAlarmCommentService.java

@@ -60,7 +60,7 @@ public class DefaultTbAlarmCommentService extends AbstractTbEntityService implem
             alarmComment.setType(AlarmCommentType.SYSTEM);
             alarmComment.setUserId(null);
             alarmComment.setComment(JacksonUtil.newObjectNode().put("text",
-                    String.format("User %s deleted his comment",
+                    String.format("Comment was deleted by user %s",
                             (user.getFirstName() == null || user.getLastName() == null) ? user.getName() : user.getFirstName() + " " + user.getLastName())));
             AlarmComment savedAlarmComment = checkNotNull(alarmCommentService.saveAlarmComment(alarm.getTenantId(), alarmComment));
             logEntityActionService.logEntityAction(alarm.getTenantId(), alarm.getId(), alarm, alarm.getCustomerId(), ActionType.DELETED_COMMENT, user, savedAlarmComment);

application/src/test/java/org/thingsboard/server/controller/AbstractWebTest.java

@@ -210,6 +210,7 @@ public abstract class AbstractWebTest extends AbstractInMemoryStorageTest {
     private static final String DIFFERENT_TENANT_ADMIN_PASSWORD = "difftenant";
 
     protected static final String CUSTOMER_USER_EMAIL = "testcustomer@thingsboard.org";
+    protected static final String SECOND_CUSTOMER_USER_EMAIL = "testsecondcustomer@thingsboard.org";
     private static final String CUSTOMER_USER_PASSWORD = "customer";
 
     protected static final String DIFFERENT_CUSTOMER_USER_EMAIL = "testdifferentcustomer@thingsboard.org";
@@ -247,6 +248,7 @@ public abstract class AbstractWebTest extends AbstractInMemoryStorageTest {
 
     protected CustomerId differentTenantCustomerId;
     protected UserId customerUserId;
+    protected UserId secondCustomerUserId;
     protected UserId differentCustomerUserId;
 
     protected UserId differentTenantCustomerUserId;
@@ -372,9 +374,17 @@ public abstract class AbstractWebTest extends AbstractInMemoryStorageTest {
         customerUser.setCustomerId(savedCustomer.getId());
         customerUser.setEmail(CUSTOMER_USER_EMAIL);
 
-        customerUser = createUserAndLogin(customerUser, CUSTOMER_USER_PASSWORD);
+        customerUser = createUserAndActivate(customerUser, CUSTOMER_USER_PASSWORD);
         customerUserId = customerUser.getId();
 
+        User secondCustomerUser = new User();
+        secondCustomerUser.setAuthority(Authority.CUSTOMER_USER);
+        secondCustomerUser.setTenantId(tenantId);
+        secondCustomerUser.setCustomerId(customerId);
+        secondCustomerUser.setEmail(SECOND_CUSTOMER_USER_EMAIL);
+        secondCustomerUser = createUserAndActivate(secondCustomerUser, CUSTOMER_USER_PASSWORD);
+        secondCustomerUserId = secondCustomerUser.getId();
+
         resetTokens();
 
         log.debug("Executed web test setup");
@@ -472,6 +482,10 @@ public abstract class AbstractWebTest extends AbstractInMemoryStorageTest {
         login(CUSTOMER_USER_EMAIL, CUSTOMER_USER_PASSWORD);
     }
 
+    protected void loginSecondCustomerUser() throws Exception {
+        login(SECOND_CUSTOMER_USER_EMAIL, CUSTOMER_USER_PASSWORD);
+    }
+
     protected void loginUser(String userName, String password) throws Exception {
         login(userName, password);
     }
@@ -586,6 +600,13 @@ public abstract class AbstractWebTest extends AbstractInMemoryStorageTest {
         return savedUser;
     }
 
+    protected User createUserAndActivate(User user, String password) throws Exception {
+        User savedUser = doPost("/api/user", user, User.class);
+        JsonNode activateRequest = getActivateRequest(password);
+        doPost("/api/noauth/activate", activateRequest).andExpect(status().isOk());
+        return savedUser;
+    }
+
     protected User createUser(User user, String password) throws Exception {
         User savedUser = doPost("/api/user", user, User.class);
         JsonNode activateRequest = getActivateRequest(password);

application/src/test/java/org/thingsboard/server/controller/AlarmCommentControllerTest.java

@@ -160,6 +160,25 @@ public class AlarmCommentControllerTest extends AbstractControllerTest {
         testLogEntityActionEntityEqClass(alarm, alarm.getId(), tenantId, customerId, tenantAdminUserId, TENANT_ADMIN_EMAIL, ActionType.UPDATED_COMMENT, 1, updatedAlarmComment);
     }
 
+    @Test
+    public void testEditOthersAlarmCommentIsProhibited() throws Exception {
+        loginCustomerUser();
+        AlarmComment alarmComment = createAlarmComment(alarm.getId());
+
+        JsonNode newComment = JacksonUtil.newObjectNode().set("text", new TextNode("Second customer rewrite"));
+        alarmComment.setComment(newComment);
+
+        loginSecondCustomerUser();
+        doPost("/api/alarm/" + alarm.getId() + "/comment", alarmComment)
+                .andExpect(status().isForbidden())
+                .andExpect(statusReason(containsString("User is not allowed to edit other user's comment")));
+
+        loginTenantAdmin();
+        doPost("/api/alarm/" + alarm.getId() + "/comment", alarmComment)
+                .andExpect(status().isForbidden())
+                .andExpect(statusReason(containsString("User is not allowed to edit other user's comment")));
+    }
+
     @Test
     public void testUpdateAlarmViaDifferentTenant() throws Exception {
         loginTenantAdmin();
@@ -209,12 +228,36 @@ public class AlarmCommentControllerTest extends AbstractControllerTest {
         AlarmComment expectedAlarmComment = AlarmComment.builder()
                 .alarmId(alarm.getId())
                 .type(AlarmCommentType.SYSTEM)
-                .comment(JacksonUtil.newObjectNode().put("text", String.format("User %s deleted his comment",
+                .comment(JacksonUtil.newObjectNode().put("text", String.format("Comment was deleted by user %s",
                         CUSTOMER_USER_EMAIL)))
                 .build();
         testLogEntityActionEntityEqClass(alarm, alarm.getId(), tenantId, customerId, customerUserId, CUSTOMER_USER_EMAIL, ActionType.DELETED_COMMENT, 1, expectedAlarmComment);
     }
 
+    @Test
+    public void testDeleteOthersAlarmCommentIsAllowedForAuthorOrTenantAdmin() throws Exception {
+        loginCustomerUser();
+        AlarmComment alarmComment = createAlarmComment(alarm.getId());
+
+        loginSecondCustomerUser();
+        Mockito.reset(tbClusterService, auditLogService);
+
+        doDelete("/api/alarm/" + alarm.getId() + "/comment/" + alarmComment.getId())
+                .andExpect(status().isForbidden())
+                .andExpect(statusReason(containsString("User is not allowed to delete other user's comment")));
+
+        loginTenantAdmin();
+        doDelete("/api/alarm/" + alarm.getId() + "/comment/" + alarmComment.getId())
+                .andExpect(status().isOk());
+        AlarmComment expectedAlarmComment = AlarmComment.builder()
+                .alarmId(alarm.getId())
+                .type(AlarmCommentType.SYSTEM)
+                .comment(JacksonUtil.newObjectNode().put("text", String.format("Comment was deleted by user %s",
+                        TENANT_ADMIN_EMAIL)))
+                .build();
+        testLogEntityActionEntityEqClass(alarm, alarm.getId(), tenantId, customerId, tenantAdminUserId, TENANT_ADMIN_EMAIL, ActionType.DELETED_COMMENT, 1, expectedAlarmComment);
+    }
+
     @Test
     public void testDeleteAlarmViaTenant() throws Exception {
         loginTenantAdmin();
@@ -234,13 +277,13 @@ public class AlarmCommentControllerTest extends AbstractControllerTest {
 
         assertThat(systemComment.getId()).isEqualTo(alarmComment.getId());
         assertThat(systemComment.getType()).isEqualTo(AlarmCommentType.SYSTEM);
-        assertThat(systemComment.getComment().get("text").asText()).isEqualTo(String.format("User %s deleted his comment",
+        assertThat(systemComment.getComment().get("text").asText()).isEqualTo(String.format("Comment was deleted by user %s",
                 TENANT_ADMIN_EMAIL));
 
         AlarmComment expectedAlarmComment = AlarmComment.builder()
                 .alarmId(alarm.getId())
                 .type(AlarmCommentType.SYSTEM)
-                .comment(JacksonUtil.newObjectNode().put("text", String.format("User %s deleted his comment",
+                .comment(JacksonUtil.newObjectNode().put("text", String.format("Comment was deleted by user %s",
                         TENANT_ADMIN_EMAIL)))
                 .build();
         testLogEntityActionEntityEqClass(alarm, alarm.getId(), tenantId, customerId, tenantAdminUserId, TENANT_ADMIN_EMAIL, ActionType.DELETED_COMMENT, 1, expectedAlarmComment);