----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62654/#review186684 -----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 88 (patched) <https://reviews.apache.org/r/62654/#comment263494> Why is this public? I don't see it is used anywhere. sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 188-195 (patched) <https://reviews.apache.org/r/62654/#comment263476> Hive should handle this issue if a user attempts to drop a non-existent database or table. If it is gotten this far, then it means Hive found the DB. How can this happen? We should avoid skipping any authorization requests for unknown data. If Hive has a bug in the future where it forgets to send the input/output data, then Sentry will allow the DROP operation? I think we should throw an exception here. But, we should investigate how this scenario can be possible. sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Line 243 (original), 255 (patched) <https://reviews.apache.org/r/62654/#comment263483> Avoid short names like 'Curr'. We could rename this method to getDatabaseName() (same to getTableName() and getUdfClassName()) sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 268 (patched) <https://reviews.apache.org/r/62654/#comment263489> What's the difference between input and output hierarchy on Hive databases? Can't they be different in some scenarios? What if input has a 'default' and output has a 'db1' ? Which one is the one we want to authorize? Same questions for getCurrTableName() and getCurrUdfClassName(). sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 279 (patched) <https://reviews.apache.org/r/62654/#comment263486> Error/warning/info messages should be clear to the user about something that is happening on the system and what they can do for it. This message is not clear to the user that Hive did not send enough information to Sentry. Also, I don't think we should log a WARN here. Better to throw an exception from the caller that some info could not be obtained? Same for getCurrTableName and getCurrUdfClassName. sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 315 (patched) <https://reviews.apache.org/r/62654/#comment263492> Is this correct? Do we get the URI from the hierarchy list? I didn't see this info when I was testing hive. If we not get this info, then we should not add this fix yet because we are leaving a security flaw when authorizing UDF (or perhaps an error). sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 339-340 (patched) <https://reviews.apache.org/r/62654/#comment263478> I don't think we should put in the comment that the caller should skip the processing. It's up to the caller what to do with it later. Let's keep what the function does only. sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 341-346 (patched) <https://reviews.apache.org/r/62654/#comment263482> What about the comments for all the parameters and exceptions? sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java Lines 348 (patched) <https://reviews.apache.org/r/62654/#comment263480> Same comment here. We just need to comment about why it returns false but not say what the caller should do. sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java Line 164 (original), 164 (patched) <https://reviews.apache.org/r/62654/#comment263493> Can we remove this and the other comment on this file? This is not related to the patch. - Sergio Pena On Sept. 28, 2017, 4:59 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62654/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2017, 4:59 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Sergio Pena. > > > Repository: sentry > > > Description > ------- > > Sentry extracts info for authorization based on input from hive instead of > parsing the sql command again > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java > 8b56c49 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java > 6c2410b > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/SentryHiveAuthorizationValidator.java > 7bf7b87 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java > 35bd68c > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SimpleSemanticAnalyzer.java > 85afe52 > > sentry-binding/sentry-binding-hive-v2/src/test/java/org/apache/sentry/binding/hive/util/TestSentryAuthorizerUtil.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/62654/diff/2/ > > > Testing > ------- > > e2e tests and system test > > > Thanks, > > Na Li > >
