You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Colin Ma <ju...@intel.com> on 2015/05/12 09:14:31 UTC
Review Request 34087: SENTRY-735: Update AuthorizationProvider and
e2e test for grant user to role
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/
-----------------------------------------------------------
Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar.
Repository: sentry
Description
-------
Update AuthorizationProvider and e2e test for grant user to role
Diffs
-----
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 06573b7
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java f57198a
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java 32b2d72
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 3a8a6ef
Diff: https://reviews.apache.org/r/34087/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/#review126549
-----------------------------------------------------------
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java (line 104)
<https://reviews.apache.org/r/34087/#comment189579>
So this is the only place to check the enforcement of user privileges.
Check that we don't miss any other places.
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java (line 131)
<https://reviews.apache.org/r/34087/#comment189569>
We may don't need to specially check the privilege for user. If it is not support, it simply doesn't have any privilege for the user.
So the call we be always:
getPrivilege(grous, users, roleSet, authroizables)
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java (line 186)
<https://reviews.apache.org/r/34087/#comment189570>
The same that we always call the getPrivileges method with groups and users, if it doesn't has user privilege, simply no additioal privilege is added.
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java (line 98)
<https://reviews.apache.org/r/34087/#comment189573>
What's the concept of "customer" here?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java (line 106)
<https://reviews.apache.org/r/34087/#comment189574>
Let's remove the trailing white space here.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java (line 139)
<https://reviews.apache.org/r/34087/#comment189575>
Let's remove the trailing white space here.
- Jerry Chen
On May 12, 2015, 7:14 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34087/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 7:14 a.m.)
>
>
> Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update AuthorizationProvider and e2e test for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 06573b7
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java f57198a
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java 32b2d72
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 3a8a6ef
>
> Diff: https://reviews.apache.org/r/34087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/#review128326
-----------------------------------------------------------
Ship it!
Ship It!
- Dapeng Sun
On 四月 11, 2016, 3:52 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34087/
> -----------------------------------------------------------
>
> (Updated 四月 11, 2016, 3:52 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update AuthorizationProvider and e2e test for grant user to role
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHookBase.java 6df939f
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 0cf0b5d
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java 14af2d4
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/
-----------------------------------------------------------
(Updated 四月 11, 2016, 7:52 a.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update AuthorizationProvider and e2e test for grant user to role
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHookBase.java 6df939f
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 0cf0b5d
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java 14af2d4
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
Diff: https://reviews.apache.org/r/34087/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/
-----------------------------------------------------------
(Updated 四月 8, 2016, 6:22 a.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update AuthorizationProvider and e2e test for grant user to role
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHookBase.java 6df939f
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 0cf0b5d
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java 14af2d4
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java 991a95f
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 8515a2b
Diff: https://reviews.apache.org/r/34087/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Colin Ma <ju...@intel.com>.
> On 四月 7, 2016, 6:23 a.m., Jerry Chen wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java, line 179
> > <https://reviews.apache.org/r/34087/diff/3/?file=1327670#file1327670line179>
> >
> > Suggest to remove:
> > Set<String> result = Sets.newHashSet();
> >
> > And use the following instead:
> > Set<String> result = policy...
Thank you for catching it.
> On 四月 7, 2016, 6:23 a.m., Jerry Chen wrote:
> > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java, line 99
> > <https://reviews.apache.org/r/34087/diff/3/?file=1327672#file1327672line99>
> >
> > Question: What is appendUserGroupsMappings for? Do we need this? Is it used only for test cases?
> >
> > The naming may need to be reconsidered if we want to keep it. Names like UserGroupsMappings is very tricky. Consider a simple name: addUserGroupMappings?
Yes, we need, currently setUserGroupsMappings() can only accpect `HashMap`, it's one-one mapping, the UserGroupMapping in PolicyFile is Multimap, it's one-many mapping, the old test case didn't cover one-many user group mapping, adding this method would be the a better solution than calling setUserGroupsMappings several times.
I will rename it to `addUserGroupMappings`, Thanks.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/#review127561
-----------------------------------------------------------
On 四月 6, 2016, 2:58 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34087/
> -----------------------------------------------------------
>
> (Updated 四月 6, 2016, 2:58 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update AuthorizationProvider and e2e test for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 0cf0b5d
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java 14af2d4
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java 991a95f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 8515a2b
>
> Diff: https://reviews.apache.org/r/34087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/#review127561
-----------------------------------------------------------
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java (line 179)
<https://reviews.apache.org/r/34087/#comment190935>
Suggest to remove:
Set<String> result = Sets.newHashSet();
And use the following instead:
Set<String> result = policy...
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java (line 99)
<https://reviews.apache.org/r/34087/#comment190937>
Question: What is appendUserGroupsMappings for? Do we need this? Is it used only for test cases?
The naming may need to be reconsidered if we want to keep it. Names like UserGroupsMappings is very tricky. Consider a simple name: addUserGroupMappings?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java (line 259)
<https://reviews.apache.org/r/34087/#comment190944>
Unclear what this is for: show the reason how the users and groups is related in testing roles for user feature?
- Jerry Chen
On April 6, 2016, 2:58 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34087/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 2:58 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update AuthorizationProvider and e2e test for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 0cf0b5d
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java 14af2d4
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java 991a95f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 8515a2b
>
> Diff: https://reviews.apache.org/r/34087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/
-----------------------------------------------------------
(Updated 四月 6, 2016, 2:58 p.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update AuthorizationProvider and e2e test for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 0cf0b5d
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java 14af2d4
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java 991a95f
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 8515a2b
Diff: https://reviews.apache.org/r/34087/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34087: SENTRY-735: Update AuthorizationProvider
and e2e test for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34087/
-----------------------------------------------------------
(Updated 四月 6, 2016, 5:30 a.m.)
Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar.
Changes
-------
Update patch
Repository: sentry
Description
-------
Update AuthorizationProvider and e2e test for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 0cf0b5d
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java 14af2d4
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java 991a95f
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 8515a2b
Diff: https://reviews.apache.org/r/34087/diff/
Testing
-------
Thanks,
Colin Ma