caigy commented on code in PR #4637:
URL: https://github.com/apache/rocketmq/pull/4637#discussion_r944062306
##########
acl/src/test/java/org/apache/rocketmq/acl/plain/PlainPermissionManagerTest.java:
##########
@@ -325,4 +336,77 @@ public void updateAccessConfigTest() {
plainAccessConfig.setTopicPerms(Lists.newArrayList("topicA=SUB"));
plainPermissionManager.updateAccessConfig(plainAccessConfig);
}
+
+ @Test
+ public void getAllAclFilesTest() {
+ final List<String> notExistList =
plainPermissionManager.getAllAclFiles("aa/bb");
+ Assertions.assertThat(notExistList).isEmpty();
+ final List<String> files = plainPermissionManager.getAllAclFiles(PATH);
+ Assertions.assertThat(files).isNotEmpty();
+ }
+
+ @Test
+ public void loadTest() {
+ plainPermissionManager.load();
+ final Map<String, DataVersion> map =
plainPermissionManager.getDataVersionMap();
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void updateAclConfigFileVersionTest() {
+ String aclFileName = "test_plain_acl";
+ Map<String, Object> updateAclConfigMap = new HashMap<>();
+ List<Map<String, Object>> versionElement = new ArrayList<>();
+ Map<String, Object> accountsMap = new LinkedHashMap<>();
+ accountsMap.put(AclConstants.CONFIG_COUNTER, 1);
+ accountsMap.put(AclConstants.CONFIG_TIME_STAMP,
System.currentTimeMillis());
+ versionElement.add(accountsMap);
+
+ updateAclConfigMap.put(AclConstants.CONFIG_DATA_VERSION,
versionElement);
+ final Map<String, Object> map =
plainPermissionManager.updateAclConfigFileVersion(aclFileName,
updateAclConfigMap);
+ Assertions.assertThat(map).isNotEmpty();
Review Comment:
It would be better to check actual data version is correct or not.
##########
acl/src/test/java/org/apache/rocketmq/acl/plain/PlainPermissionManagerTest.java:
##########
@@ -20,20 +20,29 @@
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
import java.util.HashSet;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.rocketmq.acl.common.AclConstants;
import org.apache.rocketmq.acl.common.AclException;
import org.apache.rocketmq.acl.common.AclUtils;
import org.apache.rocketmq.acl.common.Permission;
+import org.apache.rocketmq.common.AclConfig;
+import org.apache.rocketmq.common.DataVersion;
import org.apache.rocketmq.common.PlainAccessConfig;
+import org.assertj.core.api.Assertions;
import org.assertj.core.util.Lists;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
+import static
org.apache.rocketmq.acl.plain.PlainAccessControlFlowTest.DEFAULT_TOPIC;
Review Comment:
IMO this test is independent of `PlainAccessControlFlowTest`, you can
maintain default topic of this test.
##########
acl/src/test/java/org/apache/rocketmq/acl/plain/PlainPermissionManagerTest.java:
##########
@@ -325,4 +336,77 @@ public void updateAccessConfigTest() {
plainAccessConfig.setTopicPerms(Lists.newArrayList("topicA=SUB"));
plainPermissionManager.updateAccessConfig(plainAccessConfig);
}
+
+ @Test
+ public void getAllAclFilesTest() {
+ final List<String> notExistList =
plainPermissionManager.getAllAclFiles("aa/bb");
+ Assertions.assertThat(notExistList).isEmpty();
+ final List<String> files = plainPermissionManager.getAllAclFiles(PATH);
+ Assertions.assertThat(files).isNotEmpty();
+ }
+
+ @Test
+ public void loadTest() {
+ plainPermissionManager.load();
+ final Map<String, DataVersion> map =
plainPermissionManager.getDataVersionMap();
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void updateAclConfigFileVersionTest() {
+ String aclFileName = "test_plain_acl";
+ Map<String, Object> updateAclConfigMap = new HashMap<>();
+ List<Map<String, Object>> versionElement = new ArrayList<>();
+ Map<String, Object> accountsMap = new LinkedHashMap<>();
+ accountsMap.put(AclConstants.CONFIG_COUNTER, 1);
+ accountsMap.put(AclConstants.CONFIG_TIME_STAMP,
System.currentTimeMillis());
+ versionElement.add(accountsMap);
+
+ updateAclConfigMap.put(AclConstants.CONFIG_DATA_VERSION,
versionElement);
+ final Map<String, Object> map =
plainPermissionManager.updateAclConfigFileVersion(aclFileName,
updateAclConfigMap);
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void createAclAccessConfigMapTest() {
+ Map<String, Object> existedAccountMap = new HashMap<>();
+ plainAccessConfig.setAccessKey("admin123");
+ plainAccessConfig.setSecretKey("12345678");
+ plainAccessConfig.setWhiteRemoteAddress("192.168.1.1");
+ plainAccessConfig.setAdmin(false);
+ plainAccessConfig.setDefaultGroupPerm(AclConstants.SUB_PUB);
+ plainAccessConfig.setTopicPerms(Arrays.asList(DEFAULT_TOPIC + "=" +
AclConstants.PUB));
+ plainAccessConfig.setGroupPerms(Lists.newArrayList("groupA=SUB"));
+
+ final Map<String, Object> map =
plainPermissionManager.createAclAccessConfigMap(existedAccountMap,
plainAccessConfig);
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void deleteAccessConfigTest() {
+ // delete not exist accessConfig
+ final boolean flag1 =
plainPermissionManager.deleteAccessConfig("admin123");
+ assert flag1 == false;
+ //delete existed accessConfig
+ final AclConfig config = plainPermissionManager.getAllAclConfig();
+ final List<PlainAccessConfig> configs = config.getPlainAccessConfigs();
+ final PlainAccessConfig config1 = configs.get(0);
+ final String accessKey = config1.getAccessKey();
+ final boolean flag2 =
plainPermissionManager.deleteAccessConfig(accessKey);
Review Comment:
It may cause side effects if `PlainAccessConfig` is changed but not restored.
##########
acl/src/test/java/org/apache/rocketmq/acl/plain/PlainPermissionManagerTest.java:
##########
@@ -325,4 +336,77 @@ public void updateAccessConfigTest() {
plainAccessConfig.setTopicPerms(Lists.newArrayList("topicA=SUB"));
plainPermissionManager.updateAccessConfig(plainAccessConfig);
}
+
+ @Test
+ public void getAllAclFilesTest() {
+ final List<String> notExistList =
plainPermissionManager.getAllAclFiles("aa/bb");
+ Assertions.assertThat(notExistList).isEmpty();
+ final List<String> files = plainPermissionManager.getAllAclFiles(PATH);
+ Assertions.assertThat(files).isNotEmpty();
+ }
+
+ @Test
+ public void loadTest() {
+ plainPermissionManager.load();
+ final Map<String, DataVersion> map =
plainPermissionManager.getDataVersionMap();
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void updateAclConfigFileVersionTest() {
+ String aclFileName = "test_plain_acl";
+ Map<String, Object> updateAclConfigMap = new HashMap<>();
+ List<Map<String, Object>> versionElement = new ArrayList<>();
+ Map<String, Object> accountsMap = new LinkedHashMap<>();
+ accountsMap.put(AclConstants.CONFIG_COUNTER, 1);
+ accountsMap.put(AclConstants.CONFIG_TIME_STAMP,
System.currentTimeMillis());
+ versionElement.add(accountsMap);
+
+ updateAclConfigMap.put(AclConstants.CONFIG_DATA_VERSION,
versionElement);
+ final Map<String, Object> map =
plainPermissionManager.updateAclConfigFileVersion(aclFileName,
updateAclConfigMap);
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void createAclAccessConfigMapTest() {
+ Map<String, Object> existedAccountMap = new HashMap<>();
+ plainAccessConfig.setAccessKey("admin123");
+ plainAccessConfig.setSecretKey("12345678");
+ plainAccessConfig.setWhiteRemoteAddress("192.168.1.1");
+ plainAccessConfig.setAdmin(false);
+ plainAccessConfig.setDefaultGroupPerm(AclConstants.SUB_PUB);
+ plainAccessConfig.setTopicPerms(Arrays.asList(DEFAULT_TOPIC + "=" +
AclConstants.PUB));
+ plainAccessConfig.setGroupPerms(Lists.newArrayList("groupA=SUB"));
+
+ final Map<String, Object> map =
plainPermissionManager.createAclAccessConfigMap(existedAccountMap,
plainAccessConfig);
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void deleteAccessConfigTest() {
+ // delete not exist accessConfig
+ final boolean flag1 =
plainPermissionManager.deleteAccessConfig("admin123");
+ assert flag1 == false;
+ //delete existed accessConfig
+ final AclConfig config = plainPermissionManager.getAllAclConfig();
+ final List<PlainAccessConfig> configs = config.getPlainAccessConfigs();
+ final PlainAccessConfig config1 = configs.get(0);
+ final String accessKey = config1.getAccessKey();
+ final boolean flag2 =
plainPermissionManager.deleteAccessConfig(accessKey);
+ assert flag2 == true;
+
+ }
+
+ @Test
+ public void updateGlobalWhiteAddrsConfigTest() {
+ final boolean flag =
plainPermissionManager.updateGlobalWhiteAddrsConfig(Lists.newArrayList("192.168.1.2"));
+ assert flag == true;
Review Comment:
You may check the actual content of global white lists.
##########
acl/src/test/java/org/apache/rocketmq/acl/plain/PlainPermissionManagerTest.java:
##########
@@ -325,4 +336,77 @@ public void updateAccessConfigTest() {
plainAccessConfig.setTopicPerms(Lists.newArrayList("topicA=SUB"));
plainPermissionManager.updateAccessConfig(plainAccessConfig);
}
+
+ @Test
+ public void getAllAclFilesTest() {
+ final List<String> notExistList =
plainPermissionManager.getAllAclFiles("aa/bb");
+ Assertions.assertThat(notExistList).isEmpty();
+ final List<String> files = plainPermissionManager.getAllAclFiles(PATH);
+ Assertions.assertThat(files).isNotEmpty();
+ }
+
+ @Test
+ public void loadTest() {
+ plainPermissionManager.load();
+ final Map<String, DataVersion> map =
plainPermissionManager.getDataVersionMap();
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void updateAclConfigFileVersionTest() {
+ String aclFileName = "test_plain_acl";
+ Map<String, Object> updateAclConfigMap = new HashMap<>();
+ List<Map<String, Object>> versionElement = new ArrayList<>();
+ Map<String, Object> accountsMap = new LinkedHashMap<>();
+ accountsMap.put(AclConstants.CONFIG_COUNTER, 1);
+ accountsMap.put(AclConstants.CONFIG_TIME_STAMP,
System.currentTimeMillis());
+ versionElement.add(accountsMap);
+
+ updateAclConfigMap.put(AclConstants.CONFIG_DATA_VERSION,
versionElement);
+ final Map<String, Object> map =
plainPermissionManager.updateAclConfigFileVersion(aclFileName,
updateAclConfigMap);
+ Assertions.assertThat(map).isNotEmpty();
+ }
+
+ @Test
+ public void createAclAccessConfigMapTest() {
+ Map<String, Object> existedAccountMap = new HashMap<>();
+ plainAccessConfig.setAccessKey("admin123");
+ plainAccessConfig.setSecretKey("12345678");
+ plainAccessConfig.setWhiteRemoteAddress("192.168.1.1");
+ plainAccessConfig.setAdmin(false);
+ plainAccessConfig.setDefaultGroupPerm(AclConstants.SUB_PUB);
+ plainAccessConfig.setTopicPerms(Arrays.asList(DEFAULT_TOPIC + "=" +
AclConstants.PUB));
+ plainAccessConfig.setGroupPerms(Lists.newArrayList("groupA=SUB"));
+
+ final Map<String, Object> map =
plainPermissionManager.createAclAccessConfigMap(existedAccountMap,
plainAccessConfig);
+ Assertions.assertThat(map).isNotEmpty();
Review Comment:
You'd better add more checks on `map`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
