> 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? > > > > Arun Suresh wrote: > 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..
Makes sense except for the first point: 1) We still have to do the same recursive thingy during revoke.. even if we do it during grant time -- I dont see why we need to still recursively revoke, if we never store children. But I am ok to do this as a follow on as we will also touch the common code with policy files in DbWildcardPermission, and will need more testing. > 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. > > Arun Suresh wrote: > Can't catch SemanticException. Only SQLException.. You may be want to assert like this to make sure there was not an unexpected SQLException: https://github.com/apache/incubator-sentry/blob/master/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java#L186 - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22494/#review45819 ----------------------------------------------------------- On June 17, 2014, 4:50 p.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22494/ > ----------------------------------------------------------- > > (Updated June 17, 2014, 4:50 p.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/TestDatabaseProvider.java > 6187692 > > 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 > >
