Repository: sentry Updated Branches: refs/heads/master 66b32afa8 -> 370fab099
SENTRY-1217: NPE for list_sentry_privileges_by_authorizable when activeRoleSet is not set (Hao Hao, Reviewed by: Lenni Kuff) Change-Id: I8a59320d737209234fe6105a7ba734fd1df45566 Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/370fab09 Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/370fab09 Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/370fab09 Branch: refs/heads/master Commit: 370fab099b93d4265f25af01d1d864b7a829d86d Parents: 66b32af Author: hahao <[email protected]> Authored: Mon Apr 25 21:12:57 2016 -0700 Committer: hahao <[email protected]> Committed: Mon Apr 25 21:12:57 2016 -0700 ---------------------------------------------------------------------- .../thrift/SentryGenericPolicyProcessor.java | 25 +++++++++++++++----- .../TestPrivilegeOperatePersistence.java | 2 ++ .../TestSentryGenericPolicyProcessor.java | 12 +++++++++- 3 files changed, 32 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/370fab09/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java index 19c8dd8..2a287e9 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java @@ -689,11 +689,12 @@ public class SentryGenericPolicyProcessor implements SentryGenericPolicyService. requestedGroups = memberGroups; } - // Disallow non-admin to lookup roles that they are not part of + Set<String> grantedRoles = toTrimmedLower(store.getRolesByGroups(request.getComponent(), requestedGroups)); + + // If activeRoleSet is not null, disallow non-admin to lookup roles that they are not part of. if (activeRoleSet != null && !activeRoleSet.isAll()) { - Set<String> grantedRoles = toTrimmedLower(store.getRolesByGroups(request.getComponent(), requestedGroups)); - Set<String> activeRoleNames = toTrimmedLower(activeRoleSet.getRoles()); + Set<String> activeRoleNames = toTrimmedLower(activeRoleSet.getRoles()); for (String activeRole : activeRoleNames) { if (!grantedRoles.contains(activeRole)) { throw new SentryAccessDeniedException(ACCESS_DENIAL_MESSAGE @@ -703,18 +704,30 @@ public class SentryGenericPolicyProcessor implements SentryGenericPolicyService. // For non-admin, valid active roles are intersection of active roles and granted roles. validActiveRoles.addAll(activeRoleSet.isAll() ? grantedRoles : Sets.intersection(activeRoleNames, grantedRoles)); + } else { + // For non-admin, if activeRoleSet is null, valid active roles would be the granted roles. + validActiveRoles.addAll(grantedRoles); } } else { Set<String> allRoles = toTrimmedLower(store.getAllRoleNames()); - Set<String> activeRoleNames = toTrimmedLower(activeRoleSet.getRoles()); + Set<String> activeRoleNames = Sets.newHashSet(); + boolean isAllRoleSet = false; + + // If activeRoleSet (which is optional) is null, valid active role will be all roles. + if (activeRoleSet != null) { + activeRoleNames = toTrimmedLower(activeRoleSet.getRoles()); + isAllRoleSet = activeRoleSet.isAll(); + } else { + isAllRoleSet = true; + } // For admin, if requestedGroups are empty, valid active roles are intersection of active roles and all roles. // Otherwise, valid active roles are intersection of active roles and the roles of requestedGroups. if (requestedGroups == null || requestedGroups.isEmpty()) { - validActiveRoles.addAll(activeRoleSet.isAll() ? allRoles : Sets.intersection(activeRoleNames, allRoles)); + validActiveRoles.addAll(isAllRoleSet ? allRoles : Sets.intersection(activeRoleNames, allRoles)); } else { Set<String> requestedRoles = toTrimmedLower(store.getRolesByGroups(request.getComponent(), requestedGroups)); - validActiveRoles.addAll(activeRoleSet.isAll() ? allRoles : Sets.intersection(activeRoleNames, requestedRoles)); + validActiveRoles.addAll(isAllRoleSet ? allRoles : Sets.intersection(activeRoleNames, requestedRoles)); } } http://git-wip-us.apache.org/repos/asf/sentry/blob/370fab09/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java index 9cbd1bd..deefefa 100644 --- a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java +++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java @@ -966,6 +966,8 @@ public class TestPrivilegeOperatePersistence extends SentryStoreIntegrationBase assertEquals(0, sentryStore.getPrivilegesByAuthorizable(SEARCH, service1, null, Arrays.asList(new Collection(COLLECTION_NAME), new Field(FIELD_NAME))).size()); + assertEquals(1, sentryStore.getPrivilegesByAuthorizable(SEARCH, service1, Sets.newHashSet(roleName1), + Arrays.asList(new Collection(COLLECTION_NAME), new Field(FIELD_NAME))).size()); assertEquals(2, sentryStore.getPrivilegesByAuthorizable(SEARCH, service1, Sets.newHashSet(roleName1), null).size()); assertEquals(2, sentryStore.getPrivilegesByAuthorizable(SEARCH, service1, http://git-wip-us.apache.org/repos/asf/sentry/blob/370fab09/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java index 84eeb82..cc0b28e 100644 --- a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java +++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java @@ -300,17 +300,27 @@ public class TestSentryGenericPolicyProcessor extends org.junit.Assert { assertEquals(Status.OK, fromTSentryStatus(response3.getStatus())); assertEquals(2, response3.getPrivileges().size()); + // Optional parameters activeRoleSet and requested group name are both provided. TListSentryPrivilegesByAuthRequest request4 = new TListSentryPrivilegesByAuthRequest(); request4.setGroups(Sets.newHashSet(groupName)); request4.setRoleSet(new TSentryActiveRoleSet(true, null)); request4.setRequestorUserName(ADMIN_USER); - Set<String> authorizablesSet = Sets.newHashSet("Collection=c1->Field=f1"); request4.setAuthorizablesSet(authorizablesSet); TListSentryPrivilegesByAuthResponse response4 = processor.list_sentry_privileges_by_authorizable(request4); assertEquals(Status.OK, fromTSentryStatus(response4.getStatus())); assertEquals(1, response4.getPrivilegesMapByAuth().size()); + + // Optional parameters activeRoleSet and requested group name are both not provided. + TListSentryPrivilegesByAuthRequest request5 = new TListSentryPrivilegesByAuthRequest(); + request5.setRequestorUserName("not_" + ADMIN_USER); + authorizablesSet = Sets.newHashSet("Collection=c1->Field=f2"); + request5.setAuthorizablesSet(authorizablesSet); + + TListSentryPrivilegesByAuthResponse response5 = processor.list_sentry_privileges_by_authorizable(request5); + assertEquals(Status.OK, fromTSentryStatus(response5.getStatus())); + assertEquals(1, response5.getPrivilegesMapByAuth().size()); } @Test(expected=SentryConfigurationException.class)
