pzampino commented on code in PR #681: URL: https://github.com/apache/knox/pull/681#discussion_r1034240821
########## gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java: ########## @@ -84,57 +98,86 @@ public void init(FilterConfig filterConfig) throws ServletException { throw new ServletException("Unable to load principal mapping table.", e); } } - virtualGroupMapper = new VirtualGroupMapper(loadVirtualGroups(filterConfig)); - String impersonationListFromConfig = filterConfig.getInitParameter(IMPERSONATION_PARAMS); - if (impersonationListFromConfig == null || impersonationListFromConfig.isEmpty()) { - impersonationListFromConfig = filterConfig.getServletContext().getInitParameter(IMPERSONATION_PARAMS); - } - initImpersonationParamsList(impersonationListFromConfig); + + //subsequent iterations on filterConfig.getInitParameterNames() would not work if we simply used that as an enumeration + final List<String> initParameterNames = filterConfig.getInitParameterNames() == null ? Collections.emptyList() + : Collections.list(filterConfig.getInitParameterNames()); + + virtualGroupMapper = new VirtualGroupMapper(loadVirtualGroups(filterConfig, initParameterNames)); + + initImpersonationParamsList(filterConfig); + initProxyUserConfiguration(filterConfig, initParameterNames); } - /** + /* * Initialize the impersonation params list. * This list contains query params that needs to be scrubbed * from the outgoing request. - * @param impersonationListFromConfig - * @return */ - private void initImpersonationParamsList(final String impersonationListFromConfig) { + 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(null == impersonationListFromConfig || impersonationListFromConfig.isEmpty()) { - return; - } else { + + if (impersonationListFromConfig != null && !impersonationListFromConfig.isEmpty()) { /* Add configured impersonation params */ LOG.impersonationConfig(impersonationListFromConfig); final StringTokenizer t = new StringTokenizer(impersonationListFromConfig, ","); - while(t.hasMoreElements()) { + while (t.hasMoreElements()) { final String token = t.nextToken().trim(); - if(!impersonationParamsList.contains(token)) { + if (!impersonationParamsList.contains(token)) { impersonationParamsList.add(token); } } } } - private Map<String, AbstractSyntaxTree> loadVirtualGroups(FilterConfig filterConfig) { + private void initProxyUserConfiguration(FilterConfig filterConfig, List<String> initParameterNames) { + final String impersonationEnabledValue = filterConfig.getInitParameter(IMPERSONATION_ENABLED_PARAM); + this.impersonationEnabled = impersonationEnabledValue == null ? Boolean.FALSE : Boolean.parseBoolean(impersonationEnabledValue); Review Comment: Why the explicit "this" used with the impersonationEnabled reference in this method? ########## gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java: ########## @@ -50,4 +51,16 @@ public interface IdentityAsserterMessages { @Message( level = MessageLevel.INFO, text = "Using configured impersonation parameters: {0}") void impersonationConfig(String config); + + @Message( level = MessageLevel.WARN, text = "Configured proxyuser parameters are IGNORED because the HadoopAuth filter already has them.") Review Comment: This almost implies that we know the values are the same. Perhaps, something like this is better?: "Ignoring the proxyuser configuration in favor of the HadoopAuth provider's configuration." ########## gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java: ########## @@ -50,4 +51,16 @@ public interface IdentityAsserterMessages { @Message( level = MessageLevel.INFO, text = "Using configured impersonation parameters: {0}") void impersonationConfig(String config); + + @Message( level = MessageLevel.WARN, text = "Configured proxyuser parameters are IGNORED because the HadoopAuth filter already has them.") + void ignoreProxyuserConfig(); + + @Message( level = MessageLevel.DEBUG, text = "doAsUser = {0}, RemoteUser = {1} , RemoteAddress = {2}" ) + void hadoopAuthDoAsUser(String doAsUser, String remoteUser, String remoteAddr); Review Comment: Is it intentional that these logging method names reference hadoopAuth? Are they really associated with the HadoopAuth provider? ########## gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java: ########## @@ -84,57 +98,86 @@ public void init(FilterConfig filterConfig) throws ServletException { throw new ServletException("Unable to load principal mapping table.", e); } } - virtualGroupMapper = new VirtualGroupMapper(loadVirtualGroups(filterConfig)); - String impersonationListFromConfig = filterConfig.getInitParameter(IMPERSONATION_PARAMS); - if (impersonationListFromConfig == null || impersonationListFromConfig.isEmpty()) { - impersonationListFromConfig = filterConfig.getServletContext().getInitParameter(IMPERSONATION_PARAMS); - } - initImpersonationParamsList(impersonationListFromConfig); + + //subsequent iterations on filterConfig.getInitParameterNames() would not work if we simply used that as an enumeration Review Comment: Why would it not "work" on subsequent iterations? -- 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