xunliu commented on code in PR #5629:
URL: https://github.com/apache/gravitino/pull/5629#discussion_r1858247903
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -278,11 +324,13 @@ protected GrantRevokeRoleRequest
createGrantRevokeRoleRequest(
* @param isOwnerRole The role is owner role or not
*/
protected RangerRole createRangerRoleIfNotExists(String roleName, boolean
isOwnerRole) {
+ roleName = generateGravitinoRoleName(roleName);
if (isOwnerRole) {
Preconditions.checkArgument(
roleName.equalsIgnoreCase(GRAVITINO_METALAKE_OWNER_ROLE)
- || roleName.equalsIgnoreCase(GRAVITINO_CATALOG_OWNER_ROLE),
- "The role name should be GRAVITINO_METALAKE_OWNER_ROLE or
GRAVITINO_CATALOG_OWNER_ROLE");
+ || roleName.equalsIgnoreCase(GRAVITINO_CATALOG_OWNER_ROLE)
+ || roleName.equalsIgnoreCase(GRAVITINO_PLACEHOLDER_OWNER_ROLE),
+ "The role name should be GRAVITINO_METALAKE_OWNER_ROLE or
GRAVITINO_CATALOG_OWNER_ROLE or GRAVITINO_PLACEHOLDER_OWNER_ROLE");
Review Comment:
I think better change
```
"The role name should be GRAVITINO_METALAKE_OWNER_ROLE or
GRAVITINO_CATALOG_OWNER_ROLE or GRAVITINO_PLACEHOLDER_OWNER_ROLE"
```
to
```
String.format("The role name should be %s or %s or %s",
GRAVITINO_METALAKE_OWNER_ROLE, GRAVITINO_CATALOG_OWNER_ROLE,
GRAVITINO_PLACEHOLDER_OWNER_ROLE)
```
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java:
##########
@@ -1163,22 +1216,49 @@ public void testOnRevokedRolesFromGroup() {
rangerAuthHivePlugin, role, null, null, null,
Lists.newArrayList(groupName1));
}
- private void assertFindManagedPolicy(Role role, boolean policyExist) {
- role.securableObjects().stream()
- .forEach(
+ private void assertFindManagedPolicyItems(Role role, boolean
gravitinoPolicyItemExist) {
+ List<RangerPolicy> policies = findRoleResourceRelatedPolicies(role);
+
+ if (gravitinoPolicyItemExist) {
+ Assertions.assertTrue(policies.size() > 0);
+ }
+
+ policies.forEach(
+ policy -> {
+ List<RangerPolicy.RangerPolicyItem> policyItems = new ArrayList<>();
+ policyItems.addAll(policy.getPolicyItems());
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+ if (gravitinoPolicyItemExist) {
+
Assertions.assertTrue(hasGravitinoManagedPolicyItemAccess(policyItems,
role.name()));
+ } else {
+
Assertions.assertFalse(hasGravitinoManagedPolicyItemAccess(policyItems,
role.name()));
+ }
+ });
+ }
+
+ private List<RangerPolicy> findRoleResourceRelatedPolicies(Role role) {
+ return role.securableObjects().stream()
+ .flatMap(
securableObject ->
rangerAuthHivePlugin.translatePrivilege(securableObject).stream()
- .forEach(
+ .map(
rangerSecurableObject -> {
LOG.info("rangerSecurableObject: " +
rangerSecurableObject);
- if (policyExist) {
- Assertions.assertNotNull(
-
rangerHelper.findManagedPolicy(rangerSecurableObject));
- } else {
- Assertions.assertNull(
-
rangerHelper.findManagedPolicy(rangerSecurableObject));
- }
- }));
+ return
rangerHelper.findManagedPolicy(rangerSecurableObject);
+ }))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ }
+
+ public boolean hasGravitinoManagedPolicyItemAccess(
+ List<RangerPolicy.RangerPolicyItem> items, String roleName) {
+ return items.stream()
+ .anyMatch(
+ i ->
Review Comment:
I think it's better to change `i` to `item`.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -242,7 +242,45 @@ public RangerPolicy findManagedPolicy(RangerMetadataObject
rangerMetadataObject)
return policy;
}
+ public boolean
isGravitinoManagedPolicyItemAccess(RangerPolicy.RangerPolicyItem policyItem) {
+ return policyItem.getRoles().stream().anyMatch(role ->
role.startsWith(GRAVITINO_ROLE_PREFIX));
+ }
+
+ public boolean hasGravitinoManagedPolicyItem(RangerPolicy policy) {
+ List<RangerPolicy.RangerPolicyItem> policyItems = policy.getPolicyItems();
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+ return
policyItems.stream().anyMatch(this::isGravitinoManagedPolicyItemAccess);
+ }
+
+ public void removeAllGravitinoManagedPolicyItem(RangerPolicy policy) {
+ try {
+ policy.setPolicyItems(
+ policy.getPolicyItems().stream()
+ .filter(i -> !isGravitinoManagedPolicyItemAccess(i))
Review Comment:
I think better to change `i` to `item`.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -376,6 +425,8 @@ protected void updatePolicyOwner(RangerPolicy policy, Owner
preOwner, Owner newO
} else {
policyItem.getGroups().add(newOwner.name());
}
+ // mark the policy item is created by Gravitino
+ policyItem.getRoles().add(GRAVITINO_PLACEHOLDER_OWNER_ROLE);
Review Comment:
Maybe we need to add judgment？
```
if (!policyItem. getRoles().contains(GRAVITINO_PLACEHOLDER_OWNER_ROLE)) {
policyItem.getRoles().add(GRAVITINO_PLACEHOLDER_OWNER_ROLE);
}
```
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -61,6 +59,11 @@ public class RangerHelper {
public static final String GRAVITINO_METALAKE_OWNER_ROLE =
"GRAVITINO_METALAKE_OWNER_ROLE";
public static final String GRAVITINO_CATALOG_OWNER_ROLE =
"GRAVITINO_CATALOG_OWNER_ROLE";
+ // marking owner policy items
+ public static final String GRAVITINO_PLACEHOLDER_OWNER_ROLE =
"GRAVITINO_PLACEHOLDER_OWNER_ROLE";
+
+ public static final String GRAVITINO_ROLE_PREFIX = "GRAVITINO_";
+
Review Comment:
I think better change these code to
```
public static final String GRAVITINO_ROLE_PREFIX = "GRAVITINO_";
public static final String GRAVITINO_METALAKE_OWNER_ROLE =
GRAVITINO_ROLE_PREFIX + "METALAKE_OWNER_ROLE";
public static final String GRAVITINO_CATALOG_OWNER_ROLE =
GRAVITINO_ROLE_PREFIX + "CATALOG_OWNER_ROLE";
// marking owner policy items
public static final String GRAVITINO_OWNER_ROLE = GRAVITINO_ROLE_PREFIX +
"OWNER_ROLE";
```
The `GRAVITINO_PLACEHOLDER_OWNER_ROLE ` is too long.
I think we need to add a description(`GRAVITINO_ROLE_PREFIX`,
`GRAVITINO_METALAKE_OWNER_ROLE`, `GRAVITINO_CATALOG_OWNER_ROLE` and
`GRAVITINO_OWNER_ROLE`) in the
https://github.com/apache/gravitino/blob/main/docs/security/authorization-pushdown.md
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java:
##########
@@ -372,15 +368,71 @@ public void testManagedByGravitinoLabel() {
SecurableObjects.DOT_SPLITTER.splitToList(securableObject.fullName()));
names.remove(0); // remove catalog node
// Manual create the Ranger Policy
- createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names), false);
+ createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names));
+ });
+ List<String> existingPolicyNames =
+ findRoleResourceRelatedPolicies(role).stream()
+ .map(RangerPolicy::getName)
+ .collect(Collectors.toList());
+ rangerAuthHivePlugin.onRoleCreated(role);
+ findRoleResourceRelatedPolicies(role)
+ .forEach(
+ policy -> {
+ List<RangerPolicy.RangerPolicyItem> policyItems = new
ArrayList<>();
+ policyItems.addAll(policy.getPolicyItems());
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+
+ if (existingPolicyNames.contains(policy.getName())) {
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
+ !i.getRoles()
+ .contains(
+
rangerHelper.generateGravitinoRoleName(role.name()))));
+ }
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
+ i.getRoles()
+
.contains(rangerHelper.generateGravitinoRoleName(role.name()))));
+ });
+
+ rangerAuthHivePlugin.onRoleDeleted(role);
+ findRoleResourceRelatedPolicies(role)
+ .forEach(
+ policy -> {
+ List<RangerPolicy.RangerPolicyItem> policyItems = new
ArrayList<>();
+ policyItems.addAll(policy.getPolicyItems());
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+
+ if (existingPolicyNames.contains(policy.getName())) {
+ Assertions.assertFalse(
+ policyItems.stream()
+ .anyMatch(
+ i ->
Review Comment:
I think it's better to change `i` to `item`.
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java:
##########
@@ -372,15 +368,71 @@ public void testManagedByGravitinoLabel() {
SecurableObjects.DOT_SPLITTER.splitToList(securableObject.fullName()));
names.remove(0); // remove catalog node
// Manual create the Ranger Policy
- createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names), false);
+ createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names));
+ });
+ List<String> existingPolicyNames =
+ findRoleResourceRelatedPolicies(role).stream()
+ .map(RangerPolicy::getName)
+ .collect(Collectors.toList());
+ rangerAuthHivePlugin.onRoleCreated(role);
+ findRoleResourceRelatedPolicies(role)
+ .forEach(
+ policy -> {
+ List<RangerPolicy.RangerPolicyItem> policyItems = new
ArrayList<>();
+ policyItems.addAll(policy.getPolicyItems());
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+
+ if (existingPolicyNames.contains(policy.getName())) {
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
Review Comment:
I think it's better to change `i` to `item`.
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java:
##########
@@ -372,15 +368,71 @@ public void testManagedByGravitinoLabel() {
SecurableObjects.DOT_SPLITTER.splitToList(securableObject.fullName()));
names.remove(0); // remove catalog node
// Manual create the Ranger Policy
- createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names), false);
+ createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names));
+ });
+ List<String> existingPolicyNames =
+ findRoleResourceRelatedPolicies(role).stream()
+ .map(RangerPolicy::getName)
+ .collect(Collectors.toList());
+ rangerAuthHivePlugin.onRoleCreated(role);
+ findRoleResourceRelatedPolicies(role)
+ .forEach(
+ policy -> {
+ List<RangerPolicy.RangerPolicyItem> policyItems = new
ArrayList<>();
+ policyItems.addAll(policy.getPolicyItems());
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+
+ if (existingPolicyNames.contains(policy.getName())) {
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
+ !i.getRoles()
+ .contains(
+
rangerHelper.generateGravitinoRoleName(role.name()))));
+ }
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
+ i.getRoles()
+
.contains(rangerHelper.generateGravitinoRoleName(role.name()))));
+ });
+
+ rangerAuthHivePlugin.onRoleDeleted(role);
+ findRoleResourceRelatedPolicies(role)
+ .forEach(
+ policy -> {
+ List<RangerPolicy.RangerPolicyItem> policyItems = new
ArrayList<>();
+ policyItems.addAll(policy.getPolicyItems());
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+
+ if (existingPolicyNames.contains(policy.getName())) {
+ Assertions.assertFalse(
+ policyItems.stream()
+ .anyMatch(
+ i ->
+ i.getRoles()
+ .contains(
+
rangerHelper.generateGravitinoRoleName(role.name()))));
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
Review Comment:
I think it's better to change `i` to `item`.
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java:
##########
@@ -372,15 +368,71 @@ public void testManagedByGravitinoLabel() {
SecurableObjects.DOT_SPLITTER.splitToList(securableObject.fullName()));
names.remove(0); // remove catalog node
// Manual create the Ranger Policy
- createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names), false);
+ createHivePolicy(Lists.newArrayList(names),
DOT_JOINER.join(names));
+ });
+ List<String> existingPolicyNames =
+ findRoleResourceRelatedPolicies(role).stream()
+ .map(RangerPolicy::getName)
+ .collect(Collectors.toList());
+ rangerAuthHivePlugin.onRoleCreated(role);
+ findRoleResourceRelatedPolicies(role)
+ .forEach(
+ policy -> {
+ List<RangerPolicy.RangerPolicyItem> policyItems = new
ArrayList<>();
+ policyItems.addAll(policy.getPolicyItems());
+ policyItems.addAll(policy.getDenyPolicyItems());
+ policyItems.addAll(policy.getRowFilterPolicyItems());
+ policyItems.addAll(policy.getDataMaskPolicyItems());
+
+ if (existingPolicyNames.contains(policy.getName())) {
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
+ !i.getRoles()
+ .contains(
+
rangerHelper.generateGravitinoRoleName(role.name()))));
+ }
+ Assertions.assertTrue(
+ policyItems.stream()
+ .anyMatch(
+ i ->
Review Comment:
I think it's better to change `i` to `item`.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -454,8 +506,8 @@ protected void updatePolicyOwnerRole(RangerPolicy policy,
String ownerRoleName)
// Add or remove the owner role in the policy item
matchPolicyItems.forEach(
policyItem -> {
- if (!policyItem.getRoles().contains(ownerRoleName)) {
- policyItem.getRoles().add(ownerRoleName);
+ if
(!policyItem.getRoles().contains(generateGravitinoRoleName(ownerRoleName))) {
+
policyItem.getRoles().add(generateGravitinoRoleName(ownerRoleName));
}
Review Comment:
I saw many place need add role into policy item.
I think maybe we need to add a function to add a rule in the policyItem.
```
AddRoleToPolicyItemIfNoExists(PolicyItem policyItem, String roleName)
{
String gravitinoRoleName = generateGravitinoRoleName(roleName);
if (!policyItem.getRoles().contains(gravitinoRoleName) {
policyItem.getRoles().add(gravitinoRoleName);
}
}
```
--
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]
