> 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?
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. > 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. 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. > On Dec. 19, 2017, 9:26 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > > Lines 752 (patched) > > <https://reviews.apache.org/r/64317/diff/2/?file=1908510#file1908510line752> > > > > Whay are you using NoSuchObject here when you are using AccessDenied in > > rest of places. > > > > Any specific reason? > > > > I think it should be same as else where. > > Zachary Amsden wrote: > This API is requesting the groups of another user. AccessDenied makes > sense when the *requesting* user has no privilege that can be established by > group membership, but AccessDenied when the *requested* user has no group > membership doesn't. The group membership set should simply be empty IMO, but > I took the more conservative approach of keeping the same behavior of > throwing and tried to return a more appropriate exception. I'm happy to go > any direction on this though, just wanted to explain my reasoning. Both the them are users be it requestor OR UserName and i think the exception throw should be the same. - kalyan kumar ----------------------------------------------------------- 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 > >
