You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sentry.apache.org by ka...@apache.org on 2018/08/09 13:08:29 UTC
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)
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 <kk...@cloudera.com>
Authored: Thu Aug 9 08:07:11 2018 -0500
Committer: Kalyan Kumar Kalvagadda <kk...@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 ++;
}