> 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 > >
