You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Hao Hao via Review Board <no...@reviews.apache.org> on 2018/09/23 20:52:33 UTC
Review Request 68822: SENTRY-2371 Add a new thrift API for getting all
privileges a user has
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/
-----------------------------------------------------------
Review request for sentry, Na Li and Sergio Pena.
Repository: sentry
Description
-------
This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
Diffs
-----
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
Diff: https://reviews.apache.org/r/68822/diff/1/
Testing
-------
Unit test.
Thanks,
Hao Hao
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
> On Sept. 24, 2018, 11:23 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 915 (patched)
> > <https://reviews.apache.org/r/68822/diff/2/?file=2091812#file2091812line915>
> >
> > you have got groups in line 908 if the requestor is the same as principlaName. It should improve performance to reuse it under this situation
I don't think the requestor is the same as principal? Here it is getting the principal's group.
> On Sept. 24, 2018, 11:23 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
> > Line 227 (original), 229 (patched)
> > <https://reviews.apache.org/r/68822/diff/2/?file=2091814#file2091814line229>
> >
> > This is semi-public API. It is safer to add function than change the returned type of existing function. Inside implementation, you can let one function call another one to re-use code.
I was hesitate to add another API to complicate the code base. Also I do not see any other consumer of this API other than SentryPolicyStoreProcessor so I made change on top of the existing one. But if you prefer to add a new one to avoid unknown dependency, I will update this.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review208969
-----------------------------------------------------------
On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 9:31 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
>
>
> Diff: https://reviews.apache.org/r/68822/diff/2/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
> On Sept. 24, 2018, 11:23 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 915 (patched)
> > <https://reviews.apache.org/r/68822/diff/2/?file=2091812#file2091812line915>
> >
> > you have got groups in line 908 if the requestor is the same as principlaName. It should improve performance to reuse it under this situation
>
> Hao Hao wrote:
> I don't think the requestor is the same as principal? Here it is getting the principal's group.
Hmm, misinterpret your suggestion, updating it.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review208969
-----------------------------------------------------------
On Sept. 25, 2018, 9:23 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2018, 9:23 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
>
>
> Diff: https://reviews.apache.org/r/68822/diff/3/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
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/68822/#review208969
-----------------------------------------------------------
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
Lines 66 (patched)
<https://reviews.apache.org/r/68822/#comment293235>
Can you change the function from "list_sentry_privileges_for_user" to "list_sentry_privileges_by_user_and_itsgroups" to make it clear that it includes privileges directly assigned to this user and privileges through its groups?
You don't have to use "list_sentry_privileges_by_user_and_itsgroups" exactly, but use some name similar to it.
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 416 (patched)
<https://reviews.apache.org/r/68822/#comment293236>
"and the group" to "and the groups" since a user can belong to multiple groups.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
Lines 113 (patched)
<https://reviews.apache.org/r/68822/#comment293237>
change the function name from "list-privileges-for-user" to the one you will use in sentry_policy_service.thrift
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 57 (patched)
<https://reviews.apache.org/r/68822/#comment293238>
It is better to add the new function in SentryStoreInterface instead of SentryStore as this class deals with the interface, not its implementation
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 915 (patched)
<https://reviews.apache.org/r/68822/#comment293240>
you have got groups in line 908 if the requestor is the same as principlaName. It should improve performance to reuse it under this situation
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
Line 227 (original), 229 (patched)
<https://reviews.apache.org/r/68822/#comment293242>
This is semi-public API. It is safer to add function than change the returned type of existing function. Inside implementation, you can let one function call another one to re-use code.
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
Lines 26 (patched)
<https://reviews.apache.org/r/68822/#comment293243>
If you only add new function in SentryStoreInterface, you can make it part of the TestSentryStore if those testing cases don't exsit.
- Na Li
On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 9:31 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
>
>
> Diff: https://reviews.apache.org/r/68822/diff/2/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
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/68822/#review209014
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On Sept. 25, 2018, 9:23 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2018, 9:23 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
>
>
> Diff: https://reviews.apache.org/r/68822/diff/3/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
> On Sept. 25, 2018, 11:22 p.m., Na Li wrote:
> > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
> > Lines 419 (patched)
> > <https://reviews.apache.org/r/68822/diff/3/?file=2092016#file2092016line419>
> >
> > I heard today that it is possible to assign user to a role in near future. So can you change the function name to "list_sentry_privileges_by_user_and_itsroles"? it will cover privileges
> >
> > 1) assigned to user directly
> > 2) assigned to roles directly associated to a user
> > 3) assigned to roles directly associated to groups that user belongs to.
> >
> > Then when that feature of allowing user to be associated to a role is implemented, we don't need to change this API. And the intention of this function is to get all privileges for a user directly and indirectly
>
> Hao Hao wrote:
> Hmm, actually I think list_sentry_privileges_by_user_and_itsroles may be more misleading. Since people may think the API only lists privileges granted to the roles directly associated to that user.
Lina and I discussed offline, and we both think the current naming is good enough as the reason stated above.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review209007
-----------------------------------------------------------
On Sept. 25, 2018, 9:23 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2018, 9:23 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
>
>
> Diff: https://reviews.apache.org/r/68822/diff/3/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
> On Sept. 25, 2018, 11:22 p.m., Na Li wrote:
> > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
> > Lines 419 (patched)
> > <https://reviews.apache.org/r/68822/diff/3/?file=2092016#file2092016line419>
> >
> > I heard today that it is possible to assign user to a role in near future. So can you change the function name to "list_sentry_privileges_by_user_and_itsroles"? it will cover privileges
> >
> > 1) assigned to user directly
> > 2) assigned to roles directly associated to a user
> > 3) assigned to roles directly associated to groups that user belongs to.
> >
> > Then when that feature of allowing user to be associated to a role is implemented, we don't need to change this API. And the intention of this function is to get all privileges for a user directly and indirectly
Hmm, actually I think list_sentry_privileges_by_user_and_itsroles may be more misleading. Since people may think the API only lists privileges granted to the roles directly associated to that user.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review209007
-----------------------------------------------------------
On Sept. 25, 2018, 9:23 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2018, 9:23 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
>
>
> Diff: https://reviews.apache.org/r/68822/diff/3/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
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/68822/#review209007
-----------------------------------------------------------
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 419 (patched)
<https://reviews.apache.org/r/68822/#comment293272>
I heard today that it is possible to assign user to a role in near future. So can you change the function name to "list_sentry_privileges_by_user_and_itsroles"? it will cover privileges
1) assigned to user directly
2) assigned to roles directly associated to a user
3) assigned to roles directly associated to groups that user belongs to.
Then when that feature of allowing user to be associated to a role is implemented, we don't need to change this API. And the intention of this function is to get all privileges for a user directly and indirectly
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
Lines 247 (patched)
<https://reviews.apache.org/r/68822/#comment293273>
Can you change the name of this function to "listSentryPrivilegesByUsersAndRoles" to be consistent with client API function name?
- Na Li
On Sept. 25, 2018, 9:23 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2018, 9:23 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
>
>
> Diff: https://reviews.apache.org/r/68822/diff/3/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/
-----------------------------------------------------------
(Updated Sept. 25, 2018, 9:23 p.m.)
Review request for sentry, Na Li and Sergio Pena.
Repository: sentry
Description
-------
This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
Diffs (updated)
-----
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
Diff: https://reviews.apache.org/r/68822/diff/3/
Changes: https://reviews.apache.org/r/68822/diff/2-3/
Testing
-------
Unit test.
Thanks,
Hao Hao
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review208972
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
Line 227 (original), 229 (patched)
<https://reviews.apache.org/r/68822/#comment293246>
I was hesitate to add another API to complicate the code base. Also I do not see any other consumer of this API other than SentryPolicyStoreProcessor so I made change on top of the existing one. But if you prefer adding a new one, I will update this.
- Hao Hao
On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 9:31 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
>
>
> Diff: https://reviews.apache.org/r/68822/diff/2/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/
-----------------------------------------------------------
(Updated Sept. 24, 2018, 9:31 p.m.)
Review request for sentry, Na Li and Sergio Pena.
Repository: sentry
Description
-------
This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
Diffs (updated)
-----
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
Diff: https://reviews.apache.org/r/68822/diff/2/
Changes: https://reviews.apache.org/r/68822/diff/1-2/
Testing
-------
Unit test.
Thanks,
Hao Hao
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review208947
-----------------------------------------------------------
Fix it, then Ship it!
It looks good.
Just change the entityName for principalName in the below lines.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 888 (patched)
<https://reviews.apache.org/r/68822/#comment293213>
Shouldn't be 'principalName'?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 891 (patched)
<https://reviews.apache.org/r/68822/#comment293212>
shouldn't be principalName?
- Sergio Pena
On Sept. 23, 2018, 8:52 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2018, 8:52 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
>
>
> Diff: https://reviews.apache.org/r/68822/diff/1/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
> On Sept. 24, 2018, 3:35 p.m., Sergio Pena wrote:
> > Btw, are you going to have another JIRA for the client to call this new API?
>
> Hao Hao wrote:
> Sure, I can create a jira for adding the client side logical.
SENTRY-2418 is filed.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review208950
-----------------------------------------------------------
On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 9:31 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
>
>
> Diff: https://reviews.apache.org/r/68822/diff/2/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Hao Hao via Review Board <no...@reviews.apache.org>.
> On Sept. 24, 2018, 3:35 p.m., Sergio Pena wrote:
> > Btw, are you going to have another JIRA for the client to call this new API?
Sure, I can create a jira for adding the client side logical.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review208950
-----------------------------------------------------------
On Sept. 23, 2018, 8:52 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2018, 8:52 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
>
>
> Diff: https://reviews.apache.org/r/68822/diff/1/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 68822: SENTRY-2371 Add a new thrift API for
getting all privileges a user has
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68822/#review208950
-----------------------------------------------------------
Btw, are you going to have another JIRA for the client to call this new API?
- Sergio Pena
On Sept. 23, 2018, 8:52 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2018, 8:52 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This commit adds a new thrift API list_sentry_privileges_for_user to List sentry privileges granted to the given user and the group the user associated with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java 0cbd8ab0a624d4c09aead4097f72762e12d1d21b
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 2e79e5646ae9102d8c0c28da4260a539254fcd15
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 236a07bdf5191cdc0f167f20a406b721b3dc506d
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3a9623b46f7c4335db18113574170f761da9a4ca
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1eda41b2b6bd940a404cc1ba09a861fe783ead04
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java 0b4f4aa24bd3002c50bf4d80a6fa361f66052973
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java PRE-CREATION
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4
>
>
> Diff: https://reviews.apache.org/r/68822/diff/1/
>
>
> Testing
> -------
>
> Unit test.
>
>
> Thanks,
>
> Hao Hao
>
>