smolnar82 commented on code in PR #681:
URL: https://github.com/apache/knox/pull/681#discussion_r1034635599


##########
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:
   Ack; I'll change that.



##########
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:
   I'll add a more detailed explanation in the code too: `getInitParameters()` 
returns an enumeration and the first time we iterate thru on its element we can 
process the parameter names (`hasMoreElements` returns `true`). The subsequent 
calls, however, will not succeed because `getInitParameters()` returns the same 
object where the `hasMoreElements` returns `false`. In this class, the first 
event is populating virtual group mapping and then we do the same for proxyuser 
config.



##########
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:
   Because my IDE put it there :) It really does not matter, but since it's 
very rare that we use in Knox's codebase I'll remove that from here.



##########
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:
   It's kind of correlating to Hadoop's authentication in a way such as it uses 
the Hadoop authorization library for proxyuser authorization.



-- 
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