You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/10/08 02:48:25 UTC
[GitHub] [rocketmq] caigy commented on a diff in pull request #4769: [ISSUE #4688]acl module support namespace.
caigy commented on code in PR #4769:
URL: https://github.com/apache/rocketmq/pull/4769#discussion_r990571364
##########
acl/src/main/java/org/apache/rocketmq/acl/common/AclUtils.java:
##########
@@ -256,6 +260,10 @@ public static <T> T getYamlDataObject(InputStream fis, Class<T> clazz) {
public static boolean writeDataObject(String path, Map<String, Object> dataMap) {
Yaml yaml = new Yaml();
+ if (new File(path).exists()) {
Review Comment:
May be better to use `Files.exists()` to check existence of path.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainAccessResource.java:
##########
@@ -281,6 +380,43 @@ public static PlainAccessResource build(PlainAccessConfig plainAccessConfig, Rem
Permission.parseResourcePerms(plainAccessResource, false, plainAccessConfig.getGroupPerms());
Permission.parseResourcePerms(plainAccessResource, true, plainAccessConfig.getTopicPerms());
+ List<ResourceAndPerm> resourcePerms = plainAccessConfig.getResourcePerms();
+ if (resourcePerms != null && !resourcePerms.isEmpty()) {
+ for (ResourceAndPerm resource : resourcePerms) {
+ String resourceName = resource.getResource();
+ ResourceType type = resource.getType();
+ String namespace = resource.getNamespace();
+ String perm = resource.getPerm();
+ if (type == ResourceType.GROUP) {
+ String key = namespace == null ? PlainAccessResource.getRetryTopic(resourceName) :
+ MixAll.RETRY_GROUP_TOPIC_PREFIX + namespace + NAMESPACE_SEPARATOR + resourceName;
Review Comment:
Build `resourceName` for resource with namespace, then call `PlainAccessResource.getRetryTopic()`.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainAccessResource.java:
##########
@@ -109,38 +116,92 @@ public static PlainAccessResource parse(RemotingCommand request, String remoteAd
accessResource.setSignature(request.getExtFields().get(SessionCredentials.SIGNATURE));
accessResource.setSecretToken(request.getExtFields().get(SessionCredentials.SECURITY_TOKEN));
+ String namespace = new String();
+ String resource = new String();
try {
switch (request.getCode()) {
case RequestCode.SEND_MESSAGE:
final String topic = request.getExtFields().get("topic");
if (PlainAccessResource.isRetryTopic(topic)) {
- accessResource.addResourceAndPerm(getRetryTopic(request.getExtFields().get("group")), Permission.SUB);
+ String group = request.getExtFields().get("group");
+ if (group != null && group.contains(String.valueOf(NAMESPACE_SEPARATOR))) {
+ namespace = group.substring(0, group.indexOf(NAMESPACE_SEPARATOR));
+ group = group.substring(group.indexOf(NAMESPACE_SEPARATOR) + 1);
+ }
+ if (!namespace.isEmpty()) {
+ resource = MixAll.RETRY_GROUP_TOPIC_PREFIX + namespace + NAMESPACE_SEPARATOR + group;
+ } else {
+ resource = getRetryTopic(group);
+ }
+ accessResource.addResourceAndPerm(resource, Permission.SUB);
Review Comment:
Is it necessary? It seems that `getRetryTopic()` will get the same result.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionChecker.java:
##########
@@ -30,36 +31,82 @@ public void check(AccessResource checkedAccess, AccessResource ownedAccess) {
if (Permission.needAdminPerm(checkedPlainAccess.getRequestCode()) && !ownedPlainAccess.isAdmin()) {
throw new AclException(String.format("Need admin permission for request code=%d, but accessKey=%s is not", checkedPlainAccess.getRequestCode(), ownedPlainAccess.getAccessKey()));
}
- Map<String, Byte> needCheckedPermMap = checkedPlainAccess.getResourcePermMap();
- Map<String, Byte> ownedPermMap = ownedPlainAccess.getResourcePermMap();
+ Map<String, Byte> needCheckedResourcePermMap = checkedPlainAccess.getResourcePermMap();
+ Map<String, Byte> ownedResourcePermMap = ownedPlainAccess.getResourcePermMap();
+ Map<String, Map<String, Byte>> ownedNamespacePermMap = ownedPlainAccess.getNamespacePermMap();
- if (needCheckedPermMap == null) {
+ if (needCheckedResourcePermMap == null) {
// If the needCheckedPermMap is null,then return
return;
}
- if (ownedPermMap == null && ownedPlainAccess.isAdmin()) {
+ if (ownedResourcePermMap == null && ownedPlainAccess.isAdmin()) {
// If the ownedPermMap is null and it is an admin user, then return
return;
}
- for (Map.Entry<String, Byte> needCheckedEntry : needCheckedPermMap.entrySet()) {
+ for (Map.Entry<String, Byte> needCheckedEntry : needCheckedResourcePermMap.entrySet()) {
String resource = needCheckedEntry.getKey();
Byte neededPerm = needCheckedEntry.getValue();
boolean isGroup = PlainAccessResource.isRetryTopic(resource);
+ boolean isResourceContainsNamespace = PlainAccessResource.isContainNamespace(resource);
- if (ownedPermMap == null || !ownedPermMap.containsKey(resource)) {
- // Check the default perm
- byte ownedPerm = isGroup ? ownedPlainAccess.getDefaultGroupPerm() :
- ownedPlainAccess.getDefaultTopicPerm();
- if (!Permission.checkPermission(neededPerm, ownedPerm)) {
+ //the resource perm that ak owned is null or doesn't contain the resource
+ if (ownedResourcePermMap == null || !ownedResourcePermMap.containsKey(resource)) {
+ //check the namespace perm and the default perm
+ if (isMatchNamespaceAndDeafultPerm(isResourceContainsNamespace, resource, ownedNamespacePermMap, isGroup, neededPerm, ownedPlainAccess)) {
+ continue;
+ } else {
throw new AclException(String.format("No default permission for %s", PlainAccessResource.printStr(resource, isGroup)));
}
+ } else {
+ //check whether the resource perm that the ak owned is match the needed
+ if (!Permission.checkPermission(neededPerm, ownedResourcePermMap.get(resource))) {
+ //check the namespace perm and the default perm
+ if (isMatchNamespaceAndDeafultPerm(isResourceContainsNamespace, resource, ownedNamespacePermMap, isGroup, neededPerm, ownedPlainAccess)) {
+ continue;
+ } else {
+ throw new AclException(String.format("No default permission for %s", PlainAccessResource.printStr(resource, isGroup)));
+ }
+ }
continue;
}
- if (!Permission.checkPermission(neededPerm, ownedPermMap.get(resource))) {
- throw new AclException(String.format("No default permission for %s", PlainAccessResource.printStr(resource, isGroup)));
+ }
+ }
+
+ public boolean isMatchNamespaceAndDeafultPerm(boolean isResourceContainsNamespace, String resource, Map<String, Map<String, Byte>> ownedNamespacePermMap,
Review Comment:
`isMatchNamespaceOrDeafultPerm()` may be more accurate.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainAccessResource.java:
##########
@@ -281,6 +380,43 @@ public static PlainAccessResource build(PlainAccessConfig plainAccessConfig, Rem
Permission.parseResourcePerms(plainAccessResource, false, plainAccessConfig.getGroupPerms());
Permission.parseResourcePerms(plainAccessResource, true, plainAccessConfig.getTopicPerms());
+ List<ResourceAndPerm> resourcePerms = plainAccessConfig.getResourcePerms();
+ if (resourcePerms != null && !resourcePerms.isEmpty()) {
+ for (ResourceAndPerm resource : resourcePerms) {
+ String resourceName = resource.getResource();
+ ResourceType type = resource.getType();
+ String namespace = resource.getNamespace();
+ String perm = resource.getPerm();
+ if (type == ResourceType.GROUP) {
+ String key = namespace == null ? PlainAccessResource.getRetryTopic(resourceName) :
+ MixAll.RETRY_GROUP_TOPIC_PREFIX + namespace + NAMESPACE_SEPARATOR + resourceName;
+ plainAccessResource.addResourceAndPerm(key, Permission.parsePermFromString(perm));
+ } else if (type == ResourceType.TOPIC) {
+ String key = namespace == null ? resourceName : namespace + NAMESPACE_SEPARATOR + resourceName;
+ plainAccessResource.addResourceAndPerm(key, Permission.parsePermFromString(perm));
Review Comment:
use `org.apache.rocketmq.common.protocol.NamespaceUtil#wrapNamespace` to build resource with namespace.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionChecker.java:
##########
@@ -30,36 +31,82 @@ public void check(AccessResource checkedAccess, AccessResource ownedAccess) {
if (Permission.needAdminPerm(checkedPlainAccess.getRequestCode()) && !ownedPlainAccess.isAdmin()) {
throw new AclException(String.format("Need admin permission for request code=%d, but accessKey=%s is not", checkedPlainAccess.getRequestCode(), ownedPlainAccess.getAccessKey()));
}
- Map<String, Byte> needCheckedPermMap = checkedPlainAccess.getResourcePermMap();
- Map<String, Byte> ownedPermMap = ownedPlainAccess.getResourcePermMap();
+ Map<String, Byte> needCheckedResourcePermMap = checkedPlainAccess.getResourcePermMap();
+ Map<String, Byte> ownedResourcePermMap = ownedPlainAccess.getResourcePermMap();
+ Map<String, Map<String, Byte>> ownedNamespacePermMap = ownedPlainAccess.getNamespacePermMap();
- if (needCheckedPermMap == null) {
+ if (needCheckedResourcePermMap == null) {
// If the needCheckedPermMap is null,then return
return;
}
- if (ownedPermMap == null && ownedPlainAccess.isAdmin()) {
+ if (ownedResourcePermMap == null && ownedPlainAccess.isAdmin()) {
// If the ownedPermMap is null and it is an admin user, then return
return;
}
- for (Map.Entry<String, Byte> needCheckedEntry : needCheckedPermMap.entrySet()) {
+ for (Map.Entry<String, Byte> needCheckedEntry : needCheckedResourcePermMap.entrySet()) {
String resource = needCheckedEntry.getKey();
Byte neededPerm = needCheckedEntry.getValue();
boolean isGroup = PlainAccessResource.isRetryTopic(resource);
+ boolean isResourceContainsNamespace = PlainAccessResource.isContainNamespace(resource);
- if (ownedPermMap == null || !ownedPermMap.containsKey(resource)) {
- // Check the default perm
- byte ownedPerm = isGroup ? ownedPlainAccess.getDefaultGroupPerm() :
- ownedPlainAccess.getDefaultTopicPerm();
- if (!Permission.checkPermission(neededPerm, ownedPerm)) {
+ //the resource perm that ak owned is null or doesn't contain the resource
+ if (ownedResourcePermMap == null || !ownedResourcePermMap.containsKey(resource)) {
+ //check the namespace perm and the default perm
+ if (isMatchNamespaceAndDeafultPerm(isResourceContainsNamespace, resource, ownedNamespacePermMap, isGroup, neededPerm, ownedPlainAccess)) {
+ continue;
+ } else {
throw new AclException(String.format("No default permission for %s", PlainAccessResource.printStr(resource, isGroup)));
}
+ } else {
+ //check whether the resource perm that the ak owned is match the needed
+ if (!Permission.checkPermission(neededPerm, ownedResourcePermMap.get(resource))) {
+ //check the namespace perm and the default perm
+ if (isMatchNamespaceAndDeafultPerm(isResourceContainsNamespace, resource, ownedNamespacePermMap, isGroup, neededPerm, ownedPlainAccess)) {
+ continue;
Review Comment:
It seem that we should not check namespace or default permission here, e.g.: If ak owns `DENY` permission, it is not allowed to access the resource even namespace or default permission is `PUB` or `SUB`.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainAccessResource.java:
##########
@@ -281,6 +380,43 @@ public static PlainAccessResource build(PlainAccessConfig plainAccessConfig, Rem
Permission.parseResourcePerms(plainAccessResource, false, plainAccessConfig.getGroupPerms());
Permission.parseResourcePerms(plainAccessResource, true, plainAccessConfig.getTopicPerms());
+ List<ResourceAndPerm> resourcePerms = plainAccessConfig.getResourcePerms();
+ if (resourcePerms != null && !resourcePerms.isEmpty()) {
+ for (ResourceAndPerm resource : resourcePerms) {
+ String resourceName = resource.getResource();
+ ResourceType type = resource.getType();
+ String namespace = resource.getNamespace();
+ String perm = resource.getPerm();
+ if (type == ResourceType.GROUP) {
+ String key = namespace == null ? PlainAccessResource.getRetryTopic(resourceName) :
+ MixAll.RETRY_GROUP_TOPIC_PREFIX + namespace + NAMESPACE_SEPARATOR + resourceName;
+ plainAccessResource.addResourceAndPerm(key, Permission.parsePermFromString(perm));
+ } else if (type == ResourceType.TOPIC) {
+ String key = namespace == null ? resourceName : namespace + NAMESPACE_SEPARATOR + resourceName;
+ plainAccessResource.addResourceAndPerm(key, Permission.parsePermFromString(perm));
+ }
+ }
+ }
+
+ List<NamespaceAndPerm> namespacePerms = plainAccessConfig.getNamespacePerms();
+ if (namespacePerms != null && !namespacePerms.isEmpty()) {
+ Map<String, Map<String, Byte>> namespacePermMap = new HashMap<>();
+ for (NamespaceAndPerm namespace : namespacePerms) {
+ String namespaceName = namespace.getNamespace();
+ String topicPerm = namespace.getTopicPerm();
+ String groupPerm = namespace.getGroupPerm();
+ Map<String, Byte> permMap = new HashMap<>(2);
Review Comment:
Use `com.google.common.collect.Maps#newHashMapWithExpectedSize` to prevent resizing, or `java.util.Collections#singletonMap` if only one entry in this map.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainAccessResource.java:
##########
@@ -289,6 +425,42 @@ public static boolean isRetryTopic(String topic) {
return null != topic && topic.startsWith(MixAll.RETRY_GROUP_TOPIC_PREFIX);
}
+ public static boolean isContainNamespace(String resource) {
Review Comment:
Use methods provided in `NamespaceUtil` if possible, the logic of processing namespace should not be included in ACL module.
##########
acl/src/main/java/org/apache/rocketmq/acl/common/AclUtils.java:
##########
@@ -256,6 +260,10 @@ public static <T> T getYamlDataObject(InputStream fis, Class<T> clazz) {
public static boolean writeDataObject(String path, Map<String, Object> dataMap) {
Yaml yaml = new Yaml();
+ if (new File(path).exists()) {
+ String pathBak = path + ".bak";
+ copyFile(path, pathBak);
Review Comment:
use `Files.copy()` instead of recreating one.
##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainAccessResource.java:
##########
@@ -281,6 +380,43 @@ public static PlainAccessResource build(PlainAccessConfig plainAccessConfig, Rem
Permission.parseResourcePerms(plainAccessResource, false, plainAccessConfig.getGroupPerms());
Permission.parseResourcePerms(plainAccessResource, true, plainAccessConfig.getTopicPerms());
+ List<ResourceAndPerm> resourcePerms = plainAccessConfig.getResourcePerms();
Review Comment:
It may be better to wrap the process of converting new resource and namespace perm in `Permission`.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org