apolloconfig · lepdou · Mar 17, 2022 · Mar 12, 2022
diff --git a/CHANGES.md b/CHANGES.md
@@ -45,6 +45,7 @@ Apollo 2.0.0
 * [optimize import/export config](https://github.com/apolloconfig/apollo/pull/4231)
 * [Configure publish and rollback modal boxes to add scrollbars](https://github.com/apolloconfig/apollo/pull/4251)
 * [fix import config bug](https://github.com/apolloconfig/apollo/pull/4262)
+* [Fix the potential data inconsistency issue](https://github.com/apolloconfig/apollo/pull/4256)
 
 ------------------
 All issues and pull requests are [here](https://github.com/ctripcorp/apollo/milestone/8?closed=1)
diff --git a/...c/test/java/com/ctrip/framework/apollo/adminservice/controller/ItemSetControllerTest.java b/...c/test/java/com/ctrip/framework/apollo/adminservice/controller/ItemSetControllerTest.java
@@ -61,19 +61,11 @@ public void testItemSetCreated() {
     Assert.assertEquals("default", cluster.getName());
     Assert.assertEquals("application", namespace.getNamespaceName());
 
-    ItemChangeSets itemSet = new ItemChangeSets();
-    itemSet.setDataChangeLastModifiedBy("created");
     RestTemplate createdTemplate = (new TestRestTemplate()).getRestTemplate();
     createdTemplate.setMessageConverters(restTemplate.getMessageConverters());
 
     int createdSize = 3;
-    for (int i = 0; i < createdSize; i++) {
-      ItemDTO item = new ItemDTO();
-      item.setNamespaceId(namespace.getId());
-      item.setKey("key_" + i);
-      item.setValue("created_value_" + i);
-      itemSet.addCreateItem(item);
-    }
+    ItemChangeSets itemSet = mockCreateItemChangeSets(namespace, createdSize);
 
     ResponseEntity<Void> response =
         createdTemplate.postForEntity(
@@ -90,6 +82,42 @@ public void testItemSetCreated() {
     Assert.assertNotNull(item0.getDataChangeCreatedTime());
   }
 
+  @Test
+  @Sql(scripts = "/controller/test-itemset.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)
+  @Sql(scripts = "/controller/cleanup.sql", executionPhase = ExecutionPhase.AFTER_TEST_METHOD)
+  public void testItemSetCreatedWithInvalidNamespaceId() {
+    String appId = "someAppId";
+    String clusterName = "default";
+    String namespaceName = "application";
+    String someNamespaceName = "someNamespace";
+
+    NamespaceDTO namespace =
+        restTemplate.getForObject("http://localhost:" + port + "/apps/" + appId
+            + "/clusters/" + clusterName + "/namespaces/" + namespaceName, NamespaceDTO.class);
+
+    NamespaceDTO someNamespace =
+        restTemplate.getForObject("http://localhost:" + port + "/apps/" + appId
+            + "/clusters/" + clusterName + "/namespaces/" + someNamespaceName, NamespaceDTO.class);
+
+    long someNamespaceId = someNamespace.getId();
+
+    RestTemplate createdTemplate = (new TestRestTemplate()).getRestTemplate();
+    createdTemplate.setMessageConverters(restTemplate.getMessageConverters());
+
+    int createdSize = 3;
+    ItemChangeSets itemSet = mockCreateItemChangeSets(namespace, createdSize);
+    itemSet.getCreateItems().get(createdSize - 1).setNamespaceId(someNamespaceId);
+
+    ResponseEntity<Void> response =
+        createdTemplate.postForEntity(
+            "http://localhost:" + port + "/apps/" + appId + "/clusters/"
+                + clusterName + "/namespaces/" + namespaceName + "/itemset",
+            itemSet, Void.class);
+    Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode());
+    List<Item> items = itemRepository.findByNamespaceIdOrderByLineNumAsc(someNamespaceId);
+    Assert.assertEquals(0, items.size());
+  }
+
   @Test
   @Sql(scripts = "/controller/test-itemset.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)
   @Sql(scripts = "/controller/cleanup.sql", executionPhase = ExecutionPhase.AFTER_TEST_METHOD)
@@ -110,19 +138,11 @@ public void testItemSetUpdated() {
     Assert.assertEquals("default", cluster.getName());
     Assert.assertEquals("application", namespace.getNamespaceName());
 
-    ItemChangeSets createChangeSet = new ItemChangeSets();
-    createChangeSet.setDataChangeLastModifiedBy("created");
     RestTemplate createdRestTemplate = (new TestRestTemplate()).getRestTemplate();
     createdRestTemplate.setMessageConverters(restTemplate.getMessageConverters());
 
     int createdSize = 3;
-    for (int i = 0; i < createdSize; i++) {
-      ItemDTO item = new ItemDTO();
-      item.setNamespaceId(namespace.getId());
-      item.setKey("key_" + i);
-      item.setValue("created_value_" + i);
-      createChangeSet.addCreateItem(item);
-    }
+    ItemChangeSets createChangeSet = mockCreateItemChangeSets(namespace, createdSize);
 
     ResponseEntity<Void> response = createdRestTemplate.postForEntity(
         "http://localhost:" + port + "/apps/" + app.getAppId() + "/clusters/" + cluster.getName()
@@ -164,6 +184,76 @@ public void testItemSetUpdated() {
     Assert.assertNotNull(item0.getDataChangeLastModifiedTime());
   }
 
+  @Test
+  @Sql(scripts = "/controller/test-itemset.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)
+  @Sql(scripts = "/controller/cleanup.sql", executionPhase = ExecutionPhase.AFTER_TEST_METHOD)
+  public void testItemSetUpdatedWithInvalidNamespaceId() {
+    String appId = "someAppId";
+    String clusterName = "default";
+    String namespaceName = "application";
+    String someNamespaceName = "someNamespace";
+
+    NamespaceDTO namespace =
+        restTemplate.getForObject("http://localhost:" + port + "/apps/" + appId
+            + "/clusters/" + clusterName + "/namespaces/" + namespaceName, NamespaceDTO.class);
+
+    NamespaceDTO someNamespace =
+        restTemplate.getForObject("http://localhost:" + port + "/apps/" + appId
+            + "/clusters/" + clusterName + "/namespaces/" + someNamespaceName, NamespaceDTO.class);
+
+    RestTemplate createdRestTemplate = (new TestRestTemplate()).getRestTemplate();
+    createdRestTemplate.setMessageConverters(restTemplate.getMessageConverters());
+
+    int createdSize = 3;
+    ItemChangeSets createChangeSet = mockCreateItemChangeSets(namespace, createdSize);
+
+    ResponseEntity<Void> response = createdRestTemplate.postForEntity(
+        "http://localhost:" + port + "/apps/" + appId + "/clusters/" + clusterName
+            + "/namespaces/" + namespace.getNamespaceName() + "/itemset",
+        createChangeSet, Void.class);
+    Assert.assertEquals(HttpStatus.OK, response.getStatusCode());
+
+    ItemDTO[] items =
+        createdRestTemplate.getForObject(
+            "http://localhost:" + port + "/apps/" + appId + "/clusters/"
+                + clusterName + "/namespaces/" + namespace.getNamespaceName() + "/items",
+            ItemDTO[].class);
+
+    ItemChangeSets updateChangeSet = new ItemChangeSets();
+    updateChangeSet.setDataChangeLastModifiedBy("updated");
+
+    RestTemplate updatedRestTemplate = (new TestRestTemplate()).getRestTemplate();
+    updatedRestTemplate.setMessageConverters(restTemplate.getMessageConverters());
+
+    int updatedSize = 2;
+    for (int i = 0; i < updatedSize; i++) {
+      items[i].setValue("updated_value_" + i);
+      updateChangeSet.addUpdateItem(items[i]);
+    }
+
+    response = updatedRestTemplate.postForEntity(
+        "http://localhost:" + port + "/apps/" + appId + "/clusters/" + clusterName
+            + "/namespaces/" + someNamespaceName + "/itemset",
+        updateChangeSet, Void.class);
+    Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode());
+    List<Item> savedItems = itemRepository.findByNamespaceIdOrderByLineNumAsc(someNamespace.getId());
+    Assert.assertEquals(0, savedItems.size());
+  }
+
+  private ItemChangeSets mockCreateItemChangeSets(NamespaceDTO namespace, int createdSize) {
+    ItemChangeSets createChangeSet = new ItemChangeSets();
+    createChangeSet.setDataChangeLastModifiedBy("created");
+
+    for (int i = 0; i < createdSize; i++) {
+      ItemDTO item = new ItemDTO();
+      item.setNamespaceId(namespace.getId());
+      item.setKey("key_" + i);
+      item.setValue("created_value_" + i);
+      createChangeSet.addCreateItem(item);
+    }
+    return createChangeSet;
+  }
+
   @Test
   @Sql(scripts = "/controller/test-itemset.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)
   @Sql(scripts = "/controller/cleanup.sql", executionPhase = ExecutionPhase.AFTER_TEST_METHOD)
@@ -184,19 +274,11 @@ public void testItemSetDeleted() {
     Assert.assertEquals("default", cluster.getName());
     Assert.assertEquals("application", namespace.getNamespaceName());
 
-    ItemChangeSets createChangeSet = new ItemChangeSets();
-    createChangeSet.setDataChangeLastModifiedBy("created");
     RestTemplate createdTemplate = (new TestRestTemplate()).getRestTemplate();
     createdTemplate.setMessageConverters(restTemplate.getMessageConverters());
-    
+
     int createdSize = 3;
-    for (int i = 0; i < createdSize; i++) {
-      ItemDTO item = new ItemDTO();
-      item.setNamespaceId(namespace.getId());
-      item.setKey("key_" + i);
-      item.setValue("created_value_" + i);
-      createChangeSet.addCreateItem(item);
-    }
+    ItemChangeSets createChangeSet = mockCreateItemChangeSets(namespace, createdSize);
 
     ResponseEntity<Void> response = createdTemplate.postForEntity(
         "http://localhost:" + port + "/apps/" + app.getAppId() + "/clusters/" + cluster.getName()
@@ -234,4 +316,59 @@ public void testItemSetDeleted() {
     Assert.assertEquals("created", item0.getDataChangeCreatedBy());
     Assert.assertNotNull(item0.getDataChangeCreatedTime());
   }
+
+  @Test
+  @Sql(scripts = "/controller/test-itemset.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)
+  @Sql(scripts = "/controller/cleanup.sql", executionPhase = ExecutionPhase.AFTER_TEST_METHOD)
+  public void testItemSetDeletedWithInvalidNamespaceId() {
+    String appId = "someAppId";
+    String clusterName = "default";
+    String namespaceName = "application";
+    String someNamespaceName = "someNamespace";
+
+    NamespaceDTO namespace =
+        restTemplate.getForObject("http://localhost:" + port + "/apps/" + appId
+            + "/clusters/" + clusterName + "/namespaces/" + namespaceName, NamespaceDTO.class);
+
+    NamespaceDTO someNamespace =
+        restTemplate.getForObject("http://localhost:" + port + "/apps/" + appId
+            + "/clusters/" + clusterName + "/namespaces/" + someNamespaceName, NamespaceDTO.class);
+
+    RestTemplate createdTemplate = (new TestRestTemplate()).getRestTemplate();
+    createdTemplate.setMessageConverters(restTemplate.getMessageConverters());
+
+    int createdSize = 3;
+    ItemChangeSets createChangeSet = mockCreateItemChangeSets(namespace, createdSize);
+
+    ResponseEntity<Void> response = createdTemplate.postForEntity(
+        "http://localhost:" + port + "/apps/" + appId + "/clusters/" + clusterName
+            + "/namespaces/" + namespace.getNamespaceName() + "/itemset",
+        createChangeSet, Void.class);
+    Assert.assertEquals(HttpStatus.OK, response.getStatusCode());
+
+    ItemDTO[] items =
+        restTemplate.getForObject(
+            "http://localhost:" + port + "/apps/" + appId + "/clusters/"
+                + clusterName + "/namespaces/" + namespace.getNamespaceName() + "/items",
+            ItemDTO[].class);
+
+    ItemChangeSets deleteChangeSet = new ItemChangeSets();
+    deleteChangeSet.setDataChangeLastModifiedBy("deleted");
+    RestTemplate deletedTemplate = (new TestRestTemplate()).getRestTemplate();
+    deletedTemplate.setMessageConverters(restTemplate.getMessageConverters());
+
+    int deletedSize = 1;
+    for (int i = 0; i < deletedSize; i++) {
+      items[i].setValue("deleted_value_" + i);
+      deleteChangeSet.addDeleteItem(items[i]);
+    }
+
+    response = deletedTemplate.postForEntity(
+        "http://localhost:" + port + "/apps/" + appId + "/clusters/" + clusterName
+            + "/namespaces/" + someNamespaceName + "/itemset",
+        deleteChangeSet, Void.class);
+    Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode());
+    List<Item> savedItems = itemRepository.findByNamespaceIdOrderByLineNumAsc(namespace.getId());
+    Assert.assertEquals(createdSize, savedItems.size());
+  }
 }
diff --git a/apollo-adminservice/src/test/resources/controller/test-itemset.sql b/apollo-adminservice/src/test/resources/controller/test-itemset.sql
@@ -19,4 +19,8 @@ INSERT INTO Cluster (AppId, Name) VALUES ('someAppId', 'default');
 
 INSERT INTO AppNamespace (AppId, Name) VALUES ('someAppId', 'application');
 
+INSERT INTO AppNamespace (AppId, Name) VALUES ('someAppId', 'someNamespace');
+
 INSERT INTO Namespace (AppId, ClusterName, NamespaceName) VALUES ('someAppId', 'default', 'application');
+
+INSERT INTO Namespace (AppId, ClusterName, NamespaceName) VALUES ('someAppId', 'default', 'someNamespace');
diff --git a/apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java b/apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java
@@ -23,6 +23,7 @@
 import com.ctrip.framework.apollo.biz.utils.ConfigChangeContentBuilder;
 import com.ctrip.framework.apollo.common.dto.ItemChangeSets;
 import com.ctrip.framework.apollo.common.dto.ItemDTO;
+import com.ctrip.framework.apollo.common.exception.BadRequestException;
 import com.ctrip.framework.apollo.common.exception.NotFoundException;
 import com.ctrip.framework.apollo.common.utils.BeanUtils;
 import org.springframework.stereotype.Service;
@@ -36,14 +37,17 @@ public class ItemSetService {
   private final AuditService auditService;
   private final CommitService commitService;
   private final ItemService itemService;
+  private final NamespaceService namespaceService;
 
   public ItemSetService(
       final AuditService auditService,
       final CommitService commitService,
-      final ItemService itemService) {
+      final ItemService itemService,
+      final NamespaceService namespaceService) {
     this.auditService = auditService;
     this.commitService = commitService;
     this.itemService = itemService;
+    this.namespaceService = namespaceService;
   }
 
   @Transactional
@@ -54,11 +58,21 @@ public ItemChangeSets updateSet(Namespace namespace, ItemChangeSets changeSets){
   @Transactional
   public ItemChangeSets updateSet(String appId, String clusterName,
                                   String namespaceName, ItemChangeSets changeSet) {
+    Namespace namespace = namespaceService.findOne(appId, clusterName, namespaceName);
+
+    if (namespace == null) {
+      throw new NotFoundException(String.format("Namespace %s not found", namespaceName));
+    }
+
     String operator = changeSet.getDataChangeLastModifiedBy();
     ConfigChangeContentBuilder configChangeContentBuilder = new ConfigChangeContentBuilder();
 
     if (!CollectionUtils.isEmpty(changeSet.getCreateItems())) {
       for (ItemDTO item : changeSet.getCreateItems()) {
+        if (item.getNamespaceId() != namespace.getId()) {
+          throw new BadRequestException("Invalid request, item and namespace do not match!");
+        }
+
         Item entity = BeanUtils.transform(Item.class, item);
         entity.setDataChangeCreatedBy(operator);
         entity.setDataChangeLastModifiedBy(operator);
@@ -76,6 +90,9 @@ public ItemChangeSets updateSet(String appId, String clusterName,
         if (managedItem == null) {
           throw new NotFoundException(String.format("item not found.(key=%s)", entity.getKey()));
         }
+        if (managedItem.getNamespaceId() != namespace.getId()) {
+          throw new BadRequestException("Invalid request, item and namespace do not match!");
+        }
         Item beforeUpdateItem = BeanUtils.transform(Item.class, managedItem);
 
         //protect. only value,comment,lastModifiedBy,lineNum can be modified
@@ -94,6 +111,9 @@ public ItemChangeSets updateSet(String appId, String clusterName,
     if (!CollectionUtils.isEmpty(changeSet.getDeleteItems())) {
       for (ItemDTO item : changeSet.getDeleteItems()) {
         Item deletedItem = itemService.delete(item.getId(), operator);
+        if (deletedItem.getNamespaceId() != namespace.getId()) {
+          throw new BadRequestException("Invalid request, item and namespace do not match!");
+        }
         configChangeContentBuilder.deleteItem(deletedItem);
       }
       auditService.audit("ItemSet", null, Audit.OP.DELETE, operator);

diff --git a/apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.java b/apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.java
@@ -96,6 +96,11 @@ public void updateConfigItemByText(NamespaceTextModel model) {
     }
     long namespaceId = namespace.getId();
 
+    // In case someone constructs an attack scenario
+    if (model.getNamespaceId() != namespaceId) {
+      throw new BadRequestException("Invalid request, item and namespace do not match!");
+    }
+
     String configText = model.getConfigText();
 
     ConfigTextResolver resolver =