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