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-2337: [REVERT] SENTRY-2295: Owner privileges should not be granted to sentry admin users( Kalyan Kumar Kalvagadda reviewed by Lina li)
Date Thu, 09 Aug 2018 13:08:29 GMT
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 <kkalyan@cloudera.com>
Authored: Thu Aug 9 08:07:11 2018 -0500
Committer: Kalyan Kumar Kalvagadda <kkalyan@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 ++;
       }
 


Mime
View raw message