Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#4620] improvement(authz): Throw the necessary exception when handling Ranger plugin exception #6515

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+73 −49
Open

[#4620] improvement(authz): Throw the necessary exception when handling Ranger plugin exception #6515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ed323b
[#4620] improvement(authz): Throw the necessary exception when Ranger…
jerqi Feb 25, 2025
8e4b01f
change log level
jerqi Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...in/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ public Boolean onRoleAcquired(Role role) throws AuthorizationPluginException {

@Override
public Boolean onRoleDeleted(Role role) throws AuthorizationPluginException {
onRoleUpdated(
role,
role.securableObjects().stream()
.map(securableObject -> RoleChange.removeSecurableObject(role.name(), securableObject))
.toArray(RoleChange[]::new));
return chainedAction(plugin -> plugin.onRoleDeleted(role));
}

Expand Down
94 changes: 49 additions & 45 deletions ...er/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,13 @@ public Boolean onRoleDeleted(Role role) throws AuthorizationPluginException {
rangerClient.deleteRole(
rangerHelper.generateGravitinoRoleName(role.name()), rangerAdminName, rangerServiceName);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Ranger delete role: {} failed!", role, e);
if (e.getMessage().contains("No RangerRole found for name")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The e.getMessage().contains("No RangerRole found for name") is Not a stable way. I think we can use rangerClient.getRole()? to double check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if role doesn't, getRole will return exception. It isn't a good way.

// Ignore exception to support idempotent operation
LOG.info("Ranger delete role: {} failed!", role, e);
} else {
throw new AuthorizationPluginException(
"Fail to delete role %s exception: %s", role, e.getMessage());
}
}
return Boolean.TRUE;
}
Expand All @@ -292,14 +297,13 @@ public Boolean onRoleUpdated(Role role, RoleChange... changes)

List<AuthorizationSecurableObject> authzSecurableObjects =
translatePrivilege(securableObject);
authzSecurableObjects.stream()
.forEach(
authzSecurableObject -> {
if (!doAddSecurableObject(role.name(), authzSecurableObject)) {
throw new AuthorizationPluginException(
"Failed to add the securable object to the Ranger policy!");
}
});
authzSecurableObjects.forEach(
authzSecurableObject -> {
if (!doAddSecurableObject(role.name(), authzSecurableObject)) {
throw new AuthorizationPluginException(
"Failed to add the securable object to the Ranger policy!");
}
});
} else if (change instanceof RoleChange.RemoveSecurableObject) {
SecurableObject securableObject =
((RoleChange.RemoveSecurableObject) change).getSecurableObject();
Expand Down Expand Up @@ -337,16 +341,14 @@ public Boolean onRoleUpdated(Role role, RoleChange... changes)
translatePrivilege(oldSecurableObject);
List<AuthorizationSecurableObject> rangerNewSecurableObjects =
translatePrivilege(newSecurableObject);
rangerOldSecurableObjects.stream()
.forEach(
AuthorizationSecurableObject -> {
removeSecurableObject(role.name(), AuthorizationSecurableObject);
});
rangerNewSecurableObjects.stream()
.forEach(
AuthorizationSecurableObject -> {
doAddSecurableObject(role.name(), AuthorizationSecurableObject);
});
rangerOldSecurableObjects.forEach(
AuthorizationSecurableObject -> {
removeSecurableObject(role.name(), AuthorizationSecurableObject);
});
rangerNewSecurableObjects.forEach(
AuthorizationSecurableObject -> {
doAddSecurableObject(role.name(), AuthorizationSecurableObject);
});
} else {
throw new IllegalArgumentException(
"Unsupported role change type: "
Expand Down Expand Up @@ -499,23 +501,21 @@ public Boolean onOwnerSet(MetadataObject metadataObject, Owner preOwner, Owner n
LOG.warn("Grant owner role: {} failed!", ownerRoleName, e);
}

rangerSecurableObjects.stream()
.forEach(
rangerSecurableObject -> {
RangerPolicy policy = findManagedPolicy(rangerSecurableObject);
try {
if (policy == null) {
policy = addOwnerRoleToNewPolicy(rangerSecurableObject, ownerRoleName);
rangerClient.createPolicy(policy);
} else {
rangerHelper.updatePolicyOwnerRole(policy, ownerRoleName);
rangerClient.updatePolicy(policy.getId(), policy);
}
} catch (RangerServiceException e) {
throw new AuthorizationPluginException(
e, "Failed to add the owner to the Ranger!");
}
});
rangerSecurableObjects.forEach(
rangerSecurableObject -> {
RangerPolicy policy = findManagedPolicy(rangerSecurableObject);
try {
if (policy == null) {
policy = addOwnerRoleToNewPolicy(rangerSecurableObject, ownerRoleName);
rangerClient.createPolicy(policy);
} else {
rangerHelper.updatePolicyOwnerRole(policy, ownerRoleName);
rangerClient.updatePolicy(policy.getId(), policy);
}
} catch (RangerServiceException e) {
throw new AuthorizationPluginException(e, "Failed to add the owner to the Ranger!");
}
});
break;
case SCHEMA:
case TABLE:
Expand Down Expand Up @@ -576,8 +576,9 @@ public Boolean onGrantedRolesToUser(List<Role> roles, User user)
try {
rangerClient.grantRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception, support idempotent operation
LOG.warn("Grant role: {} to user: {} failed!", role, user, e);
throw new AuthorizationPluginException(
"Fail to grant role %s to user %s, exception: %s",
role.name(), user.name(), e.getMessage());
}
});

Expand Down Expand Up @@ -611,8 +612,9 @@ public Boolean onRevokedRolesFromUser(List<Role> roles, User user)
try {
rangerClient.revokeRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Revoke role: {} from user: {} failed!", role, user, e);
throw new AuthorizationPluginException(
"Fail to revoke role %s from user %s, exception: %s",
role.name(), user.name(), e.getMessage());
}
});

Expand Down Expand Up @@ -646,8 +648,9 @@ public Boolean onGrantedRolesToGroup(List<Role> roles, Group group)
try {
rangerClient.grantRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Grant role: {} to group: {} failed!", role, group, e);
throw new AuthorizationPluginException(
"Fail to grant role: %s to group %s, exception: %s.",
role, group, e.getMessage());
}
});
return Boolean.TRUE;
Expand Down Expand Up @@ -678,8 +681,9 @@ public Boolean onRevokedRolesFromGroup(List<Role> roles, Group group)
try {
rangerClient.revokeRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Revoke role: {} from group: {} failed!", role, group, e);
throw new AuthorizationPluginException(
"Fail to revoke role %s from group %s, exception: %s",
role.name(), group.name(), e.getMessage());
}
});

Expand Down
15 changes: 12 additions & 3 deletions ...rization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ protected GrantRevokeRoleRequest createGrantRevokeRoleRequest(
Set<String> groups =
StringUtils.isEmpty(groupName) ? Sets.newHashSet() : Sets.newHashSet(groupName);

if (users.size() == 0 && groups.size() == 0) {
if (users.isEmpty() && groups.isEmpty()) {
throw new AuthorizationPluginException("The user and group cannot be empty!");
}

Expand Down Expand Up @@ -278,9 +278,18 @@ protected RangerRole createRangerRoleIfNotExists(String roleName, boolean isOwne
try {
rangerRole = rangerClient.getRole(roleName, rangerAdminName, rangerServiceName);
} catch (RangerServiceException e) {
// ignore exception, If the role does not exist, then create it.
LOG.warn("The role({}) does not exist in the Ranger!", roleName);

// The client will return a error message contains `doesn't have permission` if the role does
// not exist, then create it.
if (e.getMessage().contains("User doesn't have permissions to get details")) {
LOG.warn("The role({}) does not exist in the Ranger!, e: {}", roleName, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this method ensures that that a specific ranger role exists.
If the role is not there yet, we'll create it.
This means we may want to print an info log rather than a warning, right?

If the user cannot check if a role exists or not, we still allow the function to continue. Does this mean that a user can create a role but he/she cannot view it?
The role becomes Write-Only for him/her?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes, we should use info log.

  2. Usually we use admin user to do these operations, so this case shouldn't occur.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The e.getMessage().contains("User doesn't have permissions to get details") is Not a stable way. I think we can use another way to double check it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The e.getMessage().contains("User doesn't have permissions to get details") is Not a stable way. I think we can use another way to double check it.

Good point. Maybe we need to treat this seriously. For whatever permission related errors, always raise a special exception type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no better way to check this. Do you have suggestion?

} else {
throw new AuthorizationPluginException(
"Failed to check role(%s) whether exists in the Ranger! e: %s",
roleName, e.getMessage());
}
}

try {
if (rangerRole == null) {
rangerRole = new RangerRole(roleName, RangerHelper.MANAGED_BY_GRAVITINO, null, null, null);
Expand Down
8 changes: 7 additions & 1 deletion ...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ public void testOnRoleCreated() {
RoleEntity role = mock3TableRole(currentFunName());
Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role));
verifyRoleInRanger(rangerAuthHivePlugin, role);

// Repeat to create the same to verify the idempotent operation
Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role));
}

@Test
Expand Down Expand Up @@ -266,6 +269,9 @@ public void testOnRoleDeleted() {
Assertions.assertTrue(rangerAuthHivePlugin.onRoleDeleted(role));
// Check if the policy is deleted
assertFindManagedPolicyItems(role, false);

// Repeat to delete the same role to verify the idempotent operation
Assertions.assertTrue(rangerAuthHivePlugin.onRoleDeleted(role));
}

@Test
Expand Down Expand Up @@ -294,7 +300,7 @@ public void testOnRoleDeleted2() {

// delete this role
Assertions.assertTrue(rangerAuthHivePlugin.onRoleDeleted(role));
// Because this metaobject has owner, so the policy should not be deleted
// Because this metadata object has owner, so the policy should not be deleted
assertFindManagedPolicyItems(role, false);
}

Expand Down