From 873c6afeb3b0a5ebdb6d64b1d279442ac5e3115c Mon Sep 17 00:00:00 2001 From: roryqi Date: Tue, 4 Mar 2025 12:04:51 +0800 Subject: [PATCH] [#4620] improvement(authz): Throw the necessary exception when handling 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. --- .../chain/ChainedAuthorizationPlugin.java | 5 + .../ranger/RangerAuthorizationPlugin.java | 118 +++++++++++------- .../authorization/ranger/RangerHelper.java | 31 +++-- .../ranger/integration/test/RangerHiveIT.java | 109 ++++++++++++++-- 4 files changed, 201 insertions(+), 62 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..01e743173d3 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 @@ -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 config) { @@ -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. * @@ -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; } @@ -292,14 +317,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 +361,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 +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: @@ -576,8 +596,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 +632,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 +668,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 +701,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..881e6790d9e 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!"); } @@ -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); @@ -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 matchPolicyItems = 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..37370edf4ef 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 @@ -21,6 +21,8 @@ import static org.apache.gravitino.authorization.ranger.integration.test.RangerITEnv.currentFunName; import static org.apache.gravitino.authorization.ranger.integration.test.RangerITEnv.rangerClient; import static org.apache.gravitino.authorization.ranger.integration.test.RangerITEnv.verifyRoleInRanger; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; @@ -48,11 +50,14 @@ import org.apache.gravitino.authorization.SecurableObject; import org.apache.gravitino.authorization.SecurableObjects; import org.apache.gravitino.authorization.ranger.RangerAuthorizationPlugin; +import org.apache.gravitino.authorization.ranger.RangerClientExtension; import org.apache.gravitino.authorization.ranger.RangerHadoopSQLMetadataObject; import org.apache.gravitino.authorization.ranger.RangerHadoopSQLSecurableObject; import org.apache.gravitino.authorization.ranger.RangerHelper; import org.apache.gravitino.authorization.ranger.RangerPrivileges; import org.apache.gravitino.authorization.ranger.reference.RangerDefines; +import org.apache.gravitino.authorization.ranger.reference.VXUserList; +import org.apache.gravitino.exceptions.AuthorizationPluginException; import org.apache.gravitino.integration.test.util.GravitinoITUtils; import org.apache.gravitino.meta.AuditInfo; import org.apache.gravitino.meta.GroupEntity; @@ -60,12 +65,15 @@ import org.apache.gravitino.meta.UserEntity; import org.apache.ranger.RangerServiceException; import org.apache.ranger.plugin.model.RangerPolicy; +import org.apache.ranger.plugin.model.RangerRole; +import org.glassfish.jersey.internal.guava.Sets; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -134,10 +142,29 @@ public RoleEntity mock3TableRole(String roleName) { // Use the different db.table different privilege to test OnRoleCreated() @Test - public void testOnRoleCreated() { + public void testOnRoleCreated() throws Exception { RoleEntity role = mock3TableRole(currentFunName()); + + // test to throw an exception + RangerClientExtension client = Mockito.mock(RangerClientExtension.class); + RangerClientExtension originClient = rangerAuthHivePlugin.getRangerClient(); + rangerAuthHivePlugin.setRangerClient(client); + RangerHelper originHelper = rangerAuthHivePlugin.getRangerHelper(); + + RangerHelper helper = + new RangerHelper(client, "test", "test", Sets.newHashSet(), Lists.newArrayList()); + rangerAuthHivePlugin.setRangerHelper(helper); + when(client.createRole(any(), any())).thenThrow(new RangerServiceException(new Exception(""))); + Assertions.assertThrows( + AuthorizationPluginException.class, () -> rangerAuthHivePlugin.onRoleCreated(role)); + rangerAuthHivePlugin.setRangerClient(originClient); + rangerAuthHivePlugin.setRangerHelper(originHelper); + Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); verifyRoleInRanger(rangerAuthHivePlugin, role); + + // Repeat to create the same to verify the idempotent operation + Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); } @Test @@ -256,7 +283,7 @@ public void testOnDenyRoleCreatedCatalog() { } @Test - public void testOnRoleDeleted() { + public void testOnRoleDeleted() throws Exception { // prepare to create a role RoleEntity role = mock3TableRole(currentFunName()); Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); @@ -266,6 +293,26 @@ 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 to throw an exception + RangerClientExtension client = Mockito.mock(RangerClientExtension.class); + RangerClientExtension originClient = rangerAuthHivePlugin.getRangerClient(); + RangerHelper originHelper = rangerAuthHivePlugin.getRangerHelper(); + rangerAuthHivePlugin.setRangerClient(client); + + RangerHelper helper = Mockito.mock(RangerHelper.class); + rangerAuthHivePlugin.setRangerHelper(helper); + Mockito.doThrow(new RangerServiceException(new Exception("test"))) + .when(client) + .deleteRole(any(), any(), any()); + Mockito.when(helper.getRangerRole(any())).thenReturn(Mockito.mock(RangerRole.class)); + Assertions.assertThrows( + AuthorizationPluginException.class, () -> rangerAuthHivePlugin.onRoleDeleted(role)); + rangerAuthHivePlugin.setRangerClient(originClient); + rangerAuthHivePlugin.setRangerHelper(originHelper); } @Test @@ -294,7 +341,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); } @@ -1083,7 +1130,7 @@ public void testRoleChangeCombinedOperation() { } @Test - public void testOnGrantedRolesToUser() { + public void testOnGrantedRolesToUser() throws Exception { // prepare to create a role RoleEntity role = mock3TableRole(currentFunName()); Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); @@ -1107,6 +1154,18 @@ public void testOnGrantedRolesToUser() { rangerAuthHivePlugin.onGrantedRolesToUser(Lists.newArrayList(role), userEntity1)); verifyRoleInRanger(rangerAuthHivePlugin, role, Lists.newArrayList(userName1)); + // test to throw an exception + RangerClientExtension client = Mockito.mock(RangerClientExtension.class); + RangerClientExtension originClient = rangerAuthHivePlugin.getRangerClient(); + rangerAuthHivePlugin.setRangerClient(client); + when(client.searchUser(any())).thenReturn(Mockito.mock(VXUserList.class)); + when(client.grantRole(any(), any())) + .thenThrow(new RangerServiceException(new Exception("test"))); + Assertions.assertThrows( + AuthorizationPluginException.class, + () -> rangerAuthHivePlugin.onGrantedRolesToUser(Lists.newArrayList(role), userEntity1)); + rangerAuthHivePlugin.setRangerClient(originClient); + // granted a role to the user2 String userName2 = "user2"; UserEntity userEntity2 = @@ -1125,7 +1184,7 @@ public void testOnGrantedRolesToUser() { } @Test - public void testOnRevokedRolesFromUser() { + public void testOnRevokedRolesFromUser() throws Exception { // prepare to create a role RoleEntity role = mock3TableRole(currentFunName()); Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); @@ -1152,10 +1211,22 @@ public void testOnRevokedRolesFromUser() { Assertions.assertTrue( rangerAuthHivePlugin.onRevokedRolesFromUser(Lists.newArrayList(role), userEntity1)); verifyRoleInRanger(rangerAuthHivePlugin, role, null, Lists.newArrayList(userName1)); + + // test to throw an exception + RangerClientExtension client = Mockito.mock(RangerClientExtension.class); + RangerClientExtension originClient = rangerAuthHivePlugin.getRangerClient(); + rangerAuthHivePlugin.setRangerClient(client); + when(client.searchUser(any())).thenReturn(Mockito.mock(VXUserList.class)); + when(client.revokeRole(any(), any())) + .thenThrow(new RangerServiceException(new Exception("test"))); + Assertions.assertThrows( + AuthorizationPluginException.class, + () -> rangerAuthHivePlugin.onRevokedRolesFromUser(Lists.newArrayList(role), userEntity1)); + rangerAuthHivePlugin.setRangerClient(originClient); } @Test - public void testOnGrantedRolesToGroup() { + public void testOnGrantedRolesToGroup() throws Exception { // prepare to create a role RoleEntity role = mock3TableRole(currentFunName()); Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); @@ -1179,6 +1250,18 @@ public void testOnGrantedRolesToGroup() { rangerAuthHivePlugin.onGrantedRolesToGroup(Lists.newArrayList(role), groupEntity1)); verifyRoleInRanger(rangerAuthHivePlugin, role, null, null, Lists.newArrayList(groupName1)); + // test to throw an exception + RangerClientExtension client = Mockito.mock(RangerClientExtension.class); + RangerClientExtension originClient = rangerAuthHivePlugin.getRangerClient(); + rangerAuthHivePlugin.setRangerClient(client); + when(client.createGroup(any())).thenReturn(true); + when(client.grantRole(any(), any())) + .thenThrow(new RangerServiceException(new Exception("test"))); + Assertions.assertThrows( + AuthorizationPluginException.class, + () -> rangerAuthHivePlugin.onGrantedRolesToGroup(Lists.newArrayList(role), groupEntity1)); + rangerAuthHivePlugin.setRangerClient(originClient); + // granted a role to the group2 String groupName2 = "group2"; GroupEntity groupEntity2 = @@ -1198,7 +1281,7 @@ public void testOnGrantedRolesToGroup() { } @Test - public void testOnRevokedRolesFromGroup() { + public void testOnRevokedRolesFromGroup() throws Exception { // prepare to create a role RoleEntity role = mock3TableRole(currentFunName()); Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); @@ -1227,6 +1310,18 @@ public void testOnRevokedRolesFromGroup() { rangerAuthHivePlugin.onRevokedRolesFromGroup(Lists.newArrayList(role), groupEntity1)); verifyRoleInRanger( rangerAuthHivePlugin, role, null, null, null, Lists.newArrayList(groupName1)); + + // test to throw an exception + RangerClientExtension client = Mockito.mock(RangerClientExtension.class); + RangerClientExtension originClient = rangerAuthHivePlugin.getRangerClient(); + rangerAuthHivePlugin.setRangerClient(client); + when(client.createGroup(any())).thenReturn(true); + when(client.revokeRole(any(), any())) + .thenThrow(new RangerServiceException(new Exception("test"))); + Assertions.assertThrows( + AuthorizationPluginException.class, + () -> rangerAuthHivePlugin.onRevokedRolesFromGroup(Lists.newArrayList(role), groupEntity1)); + rangerAuthHivePlugin.setRangerClient(originClient); } private void assertFindManagedPolicyItems(Role role, boolean gravitinoPolicyItemExist) {