From 63efcb8cf19ece51b637bc96cd8ea2bfdc590173 Mon Sep 17 00:00:00 2001 From: Kevin Doran Date: Wed, 20 Dec 2017 20:57:20 -0500 Subject: [PATCH] NIFIREG-75 Fix User Group Data Integrity Checks Removes user existence check from FileUserGroupProvider when group is created or updated. Replaces it with check in the Authorizer Decorator class created by Authorizer Factory, so that all providers are used. Also fixes bug when searching for group membership by user that returns results across all providers. Also updates a package in the authorizers.xml template. UI fixes for action icon enabled/disabled states in Administration. --- .../registry/authorization/UserGroup.java | 9 +++ .../authorization/AuthorizerFactory.java | 31 +++++++- ...ompositeConfigurableUserGroupProvider.java | 34 +++++++- .../authorization/CompositeUserAndGroups.java | 79 +++++++++++++++++++ .../CompositeUserGroupProvider.java | 60 ++++++++++---- .../file/FileUserGroupProvider.java | 29 ------- .../service/AuthorizationService.java | 6 +- .../src/main/resources/conf/authorizers.xml | 4 +- .../security/authorization/UserAndGroups.java | 15 ++++ .../IdentityAuthenticationProvider.java | 2 +- .../conf/secure-file/authorizers.xml | 4 +- .../conf/secure-ldap/authorizers.xml | 4 +- .../nf-registry-manage-group.html | 4 +- .../manage-user/nf-registry-manage-user.html | 2 +- .../manage-user/nf-registry-manage-user.js | 44 ++++++----- .../nf-registry-manage-bucket.html | 4 +- .../main/webapp/services/nf-registry.api.js | 1 + .../webapp/services/nf-registry.service.js | 5 +- 18 files changed, 248 insertions(+), 89 deletions(-) create mode 100644 nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserAndGroups.java diff --git a/nifi-registry-data-model/src/main/java/org/apache/nifi/registry/authorization/UserGroup.java b/nifi-registry-data-model/src/main/java/org/apache/nifi/registry/authorization/UserGroup.java index cc38c2147..1467929d7 100644 --- a/nifi-registry-data-model/src/main/java/org/apache/nifi/registry/authorization/UserGroup.java +++ b/nifi-registry-data-model/src/main/java/org/apache/nifi/registry/authorization/UserGroup.java @@ -59,4 +59,13 @@ public void addUsers(Collection users) { } } + public void addUser(Tenant user) { + if (user != null) { + if (this.users == null) { + this.users = new HashSet<>(); + } + this.users.add(user); + } + } + } diff --git a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/AuthorizerFactory.java b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/AuthorizerFactory.java index f94889f17..49b568820 100644 --- a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/AuthorizerFactory.java +++ b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/AuthorizerFactory.java @@ -570,6 +570,9 @@ public Group addGroup(Group group) throws AuthorizationAccessException { if (tenantExists(baseConfigurableUserGroupProvider, group.getIdentifier(), group.getName())) { throw new IllegalStateException(String.format("User/user group already exists with the identity '%s'.", group.getName())); } + if (!allGroupUsersExist(baseUserGroupProvider, group)) { + throw new IllegalStateException(String.format("Cannot create group '%s' with users that don't exist.", group.getName())); + } return baseConfigurableUserGroupProvider.addGroup(group); } @@ -586,6 +589,9 @@ public Group updateGroup(Group group) throws AuthorizationAccessException { if (!baseConfigurableUserGroupProvider.isConfigurable(group)) { throw new IllegalArgumentException("The specified group does not support modification."); } + if (!allGroupUsersExist(baseUserGroupProvider, group)) { + throw new IllegalStateException(String.format("Cannot update group '%s' to add users that don't exist.", group.getName())); + } return baseConfigurableUserGroupProvider.updateGroup(group); } @@ -782,10 +788,11 @@ private static boolean policyExists(final AccessPolicyProvider accessPolicyProvi } /** - * Checks if another user exists with the same identity. + * Checks if another user or group exists with the same identity. * - * @param identifier identity of the user - * @param identity identity of the user + * @param userGroupProvider the userGroupProvider to use to lookup the tenant + * @param identifier identity of the tenant + * @param identity identity of the tenant * @return true if another user exists with the same identity, false otherwise */ private static boolean tenantExists(final UserGroupProvider userGroupProvider, final String identifier, final String identity) { @@ -806,4 +813,22 @@ private static boolean tenantExists(final UserGroupProvider userGroupProvider, f return false; } + /** + * Check that all users in the group exist. + * + * @param userGroupProvider the userGroupProvider to use to lookup the users + * @param group the group whose users will be checked for existence. + * @return true if another user exists with the same identity, false otherwise + */ + private static boolean allGroupUsersExist(final UserGroupProvider userGroupProvider, final Group group) { + for (String userIdentifier : group.getUsers()) { + User user = userGroupProvider.getUser(userIdentifier); + if (user == null) { + return false; + } + } + + return true; + } + } diff --git a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeConfigurableUserGroupProvider.java b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeConfigurableUserGroupProvider.java index dfa671e8c..b65a5499e 100644 --- a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeConfigurableUserGroupProvider.java +++ b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeConfigurableUserGroupProvider.java @@ -181,17 +181,43 @@ public Group getGroup(String identifier) throws AuthorizationAccessException { @Override public UserAndGroups getUserAndGroups(String identity) throws AuthorizationAccessException { - UserAndGroups userAndGroups = configurableUserGroupProvider.getUserAndGroups(identity); - if (userAndGroups.getUser() == null) { - userAndGroups = super.getUserAndGroups(identity); + final CompositeUserAndGroups combinedResult; + + // First, lookup user and groups by identity and combine data from all providers + UserAndGroups configurableProviderResult = configurableUserGroupProvider.getUserAndGroups(identity); + UserAndGroups compositeProvidersResult = super.getUserAndGroups(identity); + + if (configurableProviderResult.getUser() != null && compositeProvidersResult.getUser() != null) { + throw new IllegalStateException("Multiple UserGroupProviders claim to provide user " + identity); + + } else if (configurableProviderResult.getUser() != null) { + combinedResult = new CompositeUserAndGroups(configurableProviderResult.getUser(), configurableProviderResult.getGroups()); + combinedResult.addAllGroups(compositeProvidersResult.getGroups()); + + } else if (compositeProvidersResult.getUser() != null) { + combinedResult = new CompositeUserAndGroups(compositeProvidersResult.getUser(), compositeProvidersResult.getGroups()); + combinedResult.addAllGroups(configurableProviderResult.getGroups()); + + } else { + return UserAndGroups.EMPTY; + } + + // Second, lookup groups containing the user identifier + // Don't have to check super.getGroups() because this step is done as part of super.getUserAndGroups(...) + String userIdentifier = combinedResult.getUser().getIdentifier(); + for (final Group group : configurableUserGroupProvider.getGroups()) { + if (group.getUsers() != null && group.getUsers().contains(userIdentifier)) { + combinedResult.addGroup(group); + } } - return userAndGroups; + return combinedResult; } @Override public void preDestruction() throws SecurityProviderDestructionException { super.preDestruction(); } + } diff --git a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserAndGroups.java b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserAndGroups.java new file mode 100644 index 000000000..5665122f1 --- /dev/null +++ b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserAndGroups.java @@ -0,0 +1,79 @@ +/* + * 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; + +import java.util.HashSet; +import java.util.Set; + +public class CompositeUserAndGroups implements UserAndGroups { + + private User user; + private Set groups; + + public CompositeUserAndGroups() { + this.user = null; + this.groups = null; + } + + public CompositeUserAndGroups(User user, Set groups) { + this.user = user; + setGroups(groups); + } + + @Override + public User getUser() { + return user; + } + + public void setUser(User user) { + this.user = user; + } + + @Override + public Set getGroups() { + return groups; + } + + public void setGroups(Set groups) { + // copy the collection so that if we add to this collection it does not modify other references + if (groups != null) { + this.groups = new HashSet<>(groups); + } else { + this.groups = null; + } + } + + public void addAllGroups(Set groups) { + if (groups != null) { + if (this.groups == null) { + this.groups = new HashSet<>(); + } + this.groups.addAll(groups); + } + } + + public void addGroup(Group group) { + if (group != null) { + if (this.groups == null) { + this.groups = new HashSet<>(); + } + this.groups.add(group); + } + } + + +} diff --git a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserGroupProvider.java b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserGroupProvider.java index 40ced18bf..76e154a08 100644 --- a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserGroupProvider.java +++ b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/CompositeUserGroupProvider.java @@ -20,6 +20,8 @@ import org.apache.nifi.registry.security.authorization.exception.AuthorizationAccessException; import org.apache.nifi.registry.security.exception.SecurityProviderCreationException; import org.apache.nifi.registry.security.exception.SecurityProviderDestructionException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.HashSet; @@ -31,6 +33,8 @@ public class CompositeUserGroupProvider implements UserGroupProvider { + private static final Logger logger = LoggerFactory.getLogger(CompositeUserGroupProvider.class); + static final String PROP_USER_GROUP_PROVIDER_PREFIX = "User Group Provider "; static final Pattern USER_GROUP_PROVIDER_PATTERN = Pattern.compile(PROP_USER_GROUP_PROVIDER_PREFIX + "\\S+"); @@ -142,33 +146,55 @@ public Group getGroup(String identifier) throws AuthorizationAccessException { @Override public UserAndGroups getUserAndGroups(String identity) throws AuthorizationAccessException { - UserAndGroups userAndGroups = null; + // This method builds a UserAndGroups response by combining the data from all providers using a two-pass approach + + CompositeUserAndGroups compositeUserAndGroups = new CompositeUserAndGroups(); + + // First pass - call getUserAndGroups(identity) on all providers, aggregate the responses, check for multiple + // user identity matches, which should not happen as identities should by globally unique. + String providerClassForUser = ""; for (final UserGroupProvider userGroupProvider : userGroupProviders) { - userAndGroups = userGroupProvider.getUserAndGroups(identity); + UserAndGroups userAndGroups = userGroupProvider.getUserAndGroups(identity); if (userAndGroups.getUser() != null) { - break; + // is this the first match on the user? + if(compositeUserAndGroups.getUser() == null) { + compositeUserAndGroups.setUser(userAndGroups.getUser()); + providerClassForUser = userGroupProvider.getClass().getName(); + } else { + logger.warn("Multiple UserGroupProviders are claiming to provide user '{}': [{} and {}] ", + identity, + userAndGroups.getUser(), + providerClassForUser, userGroupProvider.getClass().getName()); + throw new IllegalStateException("Multiple UserGroupProviders are claiming to provide user " + identity); + } + } + + if (userAndGroups.getGroups() != null) { + compositeUserAndGroups.addAllGroups(userAndGroups.getGroups()); } } - if (userAndGroups == null) { - // per API, returning non null with null user/groups - return new UserAndGroups() { - @Override - public User getUser() { - return null; - } + if (compositeUserAndGroups.getUser() == null) { + logger.debug("No user found for identity {}", identity); + return UserAndGroups.EMPTY; + } - @Override - public Set getGroups() { - return null; + // Second pass - Now that we've matched a user, call getGroups() on all providers, and + // check all groups to see if they contain the user identifier corresponding to the identity. + // This is necessary because a provider might only know about a group<->userIdentifier mapping + // without knowing the user identifier. + String userIdentifier = compositeUserAndGroups.getUser().getIdentifier(); + for (final UserGroupProvider userGroupProvider : userGroupProviders) { + for (final Group group : userGroupProvider.getGroups()) { + if (group.getUsers() != null && group.getUsers().contains(userIdentifier)) { + compositeUserAndGroups.addGroup(group); } - }; - } else { - // a delegated provider contained a matching user - return userAndGroups; + } } + + return compositeUserAndGroups; } @Override diff --git a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/file/FileUserGroupProvider.java b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/file/FileUserGroupProvider.java index e12816a26..4736fe2fb 100644 --- a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/file/FileUserGroupProvider.java +++ b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/security/authorization/file/FileUserGroupProvider.java @@ -310,9 +310,6 @@ public synchronized Group addGroup(Group group) throws AuthorizationAccessExcept final UserGroupHolder holder = userGroupHolder.get(); final Tenants tenants = holder.getTenants(); - // determine that all users in the group exist before doing anything, throw an exception if they don't - checkGroupUsers(group, tenants.getUsers().getUser()); - // create a new JAXB Group based on the incoming Group final org.apache.nifi.registry.security.authorization.file.tenants.generated.Group jaxbGroup = new org.apache.nifi.registry.security.authorization.file.tenants.generated.Group(); @@ -593,27 +590,6 @@ private org.apache.nifi.registry.security.authorization.file.tenants.generated.U return jaxbUser; } - private Set checkGroupUsers( - final Group group, - final List users) { - final Set jaxbUsers = new HashSet<>(); - for (String groupUser : group.getUsers()) { - boolean found = false; - for (org.apache.nifi.registry.security.authorization.file.tenants.generated.User jaxbUser : users) { - if (jaxbUser.getIdentifier().equals(groupUser)) { - jaxbUsers.add(jaxbUser); - found = true; - break; - } - } - - if (!found) { - throw new IllegalStateException("Unable to add group because user " + groupUser + " does not exist"); - } - } - return jaxbUsers; - } - /** * Loads the authorizations file and populates the AuthorizationsHolder, only called during start-up. * @@ -632,13 +608,8 @@ private synchronized void load() throws JAXBException, IllegalStateException, SA final UserGroupHolder userGroupHolder = new UserGroupHolder(tenants); final boolean emptyTenants = userGroupHolder.getAllUsers().isEmpty() && userGroupHolder.getAllGroups().isEmpty(); -// final boolean hasLegacyAuthorizedUsers = (legacyAuthorizedUsersFile != null && !StringUtils.isBlank(legacyAuthorizedUsersFile)); if (emptyTenants) { -// if (hasLegacyAuthorizedUsers) { -// logger.info("Loading users from legacy model " + legacyAuthorizedUsersFile + " into new users file."); -// convertLegacyAuthorizedUsers(tenants); -// } populateInitialUsers(tenants); diff --git a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/service/AuthorizationService.java b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/service/AuthorizationService.java index 5543c7ec1..219885c4a 100644 --- a/nifi-registry-framework/src/main/java/org/apache/nifi/registry/service/AuthorizationService.java +++ b/nifi-registry-framework/src/main/java/org/apache/nifi/registry/service/AuthorizationService.java @@ -16,16 +16,16 @@ */ package org.apache.nifi.registry.service; -import org.apache.nifi.registry.bucket.Bucket; import org.apache.nifi.registry.authorization.AccessPolicy; import org.apache.nifi.registry.authorization.AccessPolicySummary; import org.apache.nifi.registry.authorization.CurrentUser; import org.apache.nifi.registry.authorization.Permissions; import org.apache.nifi.registry.authorization.Resource; -import org.apache.nifi.registry.authorization.Tenant; import org.apache.nifi.registry.authorization.ResourcePermissions; +import org.apache.nifi.registry.authorization.Tenant; import org.apache.nifi.registry.authorization.User; import org.apache.nifi.registry.authorization.UserGroup; +import org.apache.nifi.registry.bucket.Bucket; import org.apache.nifi.registry.security.authorization.AccessPolicyProvider; import org.apache.nifi.registry.security.authorization.AccessPolicyProviderInitializationContext; import org.apache.nifi.registry.security.authorization.AuthorizableLookup; @@ -622,6 +622,7 @@ private Tenant tenantToDTO(org.apache.nifi.registry.security.authorization.User return null; } Tenant tenantDTO = new Tenant(user.getIdentifier(), user.getIdentity()); + tenantDTO.setConfigurable(AuthorizerCapabilityDetection.isUserConfigurable(authorizer, user)); return tenantDTO; } @@ -630,6 +631,7 @@ private Tenant tenantToDTO(org.apache.nifi.registry.security.authorization.Group return null; } Tenant tenantDTO = new Tenant(group.getIdentifier(), group.getName()); + tenantDTO.setConfigurable(AuthorizerCapabilityDetection.isGroupConfigurable(authorizer, group)); return tenantDTO; } diff --git a/nifi-registry-resources/src/main/resources/conf/authorizers.xml b/nifi-registry-resources/src/main/resources/conf/authorizers.xml index c879d2888..af0c53152 100644 --- a/nifi-registry-resources/src/main/resources/conf/authorizers.xml +++ b/nifi-registry-resources/src/main/resources/conf/authorizers.xml @@ -160,7 +160,7 @@ @@ -182,7 +182,7 @@ @@ -84,7 +84,7 @@ @@ -186,7 +186,7 @@