Skip to content

Commit

Permalink
fix the potential data inconsistency issue
Browse files Browse the repository at this point in the history
  • Loading branch information
@nobodyiam
nobodyiam committed Mar 17, 2022
1 parent eb78ceb commit ce34827
Show file tree
Hide file tree
Showing 6 changed files with 234 additions and 35 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
193 changes: 165 additions & 28 deletions ...c/test/java/com/ctrip/framework/apollo/adminservice/controller/ItemSetControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)
Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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()
Expand Down Expand Up @@ -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());
}
}
4 changes: 4 additions & 0 deletions apollo-adminservice/src/test/resources/controller/test-itemset.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
22 changes: 21 additions & 1 deletion apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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);
Expand All @@ -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
Expand All @@ -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);
Expand Down
5 changes: 5 additions & 0 deletions apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
Loading

0 comments on commit ce34827

Please sign in to comment.