----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63250/#review190068 -----------------------------------------------------------
There are couple of missing pieces 1. AuditLogger does not reacord all the the authorization requests 2. All the tests using filr based provider as removed. 3. Kerberos support for File based provider. You need not address them in this patch but please open jira's for them and link them to this jira for reference. sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrPluginImpl.java Lines 188-191 (patched) <https://reviews.apache.org/r/63250/#comment267321> class PermissionNameProvider, dpesn't have proper documentation satating what each permission means. It would be great if you could document it so that it will be easy for the sentry understand/maintain/use solr plug-in sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java Line 62 (original), 58 (patched) <https://reviews.apache.org/r/63250/#comment267322> please document this class. sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSolrPolicyEngineLocalFS.java Lines 27 (patched) <https://reviews.apache.org/r/63250/#comment267324> There are no tests use this class. we discussed about it. Please raise a jira for this and link it to this jira. sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrModelAuthorizable.java Lines 21 (patched) <https://reviews.apache.org/r/63250/#comment267326> why can't you change SolrModelAuthorizable into a abstract class and move common code in all the child classes like Admin,Collection,Config,Field,Schema into this class. When you do that your child class looks something like this. public class Field extends SolrModelAuthorizable { /** * Represents all fields */ public static final Field ALL = new Field(SolrConstants.ALL); public Field(String name) { super(name); } @Override public String toString() { return "Field [name=" + name + "]"; } @Override public String getTypeName() { return AuthorizableType.Field.name(); } } Currently there is duplicate code in all the Admin,Collection,Config,Field and Schema classes. sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrModelAuthorizables.java Lines 42-55 (patched) <https://reviews.apache.org/r/63250/#comment267327> This need not be a seperate method. As this private and is used by only one method. There call be one from method which could hold the complete implementation. sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrPrivilegeModel.java Lines 29 (patched) <https://reviews.apache.org/r/63250/#comment267329> Please add some documentation for this class. sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/validator/SolrPrivilegeValidator.java Lines 27 (patched) <https://reviews.apache.org/r/63250/#comment267328> Please add some documentaion for this class. sentry-provider/sentry-provider-db/pom.xml Lines 241-245 (patched) <https://reviews.apache.org/r/63250/#comment267331> please re-phrase this comment, Explaining the reason for excluding this jar. something like, If not excluded, wrong version of PersistenceManager is used from jdo-api. - kalyan kumar kalvagadda On Nov. 3, 2017, 9:49 p.m., Hrishikesh Gadre wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63250/ > ----------------------------------------------------------- > > (Updated Nov. 3, 2017, 9:49 p.m.) > > > Review request for sentry and Sergio Pena. > > > Bugs: SENTRY-1475 > https://issues.apache.org/jira/browse/SENTRY-1475 > > > Repository: sentry > > > Description > ------- > > - Upgraded Solr to latest version (v7.1.0) > - Introduced following new authorizable entity types, > - Admin (to authorize various cluster admin operations) > - Note that privilege configuration for Solr collections admin > operations have changed from "collection=admin->action= > <some_action>" to "admin=collections->action=<some_action>". A migration tool > will be provided as part of SENTRY-1480). > - Config (to authorize various Solr collection configuration operations) > - Schema (to authorize Solr schema changes) > - Renamed sentry-core-model-search module to sentry-core-model-solr to > reflect the fact that it represents logic for Apache SOLR and is consistent > with naming convention used for other components (e.g. kafka) > - Moved the Solr audit log functionality to sentry-binding-solr module so > that it can be integrated with the Solr/Sentry authorization plugin. > - Deleted all the custom Solr request handlers in the > sentry-solr/solr-sentry-handlers module since the Solr authorization plugin > handles the authorization of all solr requests. This module now contains just > the functionality required for implementing Solr document level security. > > > Diffs > ----- > > pom.xml af54480 > sentry-binding/sentry-binding-solr/pom.xml ed2624b > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrAuthorizationException.java > 938dbfd > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrPluginImpl.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java > 0a818e5 > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzUtil.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java > 37efa5b > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/solr/sentry/AuditLogger.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/solr/sentry/RollingFileWithoutDeleteAppender.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/HdfsTestUtil.java > 859c793 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java > 7a88d90 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/AbstractTestSearchPolicyEngine.java > 3df6ecf > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/AbstractTestSolrPolicyEngine.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/SearchPolicyTestUtil.java > e198b5c > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/SolrPolicyTestUtil.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestCollectionRequiredInRole.java > 76211dd > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchAuthorizationProviderGeneralCases.java > b4aa684 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchAuthorizationProviderSpecialCases.java > 371f361 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchModelAuthorizables.java > e7da13a > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchPolicyEngineDFS.java > 59283ea > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchPolicyEngineLocalFS.java > 0ff4502 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchPolicyNegative.java > 20fee76 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSolrAuthorizationProviderGeneralCases.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSolrAuthorizationProviderSpecialCases.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSolrModelAuthorizables.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSolrPolicyEngineDFS.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSolrPolicyEngineLocalFS.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSolrPolicyNegative.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSearch.java > de6d6e0 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/test/resources/test-authz-provider.ini > 56317db > sentry-core/pom.xml 6b91767 > sentry-core/sentry-core-model-search/pom.xml 5917a63 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/Collection.java > 26ea287 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/Field.java > 2dd9065 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchActionFactory.java > 3f10726 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java > a2b17fc > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAction.java > 48ac267 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAuthorizable.java > 5a55963 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAuthorizables.java > 2b190e5 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchPrivilegeModel.java > 9429a25 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/validator/AbstractSearchPrivilegeValidator.java > c06131c > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/validator/CollectionRequiredInPrivilege.java > 93b3861 > > sentry-core/sentry-core-model-search/src/test/java/org/apache/sentry/core/search/TestCollection.java > 2311401 > > sentry-core/sentry-core-model-search/src/test/java/org/apache/sentry/core/search/TestSearchBitFieldAction.java > 0056f40 > sentry-core/sentry-core-model-solr/pom.xml PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/AdminOperation.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/Collection.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/Config.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/Field.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/Schema.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrActionFactory.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrConstants.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrModelAction.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrModelAuthorizable.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrModelAuthorizables.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/SolrPrivilegeModel.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/main/java/org/apache/sentry/core/model/solr/validator/SolrPrivilegeValidator.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/test/java/org/apache/sentry/core/solr/TestCollection.java > PRE-CREATION > > sentry-core/sentry-core-model-solr/src/test/java/org/apache/sentry/core/solr/TestSolrBitFieldAction.java > PRE-CREATION > sentry-dist/pom.xml 2d7f57e > sentry-provider/sentry-provider-db/pom.xml cd19032 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java > d8b4887 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java > 51d6df9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java > 77d3919 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java > 34c2107 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryGMPrivilege.java > 258721e > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > 9be4a8b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java > b7f0774 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceIntegration.java > ac8b2a7 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryConfigToolSolr.java > 3685073 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java > 55831a4 > sentry-solr/pom.xml 133ea60 > sentry-solr/solr-sentry-core/pom.xml e788262 > > sentry-solr/solr-sentry-core/src/main/java/org/apache/solr/sentry/AuditLogger.java > 7f3e391 > > sentry-solr/solr-sentry-core/src/main/java/org/apache/solr/sentry/RollingFileWithoutDeleteAppender.java > f749740 > > sentry-solr/solr-sentry-core/src/main/java/org/apache/solr/sentry/SecureRequestHandlerUtil.java > be9642b > > sentry-solr/solr-sentry-core/src/main/java/org/apache/solr/sentry/SentryIndexAuthorizationSingleton.java > 8bd93ad > sentry-solr/solr-sentry-handlers/pom.xml 5b024db > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureDocumentAnalysisRequestHandler.java > 1c1f6f8 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureFieldAnalysisRequestHandler.java > 62f9a19 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureRealTimeGetHandler.java > db182ef > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureReplicationHandler.java > bdcd830 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/admin/SecureAdminHandlers.java > 44db3b4 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/admin/SecureCollectionsHandler.java > b5edf20 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/admin/SecureCoreAdminHandler.java > ff6e281 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/admin/SecureInfoHandler.java > 628d1d7 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java > 933db43 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryIndexAuthorizationComponent.java > 5fbb743 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SecureRealTimeGetComponent.java > 7d55a7f > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/update/processor/UpdateIndexAuthorizationProcessor.java > d995a7d > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/update/processor/UpdateIndexAuthorizationProcessorFactory.java > 07f7f28 > > sentry-solr/solr-sentry-handlers/src/main/resources/sentry-handlers/solr/collection1/lib/classes/empty-file-main-lib.txt > 8b13789 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/TestSecureAnalysisHandlers.java > 28406e2 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/TestSecureReplicationHandler.java > 6367814 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/admin/SecureAdminHandlersTest.java > aea44f7 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/admin/SecureCollectionsHandlerTest.java > 218302e > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/admin/SecureCoreAdminHandlerTest.java > f93fb65 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/admin/SecureInfoHandlerTest.java > 54784f4 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/QueryDocAuthorizationComponentTest.java > 1f44628 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/QueryIndexAuthorizationComponentTest.java > a1f3760 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/sentry/SentryIndexAuthorizationSingletonTest.java > c294cf3 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/sentry/SentrySingletonTestInstance.java > 579f791 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/sentry/SentryTestBase.java > e1a1ba8 > > sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/update/processor/UpdateIndexAuthorizationProcessorTest.java > 630ca7c > sentry-tests/sentry-tests-solr/pom.xml 311e441 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java > 7ddd1e2 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestCase.java > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/DocLevelGenerator.java > e50e3f8 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/DummyAuthPluginImpl.java > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java > ac676a8 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestCollAdminCoreOperations.java > b0d6db1 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestDocLevelOperations.java > 71452e2 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java > f8ed955 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestRealTimeGet.java > f9b6c07 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSentryServer.java > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSolrAdminOperations.java > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSolrCollectionOperations.java > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSolrConfigOperations.java > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSolrSchemaOperations.java > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java > 2b246b5 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java > 71c3cb6 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrAdminOperations.java > c07b3b8 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrDocLevelOperations.java > 7f1fdfd > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrQueryOperations.java > 3eb6c0f > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrUpdateOperations.java > 9412325 > > sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-managed/conf/managed-schema > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-managed/conf/solrconfig.xml > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal/conf/schema.xml > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal/conf/solrconfig.xml > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_doc_level_security/conf/schema.xml > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_doc_level_security/conf/solrconfig.xml > PRE-CREATION > > sentry-tests/sentry-tests-solr/src/test/resources/solr/security/security.json > PRE-CREATION > > > Diff: https://reviews.apache.org/r/63250/diff/5/ > > > Testing > ------- > > All Solr/Sentry unit tests passing > > > Thanks, > > Hrishikesh Gadre > >
