xunliu commented on code in PR #5629:
URL: https://github.com/apache/gravitino/pull/5629#discussion_r1851237431
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -142,7 +144,7 @@ void addPolicyItem(RangerPolicy policy, String roleName,
RangerSecurableObject s
new RangerPolicy.RangerPolicyItemAccess();
access.setType(rangerPrivilege.getName());
policyItem.getAccesses().add(access);
- policyItem.getRoles().add(roleName);
+ policyItem.getRoles().add(GRAVITINO_ROLE_PREFIX + roleName);
Review Comment:
I think we need use `generateGravitinoRoleName()` function replace
`GRAVITINO_ROLE_PREFIX + roleName`.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -252,8 +269,16 @@ protected boolean checkRangerRole(String roleName) throws
AuthorizationPluginExc
return true;
}
+ String rangerRoleName(String roleName) {
Review Comment:
Maybe `generateGravitinoRoleName` is a better name.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -234,15 +234,32 @@ public RangerPolicy
findManagedPolicy(RangerMetadataObject rangerMetadataObject)
RangerPolicy policy = policies.get(0);
// Delegating Gravitino management policies cannot contain duplicate
privilege
- policy.getPolicyItems().forEach(this::checkPolicyItemAccess);
- policy.getDenyPolicyItems().forEach(this::checkPolicyItemAccess);
- policy.getRowFilterPolicyItems().forEach(this::checkPolicyItemAccess);
- policy.getDataMaskPolicyItems().forEach(this::checkPolicyItemAccess);
+ policy.getPolicyItems().stream()
+ .filter(this::isGravitinoManagedPolicyItemAccess)
+ .forEach(this::checkPolicyItemAccess);
+ policy.getDenyPolicyItems().stream()
+ .filter(this::isGravitinoManagedPolicyItemAccess)
+ .forEach(this::checkPolicyItemAccess);
+ policy.getRowFilterPolicyItems().stream()
+ .filter(this::isGravitinoManagedPolicyItemAccess)
+ .forEach(this::checkPolicyItemAccess);
+ policy.getDataMaskPolicyItems().stream()
+ .filter(this::isGravitinoManagedPolicyItemAccess)
+ .forEach(this::checkPolicyItemAccess);
return policy;
}
+ boolean isGravitinoManagedPolicyItemAccess(RangerPolicy.RangerPolicyItem
policyItem) {
+ return policyItem.getRoles().stream().anyMatch(role ->
role.startsWith(GRAVITINO_ROLE_PREFIX));
+ }
+
+ boolean isNotGravitinoManagedPolicyItemAccess(RangerPolicy.RangerPolicyItem
policyItem) {
+ return !isGravitinoManagedPolicyItemAccess(policyItem);
Review Comment:
hi @theoryxu thank you for help review, :-)
This function to support method reference call in the here,
```
policy.setDenyPolicyItems(
policy.getDenyPolicyItems().stream()
.filter(rangerHelper::isNotGravitinoManagedPolicyItemAccess)
.collect(Collectors.toList()));
```
If we don't have this function, we can modify to
```
.filter(policyItem ->
!rangerHelper.isGravitinoManagedPolicyItemAccess(policyItem))
```
I think it's all right.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -154,8 +156,8 @@ void addPolicyItem(RangerPolicy policy, String roleName,
RangerSecurableObject s
.forEach(
policyItem -> {
// If the role is not in the policy item, then add it
- if (!policyItem.getRoles().contains(roleName)) {
- policyItem.getRoles().add(roleName);
+ if
(!policyItem.getRoles().contains(GRAVITINO_ROLE_PREFIX + roleName)) {
+ policyItem.getRoles().add(GRAVITINO_ROLE_PREFIX +
roleName);
Review Comment:
I think we need use `generateGravitinoRoleName()` function replace
`GRAVITINO_ROLE_PREFIX + roleName`.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -385,7 +411,6 @@ protected RangerPolicy
createPolicyAddResources(RangerMetadataObject metadataObj
RangerPolicy policy = new RangerPolicy();
policy.setService(rangerServiceName);
policy.setName(metadataObject.fullName());
-
policy.setPolicyLabels(Lists.newArrayList(RangerHelper.MANAGED_BY_GRAVITINO));
Review Comment:
Please also remove `RangerHelper.MANAGED_BY_GRAVITINO` variable define.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -480,8 +505,8 @@ protected void updatePolicyOwnerRole(RangerPolicy policy,
String ownerRoleName)
policyItem
.getAccesses()
.add(new
RangerPolicy.RangerPolicyItemAccess(ownerPrivilege.getName()));
- if (!policyItem.getRoles().contains(ownerRoleName)) {
- policyItem.getRoles().add(ownerRoleName);
+ if
(!policyItem.getRoles().contains(rangerRoleName(ownerRoleName))) {
+ policyItem.getRoles().add(rangerRoleName(ownerRoleName));
Review Comment:
I don't feel there's anything wrong with this code.
--
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]
