Repository: sentry Updated Branches: refs/heads/master c30d56111 -> acdb36df3
SENTRY-2337: [REVERT] SENTRY-2295: Owner privileges should not be granted to sentry admin users( Kalyan Kumar Kalvagadda reviewed by Lina li) Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/acdb36df Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/acdb36df Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/acdb36df Branch: refs/heads/master Commit: acdb36df38bfa2ebb978d879fd796b9933aa8bd0 Parents: c30d561 Author: Kalyan Kumar Kalvagadda <kkal...@cloudera.com> Authored: Thu Aug 9 08:07:11 2018 -0500 Committer: Kalyan Kumar Kalvagadda <kkal...@cloudera.com> Committed: Thu Aug 9 08:07:11 2018 -0500 ---------------------------------------------------------------------- .../thrift/SentryPolicyStoreProcessor.java | 35 ++--------------- .../db/service/persistent/SentryStore.java | 3 +- .../persistent/SentryStoreInterface.java | 8 ---- .../thrift/TestSentryPolicyStoreProcessor.java | 38 +++++++++---------- .../e2e/dbprovider/TestOwnerPrivileges.java | 40 ++++++++------------ 5 files changed, 38 insertions(+), 86 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/acdb36df/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java index 61f9168..00015ef 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java @@ -984,20 +984,6 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface { return getGroupsFromUserName(this.conf, userName); } - /** - * - * @param userName - * @return True, if the user belongs to sentry admin group, otherwise false. - * @throws SentryUserException - */ - private boolean isSentryAdminUser(String userName) throws SentryUserException { - Set<String> ownerGroups = getGroupsFromUserName(this.conf, userName); - if (inAdminGroups(ownerGroups)) { - return true; - } - return false; - } - public static Set<String> getGroupsFromUserName(Configuration conf, String userName) throws SentryUserException { String groupMapping = conf.get(ServerConfig.SENTRY_STORE_GROUP_MAPPING, @@ -1517,13 +1503,6 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface { return; } - if(request.getOwnerType() == TSentryPrincipalType.USER && - isSentryAdminUser(request.getOwnerName())) { - LOGGER.debug(String.format("%s, belongs to Sentry Admin group, Owner privilege not granted to %s", - request.getOwnerName(), request.getAuthorizable().toString())); - return; - } - TSentryPrivilege ownerPrivilege = constructOwnerPrivilege(request.getAuthorizable()); if (ownerPrivilege == null) { LOGGER.debug("Owner privilege is not added"); @@ -1611,17 +1590,9 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface { } } } - if(request.getOwnerType() == TSentryPrincipalType.USER && - isSentryAdminUser(request.getOwnerName())) { - LOGGER.debug(String.format("%s, belongs to Sentry Admin group, Owner privilege not granted to %s", - request.getOwnerName(), request.getAuthorizable().toString())); - // New Owner belongs to sentry admin group. There is no need to add owner privilege. - sentryStore.revokeOwnerPrivileges(request.getAuthorizable(),updateList); - } else { - // Revokes old owner privileges and grants owner privilege for new owner. - sentryStore.updateOwnerPrivilege(request.getAuthorizable(), request.getOwnerName(), - entityType, updateList); - } + // Revokes old owner privileges and grants owner privilege for new owner. + sentryStore.updateOwnerPrivilege(request.getAuthorizable(), request.getOwnerName(), + entityType, updateList); //TODO Implement notificationHandlerInvoker API for granting user priv and invoke it. //TODO Implement Audit Log API's and invoke them here. } http://git-wip-us.apache.org/repos/asf/sentry/blob/acdb36df/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 5644181..8b32e7b 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 @@ -2716,7 +2716,8 @@ public class SentryStore implements SentryStoreInterface { * @param updates * @throws Exception */ - public void revokeOwnerPrivileges(final TSentryAuthorizable tAuthorizable, final List<Update> updates) + @VisibleForTesting + void revokeOwnerPrivileges(final TSentryAuthorizable tAuthorizable, final List<Update> updates) throws Exception{ execute(updates, pm -> { pm.setDetachAllOnCommit(false); http://git-wip-us.apache.org/repos/asf/sentry/blob/acdb36df/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java index 164188b..0b4f4aa 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java @@ -784,14 +784,6 @@ public interface SentryStoreInterface { final List<Update> updates) throws Exception; /** - * Revokes all the owner privileges granted to an authorizable - * @param tAuthorizable authorizable for which owner privilege should be revoked. - * @param updates - * @throws Exception - */ - void revokeOwnerPrivileges(final TSentryAuthorizable tAuthorizable, final List<Update> updates)throws Exception; - - /** * Returns all roles and privileges found on the Sentry database. * * @return A mapping between role and privileges in the form [roleName, set<privileges>]. http://git-wip-us.apache.org/repos/asf/sentry/blob/acdb36df/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java index e2e8c39..94dbd70 100644 --- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java +++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java @@ -148,6 +148,7 @@ public class TestSentryPolicyStoreProcessor { Mockito.reset(sentryStore); Mockito.reset(counterWait); } + @Test(expected=SentrySiteConfigurationException.class) public void testConfigNotNotificationHandler() throws Exception { conf.set(PolicyStoreServerConfig.NOTIFICATION_HANDLERS, Object.class.getName()); @@ -357,22 +358,21 @@ public class TestSentryPolicyStoreProcessor { sentryStore, Mockito.times(1) ).alterSentryGrantOwnerPrivilege(OWNER, SentryPrincipalType.ROLE, ownerPrivilege, null); - notification.setOwnerType(TSentryPrincipalType.USER); - sentryServiceHandler.sentry_notify_hms_event(notification); - - //Verify Sentry Store is invoked to grant privilege. - Mockito.verify( - sentryStore, Mockito.times(1) - ).alterSentryGrantOwnerPrivilege(OWNER, SentryPrincipalType.USER, ownerPrivilege, null); - - Mockito.reset(sentryStore); - // Verify that owner privilege is not granted when owner belongs to sentry admin group. + // Verify that owner privilege is granted when owner belongs to sentry admin group. notification.setOwnerType(TSentryPrincipalType.USER); notification.setOwnerName(ADMIN_USER); sentryServiceHandler.sentry_notify_hms_event(notification); Mockito.verify( - sentryStore, Mockito.times(0)).alterSentryGrantOwnerPrivilege(OWNER, SentryPrincipalType.USER, + sentryStore, Mockito.times(1)).alterSentryGrantOwnerPrivilege(ADMIN_USER, SentryPrincipalType.USER, ownerPrivilege, null); + notification.setOwnerName(OWNER); + notification.setOwnerType(TSentryPrincipalType.USER); + sentryServiceHandler.sentry_notify_hms_event(notification); + + // Verify Sentry Store is invoked to grant privilege. + Mockito.verify( + sentryStore, Mockito.times(1) + ).alterSentryGrantOwnerPrivilege(OWNER, SentryPrincipalType.USER, ownerPrivilege, null); } @@ -408,13 +408,13 @@ public class TestSentryPolicyStoreProcessor { sentryStore, Mockito.times(1) ).alterSentryGrantOwnerPrivilege(OWNER, SentryPrincipalType.USER, ownerPrivilege, null); - Mockito.reset(sentryStore); - // Verify that owner privilege is not granted when owner belongs to sentry admin group. + // Mockito.reset(sentryStore); + // Verify that owner privilege is granted when owner belongs to sentry admin group. notification.setOwnerType(TSentryPrincipalType.USER); notification.setOwnerName(ADMIN_USER); sentryServiceHandler.sentry_notify_hms_event(notification); Mockito.verify( - sentryStore, Mockito.times(0)).alterSentryGrantOwnerPrivilege(OWNER, SentryPrincipalType.USER, + sentryStore, Mockito.times(1)).alterSentryGrantOwnerPrivilege(ADMIN_USER, SentryPrincipalType.USER, ownerPrivilege, null); } @@ -436,20 +436,16 @@ public class TestSentryPolicyStoreProcessor { notification.setEventType(EventType.ALTER_TABLE.toString()); - // Verify that owner privilege is not granted when owner belongs to sentry admin group. + // Verify that owner privilege is granted when owner belongs to sentry admin group. notification.setOwnerType(TSentryPrincipalType.USER); notification.setOwnerName(ADMIN_USER); sentryServiceHandler.sentry_notify_hms_event(notification); // Verify Sentry Store API to update the privilege is not invoked when ownership is transferred to // user belonging to admin group Mockito.verify( - sentryStore, Mockito.times(0) - ).updateOwnerPrivilege(Mockito.eq(authorizable), Mockito.eq(OWNER), Mockito.eq(SentryPrincipalType.ROLE), - Mockito.anyList()); - - Mockito.verify( sentryStore, Mockito.times(1) - ).revokeOwnerPrivileges(Mockito.eq(authorizable), Mockito.anyList()); + ).updateOwnerPrivilege(Mockito.eq(authorizable), Mockito.eq(ADMIN_USER), Mockito.eq(SentryPrincipalType.USER), + Mockito.anyList()); notification.setOwnerType(TSentryPrincipalType.ROLE); notification.setOwnerName(OWNER); http://git-wip-us.apache.org/repos/asf/sentry/blob/acdb36df/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java ---------------------------------------------------------------------- diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java index 2ecf8fe..70bc3d8 100644 --- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java +++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java @@ -29,6 +29,8 @@ import java.sql.Statement; import java.util.List; import java.util.Set; +import org.apache.commons.lang.StringUtils; +import org.apache.parquet.Strings; import org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationBase; import org.apache.sentry.tests.e2e.hive.StaticUserGroup; import org.apache.sentry.service.common.ServiceConstants.SentryPrincipalType; @@ -105,7 +107,7 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { // verify privileges created for new database verifyTableOwnerPrivilegeExistForEntity(statementUSER1_1, SentryPrincipalType.USER, Lists.newArrayList(USER1_1), - DB1, null, 1); + DB1, "", 1); // verify that user has all privilege on this database, i.e., "OWNER" means "ALL" // for authorization @@ -151,7 +153,7 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { Connection connectionUSER1_2 = hiveServer2.createConnection(USER1_2, USER1_2); Statement statementUSER1_2 = connectionUSER1_2.createStatement(); verifyTableOwnerPrivilegeExistForEntity(statementUSER1_2, SentryPrincipalType.USER, Lists.newArrayList(USER1_2), - DB1, null, 0); + DB1, "", 0); // verify that user user1_2 does not have any privilege on this database except create try { @@ -192,7 +194,7 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { // verify no privileges created for new database verifyTableOwnerPrivilegeExistForEntity(statement, SentryPrincipalType.USER, Lists.newArrayList(admin), - DB1, null, 0); + DB1, "", 1); statement.close(); connection.close(); @@ -225,7 +227,7 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { // verify owner privileges created for new database no longer exists verifyTableOwnerPrivilegeExistForEntity(statementUSER1_1, SentryPrincipalType.USER, Lists.newArrayList(USER1_1), - DB1, null, 0); + DB1, "", 0); statement.close(); connection.close(); @@ -374,7 +376,7 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { // verify no owner privileges created for new table verifyTableOwnerPrivilegeExistForEntity(statement, SentryPrincipalType.USER, Lists.newArrayList(admin), - DB1, tableName1, 0); + DB1, tableName1, 1); statement.close(); connection.close(); @@ -631,7 +633,7 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { // verify no owner privileges to the new owner as the owner is admin user verifyTableOwnerPrivilegeExistForEntity(statement, SentryPrincipalType.USER, Lists.newArrayList(admin), - DB1, tableName1, 0); + DB1, tableName1, 1); statement.close(); connection.close(); @@ -656,7 +658,7 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { for (String entity : entities) { String command; - if (tableName == null) { + if (Strings.isNullOrEmpty(tableName)) { command = "SHOW GRANT " + entityType.toString() + " " + entity + " ON DATABASE " + dbName; } else { command = "SHOW GRANT " + entityType.toString() + " " + entity + " ON TABLE " + dbName + "." + tableName; @@ -666,28 +668,19 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { int resultSize = 0; while(resultSet.next()) { - String actionValue = resultSet.getString(7); if (!actionValue.equalsIgnoreCase("owner")) { // only check owner privilege, and skip other privileges continue; } + if(!resultSet.getString(1).equalsIgnoreCase(dbName)) { + continue; + } - assertThat(resultSet.getString(1), equalToIgnoringCase(dbName)); // db name - - String tableNameValue = resultSet.getString(2); - if (tableName != null) { - if (!tableNameValue.equalsIgnoreCase(tableName)) { - // it is possible the entity has owner privilege on both DB and table - // only check the owner privilege on table - continue; - } - } else { - if (!tableNameValue.equalsIgnoreCase("")) { - // it is possible the entity has owner privilege on both DB and table - // only check the owner privilege on db - continue; - } + if (!StringUtils.equalsIgnoreCase(tableName, resultSet.getString(2))) { + // it is possible the entity has owner privilege on both DB and table + // only check the owner privilege on table + continue; } assertThat(resultSet.getString(3), equalToIgnoringCase(""));//partition @@ -695,7 +688,6 @@ public class TestOwnerPrivileges extends TestHDFSIntegrationBase { assertThat(resultSet.getString(5), equalToIgnoringCase(entity));//principalName assertThat(resultSet.getString(6), equalToIgnoringCase(entityType.toString()));//principalType assertThat(resultSet.getBoolean(8), is(ownerPrivilegeGrantEnabled));//grantOption - resultSize ++; }
