sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kal...@apache.org
Subject sentry git commit: SENTRY-2295: Owner privileges should not be granted to sentry admin users (Kalyan Kumar Kalvagadda reviewed by Arjun Mishra and Lina Li)
Date Tue, 03 Jul 2018 20:30:26 GMT
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 <kkalyan@cloudera.com>
Authored: Tue Jul 3 15:30:10 2018 -0500
Committer: Kalyan Kumar Kalvagadda <kkalyan@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.


Mime
View raw message