> 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.
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? > 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. 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. > 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. 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. - 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 > >
