Repository: sentry Updated Branches: refs/heads/master 360e9c304 -> 315a8913a
SENTRY-2245: Remove privileges that do not associate with a role or a user (Na Li, reviewed by Sergio Pena, Kalyan Kumar Kalvagadda) Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/315a8913 Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/315a8913 Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/315a8913 Branch: refs/heads/master Commit: 315a8913aa9dfc5fad7a9863c766267fd6f7877b Parents: 360e9c3 Author: lina.li <[email protected]> Authored: Wed Jun 6 15:18:31 2018 -0500 Committer: lina.li <[email protected]> Committed: Wed Jun 6 15:18:31 2018 -0500 ---------------------------------------------------------------------- .../db/service/persistent/SentryStore.java | 67 +++++++------------- .../db/service/persistent/TestSentryStore.java | 10 +++ 2 files changed, 34 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/315a8913/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java index 7bf3e80..e6b71b5 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java @@ -843,11 +843,11 @@ public class SentryStore { findMatchPrivilege(mRole.getPrivileges(), convertToMSentryPrivilege(tNotAll)); if (mSelect != null) { mSelect.removeRole(mRole); - pm.makePersistent(mSelect); + persistPrivilege(pm, mSelect); } if (mInsert != null) { mInsert.removeRole(mRole); - pm.makePersistent(mInsert); + persistPrivilege(pm, mInsert); } } else { // If Grant is for Either INSERT/SELECT and ALL already exists.. @@ -1072,11 +1072,11 @@ public class SentryStore { findMatchPrivilege(mUser.getPrivileges(), convertToMSentryPrivilege(tNotAll)); if (mSelect != null) { mSelect.removeUser(mUser); - pm.makePersistent(mSelect); + persistPrivilege(pm, mSelect); } if (mInsert != null) { mInsert.removeUser(mUser); - pm.makePersistent(mInsert); + persistPrivilege(pm, mInsert); } } else { // If Grant is for Either INSERT/SELECT and ALL already exists.. @@ -1421,11 +1421,7 @@ public class SentryStore { persistedPriv.removeUser(mUser); } - if (isPrivilegeStall(persistedPriv)) { - pm.deletePersistent(persistedPriv); - } else { - pm.makePersistent(persistedPriv); - } + persistPrivilege(pm, persistedPriv); } } else { @@ -1457,7 +1453,16 @@ public class SentryStore { return false; } - private boolean isPrivilegeStall(MSentryPrivilege privilege) { + private void persistPrivilege(PersistenceManager pm, MSentryPrivilege privilege) { + if (isPrivilegeStale(privilege)) { + pm.deletePersistent(privilege); + return; + } + + pm.makePersistent(privilege); + } + + private boolean isPrivilegeStale(MSentryPrivilege privilege) { if (privilege.getUsers().isEmpty() && privilege.getRoles().isEmpty()) { return true; } @@ -1465,7 +1470,7 @@ public class SentryStore { return false; } - private boolean isPrivilegeStall(MSentryGMPrivilege privilege) { + private boolean isPrivilegeStale(MSentryGMPrivilege privilege) { if (privilege.getRoles().isEmpty()) { return true; } @@ -1481,21 +1486,13 @@ public class SentryStore { persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(persistedPriv), pm); if (persistedPriv != null && !persistedPriv.getRoles().isEmpty()) { persistedPriv.removeRole(mRole); - if (isPrivilegeStall(persistedPriv)) { - pm.deletePersistent(persistedPriv); - } else { - pm.makePersistent(persistedPriv); - } + persistPrivilege(pm, persistedPriv); } currentPrivilege.setAction(AccessConstants.ALL); persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm); if (persistedPriv != null && mRole.getPrivileges().contains(persistedPriv)) { persistedPriv.removeRole(mRole); - if (isPrivilegeStall(persistedPriv)) { - pm.deletePersistent(persistedPriv); - } else { - pm.makePersistent(persistedPriv); - } + persistPrivilege(pm, persistedPriv); // add decomposted actions for (String addAction : addActions) { @@ -1520,21 +1517,13 @@ public class SentryStore { persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(persistedPriv), pm); if (persistedPriv != null && !persistedPriv.getUsers().isEmpty()) { persistedPriv.removeUser(mUser); - if (isPrivilegeStall(persistedPriv)) { - pm.deletePersistent(persistedPriv); - } else { - pm.makePersistent(persistedPriv); - } + persistPrivilege(pm, persistedPriv); } currentPrivilege.setAction(AccessConstants.ALL); persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm); if (persistedPriv != null && mUser.getPrivileges().contains(persistedPriv)) { persistedPriv.removeUser(mUser); - if (isPrivilegeStall(persistedPriv)) { - pm.deletePersistent(persistedPriv); - } else { - pm.makePersistent(persistedPriv); - } + persistPrivilege(pm, persistedPriv); // add decomposted actions for (String addAction : addActions) { @@ -1567,11 +1556,7 @@ public class SentryStore { MSentryPrivilege persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(mPrivilege), pm); if (persistedPriv != null && !persistedPriv.getRoles().isEmpty()) { persistedPriv.removeRole(mRole); - if (isPrivilegeStall(persistedPriv)) { - pm.deletePersistent(persistedPriv); - } else { - pm.makePersistent(persistedPriv); - } + persistPrivilege(pm, persistedPriv); } } } @@ -1592,11 +1577,7 @@ public class SentryStore { MSentryPrivilege persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(mPrivilege), pm); if (persistedPriv != null && !persistedPriv.getUsers().isEmpty()) { persistedPriv.removeUser(mUser); - if (isPrivilegeStall(persistedPriv)) { - pm.deletePersistent(persistedPriv); - } else { - pm.makePersistent(persistedPriv); - } + persistPrivilege(pm, persistedPriv); } } } @@ -1873,7 +1854,7 @@ public class SentryStore { private void removeStaledPrivileges(PersistenceManager pm, List<MSentryPrivilege> privilegesCopy) { List<MSentryPrivilege> stalePrivileges = new ArrayList<>(0); for (MSentryPrivilege privilege : privilegesCopy) { - if (isPrivilegeStall(privilege)) { + if (isPrivilegeStale(privilege)) { stalePrivileges.add(privilege); } } @@ -1885,7 +1866,7 @@ public class SentryStore { private void removeStaledGMPrivileges(PersistenceManager pm, List<MSentryGMPrivilege> privilegesCopy) { List<MSentryGMPrivilege> stalePrivileges = new ArrayList<>(0); for (MSentryGMPrivilege privilege : privilegesCopy) { - if (isPrivilegeStall(privilege)) { + if (isPrivilegeStale(privilege)) { stalePrivileges.add(privilege); } } http://git-wip-us.apache.org/repos/asf/sentry/blob/315a8913/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java index 95dabec..12c6d91 100644 --- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java +++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java @@ -3978,6 +3978,8 @@ public class TestSentryStore extends org.junit.Assert { // second round privilege.setAction(AccessConstants.ALL); sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, privilege); + List<MSentryPrivilege> totalPrivileges = sentryStore.getAllMSentryPrivileges(); + assertEquals(totalPrivileges.toString(),1, totalPrivileges.size()); role = sentryStore.getMSentryRoleByName(roleName); privileges = role.getPrivileges(); assertEquals(privileges.toString(), 1, privileges.size()); @@ -3985,6 +3987,8 @@ public class TestSentryStore extends org.junit.Assert { privilege.setAction(AccessConstants.INSERT); sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, privilege); // after having ALL and revoking INSERT, we should have (SELECT) + totalPrivileges = sentryStore.getAllMSentryPrivileges(); + assertEquals(totalPrivileges.toString(),1, totalPrivileges.size()); role = sentryStore.getMSentryRoleByName(roleName); privileges = role.getPrivileges(); assertEquals(privileges.toString(), 1, privileges.size()); @@ -4023,6 +4027,9 @@ public class TestSentryStore extends org.junit.Assert { // second round privilege.setAction(AccessConstants.ALL); sentryStore.alterSentryUserGrantPrivilege(grantor, userName, privilege); + List<MSentryPrivilege> totalPrivileges = sentryStore.getAllMSentryPrivileges(); + assertEquals(totalPrivileges.toString(),1, totalPrivileges.size()); + user = sentryStore.getMSentryUserByName(userName); privileges = user.getPrivileges(); assertEquals(privileges.toString(), 1, privileges.size()); @@ -4030,6 +4037,9 @@ public class TestSentryStore extends org.junit.Assert { privilege.setAction(AccessConstants.INSERT); sentryStore.alterSentryUserRevokePrivilege(grantor, userName, privilege); // after having ALL and revoking INSERT, we should have (SELECT) + totalPrivileges = sentryStore.getAllMSentryPrivileges(); + assertEquals(totalPrivileges.toString(),1, totalPrivileges.size()); + user = sentryStore.getMSentryUserByName(userName); privileges = user.getPrivileges(); assertEquals(privileges.toString(), 1, privileges.size());
