Enhancement: validation (#1869)

* Enhancement: validates http parameters using javax.validation api

* remove unnecessary validation

apollo-common/src/main/java/com/ctrip/framework/apollo/common/controller/GlobalDefaultExceptionHandler.java

@@ -13,6 +13,7 @@ import java.util.Map;
 import java.util.Optional;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
+import javax.validation.ConstraintViolationException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.slf4j.event.Level;
@@ -84,6 +85,13 @@ public class GlobalDefaultExceptionHandler {
     return handleError(request, BAD_REQUEST, ex);
   }
 
+  @ExceptionHandler(ConstraintViolationException.class)
+  public ResponseEntity<Map<String, Object>> handleConstraintViolationException(
+      HttpServletRequest request, ConstraintViolationException ex
+  ) {
+    return handleError(request, BAD_REQUEST, new BadRequestException(ex.getMessage()));
+  }
+
   private ResponseEntity<Map<String, Object>> handleError(HttpServletRequest request,
                                                           HttpStatus status, Throwable ex) {
     return handleError(request, status, ex, ERROR);

apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/RequestPrecondition.java

@@ -11,9 +11,6 @@ public class RequestPrecondition {
 
   private static String ILLEGAL_MODEL = "request model is invalid";
 
-  private static String ILLEGAL_NUMBER = "number should be positive";
-
-
   public static void checkArgumentsNotEmpty(String... args) {
     checkArguments(!StringUtils.isContainEmpty(args), CONTAIN_EMPTY_ARGUMENT);
   }
@@ -27,31 +24,4 @@ public class RequestPrecondition {
       throw new BadRequestException(String.valueOf(errorMessage));
     }
   }
-
-  public static void checkNumberPositive(int... args){
-    for (int num: args){
-      if (num <= 0){
-        throw new BadRequestException(ILLEGAL_NUMBER);
-      }
-    }
-  }
-
-  public static void checkNumberPositive(long... args){
-    for (long num: args){
-      if (num <= 0){
-        throw new BadRequestException(ILLEGAL_NUMBER);
-      }
-    }
-  }
-
-  public static void checkNumberNotNegative(int... args){
-    for (int num: args){
-      if (num < 0){
-        throw new BadRequestException(ILLEGAL_NUMBER);
-      }
-    }
-  }
-
-
-
 }

apollo-portal/pom.xml

@@ -31,6 +31,12 @@
 			<artifactId>h2</artifactId>
 			<scope>test</scope>
 		</dependency>
+		<dependency>
+			<groupId>org.mockito</groupId>
+			<artifactId>mockito-core</artifactId>
+			<version>2.15.0</version>
+			<scope>test</scope>
+		</dependency>
 	</dependencies>
 	<build>
 		<plugins>

apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java

@@ -52,8 +52,6 @@ public class ReleaseController {
                                       @PathVariable String namespaceName,
                                       @RequestBody NamespaceReleaseDTO model,
                                       HttpServletRequest request) {
-
-    checkModel(model != null);
     RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
             .getReleaseTitle()),
         "Params(releaseTitle and releasedBy) can not be empty");
@@ -91,7 +89,6 @@ public class ReleaseController {
                             @PathVariable String clusterName, @PathVariable String namespaceName,
                             @PathVariable String branchName, @RequestParam(value = "deleteBranch", defaultValue = "true") boolean deleteBranch,
                             @RequestBody NamespaceReleaseDTO model, HttpServletRequest request) {
-        checkModel(model != null);
         RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
                         .getReleaseTitle()),
                 "Params(releaseTitle and releasedBy) can not be empty");
@@ -114,7 +111,6 @@ public class ReleaseController {
                                         @PathVariable String namespaceName, @PathVariable String branchName,
                                         @RequestBody NamespaceReleaseDTO model,
                                         HttpServletRequest request) {
-        checkModel(model != null);
         RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
                         .getReleaseTitle()),
                 "Params(releaseTitle and releasedBy) can not be empty");
@@ -140,8 +136,6 @@ public class ReleaseController {
                                                @PathVariable String namespaceName, @PathVariable String branchName,
                                                @RequestBody NamespaceGrayDelReleaseDTO model,
                                                HttpServletRequest request) {
-
-        checkModel(model != null);
         RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
                         .getReleaseTitle()),
                 "Params(releaseTitle and releasedBy) can not be empty");

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java

@@ -33,9 +33,6 @@ public class ClusterController {
   @PostMapping(value = "apps/{appId}/envs/{env}/clusters")
   public ClusterDTO createCluster(@PathVariable String appId, @PathVariable String env,
                                   @Valid @RequestBody ClusterDTO cluster) {
-
-    checkModel(Objects.nonNull(cluster));
-
     String operator = userInfoHolder.getUser().getUserId();
     cluster.setDataChangeLastModifiedBy(operator);
     cluster.setDataChangeCreatedBy(operator);

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java

@@ -5,6 +5,10 @@ import com.ctrip.framework.apollo.common.utils.RequestPrecondition;
 import com.ctrip.framework.apollo.core.enums.Env;
 import com.ctrip.framework.apollo.portal.component.PermissionValidator;
 import com.ctrip.framework.apollo.portal.service.CommitService;
+import javax.validation.Valid;
+import javax.validation.constraints.Positive;
+import javax.validation.constraints.PositiveOrZero;
+import org.springframework.validation.annotation.Validated;
 import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.RequestParam;
@@ -13,7 +17,7 @@ import org.springframework.web.bind.annotation.RestController;
 import java.util.Collections;
 import java.util.List;
 
-
+@Validated
 @RestController
 public class CommitController {
 
@@ -28,16 +32,12 @@ public class CommitController {
   @GetMapping("/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/commits")
   public List<CommitDTO> find(@PathVariable String appId, @PathVariable String env,
                               @PathVariable String clusterName, @PathVariable String namespaceName,
-                              @RequestParam(defaultValue = "0") int page, @RequestParam(defaultValue = "10") int size) {
+                              @Valid @PositiveOrZero(message = "page should be positive or 0") @RequestParam(defaultValue = "0") int page,
+                              @Valid @Positive(message = "size should be positive number") @RequestParam(defaultValue = "10") int size) {
     if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, namespaceName)) {
       return Collections.emptyList();
     }
 
-    RequestPrecondition.checkNumberPositive(size);
-    RequestPrecondition.checkNumberNotNegative(page);
-
     return commitService.find(appId, Env.valueOf(env), clusterName, namespaceName, page, size);
-
   }
-
 }

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java

@@ -50,9 +50,6 @@ public class ItemController {
   public void modifyItemsByText(@PathVariable String appId, @PathVariable String env,
                                 @PathVariable String clusterName, @PathVariable String namespaceName,
                                 @RequestBody NamespaceTextModel model) {
-
-    checkModel(model != null);
-
     model.setAppId(appId);
     model.setClusterName(clusterName);
     model.setEnv(env);
@@ -141,7 +138,7 @@ public class ItemController {
 
   @PostMapping(value = "/namespaces/{namespaceName}/diff", consumes = {"application/json"})
   public List<ItemDiffs> diff(@RequestBody NamespaceSyncModel model) {
-    checkModel(Objects.nonNull(model) && !model.isInvalid());
+    checkModel(!model.isInvalid());
 
     List<ItemDiffs> itemDiffs = configService.compare(model.getSyncToNamespaces(), model.getSyncItems());
 
@@ -164,7 +161,7 @@ public class ItemController {
   @PutMapping(value = "/apps/{appId}/namespaces/{namespaceName}/items", consumes = {"application/json"})
   public ResponseEntity<Void> update(@PathVariable String appId, @PathVariable String namespaceName,
                                      @RequestBody NamespaceSyncModel model) {
-    checkModel(Objects.nonNull(model) && !model.isInvalid());
+    checkModel(!model.isInvalid());
     boolean hasPermission = permissionValidator.hasModifyNamespacePermission(appId, namespaceName);
     Env envNoPermission = null;
     // if uses has ModifyNamespace permission then he has permission

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java

@@ -12,9 +12,13 @@ import com.ctrip.framework.apollo.portal.entity.model.NamespaceReleaseModel;
 import com.ctrip.framework.apollo.portal.entity.vo.ReleaseCompareResult;
 import com.ctrip.framework.apollo.portal.listener.ConfigPublishEvent;
 import com.ctrip.framework.apollo.portal.service.ReleaseService;
+import javax.validation.Valid;
+import javax.validation.constraints.Positive;
+import javax.validation.constraints.PositiveOrZero;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.access.prepost.PreAuthorize;
+import org.springframework.validation.annotation.Validated;
 import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.PostMapping;
@@ -29,6 +33,7 @@ import java.util.Objects;
 
 import static com.ctrip.framework.apollo.common.utils.RequestPrecondition.checkModel;
 
+@Validated
 @RestController
 public class ReleaseController {
 
@@ -53,8 +58,6 @@ public class ReleaseController {
   public ReleaseDTO createRelease(@PathVariable String appId,
                                   @PathVariable String env, @PathVariable String clusterName,
                                   @PathVariable String namespaceName, @RequestBody NamespaceReleaseModel model) {
-
-    checkModel(Objects.nonNull(model));
     model.setAppId(appId);
     model.setEnv(env);
     model.setClusterName(clusterName);
@@ -85,8 +88,6 @@ public class ReleaseController {
                                       @PathVariable String env, @PathVariable String clusterName,
                                       @PathVariable String namespaceName, @PathVariable String branchName,
                                       @RequestBody NamespaceReleaseModel model) {
-
-    checkModel(Objects.nonNull(model));
     model.setAppId(appId);
     model.setEnv(env);
     model.setClusterName(branchName);
@@ -117,15 +118,12 @@ public class ReleaseController {
                                          @PathVariable String env,
                                          @PathVariable String clusterName,
                                          @PathVariable String namespaceName,
-                                         @RequestParam(defaultValue = "0") int page,
-                                         @RequestParam(defaultValue = "5") int size) {
+                                         @Valid @PositiveOrZero(message = "page should be positive or 0") @RequestParam(defaultValue = "0") int page,
+                                         @Valid @Positive(message = "size should be positive number") @RequestParam(defaultValue = "5") int size) {
     if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, namespaceName)) {
       return Collections.emptyList();
     }
 
-    RequestPrecondition.checkNumberPositive(size);
-    RequestPrecondition.checkNumberNotNegative(page);
-
     return releaseService.findAllReleases(appId, Env.valueOf(env), clusterName, namespaceName, page, size);
   }
 
@@ -134,16 +132,13 @@ public class ReleaseController {
                                              @PathVariable String env,
                                              @PathVariable String clusterName,
                                              @PathVariable String namespaceName,
-                                             @RequestParam(defaultValue = "0") int page,
-                                             @RequestParam(defaultValue = "5") int size) {
+                                             @Valid @PositiveOrZero(message = "page should be positive or 0") @RequestParam(defaultValue = "0") int page,
+                                             @Valid @Positive(message = "size should be positive number") @RequestParam(defaultValue = "5") int size) {
 
     if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, namespaceName)) {
       return Collections.emptyList();
     }
 
-    RequestPrecondition.checkNumberPositive(size);
-    RequestPrecondition.checkNumberNotNegative(page);
-
     return releaseService.findActiveReleases(appId, Env.valueOf(env), clusterName, namespaceName, page, size);
   }
 

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ServerConfigController.java

@@ -2,10 +2,11 @@ package com.ctrip.framework.apollo.portal.controller;
 
 
 import com.ctrip.framework.apollo.common.utils.BeanUtils;
-import com.ctrip.framework.apollo.common.utils.RequestPrecondition;
 import com.ctrip.framework.apollo.portal.entity.po.ServerConfig;
 import com.ctrip.framework.apollo.portal.repository.ServerConfigRepository;
 import com.ctrip.framework.apollo.portal.spi.UserInfoHolder;
+import java.util.Objects;
+import javax.validation.Valid;
 import org.springframework.security.access.prepost.PreAuthorize;
 import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
@@ -13,10 +14,6 @@ import org.springframework.web.bind.annotation.PostMapping;
 import org.springframework.web.bind.annotation.RequestBody;
 import org.springframework.web.bind.annotation.RestController;
 
-import java.util.Objects;
-
-import static com.ctrip.framework.apollo.common.utils.RequestPrecondition.checkModel;
-
 /**
  * 配置中心本身需要一些配置,这些配置放在数据库里面
  */
@@ -33,11 +30,7 @@ public class ServerConfigController {
 
   @PreAuthorize(value = "@permissionValidator.isSuperAdmin()")
   @PostMapping("/server/config")
-  public ServerConfig createOrUpdate(@RequestBody ServerConfig serverConfig) {
-
-    checkModel(Objects.nonNull(serverConfig));
-    RequestPrecondition.checkArgumentsNotEmpty(serverConfig.getKey(), serverConfig.getValue());
-
+  public ServerConfig createOrUpdate(@Valid @RequestBody ServerConfig serverConfig) {
     String modifiedBy = userInfoHolder.getUser().getUserId();
 
     ServerConfig storedConfig = serverConfigRepository.findByKey(serverConfig.getKey());

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/po/ServerConfig.java

@@ -2,6 +2,7 @@ package com.ctrip.framework.apollo.portal.entity.po;
 
 import com.ctrip.framework.apollo.common.entity.BaseEntity;
 
+import javax.validation.constraints.NotBlank;
 import org.hibernate.annotations.SQLDelete;
 import org.hibernate.annotations.Where;
 
@@ -17,9 +18,11 @@ import javax.persistence.Table;
 @SQLDelete(sql = "Update ServerConfig set isDeleted = 1 where id = ?")
 @Where(clause = "isDeleted = 0")
 public class ServerConfig extends BaseEntity {
+  @NotBlank(message = "ServerConfig.Key cannot be blank")
   @Column(name = "Key", nullable = false)
   private String key;
 
+  @NotBlank(message = "ServerConfig.Value cannot be blank")
   @Column(name = "Value", nullable = false)
   private String value;
 

apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java

 package com.ctrip.framework.apollo;
 
 import com.ctrip.framework.apollo.openapi.auth.ConsumerPermissionValidator;
-import org.springframework.beans.factory.annotation.Qualifier;
+import com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil;
+import com.ctrip.framework.apollo.portal.component.PermissionValidator;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Primary;
@@ -13,6 +14,8 @@ import static org.mockito.Mockito.when;
 /**
  * Created by kezhenxu at 2019/1/8 20:19.
  *
+ * Configuration class that will disable authorization.
+ *
  * @author kezhenxu (kezhenxu94@163.com)
  */
 @Profile("skipAuthorization")
@@ -20,10 +23,25 @@ import static org.mockito.Mockito.when;
 public class SkipAuthorizationConfiguration {
   @Primary
   @Bean
-  @Qualifier("consumerPermissionValidator")
   public ConsumerPermissionValidator consumerPermissionValidator() {
-    ConsumerPermissionValidator mock = mock(ConsumerPermissionValidator.class);
+    final ConsumerPermissionValidator mock = mock(ConsumerPermissionValidator.class);
     when(mock.hasCreateNamespacePermission(any(), any())).thenReturn(true);
     return mock;
   }
+
+  @Primary
+  @Bean
+  public ConsumerAuthUtil consumerAuthUtil() {
+    final ConsumerAuthUtil mock = mock(ConsumerAuthUtil.class);
+    when(mock.getConsumerId(any())).thenReturn(1L);
+    return mock;
+  }
+
+  @Primary
+  @Bean("permissionValidator")
+  public PermissionValidator permissionValidator() {
+    final PermissionValidator mock = mock(PermissionValidator.class);
+    when(mock.isSuperAdmin()).thenReturn(true);
+    return mock;
+  }
 }

apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/AbstractIntegrationTest.java

 package com.ctrip.framework.apollo.portal;
 
 
+import com.ctrip.framework.apollo.SkipAuthorizationConfiguration;
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.test.context.SpringBootTest;
@@ -13,11 +14,14 @@ import org.springframework.web.client.RestTemplate;
 import javax.annotation.PostConstruct;
 
 @RunWith(SpringJUnit4ClassRunner.class)
-@SpringBootTest(classes = PortalApplication.class, webEnvironment = WebEnvironment.RANDOM_PORT)
+@SpringBootTest(classes = {
+    PortalApplication.class,
+    SkipAuthorizationConfiguration.class
+}, webEnvironment = WebEnvironment.RANDOM_PORT)
 public abstract class AbstractIntegrationTest {
 
   protected RestTemplate restTemplate = (new TestRestTemplate()).getRestTemplate();
- 
+
   @PostConstruct
   private void postConstruct() {
     System.setProperty("spring.profiles.active", "test");
@@ -27,4 +31,7 @@ public abstract class AbstractIntegrationTest {
   @Value("${local.server.port}")
   int port;
 
+  protected String url(String path) {
+    return "http://localhost:" + port + path;
+  }
 }

apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/AbstractUnitTest.java

 package com.ctrip.framework.apollo.portal;
 
 import org.junit.runner.RunWith;
-import org.mockito.runners.MockitoJUnitRunner;
+import org.mockito.junit.MockitoJUnitRunner;
 
-@RunWith(MockitoJUnitRunner.class)
+@RunWith(MockitoJUnitRunner.Silent.class)
 public abstract class AbstractUnitTest {
 
 }

apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/CommitControllerTest.java

+package com.ctrip.framework.apollo.portal.controller;
+
+import com.ctrip.framework.apollo.portal.AbstractIntegrationTest;
+import java.util.List;
+import org.junit.Test;
+import org.springframework.web.client.HttpClientErrorException;
+import static org.hamcrest.core.StringContains.containsString;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+
+/**
+ * Created by kezhenxu at 2019/1/14 12:49.
+ *
+ * @author kezhenxu (kezhenxu at lizhi dot fm)
+ */
+public class CommitControllerTest extends AbstractIntegrationTest {
+
+  @Test
+  public void shouldFailWhenPageOrSiseIsNegative() {
+    try {
+      restTemplate.getForEntity(
+          url("/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/commits?page=-1"),
+          List.class, "1", "env", "cl", "ns"
+      );
+      fail("should throw");
+    } catch (final HttpClientErrorException e) {
+      assertThat(
+          new String(e.getResponseBodyAsByteArray()), containsString("page should be positive or 0")
+      );
+    }
+    try {
+      restTemplate.getForEntity(
+          url("/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/commits?size=0"),
+          List.class, "1", "env", "cl", "ns"
+      );
+      fail("should throw");
+    } catch (final HttpClientErrorException e) {
+      assertThat(
+          new String(e.getResponseBodyAsByteArray()), containsString("size should be positive number")
+      );
+    }
+  }
+}

apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ServerConfigControllerTest.java

+package com.ctrip.framework.apollo.portal.controller;
+
+import com.ctrip.framework.apollo.portal.AbstractIntegrationTest;
+import com.ctrip.framework.apollo.portal.entity.po.ServerConfig;
+import org.junit.Assert;
+import org.junit.Test;
+import org.springframework.http.ResponseEntity;
+import org.springframework.test.context.ActiveProfiles;
+import org.springframework.web.client.HttpClientErrorException;
+import static org.hamcrest.core.StringContains.containsString;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Created by kezhenxu at 2019/1/14 13:24.
+ *
+ * @author kezhenxu (kezhenxu at lizhi dot fm)
+ */
+@ActiveProfiles("skipAuthorization")
+public class ServerConfigControllerTest extends AbstractIntegrationTest {
+  @Test
+  public void shouldSuccessWhenParameterValid() {
+    ServerConfig serverConfig = new ServerConfig();
+    serverConfig.setKey("validKey");
+    serverConfig.setValue("validValue");
+    ResponseEntity<ServerConfig> responseEntity = restTemplate.postForEntity(
+        url("/server/config"), serverConfig, ServerConfig.class
+    );
+    assertEquals(responseEntity.getBody().getKey(), serverConfig.getKey());
+    assertEquals(responseEntity.getBody().getValue(), serverConfig.getValue());
+  }
+
+  @Test
+  public void shouldFailWhenParameterInvalid() {
+    ServerConfig serverConfig = new ServerConfig();
+    serverConfig.setKey("  ");
+    serverConfig.setValue("valid");
+    try {
+      restTemplate.postForEntity(
+          url("/server/config"), serverConfig, ServerConfig.class
+      );
+      Assert.fail("Should throw");
+    } catch (final HttpClientErrorException e) {
+      assertThat(
+          new String(e.getResponseBodyAsByteArray()),
+          containsString("ServerConfig.Key cannot be blank")
+      );
+    }
+    serverConfig.setKey("valid");
+    serverConfig.setValue("   ");
+    try {
+      restTemplate.postForEntity(
+          url("/server/config"), serverConfig, ServerConfig.class
+      );
+      Assert.fail("Should throw");
+    } catch (final HttpClientErrorException e) {
+      assertThat(
+          new String(e.getResponseBodyAsByteArray()),
+          containsString("ServerConfig.Value cannot be blank")
+      );
+    }
+  }
+}