mneethiraj commented on code in PR #1000: URL: https://github.com/apache/ranger/pull/1000#discussion_r3373884688
########## security-admin/src/main/java/org/apache/ranger/common/RangerSuperUserConfig.java: ########## @@ -0,0 +1,249 @@ +/* + * 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.ranger.common; + +import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.StringUtils; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +/** + * Configuration-based Ranger Admin superusers and super groups. + * When {@code ranger.admin.super.users} or + * {@code ranger.admin.super.groups} are set, matching authenticated users + * receive full Ranger administrative privileges (system admin and key admin + * capabilities) without requiring corresponding roles in the Ranger database. + */ +public final class RangerSuperUserConfig { + private static volatile RangerSuperUserConfig instance; + + private final Set<String> superUsers; + private final Set<String> superUserGroups; + private final boolean superUsersConfigured; + private final boolean superGroupsConfigured; + private final boolean enabled; + + private RangerSuperUserConfig() { + superUsers = parsePropertyValues( + PropertiesUtil.getPropertyStringList( + RangerConstants.RANGER_ADMIN_SUPER_USERS)); + superUserGroups = parsePropertyValues( + PropertiesUtil.getPropertyStringList( + RangerConstants.RANGER_ADMIN_SUPER_GROUPS)); + superUsersConfigured = !superUsers.isEmpty(); + superGroupsConfigured = !superUserGroups.isEmpty(); + enabled = superUsersConfigured || superGroupsConfigured; + } + + /** + * Clears cached config snapshot. For unit tests only after + * {@link PropertiesUtil} properties change. + */ + public static synchronized void resetForTests() { + instance = null; + } + + /** + * @return true when {@code ranger.admin.super.users} or + * {@code ranger.admin.super.groups} has at least one non-blank + * entry + */ + public static boolean isEnabled() { + return getInstance().enabled; + } + + /** + * @return true when {@code ranger.admin.super.groups} has at least one + * non-blank entry + */ + public static boolean isSuperGroupsConfigured() { + return getInstance().superGroupsConfigured; + } + + /** + * @param userName authenticated username + * @return true when userName matches configured superusers + */ + public static boolean isSuperUser(final String userName) { + final boolean isSuperUser; + RangerSuperUserConfig cfg = getInstance(); + + if (StringUtils.isBlank(userName) || !cfg.enabled) { + isSuperUser = false; + } else if (cfg.superUsersConfigured && cfg.matchesUser(userName)) { + isSuperUser = true; + } else { + isSuperUser = false; + } + + return isSuperUser; + } + + /** + * @param userName authenticated user name + * @param userGroups group names from Ranger user store + * @return true when userName or groups match configured super users/groups + */ + public static boolean isSuperUser(final String userName, final Set<String> userGroups) { + final boolean isSuperUser; + RangerSuperUserConfig cfg = getInstance(); + if (StringUtils.isBlank(userName) || !cfg.enabled) { + isSuperUser = false; + } else if (cfg.superUsersConfigured && cfg.matchesUser(userName)) { + isSuperUser = true; + } else { + isSuperUser = cfg.superGroupsConfigured && cfg.matchesGroups(userGroups); + } + + return isSuperUser; + } + + /** + * Merges config super-user admin roles with existing portal roles. + * Used for Spring Security authentication and session role lists. + * + * @param existingRoles DB portal roles (may be null) + * @param includeRoleUser when true, adds {@code ROLE_USER} (session lists) + * @return merged role list with stable ordering + */ + public static List<String> mergeConfigSuperUserRoles(final Collection<String> existingRoles, final boolean includeRoleUser) { + LinkedHashSet<String> merged = new LinkedHashSet<>(); + + merged.add(RangerConstants.ROLE_SYS_ADMIN); + merged.add(RangerConstants.ROLE_KEY_ADMIN); + + if (includeRoleUser) { + merged.add(RangerConstants.ROLE_USER); + } + + if (existingRoles != null) { + merged.addAll(existingRoles); + } + + return new ArrayList<>(merged); + } + + /** + * Admin roles exposed on {@code GET /user/profile} for config super-users. + */ + public static List<String> getConfigSuperUserProfileRoles() { + return mergeConfigSuperUserRoles(Collections.emptyList(), false); + } + + private static Set<String> parsePropertyValues(final String[] values) { + Set<String> parsed = new HashSet<>(); + + if (values != null) { + for (String value : values) { + if (StringUtils.isNotBlank(value)) { + parsed.add(value.trim()); + } + } + } + + return Collections.unmodifiableSet(parsed); + } + + private static RangerSuperUserConfig getInstance() { + RangerSuperUserConfig cfg = instance; + + if (cfg == null) { + synchronized (RangerSuperUserConfig.class) { + cfg = instance; + if (cfg == null) { + cfg = new RangerSuperUserConfig(); + instance = cfg; + } + } + } + + return cfg; + } + + private boolean matchesUser(final String userName) { + final boolean matched; + + if (StringUtils.isBlank(userName)) { + matched = false; + } else { + // Case-insensitive match; "*" wildcard is not handled by CollectionUtils.contains + boolean result = false; + + for (String configuredUser : superUsers) { + if ("*".equals(configuredUser) || configuredUser.equalsIgnoreCase(userName)) { + result = true; + break; + } + } + + matched = result; + } + + return matched; + } + + private boolean matchesGroups(final Set<String> userGroups) { + final boolean matched; + + if (CollectionUtils.isEmpty(userGroups)) { + matched = false; + } else { + // Case-insensitive match; GROUP_PUBLIC is not handled by CollectionUtils.containsAny + boolean result = false; + + for (String configuredGroup : superUserGroups) { + if (RangerConstants.GROUP_PUBLIC.equalsIgnoreCase(configuredGroup) || containsGroupIgnoreCase(userGroups, configuredGroup)) { Review Comment: Allowing `public` as super groups doesn't seem a valid use case. I suggest removing this. ########## security-admin/src/main/java/org/apache/ranger/common/RangerSuperUserConfig.java: ########## @@ -0,0 +1,249 @@ +/* + * 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.ranger.common; + +import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.StringUtils; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +/** + * Configuration-based Ranger Admin superusers and super groups. + * When {@code ranger.admin.super.users} or + * {@code ranger.admin.super.groups} are set, matching authenticated users + * receive full Ranger administrative privileges (system admin and key admin + * capabilities) without requiring corresponding roles in the Ranger database. + */ +public final class RangerSuperUserConfig { + private static volatile RangerSuperUserConfig instance; + + private final Set<String> superUsers; + private final Set<String> superUserGroups; + private final boolean superUsersConfigured; + private final boolean superGroupsConfigured; + private final boolean enabled; + + private RangerSuperUserConfig() { + superUsers = parsePropertyValues( + PropertiesUtil.getPropertyStringList( + RangerConstants.RANGER_ADMIN_SUPER_USERS)); + superUserGroups = parsePropertyValues( + PropertiesUtil.getPropertyStringList( + RangerConstants.RANGER_ADMIN_SUPER_GROUPS)); + superUsersConfigured = !superUsers.isEmpty(); + superGroupsConfigured = !superUserGroups.isEmpty(); + enabled = superUsersConfigured || superGroupsConfigured; + } + + /** + * Clears cached config snapshot. For unit tests only after + * {@link PropertiesUtil} properties change. + */ + public static synchronized void resetForTests() { + instance = null; + } + + /** + * @return true when {@code ranger.admin.super.users} or + * {@code ranger.admin.super.groups} has at least one non-blank + * entry + */ + public static boolean isEnabled() { + return getInstance().enabled; + } + + /** + * @return true when {@code ranger.admin.super.groups} has at least one + * non-blank entry + */ + public static boolean isSuperGroupsConfigured() { + return getInstance().superGroupsConfigured; + } + + /** + * @param userName authenticated username + * @return true when userName matches configured superusers + */ + public static boolean isSuperUser(final String userName) { + final boolean isSuperUser; + RangerSuperUserConfig cfg = getInstance(); + + if (StringUtils.isBlank(userName) || !cfg.enabled) { + isSuperUser = false; + } else if (cfg.superUsersConfigured && cfg.matchesUser(userName)) { + isSuperUser = true; + } else { + isSuperUser = false; + } + + return isSuperUser; + } + + /** + * @param userName authenticated user name + * @param userGroups group names from Ranger user store + * @return true when userName or groups match configured super users/groups + */ + public static boolean isSuperUser(final String userName, final Set<String> userGroups) { + final boolean isSuperUser; + RangerSuperUserConfig cfg = getInstance(); + if (StringUtils.isBlank(userName) || !cfg.enabled) { + isSuperUser = false; + } else if (cfg.superUsersConfigured && cfg.matchesUser(userName)) { + isSuperUser = true; + } else { + isSuperUser = cfg.superGroupsConfigured && cfg.matchesGroups(userGroups); + } + + return isSuperUser; + } + + /** + * Merges config super-user admin roles with existing portal roles. + * Used for Spring Security authentication and session role lists. + * + * @param existingRoles DB portal roles (may be null) + * @param includeRoleUser when true, adds {@code ROLE_USER} (session lists) + * @return merged role list with stable ordering + */ + public static List<String> mergeConfigSuperUserRoles(final Collection<String> existingRoles, final boolean includeRoleUser) { + LinkedHashSet<String> merged = new LinkedHashSet<>(); + + merged.add(RangerConstants.ROLE_SYS_ADMIN); + merged.add(RangerConstants.ROLE_KEY_ADMIN); + + if (includeRoleUser) { + merged.add(RangerConstants.ROLE_USER); + } + + if (existingRoles != null) { + merged.addAll(existingRoles); + } + + return new ArrayList<>(merged); + } + + /** + * Admin roles exposed on {@code GET /user/profile} for config super-users. + */ + public static List<String> getConfigSuperUserProfileRoles() { + return mergeConfigSuperUserRoles(Collections.emptyList(), false); + } + + private static Set<String> parsePropertyValues(final String[] values) { + Set<String> parsed = new HashSet<>(); + + if (values != null) { + for (String value : values) { + if (StringUtils.isNotBlank(value)) { + parsed.add(value.trim()); + } + } + } + + return Collections.unmodifiableSet(parsed); + } + + private static RangerSuperUserConfig getInstance() { + RangerSuperUserConfig cfg = instance; + + if (cfg == null) { + synchronized (RangerSuperUserConfig.class) { + cfg = instance; + if (cfg == null) { + cfg = new RangerSuperUserConfig(); + instance = cfg; + } + } + } + + return cfg; + } + + private boolean matchesUser(final String userName) { + final boolean matched; + + if (StringUtils.isBlank(userName)) { + matched = false; + } else { + // Case-insensitive match; "*" wildcard is not handled by CollectionUtils.contains + boolean result = false; + + for (String configuredUser : superUsers) { + if ("*".equals(configuredUser) || configuredUser.equalsIgnoreCase(userName)) { Review Comment: - `'*` as valid value doesn't seem correct for super.users. I suggest removing this. - is it necessary to do `equalsIgnoreCase()` for user and group names? Please look at rest of the code base and keep this consistent. Please review and update line 218, and any other such occurances. ########## security-admin/src/main/java/org/apache/ranger/biz/UserMgr.java: ########## @@ -1153,6 +1159,43 @@ public Collection<String> getRolesByLoginId(String loginId) { return roleList; } + /** + * Returns DB roles augmented with config super-user roles for + * Spring Security authentication. + * Does not modify {@code x_portal_user_role}; use + * {@link #getRolesByLoginId(String)} for DB-only roles. + * + * @param loginId portal login id + * @return DB roles plus config super-user roles when applicable + */ + public Collection<String> getAuthenticationRolesByLoginId(final String loginId) { Review Comment: Why not update existing method `getRolesByLoginId()` instead of introducing new method `getAuthenticationRolesByLoginId()`? ########## security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java: ########## @@ -457,7 +457,8 @@ public VXUserList searchXUsers(@Context HttpServletRequest request, @QueryParam( hasRole = !userRolesList.contains(RangerConstants.ROLE_KEY_ADMIN) ? userRolesList.add(RangerConstants.ROLE_KEY_ADMIN) : hasRole; hasRole = !userRolesList.contains(RangerConstants.ROLE_KEY_ADMIN_AUDITOR) ? userRolesList.add(RangerConstants.ROLE_KEY_ADMIN_AUDITOR) : hasRole; hasRole = !userRolesList.contains(RangerConstants.ROLE_USER) ? userRolesList.add(RangerConstants.ROLE_USER) : hasRole; - } else if (loggedInVXUser.getUserRoleList().contains(RangerConstants.ROLE_USER)) { + } else if (userSession.isSingleRoleUserSession()) { Review Comment: with updated line 460, following condition in line 462 will never trigger `true`: `userRolesList.size() != 1` - which means the exception at line 464 will never be thrown. Please review this change and update. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
