tengqm commented on code in PR #5629:
URL: https://github.com/apache/gravitino/pull/5629#discussion_r1851201183
##########
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:
Not sure if this function is needed.
##########
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:
Not sure if this is a good pattern.
When you do a `getRoles()` call, I'd guess that you are creating a copy of
the roles.
The `.add()` call is then performed on your new copy.
To make the semantics clear and solid, I'd suggest we refactor policyItem to
add a
`hasRole()` method and `addRole` method.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -224,8 +225,7 @@ public RangerPolicy findManagedPolicy(RangerMetadataObject
rangerMetadataObject)
}
// Only return the policies that are managed by Gravitino.
if (policies.size() > 1) {
- throw new AuthorizationPluginException(
- "Every metadata object has only a Gravitino managed policy.");
+ throw new AuthorizationPluginException("Every metadata object has only a
policy.");
Review Comment:
```suggestion
throw new AuthorizationPluginException("Each metadata object can have
at most one policy.");
```
--
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]
