Skip to content

Commit

Permalink
[#4620] improvement(authz): Throw the necessary exception when handli…
Browse files Browse the repository at this point in the history 
…ng Ranger plugin exception (#6515)

### What changes were proposed in this pull request?

Don't ignore the necessary exception. We should throw necessary
exception from underlying system,

### Why are the changes needed?

Fix: #4620

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

New uts and existing uts.
  • Loading branch information
@jerqi
jerqi authored Mar 4, 2025
1 parent 783f990 commit 873c6af
Show file tree
Hide file tree
Showing 4 changed files with 201 additions and 62 deletions.
5 changes: 5 additions & 0 deletions ...in/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java
View file
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
118 changes: 71 additions & 47 deletions ...er/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ public abstract class RangerAuthorizationPlugin

protected String metalake;
protected final String rangerServiceName;
protected final RangerClientExtension rangerClient;
protected final RangerHelper rangerHelper;
protected RangerClientExtension rangerClient;
protected RangerHelper rangerHelper;
@VisibleForTesting public final String rangerAdminName;

protected RangerAuthorizationPlugin(String metalake, Map<String, String> config) {
Expand Down Expand Up @@ -108,6 +108,26 @@ public String getMetalake() {
return metalake;
}

@VisibleForTesting
public RangerHelper getRangerHelper() {
return rangerHelper;
}

@VisibleForTesting
public void setRangerHelper(RangerHelper rangerHelper) {
this.rangerHelper = rangerHelper;
}

@VisibleForTesting
public RangerClientExtension getRangerClient() {
return rangerClient;
}

@VisibleForTesting
public void setRangerClient(RangerClientExtension rangerClient) {
this.rangerClient = rangerClient;
}

/**
* Set the Ranger policy resource defines rule.
*
Expand Down Expand Up @@ -273,8 +293,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 (rangerHelper.getRangerRole(role.name()) == null) {
// 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 +317,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 +361,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 +521,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 +596,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 +632,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 +668,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 +701,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
31 changes: 23 additions & 8 deletions ...rization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java
View file
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 @@ -274,13 +274,8 @@ protected RangerRole createRangerRoleIfNotExists(String roleName, boolean isOwne
GRAVITINO_METALAKE_OWNER_ROLE, GRAVITINO_CATALOG_OWNER_ROLE, GRAVITINO_OWNER_ROLE));
}

RangerRole rangerRole = null;
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);
}
RangerRole rangerRole = getRangerRole(roleName);

try {
if (rangerRole == null) {
rangerRole = new RangerRole(roleName, RangerHelper.MANAGED_BY_GRAVITINO, null, null, null);
Expand All @@ -293,6 +288,26 @@ protected RangerRole createRangerRoleIfNotExists(String roleName, boolean isOwne
return rangerRole;
}

public RangerRole getRangerRole(String roleName) {
RangerRole rangerRole = null;
try {
rangerRole = rangerClient.getRole(roleName, rangerAdminName, rangerServiceName);
} catch (RangerServiceException e) {

// The client will return a error message contains `doesn't have permission` if the role does
// not exist, then create it.
if (e.getMessage() != null
&& e.getMessage().contains("User doesn't have permissions to get details")) {
LOG.warn("The role({}) does not exist in the Ranger!, e: {}", roleName, e);
} else {
throw new AuthorizationPluginException(
"Failed to check role(%s) whether exists in the Ranger! e: %s",
roleName, e.getMessage());
}
}
return rangerRole;
}

protected void updatePolicyOwner(RangerPolicy policy, Owner preOwner, Owner newOwner) {
// Find matching policy items based on the owner's privileges
List<RangerPolicy.RangerPolicyItem> matchPolicyItems =
Expand Down
Loading

0 comments on commit 873c6af

Please sign in to comment.