You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Lenni Kuff <ls...@cloudera.com> on 2014/11/27 08:18:37 UTC
Review Request 28507: SENTRY-543: Sentry Store permission dump
incorrect after recursive revoke
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28507/
-----------------------------------------------------------
Review request for sentry.
Repository: sentry
Description
-------
A revoke on a parent authorizeable was not taking into account the privilege action when removing child privileges. This mean that a REVOKE SELECT ON DATABASE would also delete privileges with an action of ALL or INSERT on child tables/columns. This fix is to verify that each of the child privileges collected "implies" the parent privilege.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 073bb33
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e8be81f
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 3189955
Diff: https://reviews.apache.org/r/28507/diff/
Testing
-------
Added new tests.
Thanks,
Lenni Kuff
Re: Review Request 28507: SENTRY-543: Sentry Store permission dump
incorrect after recursive revoke
Posted by Xiaomeng Huang <xi...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28507/#review63192
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/28507/#comment105380>
Hi, I think we don't need to get all child privileges and then do the filter. We can just add a query filter like:
if parent's action is not ALL
filters.append(" && action == \"" + parent.getAction() + "\"");
This approach make the query clearly and improve performance.
Hi Lenni, the patch looks fine me. If you use implies approach, a bug SENTRY-553 may block your test case in TestDatabaseProvider#testRevokeAllOnServer, I have fixed it, if you have time, please have a look.
I also have another approach to add a query filter for action and comment in your patch, I think it also can work.
Other comments, there are some extra whitespaces in your patch.
- Xiaomeng Huang
On 十一月 27, 2014, 7:18 a.m., Lenni Kuff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28507/
> -----------------------------------------------------------
>
> (Updated 十一月 27, 2014, 7:18 a.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> A revoke on a parent authorizeable was not taking into account the privilege action when removing child privileges. This mean that a REVOKE SELECT ON DATABASE would also delete privileges with an action of ALL or INSERT on child tables/columns. This fix is to verify that each of the child privileges collected "implies" the parent privilege.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 073bb33
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e8be81f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 3189955
>
> Diff: https://reviews.apache.org/r/28507/diff/
>
>
> Testing
> -------
>
> Added new tests.
>
>
> Thanks,
>
> Lenni Kuff
>
>