----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67134/#review204880 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 276 (patched) <https://reviews.apache.org/r/67134/#comment287678> This error will be displayed on the Hive console. Should we write a better message? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 281-285 (patched) <https://reviews.apache.org/r/67134/#comment287675> You can pass the returned List<> from getAuthzHierarchy to the Set constructor and it will copy the items for you. See this example: List<Integer> sourceList = Lists.newArrayList(0, 1, 2, 3, 4, 5); Set<Integer> targetSet = Sets.newHashSet(sourceList); sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 287 (patched) <https://reviews.apache.org/r/67134/#comment287673> Space here >>principalToPrivMap sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 288-289 (patched) <https://reviews.apache.org/r/67134/#comment287676> You can use a singleton instead of creating a new hash set everytime. Set<String> users = Collections.singleton(authenticator.getUserName()); sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 291 (patched) <https://reviews.apache.org/r/67134/#comment287677> I don't feel this line should be here. If this is required only to get the active roles, then we should move the method that allow us to get the active roles. Is there a utility class that we used where we can move it? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 292-295 (patched) <https://reviews.apache.org/r/67134/#comment287674> We should be able to reduce these two calls to one single call that returns all privileges for roles, users (and in the future groups). The refactor should not be big. You can create a class that wraps the different map objects returned by the Server, then you can return that wrap object instead of the map. For instance: public SentryObjectPrivileges listPrivilegsbyAuthorizable() { TListSentryPrivilegesByAuthResponse response = client.list_...(); SentryObjectPrivileges privs = new SentryObjectPrivileges() privs.setPrivilegesForRoles(privs.getPrivilegesMapByAuth()); privs.setPrivilegesForUsers(privs.getPrivilegesMapByAuthForUsers()); return privs; } That's an example. You can come up with a different thing, but that would allow us to obtain all privileges for a specific object regardless of the entity type. I think we would need to rename the method too to avoid a confusion of the methods that return either users or roles. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 297 (patched) <https://reviews.apache.org/r/67134/#comment287679> This error will be displayed on the Hive console. Should we write a better message? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 300-314 (patched) <https://reviews.apache.org/r/67134/#comment287680> I honestly didn't get this. There are 4 for() here, isn't that too much? Is there a way to reduce this? I also see a variable name rolePrivilegesMap, but we can get privileges for users, can't we? - Sergio Pena On June 15, 2018, 8:08 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67134/ > ----------------------------------------------------------- > > (Updated June 15, 2018, 8:08 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and > Sergio Pena. > > > Repository: sentry > > > Description > ------- > > Currently Sentry doesn't support Hive command to show privileges on > authorizables without mentioning any role or user name > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > 23246c903 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java > f0b4b4466 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java > 2e3fd7f36 > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java > 6f38ed20d > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 45dce0e7c > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestShowGrants.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/67134/diff/7/ > > > Testing > ------- > > $ mvn -f sentry-binding/pom.xml test > $ mvn -f sentry-provider/pom.xml test > > > Thanks, > > Arjun Mishra > >
