You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sravya Tirukkovalur <sr...@cloudera.com> on 2014/08/01 00:47:36 UTC
Re: Review Request 24151: SENTRY-327 Support auth admin delegation via SQL
construct 'with grant option'
-----------------------------------------------------------
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
>
>
Re: Review Request 24151: SENTRY-327 Support auth admin delegation via SQL
construct 'with grant option'
Posted by Xiaomeng Huang <xi...@intel.com>.
> On July 31, 2014, 10:47 p.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.
> >
>
> Sun Dapeng wrote:
> Yes, this will be fixed.
In my opinion, SentryAccessDeniedException is meaning access data denied, and NoGrantOption is grant privilege denied.
I want to change the name of the exception to SentryGrantDeniedException.
And SentryGrantDeniedException will be parallel with SentryAccessDeniedException, both extends SentryUserException.
This is just my thoughts, if you think we can reuse the SentryAccessDeniedException to instead of grantoption exception. It's OK on my side, I also think it's simple and clearly.
> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, lines 234-237
> > <https://reviews.apache.org/r/24151/diff/1/?file=646825#file646825line234>
> >
> > URI is a child of server, so we need to check serverName equality even when uris are same.
fixed
> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, line 239
> > <https://reviews.apache.org/r/24151/diff/1/?file=646825#file646825line239>
> >
> > 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?
agreed. fixed
> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 468
> > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line468>
> >
> > I am not exactly sure why need this? I might be missing some thing.
fixed.
> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 492
> > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line492>
> >
> > would'nt the table name always __NULL__ here as we have a check earlier?
agreed and fixed. Refactor revoke with grantOption
> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1366
> > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line1366>
> >
> > 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?
It's a good comment. fixed. thx.
> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryPrivilege.java, line 56
> > <https://reviews.apache.org/r/24151/diff/1/?file=646838#file646838line56>
> >
> > Minor: may be just use all? As we do not support select on uri?
fixed
> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, line 367
> > <https://reviews.apache.org/r/24151/diff/1/?file=646839#file646839line367>
> >
> > Nit: Change comment to "insert" ?
fixed
- Xiaomeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24151/#review49253
-----------------------------------------------------------
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
>
>
Re: Review Request 24151: SENTRY-327 Support auth admin delegation via SQL
construct 'with grant option'
Posted by Sun Dapeng <da...@intel.com>.
> 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
>
>