----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37058/#review93999 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 494) <https://reviews.apache.org/r/37058/#comment148491> nit: space after "," here and on line 487 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 89) <https://reviews.apache.org/r/37058/#comment148492> can you just delete this test while you are here? Doesn't look like it is useful. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 325) <https://reviews.apache.org/r/37058/#comment148493> extra ; sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 400) <https://reviews.apache.org/r/37058/#comment148494> delete sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 502) <https://reviews.apache.org/r/37058/#comment148495> delete sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 928) <https://reviews.apache.org/r/37058/#comment148496> You might want to check your IDE settings. Looks like lots of formatting fixes which add some noise to the review (not to discourage you from fixing up the code blocks you are actually working on). sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 2085) <https://reviews.apache.org/r/37058/#comment148497> Use javadoc style comments: /** * SENTY-827 */ sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 2115) <https://reviews.apache.org/r/37058/#comment148498> Is it interesting to test multiple DBs/TBLs? Also, this should be: for (String db: dbs) <- note the plural on the array name sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java (line 2145) <https://reviews.apache.org/r/37058/#comment148499> verify that a SELECT on the table fails? - Lenni Kuff On Aug. 4, 2015, 12:26 a.m., Ryan Pridgeon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37058/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2015, 12:26 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > SENTRY-827: Allow Select,Insert and ALL on Server scope priviliges. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 2a60a23 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 9c2d384 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 09b3d99 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java > f9e8f80 > > Diff: https://reviews.apache.org/r/37058/diff/ > > > Testing > ------- > > Grant ALL , SELECT and INSERT to three different roles, mapped to three > different roles: > > ADMINGROUP:server_all > USERGROUP1:server_select > USERGROUP2:server_insert > > I then checked each level to ensure that they did not reflect that of ALL: > > server_select: Pass SELECT * ; Fail LOAD DATA IN PATH > server_insert: Fail SELECT * ; Pass LOAD DATA IN PATH > server_all: Pass SELECT * ; Pass LOAD DATA IN PATH > > *****admiditly someone had already remedied this. You could still only revoke > ALL form the server scope however*** > > Lastly I ensured that ADMINGROUP could revoke the individual privilige from > the server scope. > > > Thanks, > > Ryan Pridgeon > >
