You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sergio Pena via Review Board <no...@reviews.apache.org> on 2018/06/19 15:25:37 UTC
Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67646/
-----------------------------------------------------------
Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
Bugs: sentry-2272
https://issues.apache.org/jira/browse/sentry-2272
Repository: sentry
Description
-------
This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
Diff: https://reviews.apache.org/r/67646/diff/1/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On June 27, 2018, 1:08 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
> > Lines 230 (patched)
> > <https://reviews.apache.org/r/67646/diff/1/?file=2042347#file2042347line230>
> >
> > Could you change the comment as below.
> >
> > Sentry only stores user information when privileges are granted.User is deleted when there are no privileges associated to avoid stale data.
Is it necessary to change this? Both comments are correct.
> On June 27, 2018, 1:08 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
> > Lines 232-234 (patched)
> > <https://reviews.apache.org/r/67646/diff/1/?file=2042347#file2042347line232>
> >
> > Can you re-phrase this sentense. It is confusing.
> >
> > You could simple empty list is returned when the use is not found.
> >
> > Below sentense is not accurate. We end up in this situation when there are no privileges granted to the user. Hive can not perform any checks to avoid this situation.
> >
> > "For user checking, Hive must check that the user actually exists before calling this API.
Why Hive cannot perform this? I left the comment that 'Hive must' but not necessary means that Hive does. This comment is meant to explain Sentry should not check for the user but Hive should check it.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67646/#review205426
-----------------------------------------------------------
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On June 27, 2018, 1:08 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
> > Lines 230 (patched)
> > <https://reviews.apache.org/r/67646/diff/1/?file=2042347#file2042347line230>
> >
> > Could you change the comment as below.
> >
> > Sentry only stores user information when privileges are granted.User is deleted when there are no privileges associated to avoid stale data.
>
> Sergio Pena wrote:
> Is it necessary to change this? Both comments are correct.
The comment says "deletes the user when privileges are deleted". This is not clear. we delete the user when all the privileges assocated with the user are deleted.
> On June 27, 2018, 1:08 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
> > Lines 232-234 (patched)
> > <https://reviews.apache.org/r/67646/diff/1/?file=2042347#file2042347line232>
> >
> > Can you re-phrase this sentense. It is confusing.
> >
> > You could simple empty list is returned when the use is not found.
> >
> > Below sentense is not accurate. We end up in this situation when there are no privileges granted to the user. Hive can not perform any checks to avoid this situation.
> >
> > "For user checking, Hive must check that the user actually exists before calling this API.
>
> Sergio Pena wrote:
> Why Hive cannot perform this? I left the comment that 'Hive must' but not necessary means that Hive does. This comment is meant to explain Sentry should not check for the user but Hive should check it.
Let met re-phrase my previous comment. Comment could be simple. When the user is not found empty list is returned. API "showPrivileges" is public which can be used in any way.
Sentry throws this exception when there are no permissions granted to the user. What can Hive check in this scenario?
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67646/#review205426
-----------------------------------------------------------
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On June 27, 2018, 1:08 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
> > Lines 232-234 (patched)
> > <https://reviews.apache.org/r/67646/diff/1/?file=2042347#file2042347line232>
> >
> > Can you re-phrase this sentense. It is confusing.
> >
> > You could simple empty list is returned when the use is not found.
> >
> > Below sentense is not accurate. We end up in this situation when there are no privileges granted to the user. Hive can not perform any checks to avoid this situation.
> >
> > "For user checking, Hive must check that the user actually exists before calling this API.
>
> Sergio Pena wrote:
> Why Hive cannot perform this? I left the comment that 'Hive must' but not necessary means that Hive does. This comment is meant to explain Sentry should not check for the user but Hive should check it.
>
> kalyan kumar kalvagadda wrote:
> Let met re-phrase my previous comment. Comment could be simple. When the user is not found empty list is returned. API "showPrivileges" is public which can be used in any way.
>
> Sentry throws this exception when there are no permissions granted to the user. What can Hive check in this scenario?
This API is called by 'show grant user user1', so Hive can check if the 'show grant user user1' exists in the Linux system before calling this API. That's the check that Hive must do.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67646/#review205426
-----------------------------------------------------------
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67646/#review205426
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
Lines 230 (patched)
<https://reviews.apache.org/r/67646/#comment288333>
Could you change the comment as below.
Sentry only stores user information when privileges are granted.User is deleted when there are no privileges associated to avoid stale data.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
Lines 232-234 (patched)
<https://reviews.apache.org/r/67646/#comment288334>
Can you re-phrase this sentense. It is confusing.
You could simple empty list is returned when the use is not found.
Below sentense is not accurate. We end up in this situation when there are no privileges granted to the user. Hive can not perform any checks to avoid this situation.
"For user checking, Hive must check that the user actually exists before calling this API.
- kalyan kumar kalvagadda
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On June 20, 2018, 4:04 p.m., Arjun Mishra wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
> > Lines 235 (patched)
> > <https://reviews.apache.org/r/67646/diff/1/?file=2042347#file2042347line235>
> >
> > Should this be a WARN level message?
I don't think we should warn admins that the user does not exist. This log would happen if they run 'show grant user unknown_user'.
I feel a warning should be a message to alarm the user of something weird is happening and should be investigated but Sentry can continue without issues.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67646/#review205091
-----------------------------------------------------------
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
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/67646/#review205091
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
Lines 235 (patched)
<https://reviews.apache.org/r/67646/#comment288051>
Should this be a WARN level message?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
Lines 259 (patched)
<https://reviews.apache.org/r/67646/#comment288054>
Should this be a WARN level message?
- Arjun Mishra
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
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/67646/#review205186
-----------------------------------------------------------
Ship it!
Ship It!
- Arjun Mishra
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 67646: SENTRY-2272: Fix the sentry store logic for
listing user privileges
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67646/#review205447
-----------------------------------------------------------
Ship it!
- kalyan kumar kalvagadda
On June 19, 2018, 3:25 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67646/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:25 p.m.)
>
>
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
>
>
> Bugs: sentry-2272
> https://issues.apache.org/jira/browse/sentry-2272
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch catches the NoSuchObject Exception on the Sentry Hive binding when the SHOW GRANT USER is executed, and it returns an empty list of privileges for the requested user so that Hive does not display a nasty error message on the console.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java 321701d8662364f0a48899c1d8d5c75cc2ce62ff
>
>
> Diff: https://reviews.apache.org/r/67646/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>