moresandeep commented on code in PR #1043:
URL: https://github.com/apache/knox/pull/1043#discussion_r2097948923


##########
gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java:
##########
@@ -62,286 +35,318 @@
 import org.apache.knox.gateway.util.AuthorizationException;
 import org.apache.knox.gateway.util.HttpExceptionUtils;
 
+import javax.security.auth.Subject;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletContext;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.security.AccessController;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+import static 
org.apache.knox.gateway.identityasserter.common.filter.AbstractIdentityAsserterDeploymentContributor.IMPERSONATION_PARAMS;
+import static 
org.apache.knox.gateway.identityasserter.common.filter.AbstractIdentityAsserterDeploymentContributor.ROLE;
+import static 
org.apache.knox.gateway.identityasserter.common.filter.VirtualGroupMapper.addRequestFunctions;
+
 public class CommonIdentityAssertionFilter extends 
AbstractIdentityAssertionFilter {
-  private static final IdentityAsserterMessages LOG = 
MessagesFactory.get(IdentityAsserterMessages.class);
-
-  public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "group.mapping.";
-  public static final String GROUP_PRINCIPAL_MAPPING = 
"group.principal.mapping";
-  public static final String PRINCIPAL_MAPPING = "principal.mapping";
-  public static final String ADVANCED_PRINCIPAL_MAPPING = 
"expression.principal.mapping";
-  private static final String PRINCIPAL_PARAM = "user.name";
-  private static final String DOAS_PRINCIPAL_PARAM = "doAs";
-  static final String IMPERSONATION_ENABLED_PARAM = 
AuthFilterUtils.PROXYUSER_PREFIX + ".impersonation.enabled";
-
-  private SimplePrincipalMapper mapper = new SimplePrincipalMapper();
-  private final Parser parser = new Parser();
-  private VirtualGroupMapper virtualGroupMapper;
-  /* List of all default and configured impersonation params */
-  protected final List<String> impersonationParamsList = new ArrayList<>();
-  protected boolean impersonationEnabled;
-  private AbstractSyntaxTree expressionPrincipalMapping;
-  private String topologyName;
-
-  @Override
-  public void init(FilterConfig filterConfig) throws ServletException {
-    topologyName = (String) 
filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_CLUSTER_ATTRIBUTE);
-    String principalMapping = filterConfig.getInitParameter(PRINCIPAL_MAPPING);
-    if (principalMapping == null || principalMapping.isEmpty()) {
-      principalMapping = 
filterConfig.getServletContext().getInitParameter(PRINCIPAL_MAPPING);
+    public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "group.mapping.";
+    public static final String GROUP_PRINCIPAL_MAPPING = 
"group.principal.mapping";
+    public static final String PRINCIPAL_MAPPING = "principal.mapping";
+    public static final String ADVANCED_PRINCIPAL_MAPPING = 
"expression.principal.mapping";
+    private static final IdentityAsserterMessages LOG = 
MessagesFactory.get(IdentityAsserterMessages.class);
+    private static final String PRINCIPAL_PARAM = "user.name";
+    private static final String DOAS_PRINCIPAL_PARAM = "doAs";
+    /* List of all default and configured impersonation params */
+    protected final List<String> impersonationParamsList = new ArrayList<>();
+    private final Parser parser = new Parser();
+    protected boolean impersonationEnabled;
+    private SimplePrincipalMapper mapper = new SimplePrincipalMapper();
+    private VirtualGroupMapper virtualGroupMapper;
+    private AbstractSyntaxTree expressionPrincipalMapping;
+    private String topologyName;
+
+    private static List<String> virtualGroupParameterNames(List<String> 
initParameterNames) {
+        return initParameterNames == null ? new ArrayList<>()
+                : initParameterNames.stream().filter(name -> 
name.startsWith(VIRTUAL_GROUP_MAPPING_PREFIX)).collect(Collectors.toList());
     }
-    String groupPrincipalMapping = 
filterConfig.getInitParameter(GROUP_PRINCIPAL_MAPPING);
-    if (groupPrincipalMapping == null || groupPrincipalMapping.isEmpty()) {
-      groupPrincipalMapping = 
filterConfig.getServletContext().getInitParameter(GROUP_PRINCIPAL_MAPPING);
-    }
-    if (principalMapping != null && !principalMapping.isEmpty() || 
groupPrincipalMapping != null && !groupPrincipalMapping.isEmpty()) {
-      try {
-        mapper.loadMappingTable(principalMapping, groupPrincipalMapping);
-      } catch (PrincipalMappingException e) {
-        throw new ServletException("Unable to load principal mapping table.", 
e);
-      }
+
+    private static String[] unique(String[] groups) {
+        return new HashSet<>(Arrays.asList(groups)).toArray(new String[0]);
     }
-    expressionPrincipalMapping = parseAdvancedPrincipalMapping(filterConfig);
 
-    final List<String> initParameterNames = 
AuthFilterUtils.getInitParameterNamesAsList(filterConfig);
+    @Override
+    public void init(FilterConfig filterConfig) throws ServletException {
+        topologyName = (String) 
filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_CLUSTER_ATTRIBUTE);
+        String principalMapping = 
filterConfig.getInitParameter(PRINCIPAL_MAPPING);
+        if (principalMapping == null || principalMapping.isEmpty()) {
+            principalMapping = 
filterConfig.getServletContext().getInitParameter(PRINCIPAL_MAPPING);
+        }
+        String groupPrincipalMapping = 
filterConfig.getInitParameter(GROUP_PRINCIPAL_MAPPING);
+        if (groupPrincipalMapping == null || groupPrincipalMapping.isEmpty()) {
+            groupPrincipalMapping = 
filterConfig.getServletContext().getInitParameter(GROUP_PRINCIPAL_MAPPING);
+        }
+        if (principalMapping != null && !principalMapping.isEmpty() || 
groupPrincipalMapping != null && !groupPrincipalMapping.isEmpty()) {
+            try {
+                mapper.loadMappingTable(principalMapping, 
groupPrincipalMapping);
+            } catch (PrincipalMappingException e) {
+                throw new ServletException("Unable to load principal mapping 
table.", e);
+            }
+        }
+        expressionPrincipalMapping = 
parseAdvancedPrincipalMapping(filterConfig);
 
-    virtualGroupMapper = new 
VirtualGroupMapper(loadVirtualGroups(filterConfig, initParameterNames));
+        final List<String> initParameterNames = 
AuthFilterUtils.getInitParameterNamesAsList(filterConfig);
 
-    initImpersonationParamsList(filterConfig);
-    initProxyUserConfiguration(filterConfig, initParameterNames);
-  }
+        virtualGroupMapper = new 
VirtualGroupMapper(loadVirtualGroups(filterConfig, initParameterNames));
 
-  private AbstractSyntaxTree parseAdvancedPrincipalMapping(FilterConfig 
filterConfig) {
-    String expression = 
filterConfig.getInitParameter(ADVANCED_PRINCIPAL_MAPPING);
-    if (StringUtils.isBlank(expression)) {
-      expression = 
filterConfig.getServletContext().getInitParameter(ADVANCED_PRINCIPAL_MAPPING);
+        initImpersonationParamsList(filterConfig);
+        initProxyUserConfiguration(filterConfig, initParameterNames);
     }
-    return StringUtils.isBlank(expression) ? null : parser.parse(expression);
-  }
-
-  /*
-   * Initialize the impersonation params list.
-   * This list contains query params that needs to be scrubbed
-   * from the outgoing request.
-   */
-  private void initImpersonationParamsList(FilterConfig filterConfig) {
-    String impersonationListFromConfig = 
filterConfig.getInitParameter(IMPERSONATION_PARAMS);
-    if (impersonationListFromConfig == null || 
impersonationListFromConfig.isEmpty()) {
-      impersonationListFromConfig = 
filterConfig.getServletContext().getInitParameter(IMPERSONATION_PARAMS);
+
+    private AbstractSyntaxTree parseAdvancedPrincipalMapping(FilterConfig 
filterConfig) {
+        String expression = 
filterConfig.getInitParameter(ADVANCED_PRINCIPAL_MAPPING);
+        if (StringUtils.isBlank(expression)) {
+            expression = 
filterConfig.getServletContext().getInitParameter(ADVANCED_PRINCIPAL_MAPPING);
+        }
+        return StringUtils.isBlank(expression) ? null : 
parser.parse(expression);
     }
 
-    /* Add default impersonation params */
-    impersonationParamsList.add(DOAS_PRINCIPAL_PARAM);
-    impersonationParamsList.add(PRINCIPAL_PARAM);
-
-    if (impersonationListFromConfig != null && 
!impersonationListFromConfig.isEmpty()) {
-      /* Add configured impersonation params */
-      LOG.impersonationConfig(impersonationListFromConfig);
-      final StringTokenizer t = new 
StringTokenizer(impersonationListFromConfig, ",");
-      while (t.hasMoreElements()) {
-        final String token = t.nextToken().trim();
-        if (!impersonationParamsList.contains(token)) {
-          impersonationParamsList.add(token);
+    /*
+     * Initialize the impersonation params list.
+     * This list contains query params that needs to be scrubbed
+     * from the outgoing request.
+     */
+    private void initImpersonationParamsList(FilterConfig filterConfig) {
+        String impersonationListFromConfig = 
filterConfig.getInitParameter(IMPERSONATION_PARAMS);
+        if (impersonationListFromConfig == null || 
impersonationListFromConfig.isEmpty()) {
+            impersonationListFromConfig = 
filterConfig.getServletContext().getInitParameter(IMPERSONATION_PARAMS);
+        }
+
+        /* Add default impersonation params */
+        impersonationParamsList.add(DOAS_PRINCIPAL_PARAM);
+        impersonationParamsList.add(PRINCIPAL_PARAM);
+
+        if (impersonationListFromConfig != null && 
!impersonationListFromConfig.isEmpty()) {
+            /* Add configured impersonation params */
+            LOG.impersonationConfig(impersonationListFromConfig);
+            final StringTokenizer t = new 
StringTokenizer(impersonationListFromConfig, ",");
+            while (t.hasMoreElements()) {
+                final String token = t.nextToken().trim();
+                if (!impersonationParamsList.contains(token)) {
+                    impersonationParamsList.add(token);
+                }
+            }
         }
-      }
     }
-  }
-
-  private void initProxyUserConfiguration(FilterConfig filterConfig, 
List<String> initParameterNames) {
-    final String impersonationEnabledValue = 
filterConfig.getInitParameter(IMPERSONATION_ENABLED_PARAM);
-    impersonationEnabled = impersonationEnabledValue == null ? Boolean.FALSE : 
Boolean.parseBoolean(impersonationEnabledValue);
-
-    if (impersonationEnabled) {
-      if (AuthFilterUtils.hasProxyConfig(topologyName, "HadoopAuth")) {
-        LOG.ignoreProxyuserConfig();
-        impersonationEnabled = false; //explicitly set to false to avoid 
redundant authorization attempts at request processing time
-      } else {
-        AuthFilterUtils.refreshSuperUserGroupsConfiguration(filterConfig, 
initParameterNames, topologyName, ROLE);
-        
filterConfig.getServletContext().setAttribute(ContextAttributes.IMPERSONATION_ENABLED_ATTRIBUTE,
 Boolean.TRUE);
-      }
-    } else {
-      
filterConfig.getServletContext().setAttribute(ContextAttributes.IMPERSONATION_ENABLED_ATTRIBUTE,
 Boolean.FALSE);
+
+    private void initProxyUserConfiguration(FilterConfig filterConfig, 
List<String> initParameterNames) {
+        impersonationEnabled = shouldAddImpersonationProvider(filterConfig);
+        if (impersonationEnabled) {
+            if (AuthFilterUtils.hasProxyConfig(topologyName, "HadoopAuth")) {
+                LOG.ignoreProxyuserConfig();
+                impersonationEnabled = false; //explicitly set to false to 
avoid redundant authorization attempts at request processing time
+            } else {
+                
AuthFilterUtils.refreshSuperUserGroupsConfiguration(filterConfig, 
initParameterNames, topologyName, ROLE);
+                
filterConfig.getServletContext().setAttribute(ContextAttributes.IMPERSONATION_ENABLED_ATTRIBUTE,
 Boolean.TRUE);
+            }
+        } else {
+            
filterConfig.getServletContext().setAttribute(ContextAttributes.IMPERSONATION_ENABLED_ATTRIBUTE,
 Boolean.FALSE);
+        }
     }
-  }
 
-  boolean isImpersonationEnabled() {
-    return impersonationEnabled;
-  }
+    /**
+     * Check if the impersonation provider should be added to the filter chain 
based on
+     * user and group impersonation settings.
+     *
+     * @param filterConfig
+     * @return
+     */
+    boolean shouldAddImpersonationProvider(final FilterConfig filterConfig) {

Review Comment:
   @zeroflag yup! 



-- 
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: dev-unsubscr...@knox.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to