> On Dec. 19, 2017, 9:26 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
> > Line 837 (original), 837 (patched)
> > <https://reviews.apache.org/r/64317/diff/2/?file=1908499#file1908499line838>
> >
> > This is not the appropriate error that should be logged.
> >
> > SemanticException is not not right exception, please throw appropriate
> > exception and handle it.
>
> Zachary Amsden wrote:
> I'm confused by review board - I don't see the line you are mentioning
> being changed, nor do I remember writing any code related to
> SemanticException - what line in particular is this about?
>
> kalyan kumar kalvagadda wrote:
> Moving the try and bringing below logic into a try block
>
> Set<String> userPrivileges = authProvider.getPolicyEngine().getPrivileges(
> authProvider.getGroupMapping().getGroups(userName),
> Sets.newHashSet(userName),
> hiveAuthzBinding.getActiveRoleSet(), hiveAuthzBinding.getAuthServer());
>
>
>
> by doing that, this method would throw SemanticException when getGroups
> in above code throws SentryGroupNotFoundException.
I think you misread, instead of re-throwing the exception, I swallow it and do
the user privilege lookup with an empty group set (which I believe is the right
behavior if you want user based authorization).
```java
try {
Set<String> groups;
try {
groups = authProvider.getGroupMapping().getGroups(userName);
} catch (SentryGroupNotFoundException e) {
groups = Collections.emptySet();
LOG.debug("Could not find groups for user: " + userName);
}
Set<String> userPrivileges =
authProvider.getPolicyEngine().getPrivileges(groups,
Sets.newHashSet(userName),
hiveAuthzBinding.getActiveRoleSet(),
hiveAuthzBinding.getAuthServer());
```
> On Dec. 19, 2017, 9:26 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
> > Lines 99 (patched)
> > <https://reviews.apache.org/r/64317/diff/2/?file=1908508#file1908508line99>
> >
> > You can not silently consume the exception here. It should be thrown in
> > the caller where it can create appripriate error message.
> >
> > If you comsume this exception, AuthorizationException will be throw
> > with message saying user doesn not have privileges to perfrom a operation.
> >
> > Which is not the case.
>
> Zachary Amsden wrote:
> This should not be throwing at all - hasAccess is a public API. If the
> user has unknown group affiliation, based on the group permissions, there
> should be no access. Exactly why can be probed with other calls (getGroups).
>
> Re-throwing the exception from here virally introduces it to all of the
> upper layers and rapidly balloons into exactly the nightmare this patch is
> trying to avoid.
>
> kalyan kumar kalvagadda wrote:
> I just looked into Impala code. I see what you are saying.
>
> My assumption when I gave above comment was that method hasAccess is
> called in sentry bindings and was suggesting that sentry bindings should
> handle the SentryGroupNotFoundException.
>
> I now see that Impala code calls method hasAccess directly.
>
>
> but my consern still is valid, we whould be throwing Authorization
> exception with out proper reason.
>
> kalyan kumar kalvagadda wrote:
> At least add an error that access is desined becasue group is not found
> for the user. That will help in troubleshooting.
Done. I think I forgot to update the diff because for some reason my local
version has this logging whereas review board does not.
- Zachary
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64317/#review194177
-----------------------------------------------------------
On Dec. 5, 2017, 12:55 a.m., Zachary Amsden wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64317/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2017, 12:55 a.m.)
>
>
> Review request for sentry and Na Li.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Instead of leaking new exceptions outside the API, use the
> existing authorization exceptions to indicate authorization
> failure when a user has no group configured.
>
>
> Diffs
> -----
>
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
> 8ce7a02ed4c565e34229a5c80c1b4fd1a84bad19
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
> 9c60c22aac826affd05cdf28b3816c68c139326d
>
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java
> a41d1bd533157c96430c3bf3569e1612db77c7b2
>
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrPluginImpl.java
> 91d08f0bc7f344c87e5bfb1e11b4b68728e676be
>
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
> 803e5eabf322cd120456a78c57f127ed4c94f5fc
>
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java
> f060b82da44f642e9a1dbff86e6e834fbc09cb2b
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryGroupNotFoundException.java
> b978df69df1d777311146406278444ae4e7f83ee
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java
> 2d82bcfcd5343d1b130df2f723d33a106d36ea81
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/GroupMappingService.java
> 7e85261070f133a6886434732d23d5a72894f8ef
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupMappingService.java
> bde53d5f640c98f41dea54d54dfe708ffee5dcd3
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
> 005724f3e3f8c623c2a266f60825cf77ac1ea777
>
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestNoAuthorizationProvider.java
> fe01b062c592e17ffa336552986e83f3f5f294e3
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
> 650880bb682d76c000fa51b497fae484c257b342
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java
> 6597a7ca724d1377ad07d8bc18530eb89b659693
>
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupMappingService.java
> 54474203aed4868c3bde8450d4d27427fa1de7f6
>
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestLocalGroupMapping.java
> 9864b82bfd9c499ab2b1f8ba9d4664fe19899d4e
>
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java
> 2338ab8375a6381e8d5fc8b38f766789187f69af
>
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java
> 02ac51454a13c0c1c61bb8684872e4815bd88b97
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java
> 02ac51454a13c0c1c61bb8684872e4815bd88b97
>
>
> Diff: https://reviews.apache.org/r/64317/diff/2/
>
>
> Testing
> -------
>
> Running JUnit tests with mvn install.
>
>
> Thanks,
>
> Zachary Amsden
>
>
