You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org> on 2018/06/05 17:34:17 UTC

Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/
-----------------------------------------------------------

Review request for sentry, Na Li and Sergio Pena.

Bugs: SENTRY-2257
https://issues.apache.org/jira/browse/SENTRY-2257

Repository: sentry

Description
-------

Implement functionality in sentry store to update owner privilege on an authorizable.

Here is the approach.

There are two new API's that are exposed.

To list the owner privileges granted to an authorizable
1. update the owner privilege to new owner

Here is the Flow.
1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.

This way all the updated listed below happen in the same transaction
1. Revoking the exixting owner privilage for authorizable
2. Granting new owner privilege fot authorizable.
3. Adding delta update for owner privilege revoked
4. Adding delta update for owner privilege granted.

Diffs
-----

sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c

Diff: https://reviews.apache.org/r/67452/diff/1/

Testing
-------

Added new tests to verify new functionality added.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 865 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line865>
> >
> >     There exists a alterSentryRoleGrantPrivilegeCore() and alterSentryUserGrantPrivilegeCore() methods already. is there a need to have this method? Can we have an if() section in the caller and make the right call instead?

Current API's alterSentryRoleGrantPrivilegeCore and alterSentryUserGrantPrivilegeCore are specific to DB based privileges. For example: if insert/select are granted, these API's check if there is a privilege with "ALL" then prvilege is not actually added. Likewise if ALL is granted, and there are insert/select privileges already, they are removed. None of them are correct for owner privilge so I had to add new method.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 2285 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line2285>
> >
> >     This method looks pretty similar to getMSentryPrivilegesByAuth() except that it sets the OWNER as action. Isn't better to add the action as parameter and reuse the other method instead?

There are two changes. 1. setting the action as OWNER 2. Adding fetch group. This is removed in this API as it is not needed. I can do it but confusing.

If it were just setting the action as owner i seriously considered using the same API.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 2982-2989 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line2982>
> >
> >     This comment is orphan or out of place. I see two header comments in this method.

I realized it. I was actually removed in patch i submitted immediatly.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3000 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line3000>
> >
> >     Shouldn't we search for the current owner privileges of the tAuthorizable here and delete those instead of passing them as a parameter? Being a public api this could be prone to errors if we expect the caller should pass those privileges to revoke.

I have done that deliberatly. We need to create delta updates for all the privileges that are being removed and he privile that is granted. This is done using the sentry plug-in invoked in processor. If you look at the descrption of the review i have explained it.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3016 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line3016>
> >
> >     There should be a better way to delete privileges in one call instead of one at a time. Like DELETE FROM mPrivilege WHERE type = OWNER & db = db1 && table = t1, right?

I has to interate through all them to see if any of the user entry would be stale because of this delete and remove it so I was removing one at a time which i can not avoid.

Yes, privilges can removed with single query. Normally there will be only one privilege that will be moved unless there is some issue. Do you still feel that i should build a query to delete all of them at once?


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3020 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line3020>
> >
> >     We should check what info this object brings. Owner privileges can only be granted to databases and tables.

This logic is added in SentryPolicyStoreProcessor where this API is invoked. You will see it in my next patch.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3031-3032 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line3031>
> >
> >     Is it only for dropping privileges? I think the message is incorrect here.

It is in-correct. I will fix it.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 4953 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line4953>
> >
> >     Why is the number 3 needed? Theexecute() method for a single update has 2 because it only adds up to to updates, but what about here? does it need a comment on why 3 is a good number?

Excute method at will minimum will have two updates and one TransactionBlock. It boils down to two DeltaTransactionBlock and one TransactionBlock making it to 3. I can add a comment.


> On June 5, 2018, 7:40 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 4954-4955 (patched)
> > <https://reviews.apache.org/r/67452/diff/2/?file=2035332#file2035332line4954>
> >
> >     Our coding convention is to have spaces between (), like if () and for (). Also a space between the :, like update : updates

will fix it.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/#review204340
-----------------------------------------------------------


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204340
-----------------------------------------------------------




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 865 (patched)
<https://reviews.apache.org/r/67452/#comment286802>

    There exists a alterSentryRoleGrantPrivilegeCore() and alterSentryUserGrantPrivilegeCore() methods already. is there a need to have this method? Can we have an if() section in the caller and make the right call instead?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2285 (patched)
<https://reviews.apache.org/r/67452/#comment286801>

    This method looks pretty similar to getMSentryPrivilegesByAuth() except that it sets the OWNER as action. Isn't better to add the action as parameter and reuse the other method instead?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2982-2989 (patched)
<https://reviews.apache.org/r/67452/#comment286781>

    This comment is orphan or out of place. I see two header comments in this method.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3000 (patched)
<https://reviews.apache.org/r/67452/#comment286790>

    Shouldn't we search for the current owner privileges of the tAuthorizable here and delete those instead of passing them as a parameter? Being a public api this could be prone to errors if we expect the caller should pass those privileges to revoke.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3016 (patched)
<https://reviews.apache.org/r/67452/#comment286793>

    There should be a better way to delete privileges in one call instead of one at a time. Like DELETE FROM mPrivilege WHERE type = OWNER & db = db1 && table = t1, right?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3020 (patched)
<https://reviews.apache.org/r/67452/#comment286795>

    We should check what info this object brings. Owner privileges can only be granted to databases and tables.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3031-3032 (patched)
<https://reviews.apache.org/r/67452/#comment286797>

    Is it only for dropping privileges? I think the message is incorrect here.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 4953 (patched)
<https://reviews.apache.org/r/67452/#comment286785>

    Why is the number 3 needed? Theexecute() method for a single update has 2 because it only adds up to to updates, but what about here? does it need a comment on why 3 is a good number?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 4954-4955 (patched)
<https://reviews.apache.org/r/67452/#comment286786>

    Our coding convention is to have spaces between (), like if () and for (). Also a space between the :, like update : updates


- Sergio Pena


On June 5, 2018, 7:34 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 5, 2018, 7:34 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/3/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On June 5, 2018, 8:16 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 865 (patched)
> > <https://reviews.apache.org/r/67452/diff/3/?file=2035335#file2035335line865>
> >
> >     Is the function only used for owner privilege? If so, can you make the comment and name clear?
> >     
> >     If not, then the processing is very different from current behavior in alterSentryRoleGrantPrivilegeCore and alterSentryUserGrantPrivilegeCore. It can easily cause confusion

Current API's alterSentryRoleGrantPrivilegeCore and alterSentryUserGrantPrivilegeCore are specific to DB based privileges. For example: if insert/select are granted, these API's check if there is a privilege with "ALL" then prvilege is not actually added. Likewise if ALL is granted, and there are insert/select privileges already, they are removed. None of them are correct for owner privilge so I had to add new method.


Comming to to your point af changing the comment, I was not sure of it. This APi's can be used other privileges that might be added which don't want to use the above logic. What do you say?


> On June 5, 2018, 8:16 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 2995 (patched)
> > <https://reviews.apache.org/r/67452/diff/3/?file=2035335#file2035335line2995>
> >
> >     You need to generate the updates for old owner privilege, and new owner privilege, so they can be saved and used for HDFS sync.

Yes, that is what List<Update> updates contain. It has updates to remove current owner privileges and an update granting new owner privilege.


> On June 5, 2018, 8:16 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3002 (patched)
> > <https://reviews.apache.org/r/67452/diff/3/?file=2035335#file2035335line3002>
> >
> >     you should call mPriv.removeUser() for its user, and removeRole for its role. It is possible the owner is a role.

I'm calling pm.deletePersistent(mPriv) which removes privilge from both the users and roles.I don't have to call removeUser OR removeRole explicitly.

Reason I have special logic for user is to remove them if they are stale. I do not need logic for roles as they should not be removed.


> On June 5, 2018, 8:16 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3004 (patched)
> > <https://reviews.apache.org/r/67452/diff/3/?file=2035335#file2035335line3004>
> >
> >     it is better to use function isUserStale
> >     
> >       private boolean isUserStale(MSentryUser user) {
> >         if (user.getPrivileges().isEmpty() && user.getRoles().isEmpty()) {
> >           return true;
> >         }
> >     
> >         return false;
> >       }
> >     
> >     If a user has role, it should not be removed

It's better to use such method but I can not use it here as the user still has one privilege here.
Yes, I should check for roles as well. I will update it.


> On June 5, 2018, 8:16 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3008 (patched)
> > <https://reviews.apache.org/r/67452/diff/3/?file=2035335#file2035335line3008>
> >
> >     You don't need to remove this privilege and then create it in alterSentryGrantPrivilegeCore. You can attach owner to this privielge and then save it.

I can not assume that there will be only one privilege that that is revoked. Logic here is assumes that there could mutiple of them. That is why we check for all the owner privileges that are granted to that authorizable. I did not want to mix that with a grant.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/#review204344
-----------------------------------------------------------


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On June 5, 2018, 8:16 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 865 (patched)
> > <https://reviews.apache.org/r/67452/diff/3/?file=2035335#file2035335line865>
> >
> >     Is the function only used for owner privilege? If so, can you make the comment and name clear?
> >     
> >     If not, then the processing is very different from current behavior in alterSentryRoleGrantPrivilegeCore and alterSentryUserGrantPrivilegeCore. It can easily cause confusion
> 
> kalyan kumar kalvagadda wrote:
>     Current API's alterSentryRoleGrantPrivilegeCore and alterSentryUserGrantPrivilegeCore are specific to DB based privileges. For example: if insert/select are granted, these API's check if there is a privilege with "ALL" then prvilege is not actually added. Likewise if ALL is granted, and there are insert/select privileges already, they are removed. None of them are correct for owner privilge so I had to add new method.
>     
>     
>     Comming to to your point af changing the comment, I was not sure of it. This APi's can be used other privileges that might be added which don't want to use the above logic. What do you say?

if you don't limit the new API to owner privilege, and keep its behavior different from existing function, you guarantee that some bug will be created in the future, and the behavior will change. That is not desirable.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/#review204344
-----------------------------------------------------------


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204344
-----------------------------------------------------------




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 865 (patched)
<https://reviews.apache.org/r/67452/#comment286804>

    Is the function only used for owner privilege? If so, can you make the comment and name clear?
    
    If not, then the processing is very different from current behavior in alterSentryRoleGrantPrivilegeCore and alterSentryUserGrantPrivilegeCore. It can easily cause confusion



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2995 (patched)
<https://reviews.apache.org/r/67452/#comment286808>

    You need to generate the updates for old owner privilege, and new owner privilege, so they can be saved and used for HDFS sync.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3002 (patched)
<https://reviews.apache.org/r/67452/#comment286806>

    you should call mPriv.removeUser() for its user, and removeRole for its role. It is possible the owner is a role.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3004 (patched)
<https://reviews.apache.org/r/67452/#comment286805>

    it is better to use function isUserStale
    
      private boolean isUserStale(MSentryUser user) {
        if (user.getPrivileges().isEmpty() && user.getRoles().isEmpty()) {
          return true;
        }
    
        return false;
      }
    
    If a user has role, it should not be removed



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3008 (patched)
<https://reviews.apache.org/r/67452/#comment286807>

    You don't need to remove this privilege and then create it in alterSentryGrantPrivilegeCore. You can attach owner to this privielge and then save it.


- Na Li


On June 5, 2018, 7:34 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 5, 2018, 7:34 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/3/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204552
-----------------------------------------------------------




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 796 (original), 799 (patched)
<https://reviews.apache.org/r/67452/#comment287114>

    You mean 'User does not exist', right? Might sounds like you haven't confirmed it does not exist, but you already did.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 809-815 (original), 814-820 (patched)
<https://reviews.apache.org/r/67452/#comment287116>

    Are we going to allow that a role gets the all privilege granted if the owner privilege is already set?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 827-835 (original), 832-840 (patched)
<https://reviews.apache.org/r/67452/#comment287115>

    Should we do the same thing for OWNER? Are we going to allow a role is granted insert/select while having the owner privilege?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 1942 (patched)
<https://reviews.apache.org/r/67452/#comment287117>

    New line here



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2048-2051 (patched)
<https://reviews.apache.org/r/67452/#comment287120>

    It needs better description of the method, parameters and throw exceptions in this comment.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2053 (patched)
<https://reviews.apache.org/r/67452/#comment287119>

    How is this class going to be used?
    
    listSentryPrivilegesByAuthorizable() is used to return all privileges from an authorizable. This may be used by the SHOW GRANT ON <OBJECT> command, right? What about this method?


- Sergio Pena


On June 11, 2018, 3:41 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 11, 2018, 3:41 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/7/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated June 11, 2018, 3:41 p.m.)

Review request for sentry, Na Li and Sergio Pena.

Changes
-------

Rebased the patch.

Bugs: SENTRY-2257
https://issues.apache.org/jira/browse/SENTRY-2257

Repository: sentry

Description
-------

Implement functionality in sentry store to update owner privilege on an authorizable.

Here is the approach.

There are two new API's that are exposed.

To list the owner privileges granted to an authorizable
1. update the owner privilege to new owner

Diffs (updated)
-----

sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80

Diff: https://reviews.apache.org/r/67452/diff/7/

Changes: https://reviews.apache.org/r/67452/diff/6-7/

Testing
-------

Added new tests to verify new functionality added.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated June 7, 2018, 8:51 p.m.)

Review request for sentry, Na Li and Sergio Pena.

Changes
-------

addressed comment from sergio.

Bugs: SENTRY-2257
https://issues.apache.org/jira/browse/SENTRY-2257

Repository: sentry

Description
-------

Implement functionality in sentry store to update owner privilege on an authorizable.

Here is the approach.

There are two new API's that are exposed.

To list the owner privileges granted to an authorizable
1. update the owner privilege to new owner

Diffs (updated)
-----

sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e6b71b5a4a97deafdf955bd70f0ead760e5fdb1a
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 12c6d916cf449499c1cceb6ddc50e68c88a6606e

Diff: https://reviews.apache.org/r/67452/diff/6/

Changes: https://reviews.apache.org/r/67452/diff/5-6/

Testing
-------

Added new tests to verify new functionality added.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204459
-----------------------------------------------------------




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2996 (patched)
<https://reviews.apache.org/r/67452/#comment286946>

    can you 
    1) add a public function like 
    
    GrantOwnerPrivilege(final TSentryAuthorizable tAuthorizable,
          String ownerName,  SentryEntityType entityType,
          final List<Update> updates)
          
    2) change updateOwnerPrivilege to protected function
    3) Inside this new function, calls listSentryOwnerPrivilegesByAuthorizable, and then updateOwnerPrivilege?
    
    In this way, caller can call one function to grant owner privilege when table is created or when owner is changed.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2999 (patched)
<https://reviews.apache.org/r/67452/#comment286947>

    How is the content of updates generated? This function does not generate its content.
    
    You can use similar approach to generate update "Update update = privilegesUpdateMap.get(privilege);"
    
      public void alterSentryUserGrantPrivileges(final String grantorPrincipal,
          final String userName, final Set<TSentryPrivilege> privileges,
          final Map<TSentryPrivilege, Update> privilegesUpdateMap) throws Exception {
    
        try {
          MSentryUser userEntry = getMSentryUserByName(userName, false);
          if (userEntry == null) {
            createSentryUser(userName);
          }
        } catch (SentryAlreadyExistsException e) {
          // the user may be created by other thread, so swallow the exception and proeed
        }
    
        Preconditions.checkNotNull(privilegesUpdateMap);
        for (TSentryPrivilege privilege : privileges) {
          Update update = privilegesUpdateMap.get(privilege);
          if (update != null) {
            alterSentryUserGrantPrivilege(grantorPrincipal, userName, privilege,
                update);
          } else {
            alterSentryUserGrantPrivilege(grantorPrincipal, userName, privilege);
          }
        }
      }


- Na Li


On June 7, 2018, 4:54 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 7, 2018, 4:54 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e6b71b5a4a97deafdf955bd70f0ead760e5fdb1a 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 12c6d916cf449499c1cceb6ddc50e68c88a6606e 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/5/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204461
-----------------------------------------------------------


Ship it!




Ship It!

- Na Li


On June 7, 2018, 4:54 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 7, 2018, 4:54 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e6b71b5a4a97deafdf955bd70f0ead760e5fdb1a 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 12c6d916cf449499c1cceb6ddc50e68c88a6606e 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/5/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated June 7, 2018, 4:54 p.m.)

Review request for sentry, Na Li and Sergio Pena.

Changes
-------

Addressed review comments.

Bugs: SENTRY-2257
https://issues.apache.org/jira/browse/SENTRY-2257

Repository: sentry

Description
-------

Implement functionality in sentry store to update owner privilege on an authorizable.

Here is the approach.

There are two new API's that are exposed.

To list the owner privileges granted to an authorizable
1. update the owner privilege to new owner

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/67452/diff/5/

Changes: https://reviews.apache.org/r/67452/diff/4-5/

Testing
-------

Added new tests to verify new functionality added.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On June 6, 2018, 7:42 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2002 (patched)
> > <https://reviews.apache.org/r/67452/diff/4/?file=2036072#file2036072line2002>
> >
> >     YOu want to grant owner privilege using alterSentryGrantPrivilegeCore, but your test is using alterSentryRoleGrantPrivilege and  alterSentryUserGrantPrivilege. Why is that?

alterSentryGrantPrivilegeCore is private sentry store method used by updateOwnerPrivilege. but the alterSentryRoleGrantPrivilege and  alterSentryUserGrantPrivilege are the actual publicn API's to grant/revoke oser privielges.

alterSentryGrantPrivilegeCore is indirectly tested as we testing updateOwnerPrivilege.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/#review204421
-----------------------------------------------------------


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On June 6, 2018, 7:42 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2002 (patched)
> > <https://reviews.apache.org/r/67452/diff/4/?file=2036072#file2036072line2002>
> >
> >     YOu want to grant owner privilege using alterSentryGrantPrivilegeCore, but your test is using alterSentryRoleGrantPrivilege and  alterSentryUserGrantPrivilege. Why is that?
> 
> kalyan kumar kalvagadda wrote:
>     alterSentryGrantPrivilegeCore is private sentry store method used by updateOwnerPrivilege. but the alterSentryRoleGrantPrivilege and  alterSentryUserGrantPrivilege are the actual publicn API's to grant/revoke oser privielges.
>     
>     alterSentryGrantPrivilegeCore is indirectly tested as we testing updateOwnerPrivilege.

In production code, are you going to add owner privilege for the first time by using alterSentryRoleGrantPrivilege or alterSentryUserGrantPrivilege, but when there is existing owner privilege, you are going to call updateOwnerPrivilege, which uses alterSentryGrantPrivilegeCore? 

This is not consistent. Regardless there is existing owner privilege, adding new owner privilege should call the same function. It is better to expose a public function for adding owner privilege. When there is existing owner privilege whose owner is not the same as new owner, remove old owner privilege. Then add new owner privilege using the same function. 

You need to test the following cases:
1) adding all privileges after adding owner privilege won’t removing owner privilege, and 
2) adding owner privilege after there is all privilege succeeds
3) revoking owner privilege does not cause existing all privilege to be decomposed.
4) revplomg all privilege does not cause existing owner privilege to be removed.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/#review204421
-----------------------------------------------------------


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204421
-----------------------------------------------------------




sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2002 (patched)
<https://reviews.apache.org/r/67452/#comment286921>

    YOu want to grant owner privilege using alterSentryGrantPrivilegeCore, but your test is using alterSentryRoleGrantPrivilege and  alterSentryUserGrantPrivilege. Why is that?


- Na Li


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On June 6, 2018, 7:26 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3004 (patched)
> > <https://reviews.apache.org/r/67452/diff/4/?file=2036071#file2036071line3004>
> >
> >     You can remove the privilege from user, and then call isUserStale(MSentryUser user). In this way, if we associate user with something else, the only place to change is isUserStale(). 
> >     
> >     In your current implemenation, it is very easy to have a bug when new feature is added related to user.

will fix. I will reabse the patch once you commit isUserStale() method.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/#review204415
-----------------------------------------------------------


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204415
-----------------------------------------------------------




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3004 (patched)
<https://reviews.apache.org/r/67452/#comment286915>

    You can remove the privilege from user, and then call isUserStale(MSentryUser user). In this way, if we associate user with something else, the only place to change is isUserStale(). 
    
    In your current implemenation, it is very easy to have a bug when new feature is added related to user.


- Na Li


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On June 6, 2018, 7:41 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 1978 (patched)
> > <https://reviews.apache.org/r/67452/diff/4/?file=2036072#file2036072line1978>
> >
> >     it seems you did not test listSentryOwnerPrivilegesByAuthorizable when old owner privilege has grant option. Can you add that test case?

ok. i will


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67452/#review204420
-----------------------------------------------------------


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

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/67452/#review204420
-----------------------------------------------------------




sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 1978 (patched)
<https://reviews.apache.org/r/67452/#comment286920>

    it seems you did not test listSentryOwnerPrivilegesByAuthorizable when old owner privilege has grant option. Can you add that test case?


- Na Li


On June 6, 2018, 4:01 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67452/
> -----------------------------------------------------------
> 
> (Updated June 6, 2018, 4:01 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2257
>     https://issues.apache.org/jira/browse/SENTRY-2257
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Implement functionality in sentry store to update owner privilege on an authorizable.
> 
> Here is the approach.
> 
> There are two new API's that are exposed. 
> 
> To list the owner privileges granted to an authorizable
> 1. update the owner privilege to new owner
> 
> Here is the Flow.
> 1. SentryPolicyStoreProcessor would first get the list of privileges that are to be revoked.
> 2. Using the list of privileges that are to be revoked, list of PermissionsUpdate is generated using SentryPlug-in
> 3. SentryPolicyStoreProcessor would then use the new API to update the owner privileges.
> 
> This way all the updated listed below happen in the same transaction
> 1. Revoking the exixting owner privilage for authorizable
> 2. Granting new owner privilege fot authorizable.
> 3. Adding delta update for owner privilege revoked
> 4. Adding delta update for owner privilege granted.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc 
>   sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java e2d24e53d0d3f8a738066cfedc85b11f6535f45c 
> 
> 
> Diff: https://reviews.apache.org/r/67452/diff/4/
> 
> 
> Testing
> -------
> 
> Added new tests to verify new functionality added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated June 6, 2018, 4:01 p.m.)

Review request for sentry, Na Li and Sergio Pena.

Changes
-------

addressed review comments.

Bugs: SENTRY-2257
https://issues.apache.org/jira/browse/SENTRY-2257

Repository: sentry

Description
-------

Implement functionality in sentry store to update owner privilege on an authorizable.

Here is the approach.

There are two new API's that are exposed.

To list the owner privileges granted to an authorizable
1. update the owner privilege to new owner

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/67452/diff/4/

Changes: https://reviews.apache.org/r/67452/diff/3-4/

Testing
-------

Added new tests to verify new functionality added.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated June 5, 2018, 7:34 p.m.)

Review request for sentry, Na Li and Sergio Pena.

Bugs: SENTRY-2257
https://issues.apache.org/jira/browse/SENTRY-2257

Repository: sentry

Description
-------

Implement functionality in sentry store to update owner privilege on an authorizable.

Here is the approach.

There are two new API's that are exposed.

To list the owner privileges granted to an authorizable
1. update the owner privilege to new owner

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/67452/diff/3/

Changes: https://reviews.apache.org/r/67452/diff/2-3/

Testing
-------

Added new tests to verify new functionality added.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 67452: SENTRY-2257: Implement Sentry store API to update owner privilege on a authorizable

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated June 5, 2018, 6:11 p.m.)

Review request for sentry, Na Li and Sergio Pena.

Bugs: SENTRY-2257
https://issues.apache.org/jira/browse/SENTRY-2257

Repository: sentry

Description
-------

Implement functionality in sentry store to update owner privilege on an authorizable.

Here is the approach.

There are two new API's that are exposed.

To list the owner privileges granted to an authorizable
1. update the owner privilege to new owner

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/67452/diff/2/

Changes: https://reviews.apache.org/r/67452/diff/1-2/

Testing
-------

Added new tests to verify new functionality added.

Thanks,

kalyan kumar kalvagadda