> On June 16, 2014, 11:58 p.m., Sravya Tirukkovalur wrote: > > Thanks for the patch Arun! > > > > I think there is a benefit in handling it the other way round, where we > > only store the parent privileges, and have this complex logic at granting > > time rather than revoking time. So, > > - grant all on database db1 to role role1 > > - grant all on server server1 to role role1 > > should result in only one privilege, which is all on server. So even the > > show grants is cleaner, we will end up storing less data in the backend, > > and revoke would be straight forward. > > > > In either case, I think we should add more test cases, as we are increasing > > the semantics of these auth DDL statements. Feel free to add these tests to > > TestDatabaseProvider where other auth DDL tests live. > > > > Revoke all on server after: > > - grant all on db > > - grant all on table > > - grant select on table > > - grant insert on table > > - grant all on uri > > - grant select on uri > > - grant insert on uri > > > > Revoke all on database after: > > - grant all on db > > - grant all on table > > - grant select on table > > - grant insert on table > > > > Revoke all on table after: > > - grant all on table > > - grant select on table > > - grant insert on table > > > > Revoke select on table after: > > - grant all on table > > - grant select on table > > - grant insert on table > > > > Revoke insert on table after: > > - grant all on table > > - grant select on table > > - grant insert on table > > > > > > Curious about what happens to the granting time, when we update the > > privileges? > >
Thank you for the review Sravya !! I've add all the test cases you recommended.. I totally agree with you on performing the logic during GRANT.. But I am hesitant to do it rite now for the following reasons : 1) We still have to do the same recursive thingy during revoke.. even if we do it during grant time 2) We don't gain too much in terms of storage space rite now.. since the Privilege row still exits.. only the Priv->Role mapping is removed. We should re-look at this after that is done (I think there is already a JIRA for that) 3) Our first (and only major performance issue) was the GRANTs times were increasing very fast with num privileges. Even though it is not that bad now, it is still slightly linear. In light of that, I do not want to add any more complexity to GRANT rite now considering the fact that usually far more GRANTS are made than REVOKES 4) From an end-point user, it will not add any perceptible change in the way he/she grants/revokes, since like I mentioned, when a user requests Privileges for an Object all existing Privileges for its hierarchy are already sent and will be handled correctly at the DbWildcardPermission's implies method. That said, I think we should raise a follow-up ticket for that.. > On June 16, 2014, 11:58 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 289 > > <https://reviews.apache.org/r/22494/diff/4/?file=609398#file609398line289> > > > > Dont we have to remove it both ways? Nope... apparantly only one-way is enough.. I verified on the Db (also if this wern't true we would have a lot of our exisiting tests failing :)) > On June 16, 2014, 11:58 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 378 > > <https://reviews.apache.org/r/22494/diff/4/?file=609398#file609398line378> > > > > remove the privilege as well from role? See comment above.. > On June 16, 2014, 11:58 p.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java, > > line 185 > > <https://reviews.apache.org/r/22494/diff/4/?file=609399#file609399line185> > > > > revoke of insert on db should not revoke select on child table, isn't > > it? I will remove this test-case here... in any case, I had set this for @Ignore.. It was meant for SENTRY-303.. will fix and add to that patch > On June 16, 2014, 11:58 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > lines 380-401 > > <https://reviews.apache.org/r/22494/diff/4/?file=609398#file609398line380> > > > > Can increase code reuse here in these two if else blocks, logic seems > > to be same except for the action. Will do.. thnx > On June 16, 2014, 11:58 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 387 > > <https://reviews.apache.org/r/22494/diff/4/?file=609398#file609398line387> > > > > append this privilege outside this if block? Actually.. even though JDO forums talk about appending to both role and privilege in an m:n join... I've noticed that we need to do that only for new privileges > On June 16, 2014, 11:58 p.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java, > > line 164 > > <https://reviews.apache.org/r/22494/diff/4/?file=609399#file609399line164> > > > > you may want to assert semanticexception here and else where. Can't catch SemanticException. Only SQLException.. - Arun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22494/#review45819 ----------------------------------------------------------- On June 17, 2014, 5:56 a.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22494/ > ----------------------------------------------------------- > > (Updated June 17, 2014, 5:56 a.m.) > > > Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Bugs: SENTRY-281 > https://issues.apache.org/jira/browse/SENTRY-281 > > > Repository: sentry > > > Description > ------- > > Fix for SENTRY-281 : > > A revoke on a parent privilege should trickle to the child. > For eg : > > 1) GRANT SELECT on DATABASE db1 to ROLE role1; > 2) REVOKE ALL on SERVER server1 from ROLE role1; > > Should result in zero privileges for role1 > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 91669d6 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java > 46d9332 > > Diff: https://reviews.apache.org/r/22494/diff/ > > > Testing > ------- > > Tested on sentry-hive-tests > > > Thanks, > > Arun Suresh > >
