You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by Arun Suresh <ar...@gmail.com> on 2014/06/04 19:36:23 UTC

Review Request 22240: SENTRY-247 : Go back to using filter push down once the bugs are fixe

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22240/
-----------------------------------------------------------

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-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBModelAuthorizables.java f4b32e1 
  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 ff656aa 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java 9c0c8b5 
  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/TestSandboxOps.java 10c7b82 

Diff: https://reviews.apache.org/r/22240/diff/


Testing
-------


Thanks,

Arun Suresh

Re: Review Request 22240: SENTRY-247 : Go back to using filter push down once the bugs are fixe

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22240/#review45171
-----------------------------------------------------------


We should only allow "use default" for any user as long as the user has some privileges on the server. But we should still restrict the data objects in the default data base. For example: This test case should fail (but passes with the patch):
@Test
  public void defaultNegativeTest() throws Exception{
    policyFile
        .addRolesToGroup(USERGROUP1, "all_db1", "load_data")
        .addPermissionsToRole("all_db1", "server=server1->db=DB_1")
        .setUserGroupMapping(StaticUserGroup.getStaticMapping());
    writePolicyFile(policyFile);

    Connection connection = context.createConnection(ADMIN1);
    Statement statement = context.createStatement(connection);
    statement.execute("create table tab1(a int)");

    // setup db objects needed by the test
    connection = context.createConnection(USER1_1);
    statement = context.createStatement(connection);
    statement.execute("select * from tab1");
  }

and this should pass:
@Test
  public void defaultTest() throws Exception{

    policyFile
        .addRolesToGroup(USERGROUP1, "all_db1", "load_data")
        .addPermissionsToRole("all_db1", "server=server1->db=DB_1")
        .setUserGroupMapping(StaticUserGroup.getStaticMapping());
    writePolicyFile(policyFile);

    // setup db objects needed by the test
    Connection connection = context.createConnection(USER1_1);
    Statement statement = context.createStatement(connection);
    statement.execute("use default");

  }


sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSandboxOps.java
<https://reviews.apache.org/r/22240/#comment79879>

    This is fine, or you could also just ignore this test with @Ignore, so that it would be skipped.


- Sravya Tirukkovalur


On June 6, 2014, 11:52 p.m., Arun Suresh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22240/
> -----------------------------------------------------------
> 
> (Updated June 6, 2014, 11:52 p.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-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBModelAuthorizables.java f4b32e1 
>   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 a959b13 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 322e90e 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java 9c0c8b5 
>   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/TestSandboxOps.java 10c7b82 
> 
> Diff: https://reviews.apache.org/r/22240/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arun Suresh
> 
>

Re: Review Request 22240: SENTRY-247 : Go back to using filter push down once the bugs are fixe

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22240/#review45183
-----------------------------------------------------------


use default fails for a user with "server=server1->db=DB_2->table=tab_2->action=select" permissions. There is also a test case in testDefaultDbPrivilege() which fails.You may want to have "SOME/ANY" privilege which is only used for switch database instead.

- Sravya Tirukkovalur


On June 10, 2014, 12:38 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, 12:38 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-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
> 
>

Re: Review Request 22240: SENTRY-247 : Go back to using filter push down once the bugs are fixe

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.


> On June 10, 2014, 10:46 p.m., Sravya Tirukkovalur wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java, lines 461-463
> > <https://reviews.apache.org/r/22240/diff/7/?file=606349#file606349line461>
> >
> >     This can just be: 
> >     
> >     Table currTbl = Table.ALL;
> >           if ((DEFAULT_DATABASE_NAME.equalsIgnoreCase(currDB.getName()) &&
> >               "false".equalsIgnoreCase(authzConf.
> >                   get(HiveAuthzConf.AuthzConfVars.AUTHZ_RESTRICT_DEFAULT_DB.getVar(), "false")))
> >                   ) {
> >             currDB = Database.ALL;
> >             currTbl = Table.SOME;
> >           }

this doesnt look like has been resolved?


- Sravya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22240/#review45306
-----------------------------------------------------------


On June 10, 2014, 10:52 p.m., Arun Suresh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22240/
> -----------------------------------------------------------
> 
> (Updated June 10, 2014, 10:52 p.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 
> 
> Diff: https://reviews.apache.org/r/22240/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arun Suresh
> 
>

Re: Review Request 22240: SENTRY-247 : Go back to using filter push down once the bugs are fixe

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22240/#review45306
-----------------------------------------------------------


Ok, the simpler case is too open :-). So lets go ahead with the current approach.


sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
<https://reviews.apache.org/r/22240/#comment80100>

    This can just be: 
    
    Table currTbl = Table.ALL;
          if ((DEFAULT_DATABASE_NAME.equalsIgnoreCase(currDB.getName()) &&
              "false".equalsIgnoreCase(authzConf.
                  get(HiveAuthzConf.AuthzConfVars.AUTHZ_RESTRICT_DEFAULT_DB.getVar(), "false")))
                  ) {
            currDB = Database.ALL;
            currTbl = Table.SOME;
          }


- 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
> 
>

Re: Review Request 22240: SENTRY-247 : Go back to using filter push down once the bugs are fixe

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.

-----------------------------------------------------------
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
> 
>