[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider

[GitBox <gi...@apache.org>]

pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r809173808



##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,37 @@
  */
 package org.apache.knox.gateway;
 
+import java.util.Set;
+
 import org.apache.knox.gateway.i18n.messages.Message;
 import org.apache.knox.gateway.i18n.messages.MessageLevel;
 import org.apache.knox.gateway.i18n.messages.Messages;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.SyntaxException;
 
 @Messages(logger="org.apache.knox.gateway")
 public interface IdentityAsserterMessages {
   @Message( level = MessageLevel.ERROR, text = "Required subject/identity not available.  Check authentication/federation provider for proper configuration." )
   void subjectNotAvailable();
+
+  @Message( level = MessageLevel.WARN, text = "Virtual group name is missing after dot character.")
+  void missingVirtualGroupName();
+
+  @Message( level = MessageLevel.WARN, text = "Parse error: {2}. At {0}={1}")
+  void parseError(String key, String script, SyntaxException e);
+
+  @Message( level = MessageLevel.WARN, text = "Invalid result: {2}. Expected boolean when evaluating: {1}. For virtualGroup: {0}")

Review comment:
       My initial feeling about this is that it should be DEBUG rather than WARN.

##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,37 @@
  */
 package org.apache.knox.gateway;
 
+import java.util.Set;
+
 import org.apache.knox.gateway.i18n.messages.Message;
 import org.apache.knox.gateway.i18n.messages.MessageLevel;
 import org.apache.knox.gateway.i18n.messages.Messages;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.SyntaxException;
 
 @Messages(logger="org.apache.knox.gateway")
 public interface IdentityAsserterMessages {
   @Message( level = MessageLevel.ERROR, text = "Required subject/identity not available.  Check authentication/federation provider for proper configuration." )
   void subjectNotAvailable();
+
+  @Message( level = MessageLevel.WARN, text = "Virtual group name is missing after dot character.")
+  void missingVirtualGroupName();
+
+  @Message( level = MessageLevel.WARN, text = "Parse error: {2}. At {0}={1}")

Review comment:
       Assuming this is in reference to the mapping expression param values, perhaps something like "Invalid mapping parameter value: {2}. At {0}={1}"? I would have to see some examples of the parsing error messages though to really comment on this message.

##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,37 @@
  */
 package org.apache.knox.gateway;
 
+import java.util.Set;
+
 import org.apache.knox.gateway.i18n.messages.Message;
 import org.apache.knox.gateway.i18n.messages.MessageLevel;
 import org.apache.knox.gateway.i18n.messages.Messages;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.SyntaxException;
 
 @Messages(logger="org.apache.knox.gateway")
 public interface IdentityAsserterMessages {
   @Message( level = MessageLevel.ERROR, text = "Required subject/identity not available.  Check authentication/federation provider for proper configuration." )
   void subjectNotAvailable();
+
+  @Message( level = MessageLevel.WARN, text = "Virtual group name is missing after dot character.")

Review comment:
       Is this in reference to param names? If so, perhaps something like "Invalid mapping parameter name: Missing required group name."

##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,37 @@
  */
 package org.apache.knox.gateway;
 
+import java.util.Set;
+
 import org.apache.knox.gateway.i18n.messages.Message;
 import org.apache.knox.gateway.i18n.messages.MessageLevel;
 import org.apache.knox.gateway.i18n.messages.Messages;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.SyntaxException;
 
 @Messages(logger="org.apache.knox.gateway")
 public interface IdentityAsserterMessages {
   @Message( level = MessageLevel.ERROR, text = "Required subject/identity not available.  Check authentication/federation provider for proper configuration." )
   void subjectNotAvailable();
+
+  @Message( level = MessageLevel.WARN, text = "Virtual group name is missing after dot character.")

Review comment:
       Did we agree that these are not necessarily "virtual" groups? Should be drop the "virtual" throughout?

##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,37 @@
  */
 package org.apache.knox.gateway;
 
+import java.util.Set;
+
 import org.apache.knox.gateway.i18n.messages.Message;
 import org.apache.knox.gateway.i18n.messages.MessageLevel;
 import org.apache.knox.gateway.i18n.messages.Messages;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.SyntaxException;
 
 @Messages(logger="org.apache.knox.gateway")
 public interface IdentityAsserterMessages {
   @Message( level = MessageLevel.ERROR, text = "Required subject/identity not available.  Check authentication/federation provider for proper configuration." )
   void subjectNotAvailable();
+
+  @Message( level = MessageLevel.WARN, text = "Virtual group name is missing after dot character.")
+  void missingVirtualGroupName();
+
+  @Message( level = MessageLevel.WARN, text = "Parse error: {2}. At {0}={1}")
+  void parseError(String key, String script, SyntaxException e);
+
+  @Message( level = MessageLevel.WARN, text = "Invalid result: {2}. Expected boolean when evaluating: {1}. For virtualGroup: {0}")
+  void invalidResult(String virtualGroupName, Ast ast, Object result);
+
+  @Message( level = MessageLevel.DEBUG, text = "Adding user: {0} to virtual group: {1} using predicate: {2}")
+  void addingUserToVirtualGroup(String username, String virtualGroupName, Ast ast);
+
+  @Message( level = MessageLevel.DEBUG, text = "Not adding user: {0} to virtual group: {1} using predicate: {2}")
+  void notAddingUserToVirtualGroup(String username, String virtualGroupName, Ast ast);

Review comment:
       Do we really need to log this, even at DEBUG level?

##########
File path: gateway-util-common/src/main/java/org/apache/knox/gateway/plang/Ast.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.knox.gateway.plang;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class Ast {

Review comment:
       What is Ast?

##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,37 @@
  */
 package org.apache.knox.gateway;
 
+import java.util.Set;
+
 import org.apache.knox.gateway.i18n.messages.Message;
 import org.apache.knox.gateway.i18n.messages.MessageLevel;
 import org.apache.knox.gateway.i18n.messages.Messages;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.SyntaxException;
 
 @Messages(logger="org.apache.knox.gateway")
 public interface IdentityAsserterMessages {
   @Message( level = MessageLevel.ERROR, text = "Required subject/identity not available.  Check authentication/federation provider for proper configuration." )
   void subjectNotAvailable();
+
+  @Message( level = MessageLevel.WARN, text = "Virtual group name is missing after dot character.")
+  void missingVirtualGroupName();
+
+  @Message( level = MessageLevel.WARN, text = "Parse error: {2}. At {0}={1}")
+  void parseError(String key, String script, SyntaxException e);
+
+  @Message( level = MessageLevel.WARN, text = "Invalid result: {2}. Expected boolean when evaluating: {1}. For virtualGroup: {0}")
+  void invalidResult(String virtualGroupName, Ast ast, Object result);
+
+  @Message( level = MessageLevel.DEBUG, text = "Adding user: {0} to virtual group: {1} using predicate: {2}")
+  void addingUserToVirtualGroup(String username, String virtualGroupName, Ast ast);
+
+  @Message( level = MessageLevel.DEBUG, text = "Not adding user: {0} to virtual group: {1} using predicate: {2}")
+  void notAddingUserToVirtualGroup(String username, String virtualGroupName, Ast ast);
+
+  @Message( level = MessageLevel.DEBUG, text = "Checking user: {0} (with groups: {1}) whether to add virtualGroup: {2} using predicate: {3}")
+  void checkingVirtualGroup(String userName, Set<String> userGroups, String virtualGroupName, Ast ast);
+
+  @Message( level = MessageLevel.INFO, text = "User: {0} (with groups: {1}) added to virtual groups: {2}")

Review comment:
       Do we need this since all the user's groups will be enumerated in the audit log? Well, assuming that these groups DO show up in those log messages.

##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/VirtualGroupMapper.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.knox.gateway.identityasserter.common.filter;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import javax.servlet.ServletRequest;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpSession;
+
+import org.apache.knox.gateway.IdentityAsserterMessages;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.plang.Arity;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Interpreter;
+
+public class VirtualGroupMapper {
+    private final IdentityAsserterMessages LOG = MessagesFactory.get(IdentityAsserterMessages.class);
+    private final Map<String, Ast> virtualGroupToPredicateMap;
+
+    public VirtualGroupMapper(Map<String, Ast> virtualGroupToPredicateMap) {
+        this.virtualGroupToPredicateMap = virtualGroupToPredicateMap;
+    }
+
+    /**
+     *  @return all virtual groups where the corresponding predicate matches
+     */
+    public Set<String> virtualGroupsOfUser(String username, Set<String> groups, ServletRequest request) {

Review comment:
       I would expect a method name like mapGroups(String, Set<String>, ServletRequest) since this class is a GroupMapper.

##########
File path: gateway-util-common/src/main/java/org/apache/knox/gateway/plang/Parser.java
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.knox.gateway.plang;
+
+import java.io.IOException;
+import java.io.PushbackReader;
+import java.io.StringReader;
+
+public class Parser {

Review comment:
       Should all of this parsing-related code go into the gateway-util-common module or its own module?

##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java
##########
@@ -59,6 +74,55 @@ public void init(FilterConfig filterConfig) throws ServletException {
         throw new ServletException("Unable to load principal mapping table.", e);
       }
     }
+    virtualGroupMapper = new VirtualGroupMapper(loadVirtualGroups(filterConfig));
+  }
+
+  private Map<String, Ast> loadVirtualGroups(FilterConfig filterConfig) {
+    Map<String, Ast> predicateToGroupMapping = new HashMap<>();
+    loadVirtualGroupConfig(filterConfig, predicateToGroupMapping);
+    if (predicateToGroupMapping.isEmpty() && filterConfig.getServletContext() != null) {
+      loadVirtualGroupConfig(filterConfig.getServletContext(), predicateToGroupMapping);
+    }
+    if (predicateToGroupMapping.keySet().stream().anyMatch(StringUtils::isBlank)) {
+      LOG.missingVirtualGroupName();
+    }
+    return predicateToGroupMapping;
+  }
+
+  private void loadVirtualGroupConfig(FilterConfig config, Map<String, Ast> result) {
+    for (String paramName : virtualGroupParameterNames(config.getInitParameterNames())) {

Review comment:
       Seems like unnecessarily iterating over the group mapping params twice; It's not clear how much value is added by the virtualGroupParameterNames() method.




-- 
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-unsubscribe@knox.apache.org

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