Repository: incubator-sentry Updated Branches: refs/heads/master 437f6d96c -> d88c157d8
SENTRY-552: Sentry Store recursive revoke of privilege levels < ALL does not properly downgrade child privileges (Dapeng Sun 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/d88c157d Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/d88c157d Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/d88c157d Branch: refs/heads/master Commit: d88c157d83c10fdf66b76ead7b7c16fe60d82e26 Parents: 437f6d9 Author: Lenni Kuff <[email protected]> Authored: Tue Dec 2 00:30:22 2014 -0800 Committer: Lenni Kuff <[email protected]> Committed: Tue Dec 2 00:32:26 2014 -0800 ---------------------------------------------------------------------- .../db/service/model/MSentryPrivilege.java | 5 + .../db/service/persistent/SentryStore.java | 18 ++ .../db/service/persistent/TestSentryStore.java | 203 ++++++++++++++++++- .../e2e/dbprovider/TestDatabaseProvider.java | 37 +++- 4 files changed, 251 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/d88c157d/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java index ed081a3..1c68a0f 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java @@ -307,4 +307,9 @@ public class MSentryPrivilege { return SentryStore.isNULL(s); } + public boolean isActionALL() { + return AccessConstants.ACTION_ALL.equalsIgnoreCase(action) + || AccessConstants.ALL.equals(action); + } + } http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/d88c157d/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 073bb33..7d2cb12 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 @@ -581,6 +581,24 @@ public class SentryStore { && (!isNULL(childPriv.getColumnName()))) { populateChildren(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. + // For example: a user has a privilege âAll on Col1â, + // if the operation is âREVOKE INSERT on tableâ + // the privilege should be the child of table level privilege. + // but the privilege may still have other meaning, likes "SELECT on Col1". + // and the privileges like "SELECT on Col1" should not be revoke. + if (!priv.isActionALL()) { + if (childPriv.isActionALL()) { + // If the child privilege is All, we should convert it to the same + // privilege with parent + childPriv.setAction(priv.getAction()); + } + // Only include privilege that imply the parent privilege. + if (!priv.implies(childPriv)) { + continue; + } + } children.add(childPriv); } } http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/d88c157d/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 5bb00f5..9dca5fb 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 @@ -404,9 +404,12 @@ public class TestSentryStore { assertEquals(privileges.toString(), 0, privileges.size()); } + /** + * Regression test for SENTRY-74 and SENTRY-552 + */ @Test public void testGrantRevokePrivilegeWithColumn() throws Exception { - String roleName = "test-privilege"; + String roleName = "test-col-privilege"; String grantor = "g1"; String server = "server1"; String db = "db1"; @@ -422,6 +425,8 @@ public class TestSentryStore { privilege.setColumnName(column1); privilege.setAction(AccessConstants.ALL); privilege.setCreateTime(System.currentTimeMillis()); + + // Grant ALL on c1 and c2 assertEquals(seqId + 1, sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, privilege) .getSequenceId()); privilege.setColumnName(column2); @@ -430,22 +435,32 @@ public class TestSentryStore { MSentryRole role = sentryStore.getMSentryRoleByName(roleName); Set<MSentryPrivilege> privileges = role.getPrivileges(); assertEquals(privileges.toString(), 2, privileges.size()); + + // Revoke SELECT on c2 privilege.setAction(AccessConstants.SELECT); assertEquals(seqId + 3, sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, privilege) .getSequenceId()); - // after having ALL and revoking SELECT, we should have INSERT + // At this point c1 has ALL privileges and c2 should have INSERT after revoking SELECT role = sentryStore.getMSentryRoleByName(roleName); privileges = role.getPrivileges(); assertEquals(privileges.toString(), 2, privileges.size()); - MSentryPrivilege mPrivilege = Iterables.get(privileges, 0); - assertEquals(server, mPrivilege.getServerName()); - assertEquals(db, mPrivilege.getDbName()); - assertEquals(table, mPrivilege.getTableName()); - assertEquals(AccessConstants.INSERT, mPrivilege.getAction()); - assertFalse(mPrivilege.getGrantOption()); + for (MSentryPrivilege mPrivilege: privileges) { + assertEquals(server, mPrivilege.getServerName()); + assertEquals(db, mPrivilege.getDbName()); + assertEquals(table, mPrivilege.getTableName()); + assertFalse(mPrivilege.getGrantOption()); + if (mPrivilege.getColumnName().equals(column1)) { + assertEquals(AccessConstants.ALL, mPrivilege.getAction()); + } else if (mPrivilege.getColumnName().equals(column2)) { + assertEquals(AccessConstants.INSERT, mPrivilege.getAction()); + } else { + fail("Unexpected column name: " + mPrivilege.getColumnName()); + } + } - // after revoking table level privilege, we will remove the two column level items + // after revoking INSERT table level privilege will remove privileges from column2 + // and downgrade column1 to SELECT privileges. privilege = new TSentryPrivilege(); privilege.setPrivilegeScope("TABLE"); privilege.setServerName(server); @@ -457,9 +472,179 @@ public class TestSentryStore { .getSequenceId()); role = sentryStore.getMSentryRoleByName(roleName); privileges = role.getPrivileges(); + assertEquals(privileges.toString(), 1, privileges.size()); + assertEquals(column1, Iterables.get(privileges, 0).getColumnName()); + assertEquals(AccessConstants.SELECT, Iterables.get(privileges, 0).getAction()); + + // Revoke ALL from the table should now remove all the column privileges. + privilege.setAction(AccessConstants.ALL); + privilege.setCreateTime(System.currentTimeMillis()); + assertEquals(seqId + 5, sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, privilege) + .getSequenceId()); + role = sentryStore.getMSentryRoleByName(roleName); + privileges = role.getPrivileges(); assertEquals(privileges.toString(), 0, privileges.size()); } + /** + * Regression test for SENTRY-552 + */ + @Test + public void testGrantRevokeTablePrivilegeDowngradeByDb() throws Exception { + String roleName = "test-table-db-downgrade-privilege"; + String grantor = "g1"; + String server = "server1"; + String db = "db1"; + String table1 = "tbl1"; + String table2 = "tbl2"; + long seqId = sentryStore.createSentryRole(roleName).getSequenceId(); + TSentryPrivilege privilegeTable1 = new TSentryPrivilege(); + privilegeTable1.setPrivilegeScope("TABLE"); + privilegeTable1.setServerName(server); + privilegeTable1.setDbName(db); + privilegeTable1.setTableName(table1); + privilegeTable1.setAction(AccessConstants.ALL); + privilegeTable1.setCreateTime(System.currentTimeMillis()); + TSentryPrivilege privilegeTable2 = privilegeTable1.deepCopy();; + privilegeTable2.setTableName(table2); + + // Grant ALL on table1 and table2 + assertEquals(seqId + 1, sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, privilegeTable1) + .getSequenceId()); + assertEquals(seqId + 2, sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, privilegeTable2) + .getSequenceId()); + MSentryRole role = sentryStore.getMSentryRoleByName(roleName); + Set<MSentryPrivilege> privileges = role.getPrivileges(); + assertEquals(privileges.toString(), 2, privileges.size()); + + // Revoke SELECT on table2 + privilegeTable2.setAction(AccessConstants.SELECT); + assertEquals(seqId + 3, sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, privilegeTable2) + .getSequenceId()); + // after having ALL and revoking SELECT, we should have INSERT + role = sentryStore.getMSentryRoleByName(roleName); + privileges = role.getPrivileges(); + assertEquals(privileges.toString(), 2, privileges.size()); + + // At this point table1 has ALL privileges and table2 should have INSERT after revoking SELECT + role = sentryStore.getMSentryRoleByName(roleName); + privileges = role.getPrivileges(); + assertEquals(privileges.toString(), 2, privileges.size()); + for (MSentryPrivilege mPrivilege: privileges) { + assertEquals(server, mPrivilege.getServerName()); + assertEquals(db, mPrivilege.getDbName()); + assertFalse(mPrivilege.getGrantOption()); + if (mPrivilege.getTableName().equals(table1)) { + assertEquals(AccessConstants.ALL, mPrivilege.getAction()); + } else if (mPrivilege.getTableName().equals(table2)) { + assertEquals(AccessConstants.INSERT, mPrivilege.getAction()); + } else { + fail("Unexpected table name: " + mPrivilege.getTableName()); + } + } + + // Revoke INSERT on Database + privilegeTable2.setAction(AccessConstants.INSERT); + privilegeTable2.setPrivilegeScope("DATABASE"); + privilegeTable2.unsetTableName(); + assertEquals(seqId + 4, sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, privilegeTable2) + .getSequenceId()); + role = sentryStore.getMSentryRoleByName(roleName); + privileges = role.getPrivileges(); + + // after revoking INSERT database level privilege will remove privileges from table2 + // and downgrade table1 to SELECT privileges. + assertEquals(privileges.toString(), 1, privileges.size()); + MSentryPrivilege mPrivilege = Iterables.get(privileges, 0); + assertEquals(server, mPrivilege.getServerName()); + assertEquals(db, mPrivilege.getDbName()); + assertEquals(table1, mPrivilege.getTableName()); + assertEquals(AccessConstants.SELECT, mPrivilege.getAction()); + assertFalse(mPrivilege.getGrantOption()); + } + + /** + * Regression test for SENTRY-552 + */ + @Test + public void testGrantRevokeColumnPrivilegeDowngradeByDb() throws Exception { + String roleName = "test-column-db-downgrade-privilege"; + String grantor = "g1"; + String server = "server1"; + String db = "db1"; + String table = "tbl1"; + String column1 = "c1"; + String column2 = "c2"; + long seqId = sentryStore.createSentryRole(roleName).getSequenceId(); + TSentryPrivilege privilegeCol1 = new TSentryPrivilege(); + privilegeCol1.setPrivilegeScope("COLUMN"); + privilegeCol1.setServerName(server); + privilegeCol1.setDbName(db); + privilegeCol1.setTableName(table); + privilegeCol1.setColumnName(column1); + privilegeCol1.setAction(AccessConstants.ALL); + privilegeCol1.setCreateTime(System.currentTimeMillis()); + TSentryPrivilege privilegeCol2 = privilegeCol1.deepCopy();; + privilegeCol2.setColumnName(column2); + + // Grant ALL on column1 and column2 + assertEquals(seqId + 1, sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, privilegeCol1) + .getSequenceId()); + assertEquals(seqId + 2, sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, privilegeCol2) + .getSequenceId()); + MSentryRole role = sentryStore.getMSentryRoleByName(roleName); + Set<MSentryPrivilege> privileges = role.getPrivileges(); + assertEquals(privileges.toString(), 2, privileges.size()); + + // Revoke SELECT on column2 + privilegeCol2.setAction(AccessConstants.SELECT); + assertEquals(seqId + 3, sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, privilegeCol2) + .getSequenceId()); + // after having ALL and revoking SELECT, we should have INSERT + role = sentryStore.getMSentryRoleByName(roleName); + privileges = role.getPrivileges(); + assertEquals(privileges.toString(), 2, privileges.size()); + + // At this point column1 has ALL privileges and column2 should have INSERT after revoking SELECT + role = sentryStore.getMSentryRoleByName(roleName); + privileges = role.getPrivileges(); + assertEquals(privileges.toString(), 2, privileges.size()); + for (MSentryPrivilege mPrivilege: privileges) { + assertEquals(server, mPrivilege.getServerName()); + assertEquals(db, mPrivilege.getDbName()); + assertEquals(table, mPrivilege.getTableName()); + assertFalse(mPrivilege.getGrantOption()); + if (mPrivilege.getColumnName().equals(column1)) { + assertEquals(AccessConstants.ALL, mPrivilege.getAction()); + } else if (mPrivilege.getColumnName().equals(column2)) { + assertEquals(AccessConstants.INSERT, mPrivilege.getAction()); + } else { + fail("Unexpected column name: " + mPrivilege.getColumnName()); + } + } + + // Revoke INSERT on Database + privilegeCol2.setAction(AccessConstants.INSERT); + privilegeCol2.setPrivilegeScope("DATABASE"); + privilegeCol2.unsetTableName(); + privilegeCol2.unsetColumnName(); + assertEquals(seqId + 4, sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, privilegeCol2) + .getSequenceId()); + role = sentryStore.getMSentryRoleByName(roleName); + privileges = role.getPrivileges(); + + // after revoking INSERT database level privilege will remove privileges from column2 + // and downgrade column1 to SELECT privileges. + assertEquals(privileges.toString(), 1, privileges.size()); + MSentryPrivilege mPrivilege = Iterables.get(privileges, 0); + assertEquals(server, mPrivilege.getServerName()); + assertEquals(db, mPrivilege.getDbName()); + assertEquals(table, mPrivilege.getTableName()); + assertEquals(column1, mPrivilege.getColumnName()); + assertEquals(AccessConstants.SELECT, mPrivilege.getAction()); + assertFalse(mPrivilege.getGrantOption()); + } + @Test public void testGrantRevokePrivilegeWithGrantOption() throws Exception { String roleName = "test-grantOption-table"; http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/d88c157d/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java ---------------------------------------------------------------------- diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java index 3189955..4a475ba 100644 --- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java +++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java @@ -122,10 +122,11 @@ public class TestDatabaseProvider extends AbstractTestWithStaticConfiguration { connection.close(); } - - // SENTRY-303 tests + /** + * Regression test for SENTRY-303 and SENTRY-543. + */ @Test - public void testGrantSELECTonDb() throws Exception { + public void testGrantRevokeSELECTonDb() throws Exception { File dataFile = doSetupForGrantDbTests(); Connection connection = context.createConnection(ADMIN1); @@ -157,6 +158,36 @@ public class TestDatabaseProvider extends AbstractTestWithStaticConfiguration { } catch (Exception e) { // Ignore } + + connection = context.createConnection(ADMIN1); + statement = context.createStatement(connection); + + // SENTRY-543 - Verify recursive revoke of SELECT on database removes the correct + // privileges. + statement.execute("USE " + DB1); + statement.execute("GRANT ALL ON TABLE t1 TO ROLE user_role"); + statement.execute("REVOKE SELECT ON DATABASE " + DB1 + " FROM ROLE user_role"); + statement.close(); + connection.close(); + + // Switch to user1 - they should not have no access on t1 and t2 now. + connection = context.createConnection(USER1_1); + statement = context.createStatement(connection); + + try { + // SELECT is not allowed + statement.execute("SELECT * FROM " + DB1 + ".t1"); + fail("SELECT should have been revoked from t1"); + } catch (Exception e) { + // Ignore + } + try { + // SELECT is not allowed + statement.execute("SELECT * FROM " + DB1 + ".t2"); + fail("SELECT should have been revoked from t2"); + } catch (Exception e) { + // Ignore + } statement.close(); connection.close(); }
