Repository: incubator-sentry Updated Branches: refs/heads/master eb53de39e -> ea8663e98
SENTRY-547: Drop table may deadlock, getChildPrivileges should be in one transaction with revoke (Xiaomeng Huang via Lenni Kuff) Project: http://git-wip-us.apache.org/repos/asf/incubator-sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-sentry/commit/ea8663e9 Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/ea8663e9 Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/ea8663e9 Branch: refs/heads/master Commit: ea8663e98569d3919888ae8054ab1df0c7ae0423 Parents: eb53de3 Author: Lenni Kuff <[email protected]> Authored: Sat Dec 6 00:18:04 2014 -0800 Committer: Lenni Kuff <[email protected]> Committed: Sat Dec 6 00:18:33 2014 -0800 ---------------------------------------------------------------------- .../db/service/persistent/SentryStore.java | 108 +++++++++---------- .../db/service/persistent/TestSentryStore.java | 40 +++++++ 2 files changed, 90 insertions(+), 58 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/ea8663e9/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java index 7d2cb12..83ac3be 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java @@ -69,6 +69,8 @@ import org.apache.sentry.provider.db.service.thrift.TSentryRole; import org.apache.sentry.service.thrift.ServiceConstants.PrivilegeScope; import org.apache.sentry.service.thrift.ServiceConstants.ServerConfig; import org.datanucleus.store.rdbms.exceptions.MissingTableException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import com.codahale.metrics.Gauge; import com.google.common.annotations.VisibleForTesting; @@ -79,8 +81,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * SentryStore is the data access object for Sentry data. Strings @@ -504,7 +504,7 @@ public class SentryStore { privilegeGraph.add(mFalse); } // Get the privilege graph - populateChildren(Sets.newHashSet(roleName), mPrivilege, privilegeGraph); + populateChildren(pm, Sets.newHashSet(roleName), mPrivilege, privilegeGraph); for (MSentryPrivilege childPriv : privilegeGraph) { revokePartial(pm, tPrivilege, mRole, childPriv); } @@ -567,19 +567,21 @@ public class SentryStore { /** - * Explore Privilege graph and collect child privileges + * Explore Privilege graph and collect child privileges. + * The responsibility to commit/rollback the transaction should be handled by the caller. */ - private void populateChildren(Set<String> roleNames, MSentryPrivilege priv, + private void populateChildren(PersistenceManager pm, Set<String> roleNames, MSentryPrivilege priv, Set<MSentryPrivilege> children) throws SentryInvalidInputException { + Preconditions.checkNotNull(pm); if ((!isNULL(priv.getServerName())) || (!isNULL(priv.getDbName())) || (!isNULL(priv.getTableName()))) { // Get all TableLevel Privs - Set<MSentryPrivilege> childPrivs = getChildPrivileges(roleNames, priv); + Set<MSentryPrivilege> childPrivs = getChildPrivileges(pm, roleNames, priv); for (MSentryPrivilege childPriv : childPrivs) { // Only recurse for table level privs.. if ((!isNULL(childPriv.getDbName())) && (!isNULL(childPriv.getTableName())) && (!isNULL(childPriv.getColumnName()))) { - populateChildren(roleNames, childPriv, children); + populateChildren(pm, roleNames, childPriv, children); } // The method getChildPrivileges() didn't do filter on "action", // if the action is not "All", it should judge the action of children privilege. @@ -604,61 +606,51 @@ public class SentryStore { } } - private Set<MSentryPrivilege> getChildPrivileges(Set<String> roleNames, + private Set<MSentryPrivilege> getChildPrivileges(PersistenceManager pm, Set<String> roleNames, MSentryPrivilege parent) throws SentryInvalidInputException { // Column and URI do not have children - if ((!isNULL(parent.getColumnName()))||(!isNULL(parent.getURI()))) return new HashSet<MSentryPrivilege>(); - boolean rollbackTransaction = true; - PersistenceManager pm = null; - try { - pm = openTransaction(); - Query query = pm.newQuery(MSentryPrivilege.class); - query - .declareVariables("org.apache.sentry.provider.db.service.model.MSentryRole role"); - List<String> rolesFiler = new LinkedList<String>(); - for (String rName : roleNames) { - rolesFiler.add("role.roleName == \"" + rName.trim().toLowerCase() + "\""); - } - StringBuilder filters = new StringBuilder("roles.contains(role) " - + "&& (" + Joiner.on(" || ").join(rolesFiler) + ")"); - filters.append(" && serverName == \"" + parent.getServerName() + "\""); - if (!isNULL(parent.getDbName())) { - filters.append(" && dbName == \"" + parent.getDbName() + "\""); - if (!isNULL(parent.getTableName())) { - filters.append(" && tableName == \"" + parent.getTableName() + "\""); - filters.append(" && columnName != \"__NULL__\""); - } else { - filters.append(" && tableName != \"__NULL__\""); - } + if ((!isNULL(parent.getColumnName())) || (!isNULL(parent.getURI()))) { + return new HashSet<MSentryPrivilege>(); + } + + Query query = pm.newQuery(MSentryPrivilege.class); + query.declareVariables("org.apache.sentry.provider.db.service.model.MSentryRole role"); + List<String> rolesFiler = new LinkedList<String>(); + for (String rName : roleNames) { + rolesFiler.add("role.roleName == \"" + rName.trim().toLowerCase() + "\""); + } + StringBuilder filters = new StringBuilder("roles.contains(role) " + + "&& (" + Joiner.on(" || ").join(rolesFiler) + ")"); + filters.append(" && serverName == \"" + parent.getServerName() + "\""); + if (!isNULL(parent.getDbName())) { + filters.append(" && dbName == \"" + parent.getDbName() + "\""); + if (!isNULL(parent.getTableName())) { + filters.append(" && tableName == \"" + parent.getTableName() + "\""); + filters.append(" && columnName != \"__NULL__\""); } else { - filters.append(" && (dbName != \"__NULL__\" || URI != \"__NULL__\")"); + filters.append(" && tableName != \"__NULL__\""); } + } else { + filters.append(" && (dbName != \"__NULL__\" || URI != \"__NULL__\")"); + } - query.setFilter(filters.toString()); - query - .setResult("privilegeScope, serverName, dbName, tableName, columnName," + - " URI, action, grantOption"); - Set<MSentryPrivilege> privileges = new HashSet<MSentryPrivilege>(); - for (Object[] privObj : (List<Object[]>) query.execute()) { - MSentryPrivilege priv = new MSentryPrivilege(); - priv.setPrivilegeScope((String) privObj[0]); - priv.setServerName((String) privObj[1]); - priv.setDbName((String) privObj[2]); - priv.setTableName((String) privObj[3]); - priv.setColumnName((String) privObj[4]); - priv.setURI((String) privObj[5]); - priv.setAction((String) privObj[6]); - priv.setGrantOption((Boolean) privObj[7]); - privileges.add(priv); - } - rollbackTransaction = false; - commitTransaction(pm); - return privileges; - } finally { - if (rollbackTransaction) { - rollbackTransaction(pm); - } + query.setFilter(filters.toString()); + query.setResult("privilegeScope, serverName, dbName, tableName, columnName," + + " URI, action, grantOption"); + Set<MSentryPrivilege> privileges = new HashSet<MSentryPrivilege>(); + for (Object[] privObj : (List<Object[]>) query.execute()) { + MSentryPrivilege priv = new MSentryPrivilege(); + priv.setPrivilegeScope((String) privObj[0]); + priv.setServerName((String) privObj[1]); + priv.setDbName((String) privObj[2]); + priv.setTableName((String) privObj[3]); + priv.setColumnName((String) privObj[4]); + priv.setURI((String) privObj[5]); + priv.setAction((String) privObj[6]); + priv.setGrantOption((Boolean) privObj[7]); + privileges.add(priv); } + return privileges; } private List<MSentryPrivilege> getMSentryPrivileges(TSentryPrivilege tPriv, PersistenceManager pm) { @@ -1543,9 +1535,9 @@ public class SentryStore { Set<MSentryPrivilege> privilegeGraph = Sets.newHashSet(); if (parent != null) { privilegeGraph.add(parent); - populateChildren(Sets.newHashSet(role.getRoleName()), parent, privilegeGraph); + populateChildren(pm, Sets.newHashSet(role.getRoleName()), parent, privilegeGraph); } else { - populateChildren(Sets.newHashSet(role.getRoleName()), convertToMSentryPrivilege(tPrivilege), + populateChildren(pm, Sets.newHashSet(role.getRoleName()), convertToMSentryPrivilege(tPrivilege), privilegeGraph); } // 2. revoke privilege and child privileges http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/ea8663e9/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java index 9dca5fb..8ca1f83 100644 --- a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java +++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java @@ -1260,6 +1260,46 @@ public class TestSentryStore { .size()); } + /** + * Regression test for SENTRY-547 + * Use case: + * GRANT INSERT on TABLE tbl1 to ROLE role1 + * GRANT SELECT on TABLE tbl1 to ROLE role1 + * DROP TABLE tbl1 + * + * After drop tbl1, role1 should have 0 privileges + */ + @Test + public void testDropTableWithMultiAction() throws Exception { + String roleName1 = "role1"; + String grantor = "g1"; + sentryStore.createSentryRole(roleName1); + + TSentryPrivilege privilege_tbl1 = new TSentryPrivilege(); + privilege_tbl1.setPrivilegeScope("TABLE"); + privilege_tbl1.setServerName("server1"); + privilege_tbl1.setDbName("db1"); + privilege_tbl1.setTableName("tbl1"); + privilege_tbl1.setCreateTime(System.currentTimeMillis()); + + TSentryPrivilege privilege_tbl1_insert = new TSentryPrivilege( + privilege_tbl1); + privilege_tbl1_insert.setAction(AccessConstants.INSERT); + + TSentryPrivilege privilege_tbl1_select = new TSentryPrivilege( + privilege_tbl1); + privilege_tbl1_select.setAction(AccessConstants.SELECT); + + sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName1, privilege_tbl1_insert); + sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName1, privilege_tbl1_select); + + assertEquals(2, sentryStore.getAllTSentryPrivilegesByRoleName(roleName1).size()); + + // after drop privilege_tbl1, role1 should have 0 privileges + sentryStore.dropPrivilege(toTSentryAuthorizable(privilege_tbl1)); + assertEquals(0, sentryStore.getAllTSentryPrivilegesByRoleName(roleName1).size()); + } + @Test public void testDropTableWithColumn() throws Exception { String roleName1 = "role1", roleName2 = "role2";
