> On Oct. 6, 2014, 6:36 a.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java, > > line 53 > > <https://reviews.apache.org/r/25879/diff/1/?file=699160#file699160line53> > > > > Finish comment and fix grammar. Could you also add one or two more > > sentances on what this class does? For example: > > "Callers will only receive objects back which they have privileges to > > access and all object lists (ex getAllTables()) will be filtered to exclude > > items the requetor does not have privileges to access."
Update the comments. > On Oct. 6, 2014, 6:36 a.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java, > > line 56 > > <https://reviews.apache.org/r/25879/diff/1/?file=699160#file699160line56> > > > > since this is a subclass of ObjectStore consider a name such as > > "FilteringObjectStore" or "AuthorizingObjectStore". To me, MetastoreFilter > > sounds like it might be a standalone class used to filter results returned > > by the metastore. Update the class name. > On Oct. 6, 2014, 6:36 a.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java, > > line 102 > > <https://reviews.apache.org/r/25879/diff/1/?file=699160#file699160line102> > > > > Since you repeat this message in a bunch of places, consider making it > > a constant. Also consider re-phrasing to something like: > > "Table does not exist or insufficient privileges to access: > > <dbName>.<tableName>" > > > > similar comment for the db error messages. Update the error message and make part of massage as constant. > On Oct. 6, 2014, 6:36 a.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java, > > line 109 > > <https://reviews.apache.org/r/25879/diff/1/?file=699160#file699160line109> > > > > Should this be implemented? If not, remove commented out code. The code is removed. > On Oct. 6, 2014, 6:36 a.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java, > > line 343 > > <https://reviews.apache.org/r/25879/diff/1/?file=699160#file699160line343> > > > > Instead of doing this initialization for every request or just cache > > the value and use that for future requests? Make the hiveAuthzBinding, hiveConf and serviceUsers as static variable and will be initialized only once. > On Oct. 6, 2014, 6:36 a.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java, > > line 399 > > <https://reviews.apache.org/r/25879/diff/1/?file=699160#file699160line399> > > > > Should this always return true? Update this method, the configuration "sentry.metastore.service.users" is imported to define the user who has all access to the metadata. The new test case is added for this configuration. > On Oct. 6, 2014, 6:36 a.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreFilter.java, > > line 123 > > <https://reviews.apache.org/r/25879/diff/1/?file=699162#file699162line123> > > > > It might be easier to debug test failures if you split this in to 3 > > separate test cases. something like: > > > > testFullAccess(); > > testPartialAccess(); > > testNoAccess(); Split these test cases and add new one to test the new configuration "sentry.metastore.service.users". @Lenni, thanks for your great review. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25879/#review55484 ----------------------------------------------------------- On Oct. 8, 2014, 8 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25879/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2014, 8 a.m.) > > > Review request for sentry, Lenni Kuff, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > The Metastore plugin currently enforces Sentry policies for metadata > modifications. This makes it inconsistent with Hive plugin that support > privileges for both metadata read and write. > We should support the policy enforcement for metadata read as well. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java > 4c66ffe > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/25879/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
