----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23794/#review48561 -----------------------------------------------------------
Thanks for patch Xiaomeng! I guess the main concern I have is: - Looks like you are assuming that the grantorprincipal is the roleName. But in fact it would be the userName. So you will have to get all the roles which that user is part of and then perform the check. - We should also add relevant e2e tests to make sure we do not regress. Might be easier to do this for you, if you include all of the changes in one patch(TestDatabaseProvider/TestDbEndToEnd) Thanks! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/23794/#comment85250> I am trying to understand when will this be true? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/23794/#comment85256> Requesting user name is stored as grantorPrincipal and user names are case sensitive, so they should not be lowercased. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/23794/#comment85258> At least as of now, we are storing user name as the grantorPrincipal and not the role name, for auditing purposes it would be important to know which exact user performed an operation and not just the roleName: https://github.com/apache/incubator-sentry/blob/master/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java#L346 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/23794/#comment85259> if the granting user does not have any roles assigned, then we can assume the grant check failed because of insufficient permissions. Throwing a no such object exception would be confusing. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/23794/#comment85260> - grantOptionCheck does not seem to be throwing an Exception if it privilegeSet == null? - can privilegeSet be an empty set? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/23794/#comment85267> when will p.getGrantOption() > mPri.getGrantOption() be true? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/23794/#comment85270> why are these same "g1"? Grantor is usually a user name not roleName sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/23794/#comment85292> Again, grantor != roleName, but grantor = userName sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java <https://reviews.apache.org/r/23794/#comment85298> It would be better to selectively call this new implementation, I see that there are some tests where this setup is not required. - Sravya Tirukkovalur On July 22, 2014, 7:16 a.m., Xiaomeng Huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23794/ > ----------------------------------------------------------- > > (Updated July 22, 2014, 7:16 a.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > A role has a privilege with grant option, he can grant/revoke the privilege > or child privilege to/from other roles. > A role in adminGroups, he can grant privilege to any roles. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > f8491db > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > ff8acdc > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 7637376 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java > 79579c6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java > b97db4b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > a4ae291 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > 838e8d3 > > Diff: https://reviews.apache.org/r/23794/diff/ > > > Testing > ------- > > The unit test is included in the patch. > > > Thanks, > > Xiaomeng Huang > >
