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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
