refactoring

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

@@ -92,7 +92,6 @@ import static org.thingsboard.server.controller.ControllerConstants.UUID_WIKI_LI
 public class UserController extends BaseController {
 
     public static final String USER_ID = "userId";
-    public static final String PATH = "path";
     public static final String PATHS = "paths";
     public static final String YOU_DON_T_HAVE_PERMISSION_TO_PERFORM_THIS_OPERATION = "You don't have permission to perform this operation!";
     public static final String ACTIVATE_URL_PATTERN = "%s/api/noauth/activate?activateToken=%s";
@@ -403,8 +402,8 @@ public class UserController extends BaseController {
 
     @ApiOperation(value = "Update user settings (saveUserSettings)",
             notes = "Update user settings for authorized user. Only specified json elements will be updated." +
-                    "Example: you have such settings: {A:5, B:{C:10, D:5}}. Updating it with {A:10, E:6} will result in" +
+                    "Example: you have such settings: {A:5, B:{C:10, D:20}}. Updating it with {B:{C:10, D:30}} will result in" +
-                    "{A:10, B:{C:10, D:5}}, E:6")
+                    "{A:5, B:{C:10, D:30}}. The same could be achieved by putting {B.D:30}")
     @PreAuthorize("hasAnyAuthority('SYS_ADMIN', 'TENANT_ADMIN', 'CUSTOMER_USER')")
     @PutMapping(value = "/user/settings")
     public void putUserSettings(@RequestBody JsonNode settings) throws ThingsboardException {
@@ -412,20 +411,8 @@ public class UserController extends BaseController {
         userSettingsService.updateUserSettings(currentUser.getTenantId(), currentUser.getId(), settings);
     }
 
-    @ApiOperation(value = "Update user settings (saveUserSettings)",
-            notes = "Update user settings for authorized user. Only specified json elements will be updated." +
-                    "Example: you have such settings: {A:5, B:{C:10, D:5}}. Updating it with {A:10, E:6} will result in" +
-                    "{A:10, B:{C:10, D:5}}, E:6")
-    @PreAuthorize("hasAnyAuthority('SYS_ADMIN', 'TENANT_ADMIN', 'CUSTOMER_USER')")
-    @PutMapping(value = "/user/settings/{path}")
-    public void putUserSettings(@ApiParam(value = PATH)
-                                    @PathVariable(PATH) String path, @RequestBody JsonNode settings) throws ThingsboardException {
-        SecurityUser currentUser = getCurrentUser();
-        userSettingsService.updateUserSettings(currentUser.getTenantId(), currentUser.getId(), path, settings);
-    }
-
     @ApiOperation(value = "Get user settings (getUserSettings)",
-            notes = "Fetch the User settings based on the provided User Id. " )
+            notes = "Fetch the User settings based on authorized user. " )
     @PreAuthorize("hasAnyAuthority('SYS_ADMIN', 'TENANT_ADMIN', 'CUSTOMER_USER')")
     @GetMapping(value = "/user/settings")
     public JsonNode getUserSettings() throws ThingsboardException {

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

@@ -29,7 +29,6 @@ import org.springframework.context.annotation.Primary;
 import org.springframework.http.HttpHeaders;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.web.servlet.ResultActions;
-import org.thingsboard.common.util.JacksonUtil;
 import org.thingsboard.server.common.data.Customer;
 import org.thingsboard.server.common.data.StringUtils;
 import org.thingsboard.server.common.data.Tenant;
@@ -41,7 +40,6 @@ import org.thingsboard.server.common.data.id.UserId;
 import org.thingsboard.server.common.data.page.PageData;
 import org.thingsboard.server.common.data.page.PageLink;
 import org.thingsboard.server.common.data.security.Authority;
-import org.thingsboard.server.common.data.security.UserSettings;
 import org.thingsboard.server.dao.exception.DataValidationException;
 import org.thingsboard.server.dao.user.UserDao;
 import org.thingsboard.server.service.mail.TestMailService;
@@ -755,39 +753,62 @@ public abstract class BaseUserControllerTest extends AbstractControllerTest {
         Assert.assertEquals(retrievedSettings, userSettings);
    }
 
+    @Test
+    public void testShouldNotSaveJsonWithRestrictedSymbols() throws Exception {
+        loginCustomerUser();
+
+        JsonNode userSettings = mapper.readTree("{\"A.B\":5, \"E\":18}");
+        doPost("/api/user/settings", userSettings).andExpect(status().isBadRequest());
+
+        userSettings = mapper.readTree("{\"A,B\":5, \"E\":18}");
+        doPost("/api/user/settings", userSettings).andExpect(status().isBadRequest());
+    }
+
     @Test
     public void testUpdateUserSettings() throws Exception {
         loginCustomerUser();
 
-        JsonNode userSettings = mapper.readTree("{\"A\":5, \"B\":{\"C\":5, \"D\":5}}");
+        JsonNode userSettings = mapper.readTree("{\"A\":5, \"B\":{\"C\":true, \"D\":\"stringValue\"}}");
         JsonNode savedSettings = doPost("/api/user/settings", userSettings, JsonNode.class);
         Assert.assertEquals(userSettings, savedSettings);
 
         JsonNode newSettings = mapper.readTree("{\"A\":10}");
         doPut("/api/user/settings", newSettings);
         JsonNode updatedSettings = doGet("/api/user/settings", JsonNode.class);
-        JsonNode expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"C\":5, \"D\":5}}");
+        JsonNode expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"C\":true, \"D\":\"stringValue\"}}");
         Assert.assertEquals(expectedSettings, updatedSettings);
 
-        JsonNode patchedSettings = mapper.readTree("{\"B\":{\"E\": 22}}");
+        JsonNode patchedSettings = mapper.readTree("{\"B\":{\"C\":false, \"D\":\"stringValue2\"}}");
         doPut("/api/user/settings", patchedSettings);
         updatedSettings = doGet("/api/user/settings", JsonNode.class);
-        expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"E\": 22}}");
+        expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"C\":false, \"D\":\"stringValue2\"}}");
         Assert.assertEquals(expectedSettings, updatedSettings);
 
-        patchedSettings = mapper.readTree("{\"I\": 56}");
+        patchedSettings = mapper.readTree("{\"B.D\": {\"E\": 56}}");
-        doPut("/api/user/settings/B.E", patchedSettings);
+        doPut("/api/user/settings", patchedSettings);
         updatedSettings = doGet("/api/user/settings", JsonNode.class);
-        expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"E\": {\"I\":56}}}");
+        expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"C\":false, \"D\": {\"E\": 56}}}");
         Assert.assertEquals(expectedSettings, updatedSettings);
 
-        patchedSettings = mapper.readTree("{\"I\": 76, \"F\": 92}");
+        patchedSettings = mapper.readTree("{\"B.D\": {\"E\": 76, \"F\": 92}}");
-        doPut("/api/user/settings/B.E", patchedSettings);
+        doPut("/api/user/settings", patchedSettings);
         updatedSettings = doGet("/api/user/settings", JsonNode.class);
-        expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"E\": {\"I\":76, \"F\": 92}}}");
+        expectedSettings = mapper.readTree("{\"A\":10, \"B\":{\"C\":false, \"D\": {\"E\":76, \"F\": 92}}}");
         Assert.assertEquals(expectedSettings, updatedSettings);
     }
 
+    @Test
+    public void testShouldNotUpdateUserSettingsWithNoExistingPath() throws Exception {
+        loginCustomerUser();
+
+        JsonNode userSettings = mapper.readTree("{\"A\":5, \"B\":{\"C\":true, \"D\":\"stringValue\"}}");
+        JsonNode savedSettings = doPost("/api/user/settings", userSettings, JsonNode.class);
+        Assert.assertEquals(userSettings, savedSettings);
+
+        JsonNode newSettings = mapper.readTree("{\"A.E\":10}");
+        doPut("/api/user/settings", newSettings).andExpect(status().isBadRequest());
+    }
+
     @Test
     public void testDeleteUserSettings() throws Exception {
         loginCustomerUser();

common/dao-api/src/main/java/org/thingsboard/server/dao/user/UserSettingsService.java

@@ -15,7 +15,6 @@
  */
 package org.thingsboard.server.dao.user;
 
-import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.JsonNode;
 import org.thingsboard.server.common.data.id.TenantId;
 import org.thingsboard.server.common.data.id.UserId;
@@ -26,7 +25,6 @@ import java.util.List;
 public interface UserSettingsService {
 
     void updateUserSettings(TenantId tenantId, UserId userId, JsonNode settings);
-    void updateUserSettings(TenantId tenantId, UserId userId, String path, JsonNode settings);
 
     UserSettings saveUserSettings(TenantId tenantId, UserSettings userSettings);
 

dao/src/main/java/org/thingsboard/server/dao/user/UserSettingsServiceImpl.java

@@ -20,8 +20,10 @@ import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.github.fge.jackson.NodeType;
 import com.jayway.jsonpath.DocumentContext;
 import com.jayway.jsonpath.JsonPath;
+import com.jayway.jsonpath.PathNotFoundException;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.stereotype.Service;
@@ -31,6 +33,7 @@ import org.thingsboard.server.common.data.id.TenantId;
 import org.thingsboard.server.common.data.id.UserId;
 import org.thingsboard.server.common.data.security.UserSettings;
 import org.thingsboard.server.dao.entity.AbstractCachedService;
+import org.thingsboard.server.dao.exception.DataValidationException;
 
 import java.util.ArrayList;
 import java.util.Iterator;
@@ -67,26 +70,6 @@ public class UserSettingsServiceImpl extends AbstractCachedService<UserId, UserS
         doSaveUserSettings(tenantId, newUserSettings);
     }
 
-    @Override
-    public void updateUserSettings(TenantId tenantId, UserId userId, String path, JsonNode settings) {
-        log.trace("Executing updateUserSettings for user [{}], [{}]", userId, settings);
-        validateId(userId, INCORRECT_USER_ID + userId);
-        UserSettings oldSettings = userSettingsDao.findById(tenantId, userId);
-
-        UserSettings newUserSettings = new UserSettings();
-        newUserSettings.setUserId(userId);
-
-        JsonNode oldSettingsJson = oldSettings != null ? oldSettings.getSettings() : JacksonUtil.newObjectNode();
-        DocumentContext dcSettings = JsonPath.parse(oldSettingsJson.toString());
-        dcSettings = dcSettings.set("$." + path, new ObjectMapper().convertValue(settings, new TypeReference<Map<String, Object>>(){}));
-        try {
-            newUserSettings.setSettings(new ObjectMapper().readValue(dcSettings.jsonString(), ObjectNode.class));
-        } catch (JsonProcessingException e) {
-            throw new RuntimeException(e);
-        }
-        doSaveUserSettings(tenantId, newUserSettings);
-    }
-
     @Override
     public UserSettings findUserSettings(TenantId tenantId, UserId userId) {
         log.trace("Executing findUserSettings for user [{}]", userId);
@@ -119,7 +102,7 @@ public class UserSettingsServiceImpl extends AbstractCachedService<UserId, UserS
 
     private UserSettings doSaveUserSettings(TenantId tenantId, UserSettings userSettings) {
         try {
-            //TODO: add validation for "." and ",";
+            validateJsonKeys(userSettings.getSettings());
             UserSettings saved = userSettingsDao.save(tenantId, userSettings);
             publishEvictEvent(new UserSettingsEvictEvent(userSettings.getUserId()));
             return saved;
@@ -137,14 +120,58 @@ public class UserSettingsServiceImpl extends AbstractCachedService<UserId, UserS
         cache.evict(keys);
     }
 
+    private void validateJsonKeys(JsonNode userSettings) {
+        Iterator<String> fieldNames = userSettings.fieldNames();
+        while (fieldNames.hasNext()) {
+            String fieldName = fieldNames.next();
+            if (fieldName.contains(".") || fieldName.contains(",")) {
+                throw new DataValidationException("Json field name should not contain \".\" or \",\" symbols");
+            }
+        }
+    }
+
     public JsonNode update(JsonNode mainNode, JsonNode updateNode) {
+        DocumentContext dcOldSettings = JsonPath.parse(mainNode.toString());
         Iterator<String> fieldNames = updateNode.fieldNames();
         while (fieldNames.hasNext()) {
             String fieldName = fieldNames.next();
-            JsonNode value = updateNode.get(fieldName);
+            validatePathExists(dcOldSettings, fieldName);
-            ((ObjectNode) mainNode).set(fieldName, value);
+            dcOldSettings = dcOldSettings.set("$." + fieldName, getValueByNodeType(updateNode.get(fieldName)));
+        }
+        try {
+            return new ObjectMapper().readValue(dcOldSettings.jsonString(), ObjectNode.class);
+        } catch (JsonProcessingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    private static void validatePathExists(DocumentContext dcOldSettings, String fieldName) {
+        try {
+            dcOldSettings.read("$." + fieldName);
+        }catch (PathNotFoundException e) {
+            throw new DataValidationException("Json element with path " + fieldName + "was not found");
+        }
+    }
+
+    private static Object getValueByNodeType(final JsonNode value)
+    {
+        final NodeType type = NodeType.getNodeType(value);
+        switch (type) {
+            case STRING:
+                return value.textValue();
+            case NUMBER:
+            case INTEGER:
+                return value.bigIntegerValue();
+            case NULL:
+            case ARRAY:
+                return value;
+            case OBJECT:
+                return new ObjectMapper().convertValue(value, new TypeReference<Map<String, Object>>() {});
+            case BOOLEAN:
+                return value.booleanValue();
+            default:
+                throw new UnsupportedOperationException();
         }
-        return mainNode;
     }
 
 }