From 3ed323bc1bab4673d1d81910424cf6cf29b2d263 Mon Sep 17 00:00:00 2001 From: Rory Date: Tue, 25 Feb 2025 17:27:23 +0800 Subject: [PATCH] [#4620] improvement(authz): Throw the necessary exception when Ranger plugin the exception --- .../chain/ChainedAuthorizationPlugin.java | 5 + .../ranger/RangerAuthorizationPlugin.java | 94 ++++++++++--------- .../authorization/ranger/RangerHelper.java | 15 ++- .../ranger/integration/test/RangerHiveIT.java | 8 +- 4 files changed, 73 insertions(+), 49 deletions(-) diff --git a/authorizations/authorization-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java b/authorizations/authorization-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java index 982ee38cd2e..1ff842616eb 100644 --- a/authorizations/authorization-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java +++ b/authorizations/authorization-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java @@ -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)); } diff --git a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java index 7292d537452..4f114c63985 100644 --- a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java +++ b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java @@ -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")) { + // Ignore exception to support idempotent operation + LOG.warn("Ranger delete role: {} failed!", role, e); + } else { + throw new AuthorizationPluginException( + "Fail to delete role %s exception: %s", role, e.getMessage()); + } } return Boolean.TRUE; } @@ -292,14 +297,13 @@ public Boolean onRoleUpdated(Role role, RoleChange... changes) List 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(); @@ -337,16 +341,14 @@ public Boolean onRoleUpdated(Role role, RoleChange... changes) translatePrivilege(oldSecurableObject); List 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: " @@ -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: @@ -576,8 +576,9 @@ public Boolean onGrantedRolesToUser(List 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()); } }); @@ -611,8 +612,9 @@ public Boolean onRevokedRolesFromUser(List 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()); } }); @@ -646,8 +648,9 @@ public Boolean onGrantedRolesToGroup(List 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; @@ -678,8 +681,9 @@ public Boolean onRevokedRolesFromGroup(List 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()); } }); diff --git a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java index 1ec65daea22..a4e3e15f93f 100644 --- a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java +++ b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java @@ -236,7 +236,7 @@ protected GrantRevokeRoleRequest createGrantRevokeRoleRequest( Set 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!"); } @@ -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); + } 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); diff --git a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java index 6a32ab9ea2a..0198065b9a6 100644 --- a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java +++ b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java @@ -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 @@ -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 @@ -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); }