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