> On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > 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!
Thanks Sravya! Your comments are very valuable! I will fix them and update the patch soon later. > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1403 > > <https://reviews.apache.org/r/23794/diff/1/?file=638847#file638847line1403> > > > > when will p.getGrantOption() > mPri.getGrantOption() be true? In my previous design, grantOption type is int. So I support grantOption with priority. High priority can grant/revoke low priority. Accoriding to Arun and yours' comment, I have change grantOption to Boolean in my local code. So it will not support priority in this version. > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1380 > > <https://reviews.apache.org/r/23794/diff/1/?file=638847#file638847line1380> > > > > 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 You are right, I will change it. > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1382 > > <https://reviews.apache.org/r/23794/diff/1/?file=638847#file638847line1382> > > > > 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. agreed. > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1379 > > <https://reviews.apache.org/r/23794/diff/1/?file=638847#file638847line1379> > > > > Requesting user name is stored as grantorPrincipal and user names are > > case sensitive, so they should not be lowercased. agreed. > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, > > line 278 > > <https://reviews.apache.org/r/23794/diff/1/?file=638848#file638848line278> > > > > Again, grantor != roleName, but grantor = userName > > agreed. > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java, > > line 192 > > <https://reviews.apache.org/r/23794/diff/1/?file=638852#file638852line192> > > > > It would be better to selectively call this new implementation, I see > > that there are some tests where this setup is not required. agreed > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, > > lines 68-69 > > <https://reviews.apache.org/r/23794/diff/1/?file=638848#file638848line68> > > > > why are these same "g1"? Grantor is usually a user name not roleName agreed > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, > > line 224 > > <https://reviews.apache.org/r/23794/diff/1/?file=638846#file638846line224> > > > > I am trying to understand when will this be true? I will change it to avoid to using privilegeName. > On July 24, 2014, 1:15 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1396 > > <https://reviews.apache.org/r/23794/diff/1/?file=638847#file638847line1396> > > > > - grantOptionCheck does not seem to be throwing an Exception if it > > privilegeSet == null? > > - can privilegeSet be an empty set? 1. if privilegeSet==null, hasGrant will be false, then throw exception. 2. I will add check privilegeSet.isEmpty - Xiaomeng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23794/#review48561 ----------------------------------------------------------- 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 > >
