From issues-return-86782-archive-asf-public=cust-asf.ponee.io@nifi.apache.org Wed Oct 16 19:44:15 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id D2D4218067E for ; Wed, 16 Oct 2019 21:44:14 +0200 (CEST) Received: (qmail 55791 invoked by uid 500); 16 Oct 2019 19:44:14 -0000 Mailing-List: contact issues-help@nifi.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@nifi.apache.org Delivered-To: mailing list issues@nifi.apache.org Received: (qmail 55691 invoked by uid 99); 16 Oct 2019 19:44:14 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 16 Oct 2019 19:44:14 +0000 From: GitBox To: issues@nifi.apache.org Subject: [GitHub] [nifi-registry] kevdoran commented on a change in pull request #232: NIFIREG-292 Add DB impls of UserGroupProvider and AccessPolicyProvider Message-ID: <157125505402.30498.17060230046868328786.gitbox@gitbox.apache.org> Date: Wed, 16 Oct 2019 19:44:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit kevdoran commented on a change in pull request #232: NIFIREG-292 Add DB impls of UserGroupProvider and AccessPolicyProvider URL: https://github.com/apache/nifi-registry/pull/232#discussion_r335671380 ########## File path: nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/database/DatabaseAccessPolicyProvider.java ########## @@ -0,0 +1,122 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.registry.security.authorization.database; + +import org.apache.commons.lang3.StringUtils; +import org.apache.nifi.registry.properties.NiFiRegistryProperties; +import org.apache.nifi.registry.properties.util.IdentityMapping; +import org.apache.nifi.registry.properties.util.IdentityMappingUtil; +import org.apache.nifi.registry.security.authorization.AuthorizerConfigurationContext; +import org.apache.nifi.registry.security.authorization.Group; +import org.apache.nifi.registry.security.authorization.User; +import org.apache.nifi.registry.security.authorization.annotation.AuthorizerContext; +import org.apache.nifi.registry.security.authorization.util.AccessPolicyProviderUtils; +import org.apache.nifi.registry.security.authorization.util.InitialPolicies; +import org.apache.nifi.registry.security.authorization.util.ResourceAndAction; +import org.apache.nifi.registry.security.exception.SecurityProviderCreationException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.util.CollectionUtils; + +import javax.sql.DataSource; +import java.util.Collections; +import java.util.List; +import java.util.Set; + +/** + * The NiFi Registry specific implementation of an ConfigurableAccessPolicyProvider backed by database. + */ +public class DatabaseAccessPolicyProvider extends AbstractDatabaseAccessPolicyProvider { + + private static final Logger LOGGER = LoggerFactory.getLogger(DatabaseAccessPolicyProvider.class); + + private DataSource dataSource; + private NiFiRegistryProperties properties; + + @AuthorizerContext + public void setDataSource(final DataSource dataSource) { + this.dataSource = dataSource; + } + + @AuthorizerContext + public void setProperties(final NiFiRegistryProperties properties) { + this.properties = properties; + } + + @Override + protected DataSource getDataSource() { + return dataSource; + } + + @Override + protected void populateInitialPolicies(final AuthorizerConfigurationContext configurationContext) { + final List identityMappings = Collections.unmodifiableList(IdentityMappingUtil.getIdentityMappings(properties)); + final List groupMappings = Collections.unmodifiableList(IdentityMappingUtil.getGroupMappings(properties)); + + final String initialAdminIdentity = AccessPolicyProviderUtils.getInitialAdminIdentity(configurationContext, identityMappings); + final Set nifiIdentities = AccessPolicyProviderUtils.getNiFiIdentities(configurationContext, identityMappings); + final String nifiGroupName = AccessPolicyProviderUtils.getNiFiGroupName(configurationContext, groupMappings); + + if (!StringUtils.isBlank(initialAdminIdentity)) { + LOGGER.info("Populating authorizations for Initial Admin: '" + initialAdminIdentity + "'"); + populateInitialAdmin(initialAdminIdentity); + } + + if (!CollectionUtils.isEmpty(nifiIdentities)) { + LOGGER.info("Populating authorizations for NiFi identities: [{}]", StringUtils.join(nifiIdentities, ";")); + populateNiFiIdentities(nifiIdentities); + } + + if (!StringUtils.isBlank(nifiGroupName)) { + LOGGER.info("Populating authorizations for NiFi Group: '" + nifiGroupName + "'"); + populateNiFiGroup(nifiGroupName); + } + } + Review comment: So, a few general thoughts that apply to both `AbstractDatabaseAPP->DatabaseAPP` and also `AbstractDatabaseUGP->DatabaseUGP`. Recently, I've personally been trying to avoid two things in my own code: 1. Static utility classes that do anything more than trivial checks/transforms 2. The pattern of abstract base classes that delegate required functionality to abstract protected methods. It works, but I just find it harder to understand, test, maintain, and reuse. (That might just be me!) An alternative that usually works for me is making the base class concrete and defining an interface type (or multiple interfaces) that can be injected into that class. So using DatabaseAccessPolicyProvider as an example, the concrete subclass could be collapsed into the base class, which would no longer be abstract. The class could then be injected with the DataSource and also a new interface, something like this: ```java interface IdentityMappingProvider { List getIdentityMappings() } ``` or perhaps even better: ```java interface IdentityMapper { String mapIdentity(String rawIdentity) } ``` In NiFiRegistry, we could create implementations of `IdentityMappingProvider` or `IdentityMapper` that are backed by a properties object or NiFiRegistryProperties specifically, but if we wanted to reuse these DatabaseAPP and DatabaseUGP classes in other apps, it is easier to reuse them as those apps would merely need to provide implementations that make sense in those contexts. These new interfaces would do away with the need for IdentityMappingUtil (as the logic would move to those implementing classes), as well as any location in our code that is directly coupled to NiFiRegsitryProperties solely for the purpose of passing to IdentityMappingUtil. Now in this case, constructor injection / autowiring does not work because of the way the classes are created by the framework using JAX-B and the extension/provider interface we are supporting. (Side note: I think we should revisit the way we support this as part of 1.0 or 2.0, but that is definitely outside the scope of this PR). However, I do think extending the `AuthorizerConfigurationContext` is on the table. For example: ```java public interface AuthorizerConfigurationContext { // existing methods String getIdentifier(); Map getProperties(); PropertyValue getProperty(String property); // new methods DataSource getDataSource(); IdentityMapper getIdentityMapper(); } ``` In this way, the implementations of the interfaces could be injected into `AuthorizerFactory` and passed along to extensions when `AuthorizerFactory` creates the configuration context and calls `onConfigured(context)`. Provider implementations could use those objects if needed or ignore them if not applicable to the provider impl. Let me know what you think. I know this is done/working and the changes I propose are non-trivial. I am happy with any of the following: - Leaving as is if you prefer the approach used here - Doing this refactoring later, perhaps a part of a larger set of changes (for example, there are lot of places that NiFiRegistry injections and the existing IdentityMappingUtil is used, and my suggestions here apply to those existing locations as well). - Providing some of the work that would make this easier, but contributing PRs to your branch or to master directly then we could then use to rebase this branch upon and take advantage of new interfaces/interface methods. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org With regards, Apache Git Services