----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67134/#review203164 -----------------------------------------------------------
I just need to review the DefaultSentryAccessController. Btw, I see that Hive has something here in case you want to compare it: https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java Lines 31-142 (original), 32-145 (patched) <https://reviews.apache.org/r/67134/#comment285252> This class should not be here. I removed it in another jira, mmm, I'll take a look. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java Lines 191-192 (original), 192-206 (patched) <https://reviews.apache.org/r/67134/#comment285251> I think SentryHiveAuthorizationTaskFactory is not used anymore in Sentry. This is an old implementation of the Hive authz V1. We should ignore these changes and delete the class in another jira. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 209 (patched) <https://reviews.apache.org/r/67134/#comment285254> If it is for object, why is the type == USER? If it is a bug in Hive, then, should we check this or can we ignore it? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java Lines 272 (patched) <https://reviews.apache.org/r/67134/#comment285255> Nit: space between for and (, and > and authList. sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java Lines 476-493 (patched) <https://reviews.apache.org/r/67134/#comment285250> I think TestSentryHiveAuthorizationTaskFactory is not used anymore in Sentry. This is an old implementation of the Hive authz V1. We should ignore these changes and delete the class in another jira. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1595 (original), 1595 (patched) <https://reviews.apache.org/r/67134/#comment285248> what's the difference between startsWith and endsWith? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestShowGrants.java Lines 37-38 (patched) <https://reviews.apache.org/r/67134/#comment285246> Are we sting 'show grant role' or just 'show grant'? I think to keep this variable name generic, we should use SHOW_GRANT_DB_POSITION, etc/ right? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestShowGrants.java Lines 116 (patched) <https://reviews.apache.org/r/67134/#comment285247> role2 and role3 has privileges here, but what about role1 who has all privileges in the server? How should this work? should we display it as well? - Sergio Pena On May 15, 2018, 4:17 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67134/ > ----------------------------------------------------------- > > (Updated May 15, 2018, 4:17 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-common/src/main/java/org/apache/hadoop/hive/ql/exec/SentryHivePrivilegeObjectDesc.java > 4fa4221b4 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 203632d05 > > 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 > fc2427cbf > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java > 2e3fd7f36 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > cafe2b597 > > 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/1/ > > > Testing > ------- > > $ mvn -f sentry-binding/pom.xml test > $ mvn -f sentry-provider/pom.xml test > > > Thanks, > > Arjun Mishra > >
