You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Haley Reeve via Review Board <no...@reviews.apache.org> on 2019/02/14 16:36:11 UTC
Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/
-----------------------------------------------------------
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
Repository: sentry
Description
-------
The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 94783fa
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 6a7c1f3
Diff: https://reviews.apache.org/r/69987/diff/1/
Testing
-------
Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
Thanks,
Haley Reeve
Re: Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review213085
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On Feb. 21, 2019, 8:17 p.m., Haley Reeve wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2019, 8:17 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
>
> The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
>
>
> Diff: https://reviews.apache.org/r/69987/diff/2/
>
>
> Testing
> -------
>
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
>
>
> Thanks,
>
> Haley Reeve
>
>
Re: Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
Posted by Haley Reeve via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/
-----------------------------------------------------------
(Updated Feb. 21, 2019, 8:17 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
Changes
-------
Changed logic so that the HiveAuthzConf is not needed. Instead if scheme is null, check if the URI path is absolute and if so return true.
Repository: sentry
Description
-------
The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
Diff: https://reviews.apache.org/r/69987/diff/2/
Changes: https://reviews.apache.org/r/69987/diff/1-2/
Testing
-------
Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
Thanks,
Haley Reeve
Re: Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212874
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Line 283 (original), 285 (patched)
<https://reviews.apache.org/r/69987/#comment298763>
Use PathUtils#parseLocalURI method if uri.getScheme is null to re-intantiate the object. That should work
- Arjun Mishra
On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 4:36 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
>
> The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 94783fa
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 6a7c1f3
>
>
> Diff: https://reviews.apache.org/r/69987/diff/1/
>
>
> Testing
> -------
>
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
>
>
> Thanks,
>
> Haley Reeve
>
>
Re: Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Feb. 15, 2019, 6:52 p.m., Arjun Mishra wrote:
> > Haley I think we can simplify this a lot without any refactoring by using PathUtils#parseLocalURI method.
> >
> > Since the method is isLocalUri => It is looking to check for a local URI. If uri.getScheme() throws null we can set uri = PathUtils.parseLocalURI(path). If that is still null we return false.
> >
> > Let me know your thoughts
>
> Haley Reeve wrote:
> That's an alternate way of doing it. In that case the only way we'd get to false is to catch the IllegalArgumentException PathUtils#parseLocalURI throws. I don't think it greatly simplifies anything except that we don't have to pass down the HiveAuthzConf anymore. Is adding that parameter to the function definitions undesirable?
Yeah I think so. It is too much of refactoring for not much gain. We want to check if it is a local URI. If You can't parse a local URI then it definitely isnt. So that should be sufficient.
If you want you can directly instantiate: URI uri = new URI(PathUtils.parseLocalURI(uriString)); and catch exception.
Will simplify code
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212873
-----------------------------------------------------------
On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 4:36 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
>
> The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 94783fa
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 6a7c1f3
>
>
> Diff: https://reviews.apache.org/r/69987/diff/1/
>
>
> Testing
> -------
>
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
>
>
> Thanks,
>
> Haley Reeve
>
>
Re: Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
Posted by Haley Reeve via Review Board <no...@reviews.apache.org>.
> On Feb. 15, 2019, 6:52 p.m., Arjun Mishra wrote:
> > Haley I think we can simplify this a lot without any refactoring by using PathUtils#parseLocalURI method.
> >
> > Since the method is isLocalUri => It is looking to check for a local URI. If uri.getScheme() throws null we can set uri = PathUtils.parseLocalURI(path). If that is still null we return false.
> >
> > Let me know your thoughts
That's an alternate way of doing it. In that case the only way we'd get to false is to catch the IllegalArgumentException PathUtils#parseLocalURI throws. I don't think it greatly simplifies anything except that we don't have to pass down the HiveAuthzConf anymore. Is adding that parameter to the function definitions undesirable?
- Haley
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212873
-----------------------------------------------------------
On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 4:36 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
>
> The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 94783fa
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 6a7c1f3
>
>
> Diff: https://reviews.apache.org/r/69987/diff/1/
>
>
> Testing
> -------
>
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
>
>
> Thanks,
>
> Haley Reeve
>
>
Re: Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
Posted by Haley Reeve via Review Board <no...@reviews.apache.org>.
> On Feb. 15, 2019, 6:52 p.m., Arjun Mishra wrote:
> > Haley I think we can simplify this a lot without any refactoring by using PathUtils#parseLocalURI method.
> >
> > Since the method is isLocalUri => It is looking to check for a local URI. If uri.getScheme() throws null we can set uri = PathUtils.parseLocalURI(path). If that is still null we return false.
> >
> > Let me know your thoughts
>
> Haley Reeve wrote:
> That's an alternate way of doing it. In that case the only way we'd get to false is to catch the IllegalArgumentException PathUtils#parseLocalURI throws. I don't think it greatly simplifies anything except that we don't have to pass down the HiveAuthzConf anymore. Is adding that parameter to the function definitions undesirable?
>
> Arjun Mishra wrote:
> Yeah I think so. It is too much of refactoring for not much gain. We want to check if it is a local URI. If You can't parse a local URI then it definitely isnt. So that should be sufficient.
> If you want you can directly instantiate: URI uri = new URI(PathUtils.parseLocalURI(uriString)); and catch exception.
> Will simplify code
I don't love the idea of catching the exception. And the parseLocalURI does more than we need it to. But the uriPath.isAbsolute() method looked useful. So I took another shot at the conditional without using the authz config. Uploading new diff now.
- Haley
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212873
-----------------------------------------------------------
On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 4:36 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
>
> The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 94783fa
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 6a7c1f3
>
>
> Diff: https://reviews.apache.org/r/69987/diff/1/
>
>
> Testing
> -------
>
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
>
>
> Thanks,
>
> Haley Reeve
>
>
Re: Review Request 69987: SENTRY-2497: show grant role results should
handle case where URI doesn't have a defined scheme
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212873
-----------------------------------------------------------
Haley I think we can simplify this a lot without any refactoring by using PathUtils#parseLocalURI method.
Since the method is isLocalUri => It is looking to check for a local URI. If uri.getScheme() throws null we can set uri = PathUtils.parseLocalURI(path). If that is still null we return false.
Let me know your thoughts
- Arjun Mishra
On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 4:36 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The "show grant role" logic tries to use a URI's scheme to tell whether it's a local URI or a DFS URI. However, it's valid for the scheme to be undefined. In that case Sentry throws a NPE because it's trying to access a null scheme.
>
> The logic has been updated to instead use the default filesystem set in the "fs.defaultFS" property if the scheme is not defined.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 94783fa
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java 5996b6c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 6a7c1f3
>
>
> Diff: https://reviews.apache.org/r/69987/diff/1/
>
>
> Testing
> -------
>
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test fails without code change, and succeeds with code change.
>
>
> Thanks,
>
> Haley Reeve
>
>