----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22494/#review45819 -----------------------------------------------------------
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? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/22494/#comment80828> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/22494/#comment80809> Dont we have to remove it both ways? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/22494/#comment80855> remove the privilege as well from role? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/22494/#comment80856> Can increase code reuse here in these two if else blocks, logic seems to be same except for the action. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/22494/#comment80854> append this privilege outside this if block? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/22494/#comment80858> Nit: Can you fix the indentation please? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/22494/#comment80857> Nit: Can you fix the indentation please? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80867> you may want to assert semanticexception here and else where. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80869> revoke of insert on db should not revoke select on child table, isn't it? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80860> why ignore? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80863> // test default of ALL ? Comment does not make sense? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80864> Comment does not make sense? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80879> Nit: Can we remove this comment, as this patch fixes it? Same for the below test case. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80872> this is a requirement for the test setup, so a problem here should not be ignored. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80873> same as above sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80875> wrong comment? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80882> Nit: (user_role & user_role2)? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22494/#comment80886> Nit: user_role? - Sravya Tirukkovalur On June 13, 2014, 4:29 p.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22494/ > ----------------------------------------------------------- > > (Updated June 13, 2014, 4:29 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/TestDbEndToEnd.java > 46d9332 > > Diff: https://reviews.apache.org/r/22494/diff/ > > > Testing > ------- > > Tested on sentry-hive-tests > > > Thanks, > > Arun Suresh > >
