Repository: sentry Updated Branches: refs/heads/master 9eea789b5 -> 702495b40
SENTRY-2295: Owner privileges should not be granted to sentry admin users (Kalyan Kumar Kalvagadda reviewed by Arjun Mishra and Lina Li) Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/702495b4 Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/702495b4 Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/702495b4 Branch: refs/heads/master Commit: 702495b404fd6b55587f23de14e1feedd9cb64ce Parents: 9eea789 Author: Kalyan Kumar Kalvagadda <kkal...@cloudera.com> Authored: Tue Jul 3 15:30:10 2018 -0500 Committer: Kalyan Kumar Kalvagadda <kkal...@cloudera.com> Committed: Tue Jul 3 15:30:10 2018 -0500 ---------------------------------------------------------------------- .../thrift/SentryPolicyStoreProcessor.java | 35 +++++++++- .../db/service/persistent/SentryStore.java | 3 +- .../persistent/SentryStoreInterface.java | 8 +++ .../thrift/TestSentryPolicyStoreProcessor.java | 72 +++++++++++++++++++- 4 files changed, 110 insertions(+), 8 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/702495b4/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 95ae15d..98f2e29 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,6 +984,20 @@ 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, @@ -1503,6 +1517,13 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface { return; } + if(request.getOwnerType() == TSentryObjectOwnerType.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"); @@ -1590,9 +1611,17 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface { } } } - // Revokes old owner privileges and grants owner privilege for new owner. - sentryStore.updateOwnerPrivilege(request.getAuthorizable(), request.getOwnerName(), - entityType, updateList); + if(request.getOwnerType() == TSentryObjectOwnerType.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); + } //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/702495b4/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 004f06d..ff0b4c0 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 @@ -2715,8 +2715,7 @@ public class SentryStore implements SentryStoreInterface { * @param updates * @throws Exception */ - @VisibleForTesting - void revokeOwnerPrivileges(final TSentryAuthorizable tAuthorizable, final List<Update> updates) + public 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/702495b4/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 19c2972..4443148 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,6 +784,14 @@ 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/702495b4/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 c8051e3..14a0fd8 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 @@ -32,12 +32,14 @@ import org.apache.sentry.api.common.Status; import org.apache.sentry.api.common.ThriftConstants; import org.apache.sentry.core.common.exception.SentryInvalidInputException; import org.apache.sentry.core.model.db.AccessConstants; +import org.apache.sentry.provider.common.GroupMappingService; import org.apache.sentry.provider.db.service.persistent.CounterWait; import org.apache.sentry.service.common.ServiceConstants; import org.apache.sentry.core.common.exception.SentrySiteConfigurationException; import org.apache.sentry.provider.db.service.persistent.SentryStore; import org.apache.sentry.service.common.ServiceConstants.SentryEntityType; import org.apache.sentry.service.common.ServiceConstants.ServerConfig; +import org.junit.After; import org.junit.Assert; import org.apache.hadoop.conf.Configuration; @@ -55,12 +57,34 @@ public class TestSentryPolicyStoreProcessor { private Configuration conf; private static final SentryStore sentryStore = Mockito.mock(SentryStore.class); private static final CounterWait counterWait = Mockito.mock(CounterWait.class); + private static final String ADMIN_GROUP = "admin_group"; + private static final String ADMIN_USER = "admin_user"; + private static final String NOT_ADMIN_USER = "not_admin_user"; + private static final String NOT_ADMIN_GROUP = "not_admin_group"; + + public static class MockGroupMapping implements GroupMappingService { + public MockGroupMapping(Configuration conf, String resource) { //NOPMD + } + @Override + public Set<String> getGroups(String user) { + if (user.equalsIgnoreCase(ADMIN_USER)) { + return Sets.newHashSet(ADMIN_GROUP); + } else if (user.equalsIgnoreCase(NOT_ADMIN_USER)){ + return Sets.newHashSet(NOT_ADMIN_GROUP); + } else { + return Collections.emptySet(); + } + } + } + @Before public void setup() throws Exception{ conf = new Configuration(false); //Check behaviour when DB name is not set conf.setBoolean(ServiceConstants.ServerConfig.SENTRY_ENABLE_OWNER_PRIVILEGES, true); - + conf.set(ServerConfig.ADMIN_GROUPS, ADMIN_GROUP); + conf.set(ServerConfig.SENTRY_STORE_GROUP_MAPPING, + MockGroupMapping.class.getName()); Mockito.when(sentryStore.getRoleCountGauge()).thenReturn(new Gauge< Long >() { @Override public Long getValue() { @@ -118,6 +142,12 @@ public class TestSentryPolicyStoreProcessor { return counterWait; }).when(sentryStore).getCounterWait(); } + + @After + public void reset () { + Mockito.reset(sentryStore); + Mockito.reset(counterWait); + } @Test(expected=SentrySiteConfigurationException.class) public void testConfigNotNotificationHandler() throws Exception { conf.set(PolicyStoreServerConfig.NOTIFICATION_HANDLERS, Object.class.getName()); @@ -333,6 +363,15 @@ public class TestSentryPolicyStoreProcessor { Mockito.verify( sentryStore, Mockito.times(1) ).alterSentryGrantOwnerPrivilege(OWNER, SentryEntityType.USER, ownerPrivilege, null); + + Mockito.reset(sentryStore); + // Verify that owner privilege is not granted when owner belongs to sentry admin group. + notification.setOwnerType(TSentryObjectOwnerType.USER); + notification.setOwnerName(ADMIN_USER); + sentryServiceHandler.sentry_notify_hms_event(notification); + Mockito.verify( + sentryStore, Mockito.times(0)).alterSentryGrantOwnerPrivilege(OWNER, SentryEntityType.USER, + ownerPrivilege, null); } @@ -367,11 +406,22 @@ public class TestSentryPolicyStoreProcessor { Mockito.verify( sentryStore, Mockito.times(1) ).alterSentryGrantOwnerPrivilege(OWNER, SentryEntityType.USER, ownerPrivilege, null); + + Mockito.reset(sentryStore); + // Verify that owner privilege is not granted when owner belongs to sentry admin group. + notification.setOwnerType(TSentryObjectOwnerType.USER); + notification.setOwnerName(ADMIN_USER); + sentryServiceHandler.sentry_notify_hms_event(notification); + Mockito.verify( + sentryStore, Mockito.times(0)).alterSentryGrantOwnerPrivilege(OWNER, SentryEntityType.USER, + ownerPrivilege, null); } @Test public void testAlterTableEventProcessing() throws Exception { + conf.setBoolean(ServiceConstants.ServerConfig.SENTRY_ENABLE_OWNER_PRIVILEGES, true); + SentryPolicyStoreProcessor sentryServiceHandler = new SentryPolicyStoreProcessor(ApiConstants.SentryPolicyServiceConstants.SENTRY_POLICY_SERVICE_NAME, conf, sentryStore); @@ -381,11 +431,27 @@ public class TestSentryPolicyStoreProcessor { TSentryHmsEventNotification notification = new TSentryHmsEventNotification(); notification.setId(1L); - notification.setOwnerType(TSentryObjectOwnerType.ROLE); - notification.setOwnerName(OWNER); notification.setAuthorizable(authorizable); notification.setEventType(EventType.ALTER_TABLE.toString()); + + // Verify that owner privilege is not granted when owner belongs to sentry admin group. + notification.setOwnerType(TSentryObjectOwnerType.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(SentryEntityType.ROLE), + Mockito.anyList()); + + Mockito.verify( + sentryStore, Mockito.times(1) + ).revokeOwnerPrivileges(Mockito.eq(authorizable), Mockito.anyList()); + + notification.setOwnerType(TSentryObjectOwnerType.ROLE); + notification.setOwnerName(OWNER); sentryServiceHandler.sentry_notify_hms_event(notification); //Verify Sentry Store is invoked to grant privilege.
