----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22240/#review45239 -----------------------------------------------------------
Mostly looks good, but after I reviewed the entire patch(after I put comments below), I realized there probably is a way to make it a little bit less complicated :-) As we are anyways using this "some" for only this special case of "connect" privilege, and already returning "server=+" if we do not find any other privileges. Why not just have a separate case like if(policyPart.getValue().equals(AccessConstants.SOME)) return true; in impliesKeyValue() that way we do not make any changes to the existing code paths? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java <https://reviews.apache.org/r/22240/#comment80083> This will always be true if you are in this case of switch, so might as well remove it. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java <https://reviews.apache.org/r/22240/#comment80084> Can you add a comment here that, this code is only reachable when using provider db backend for the hive operation "use default" and only when the user doesnt have any privileges on default? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java <https://reviews.apache.org/r/22240/#comment79973> Use dbName2 should pass, with given privileges for user1_1. Can you please change the test case accordingly? - Sravya Tirukkovalur On June 10, 2014, 2:15 a.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22240/ > ----------------------------------------------------------- > > (Updated June 10, 2014, 2:15 a.m.) > > > Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > Couple of Test case fixes to get the filter push down patch to work. > > NOTE : I have placed some TODOs where I make a few assumptions.. kindly review > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 812f310 > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java > 9f5035e > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Table.java > 62a0a81 > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBModelAuthorizables.java > f4b32e1 > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBWildcardPrivilege.java > cab1234 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > 3a993b0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java > 326b91d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 5560729 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > a1cf24a > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java > 2198c05 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSandboxOps.java > 5eef792 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegeAtTransform.java > 732632b > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtDatabaseScope.java > 1e93ec6 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSandboxOps.java > 10c7b82 > > Diff: https://reviews.apache.org/r/22240/diff/ > > > Testing > ------- > > > Thanks, > > Arun Suresh > >
