From b1aba93472aad9a799a8315e09e146199348ccea Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 17 Mar 2022 17:42:39 +0800 Subject: [PATCH] fix the potential data inconsistency issue (#4256) --- CHANGES.md | 1 + .../controller/ItemSetControllerTest.java | 193 +++++++++++++++--- .../resources/controller/test-itemset.sql | 4 + .../apollo/biz/service/ItemSetService.java | 22 +- .../apollo/portal/service/ItemService.java | 5 + .../portal/service/ConfigServiceTest.java | 44 +++- 6 files changed, 234 insertions(+), 35 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 2d2019ea529..5f95055674a 100644 --- 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/apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ItemSetControllerTest.java b/apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ItemSetControllerTest.java index 81c5bc414e1..51a526f08da 100644 --- a/apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ItemSetControllerTest.java +++ b/apollo-adminservice/src/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 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 response = + createdTemplate.postForEntity( + "http://localhost:" + port + "/apps/" + appId + "/clusters/" + + clusterName + "/namespaces/" + namespaceName + "/itemset", + itemSet, Void.class); + Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); + List 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 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 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 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 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 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 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 index 250561685c0..6830ec12b5a 100644 --- 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 index df931e371d2..809aaed70fe 100644 --- 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 index 53432c8e7dd..36072039fb8 100644 --- 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 = diff --git a/apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java b/apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java index b17b20c536e..36858d23750 100644 --- a/apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java +++ b/apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java @@ -19,6 +19,7 @@ import com.ctrip.framework.apollo.common.dto.ItemChangeSets; import com.ctrip.framework.apollo.common.dto.ItemDTO; import com.ctrip.framework.apollo.common.dto.NamespaceDTO; +import com.ctrip.framework.apollo.common.exception.BadRequestException; import com.ctrip.framework.apollo.core.ConfigConsts; import com.ctrip.framework.apollo.core.enums.ConfigFileFormat; import com.ctrip.framework.apollo.portal.environment.Env; @@ -74,6 +75,28 @@ public void testUpdateConfigByText() { String namespaceName = "application"; long someNamespaceId = 123L; + NamespaceTextModel model = mockNamespaceModel(appId, clusterName, namespaceName, + someNamespaceId); + List itemDTOs = mockBaseItemHas3Key(); + ItemChangeSets changeSets = new ItemChangeSets(); + changeSets.addCreateItem(new ItemDTO("d", "c", "", 4)); + + NamespaceDTO someNamespaceDto = mock(NamespaceDTO.class); + when(someNamespaceDto.getId()).thenReturn(someNamespaceId); + when(namespaceAPI.loadNamespace(appId, model.getEnv(), clusterName, namespaceName)) + .thenReturn(someNamespaceDto); + when(itemAPI.findItems(appId, Env.DEV, clusterName, namespaceName)).thenReturn(itemDTOs); + when(resolver.resolve(someNamespaceId, model.getConfigText(), itemDTOs)).thenReturn(changeSets); + + UserInfo userInfo = new UserInfo(); + userInfo.setUserId("test"); + when(userInfoHolder.getUser()).thenReturn(userInfo); + + configService.updateConfigItemByText(model); + } + + private NamespaceTextModel mockNamespaceModel(String appId, String clusterName, + String namespaceName, long someNamespaceId) { NamespaceTextModel model = new NamespaceTextModel(); model.setEnv("DEV"); model.setNamespaceName(namespaceName); @@ -81,6 +104,20 @@ public void testUpdateConfigByText() { model.setAppId(appId); model.setConfigText("a=b\nb=c\nc=d\nd=e"); model.setFormat(ConfigFileFormat.Properties.getValue()); + model.setNamespaceId(someNamespaceId); + return model; + } + + @Test(expected = BadRequestException.class) + public void testUpdateConfigByTextWithInvalidNamespaceId() { + String appId = "6666"; + String clusterName = "default"; + String namespaceName = "application"; + long someNamespaceId = 123L; + long anotherNamespaceId = 321L; + + NamespaceTextModel model = mockNamespaceModel(appId, clusterName, namespaceName, + anotherNamespaceId); List itemDTOs = mockBaseItemHas3Key(); ItemChangeSets changeSets = new ItemChangeSets(); changeSets.addCreateItem(new ItemDTO("d", "c", "", 4)); @@ -96,14 +133,9 @@ public void testUpdateConfigByText() { userInfo.setUserId("test"); when(userInfoHolder.getUser()).thenReturn(userInfo); - try { - configService.updateConfigItemByText(model); - } catch (Exception e) { - Assert.fail(); - } + configService.updateConfigItemByText(model); } - /** * a=b b=c c=d */