> On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, > > line 254 > > <https://reviews.apache.org/r/24151/diff/1/?file=646839#file646839line254> > > > > 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.
Yes, I'm agree with you, <grantorPrincipal> shouldn't be added to TSentryPrivilege or TSentryRole, it should belong to Role_Privilege_Mapping, but SENTRY is using JDO auto generate the M-N Relation Mapping, we may need separate it to two One-to-Many Relation and add an entity, all the features about Role and Privilege Persistent may need to change, could you fired a jira after this feature? > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift, > > line 35 > > <https://reviews.apache.org/r/24151/diff/1/?file=646837#file646837line35> > > > > 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. Yes, Good suggestion. > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java, > > line 20 > > <https://reviews.apache.org/r/24151/diff/1/?file=646824#file646824line20> > > > > 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. > > Yes, this will be fixed. > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java, > > line 154 > > <https://reviews.apache.org/r/24151/diff/1/?file=646818#file646818line154> > > > > Minor: May be add a positive test case for with grant option here? Yes, this will be fixed. > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java.orig, > > line 1 > > <https://reviews.apache.org/r/24151/diff/1/?file=646826#file646826line1> > > > > Looks like this file is in the patch my mistake? Yes, this will be fixed. > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1048 > > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line1048> > > > > 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? The method is do converting between MSentryPrivilege and TSentryPrivilege, it will follow the original privilege. > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1072 > > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line1072> > > > > 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? Same as above: The method is do converting between MSentryPrivilege and TSentryPrivilege, it will follow the original privilege. > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, > > line 484 > > <https://reviews.apache.org/r/24151/diff/1/?file=646830#file646830line484> > > > > 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? Same as above: The method is do converting between MSentryPrivilege and TSentryPrivilege, it will follow the original privilege. This will change, but there is no impact with result. > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithGrantOption.java, > > line 101 > > <https://reviews.apache.org/r/24151/diff/1/?file=646842#file646842line101> > > > > In all of these verifyFailureHook calls, we may want to assert that the > > hiveoperation is set correctly in the hook? Good suggestion. I will do it > On Aug. 1, 2014, 6:47 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java.orig, > > line 1 > > <https://reviews.apache.org/r/24151/diff/1/?file=646829#file646829line1> > > > > Looks like this file is part of the patch by mistake? Thank you, I will fix it. - Sun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24151/#review49253 ----------------------------------------------------------- On July 31, 2014, 7:16 p.m., Sun Dapeng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24151/ > ----------------------------------------------------------- > > (Updated July 31, 2014, 7:16 p.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 > >
