----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24151/#review49253 -----------------------------------------------------------
Nice work and very useful test cases! Some minor comments below. sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java <https://reviews.apache.org/r/24151/#comment86153> Minor: May be add a positive test case for with grant option here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java <https://reviews.apache.org/r/24151/#comment86158> Spelling of the class: SentryNoGrantOpitonException => SentryNoGrantOptionException Also, SentryAccessDeniedException is mostly used when user does is not part of sentry admin group, but is trying to create role, grant role or grant privilege. Although I see that it is also used else where, there is scope for some clean up. For example, SentryStore.getMSentryVersion() (Filed SENTRY-378 to clean up). But after this patch, looks like they both (accessdenied and nograntoption) mean more or less the same exception. We can follow up on this as a follow on. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/24151/#comment86243> URI is a child of server, so we need to check serverName equality even when uris are same. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/24151/#comment86244> Minor - Feel free to ignore: In practice serverName can never be null. So may be it is better to return false even if both server names are null? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java.orig <https://reviews.apache.org/r/24151/#comment86161> Looks like this file is in the patch my mistake? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/24151/#comment86254> I am not exactly sure why need this? I might be missing some thing. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/24151/#comment86186> would'nt the table name always __NULL__ here as we have a check earlier? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/24151/#comment86192> We may want to restrict the usage of unset? We default grantOption in thrift and MSentryPrivilege to false. But looks like when we convert thrift privilege struct to MSentryPrivilege and vice versa, we handle defaults differently by setting them as unset/null? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/24151/#comment86197> Same as above: We may want to restrict the usage of unset? We default grantOption in thrift and MSentryPrivilege to false. But looks like when we convert thrift privilege struct to MSentryPrivilege and vice versa, we handle defaults differently by setting them as unset/null? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/24151/#comment86206> Performance: getRoleNamesForGroups is building rolenames from msentryroles, and again we are building msentryrole from role name here. So there is chance for performance improvement if we create a new function which returns Set<MSentryRoles> instead? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java.orig <https://reviews.apache.org/r/24151/#comment86209> Looks like this file is part of the patch by mistake? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java <https://reviews.apache.org/r/24151/#comment86211> Same as above: We may want to restrict the usage of unset? We default grantOption in thrift and MSentryPrivilege to false. But looks like when we convert thrift privilege struct to MSentryPrivilege and vice versa, we handle defaults differently by setting them as unset/null? sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/24151/#comment86212> Can we add a comment here on the purpose of UNSET? Like: UNSET should be used only for revoking this privilege irrespective of grant option. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryPrivilege.java <https://reviews.apache.org/r/24151/#comment86220> Minor: may be just use all? As we do not support select on uri? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/24151/#comment86229> Not part of this patch, but might be worth to clean up some grantorPrincipal semantics - I do not think grantorPrincipal is required for createRole, we should instead pass it for alterSentryRoleGrantPrivilege and alterSentryRoleRevokePrivilege? - As we pass grantorPrincipal as the user invoking the thrift request(requestorUserName), we may keep it consistent and avoid possibility of passing different requestorUserName and grantorPrincipal by getting rid of grantorPrincipal field in the TSentryPrivilege and TSentryRole? We can do it as a follow on if you prefer. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/24151/#comment86234> Nit: Change comment to "insert" ? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithGrantOption.java <https://reviews.apache.org/r/24151/#comment86252> In all of these verifyFailureHook calls, we may want to assert that the hiveoperation is set correctly in the hook? - Sravya Tirukkovalur On July 31, 2014, 11:16 a.m., Sun Dapeng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24151/ > ----------------------------------------------------------- > > (Updated July 31, 2014, 11:16 a.m.) > > > Review request for sentry, Arun Suresh, Jarek Cecho, Prasad Mujumdar, and > Sravya Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > This review request contains all 8 subtasks(need apply the patch of > SENTRY-339 first) > SENTRY-340 Database implement for "with grant option" > SENTRY-341 Extend Thrift API for SentryStore to support "with grant option" > SENTRY-343 Privileges query from database support for "With Grant Option" > SENTRY-370 Judgement of MSentryPrivilege implies child privileges > SENTRY-342 Grant check with grant option > SENTRY-345 Revoke check with grant option > SENTRY-349 Extend Hive Hook with Grant Option > SENTRY-377 Add Hive e2e test for grantOption > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java > 26eea91 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 27a10ee > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > 991d734 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java > ac0d170 > sentry-core/sentry-core-common/pom.xml d1785b8 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java > 962179f > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBWildcardPrivilege.java > 896283c > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java > 9e8ac4c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > f8491db > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java.orig > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 945227e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > ff8acdc > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java.orig > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 5fd4f8f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 3bb7285 > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql > f2a62d2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.4.0.sql > f2a62d2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.4.0.sql > 70f4dbb > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.4.0.sql > 363590e > > sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.4.0.sql > 5dfae03 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > fdc7b9c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryPrivilege.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 7637376 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > a4ae291 > sentry-provider/sentry-provider-file/pom.xml b834c8e > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithGrantOption.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/24151/diff/ > > > Testing > ------- > > Unit tests in local > > > Thanks, > > Sun Dapeng > >
