You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2022/02/16 11:57:44 UTC
[GitHub] [knox] zeroflag opened a new pull request #537: KNOX-2707 - POC
zeroflag opened a new pull request #537:
URL: https://github.com/apache/knox/pull/537
## What changes were proposed in this pull request?
Sharing a proof of concept implementation of the language and the filter.
## How was this patch tested?
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r812346700
##########
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:
Ok, I can buy that argument.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r813792217
##########
File path: gateway-provider-identity-assertion-common/src/test/java/org/apache/knox/gateway/identityasserter/common/filter/VirtualGroupMapperTest.java
##########
@@ -0,0 +1,120 @@
+/*
+ * 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 static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+import static org.junit.Assert.assertEquals;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Parser;
+import org.junit.Test;
+
+@SuppressWarnings("PMD.NonStaticInitializer")
+public class VirtualGroupMapperTest {
Review comment:
I added a new one.
--
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
[GitHub] [knox] zeroflag commented on pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on pull request #537:
URL: https://github.com/apache/knox/pull/537#issuecomment-1049771263
> It's difficult for me to tell, but is there any testing of the combined (old/new) mapping configuration syntax?
Yes, in the `CommonIdentityAssertionFilterTest` there is one for testing the combination of both. Testing this feature at the Filter level is cumbersome because of the infrastructure related code and dependencies (servlet api and such), so the more extensive tests are in `VirtualGroupMapperTests`, `ParserTest` and `InterpreterTest`. Those are more focused and easier to write tests for.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811837591
##########
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:
I think this is a serious, but unlikely problem. So the predicate should always evaluate to a boolean value.
For example:
```(= username 'admin')```
But if the user specifies an expression that returns a string or a number, we won't know what to do and whether we should put the user into the virtual group or not.
--
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
[GitHub] [knox] lmccay commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r812251950
##########
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:
I'd like to see it as reusable as possible. I see this AST language approach being applicable across multiple provider types: identity assertion for group mapping, a new authorization provider for richer ACLs/rules, perhaps a new provider type for routing or being used within existing dispatches for the same. I don't know that the initial implementation needs to account for this though as we can refactor later. Putting into gateway-util-common will go a long way though. It will also make it available in a lot of other consumer modules without having to add a new dependency.
--
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
[GitHub] [knox] zeroflag edited a comment on pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag edited a comment on pull request #537:
URL: https://github.com/apache/knox/pull/537#issuecomment-1049771263
> It's difficult for me to tell, but is there any testing of the combined (old/new) mapping configuration syntax?
Yes, in the `CommonIdentityAssertionFilterTest` there is one for testing the combination of both (where we map the principal to another one and use that one from the predicate). Testing this feature at the Filter level is cumbersome because of the infrastructure related code and dependencies (servlet api and such), so the more extensive tests are in `VirtualGroupMapperTests`, `ParserTest` and `InterpreterTest`. Those are more focused and easier to write tests for.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814266523
##########
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 {
+ private final List<Ast> children = new ArrayList<>();
+ private final String token;
+
+ public Ast(String token) {
+ this.token = token;
+ }
+
+ public void addChild(Ast child) {
+ children.add(child);
+ }
+
+ @Override
+ public String toString() {
+ return isAtom() ? token : children.toString();
+ }
+
+ public String token() {
+ return token;
+ }
+
+ public boolean isStr() {
+ return token != null && token.startsWith("'") && token.endsWith("'");
+ }
+
+ public boolean isNumber() {
+ if (token == null) {
+ return false;
+ }
+ try {
+ numValue();
+ return true;
+ } catch (NumberFormatException e) {
+ return false;
+ }
+ }
+
+ public Number numValue() {
+ try {
+ return Long.parseLong(token);
+ } catch (NumberFormatException e) {
+ return Double.parseDouble(token);
+ }
+ }
+
+ public String strValue() {
+ return token.substring(1, token.length() -1);
Review comment:
Ok, I can add a check and throw an exception if it's not a string. There is already a check at the outside so it's not strictly necessary but I understand the concern.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814273246
##########
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 {
+ private final List<Ast> children = new ArrayList<>();
+ private final String token;
+
+ public Ast(String token) {
+ this.token = token;
+ }
+
+ public void addChild(Ast child) {
+ children.add(child);
+ }
+
+ @Override
+ public String toString() {
+ return isAtom() ? token : children.toString();
+ }
+
+ public String token() {
+ return token;
+ }
+
+ public boolean isStr() {
+ return token != null && token.startsWith("'") && token.endsWith("'");
+ }
+
+ public boolean isNumber() {
+ if (token == null) {
Review comment:
Interesting. I like to get over the easy checks (like if the argument is null) first and do an early return if something is trivially invalid. Then everything below that can expect valid arguments. You would probably like Lisp since it's expression based (there are no statements - including explicit `return` - in it). :)
I'll change this too.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r812316559
##########
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:
The parser covers the full syntax, it is unlikely we need to change that unless we want to add new literal syntax for something (e.g.: set/map/vector). So I think it's both stable and reusable. The interpreter is extensible as it allows us to add functions from the outside depending on the use case. We can keep it under gateway-util-common for now and extract later only if needed.
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r812350963
##########
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:
Perhaps, renaming the class to AbstractSyntaxtTree and/or some javadoc would be helpful?
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811837591
##########
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:
I think this is a serious, but unlikely problem. So the predicate should always evaluate to a boolean value.
For example:
```(= username 'admin')```
But if the user specifies an expression that returns a string or a number, we won't know what to to and whether we should put the user into the virtual group or not.
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814269836
##########
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 {
+ private final List<Ast> children = new ArrayList<>();
+ private final String token;
+
+ public Ast(String token) {
+ this.token = token;
+ }
+
+ public void addChild(Ast child) {
+ children.add(child);
+ }
+
+ @Override
+ public String toString() {
+ return isAtom() ? token : children.toString();
+ }
+
+ public String token() {
+ return token;
+ }
+
+ public boolean isStr() {
+ return token != null && token.startsWith("'") && token.endsWith("'");
+ }
+
+ public boolean isNumber() {
+ if (token == null) {
+ return false;
+ }
+ try {
+ numValue();
+ return true;
+ } catch (NumberFormatException e) {
+ return false;
+ }
+ }
+
+ public Number numValue() {
+ try {
+ return Long.parseLong(token);
+ } catch (NumberFormatException e) {
+ return Double.parseDouble(token);
+ }
+ }
+
+ public String strValue() {
+ return token.substring(1, token.length() -1);
Review comment:
I noticed that, and given the current usage, it's not concerning, but strValue() is a public method which could be called by anyone in theory, so better to be safer in that case IMO.
--
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
[GitHub] [knox] zeroflag merged pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag merged pull request #537:
URL: https://github.com/apache/knox/pull/537
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811839946
##########
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:
Yes, audit logs shows the end result. This is only useful for showing the original groups as well. How about changing it to DEBUG?
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811858355
##########
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:
Good questions. We can extract in into its own module. Although it's only few hundred lines of code. We also need to come up with a good name for the module if we choose to extract it out.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r813791683
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java
##########
@@ -17,30 +17,45 @@
*/
package org.apache.knox.gateway.identityasserter.common.filter;
+import java.io.IOException;
+import java.security.AccessController;
+import java.util.ArrayList;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
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 org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
import org.apache.knox.gateway.IdentityAsserterMessages;
import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Parser;
+import org.apache.knox.gateway.plang.SyntaxException;
+import org.apache.knox.gateway.security.GroupPrincipal;
import org.apache.knox.gateway.security.principal.PrincipalMappingException;
import org.apache.knox.gateway.security.principal.SimplePrincipalMapper;
-import java.io.IOException;
-import java.security.AccessController;
-
public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilter {
+ public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "virtual.group.mapping.";
Review comment:
I removed the virtual prefix, now it's just "group.mapping"
The provider supports the following:
* group.principal.mapping -> kminder=group1;lmccay=mrgroup,mrducks
* principal.mapping -> lmccay,kminder=hdfs;newuser=mapred
* group.mapping.\<group-name\> -> (username 'admin')
The 3rd one is the new one.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814261998
##########
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:
Ok, I'll rename it.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811831331
##########
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:
We put the users into these groups based on their LDAP/OS level groups. If we refer both of the virtual groups and LDAP/OS groups simply as groups, it can be confusing, isn't it? I'm not sure what would be the proper naming.
> Is this in reference to param names? If so, perhaps something like "Invalid mapping parameter name: Missing required group name."
Ok. I changed the the message.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811845186
##########
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:
Ok, renamed it.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811844702
##########
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:
This happens only in deployment time once, so from performance point of view it's not a big problem.
At first we try to load the groups from the FilterConfig then from the ServletContext (this is how we load other properties from other filters too).
Unfortunately FilterConfig and ServletContext doesn't have the same interface, that's why we have this duplication. The `virtualGroupParameterNames` was an attempt to reduce the duplication so at least the `if` statement is not duplicated in both methods.
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r812348933
##########
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:
DEBUG would be a good compromise.
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r812348613
##########
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:
I guess with the referenced context (e.g., mapping name, predicate and actual result), that works.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811858355
##########
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:
Good question. We can extract in into its own module. Although it is only few hundred lines of code. We also need to come up with a good name for the module if we choose to extract it out.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811838405
##########
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:
Right, this is probably overkill. It was helpful during testing and development but no longer needed. I'll remove it.
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814122509
##########
File path: gateway-provider-identity-assertion-common/src/test/java/org/apache/knox/gateway/identityasserter/filter/CommonIdentityAssertionFilterTest.java
##########
@@ -74,6 +76,7 @@ public String mapUserPrincipal(String principalName) {
return super.combineGroupMappings(mappedGroups, groups);
}
};
+ ThreadContext.put(MDC_AUDIT_CONTEXT_KEY, "dummy");
Review comment:
I'm assuming this is for backward-compatibility. Is that correct?
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814262730
##########
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 {
+ private final List<Ast> children = new ArrayList<>();
+ private final String token;
+
+ public Ast(String token) {
+ this.token = token;
+ }
+
+ public void addChild(Ast child) {
+ children.add(child);
+ }
+
+ @Override
+ public String toString() {
+ return isAtom() ? token : children.toString();
+ }
+
+ public String token() {
+ return token;
+ }
+
+ public boolean isStr() {
+ return token != null && token.startsWith("'") && token.endsWith("'");
Review comment:
Yes, that's an empty string. It is allowed like in other languages.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811838405
##########
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:
Right, this is probably overkill. I was helpful during testing and development but no longer needed. I'll remove it.
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by 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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r813791683
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java
##########
@@ -17,30 +17,45 @@
*/
package org.apache.knox.gateway.identityasserter.common.filter;
+import java.io.IOException;
+import java.security.AccessController;
+import java.util.ArrayList;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
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 org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
import org.apache.knox.gateway.IdentityAsserterMessages;
import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Parser;
+import org.apache.knox.gateway.plang.SyntaxException;
+import org.apache.knox.gateway.security.GroupPrincipal;
import org.apache.knox.gateway.security.principal.PrincipalMappingException;
import org.apache.knox.gateway.security.principal.SimplePrincipalMapper;
-import java.io.IOException;
-import java.security.AccessController;
-
public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilter {
+ public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "virtual.group.mapping.";
Review comment:
I removed the virtual prefix, now it's just "group.mapping"
The provider supports the following:
* group.principal.mapping -> kminder=group1;lmccay=mrgroup,mrducks
* principal.mapping -> lmccay,kminder=hdfs;newuser=mapred
* group.mapping.<group-name> -> (username 'admin')
The 3rd one is the new one.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814151484
##########
File path: gateway-provider-identity-assertion-common/src/test/java/org/apache/knox/gateway/identityasserter/filter/CommonIdentityAssertionFilterTest.java
##########
@@ -74,6 +76,7 @@ public String mapUserPrincipal(String principalName) {
return super.combineGroupMappings(mappedGroups, groups);
}
};
+ ThreadContext.put(MDC_AUDIT_CONTEXT_KEY, "dummy");
Review comment:
The test fails with an NPE if there is no AuditContext (auditService.getContext() returns null). It is just for making the test work.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r812796744
##########
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:
I changed it to DEBUG.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r813792540
##########
File path: gateway-provider-identity-assertion-common/src/test/java/org/apache/knox/gateway/identityasserter/common/filter/VirtualGroupMapperTest.java
##########
@@ -0,0 +1,120 @@
+/*
+ * 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 static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+import static org.junit.Assert.assertEquals;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Parser;
+import org.junit.Test;
+
+@SuppressWarnings("PMD.NonStaticInitializer")
+public class VirtualGroupMapperTest {
Review comment:
I added a new one.
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r814235328
##########
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 {
+ private final List<Ast> children = new ArrayList<>();
+ private final String token;
+
+ public Ast(String token) {
+ this.token = token;
+ }
+
+ public void addChild(Ast child) {
+ children.add(child);
+ }
+
+ @Override
+ public String toString() {
+ return isAtom() ? token : children.toString();
+ }
+
+ public String token() {
+ return token;
+ }
+
+ public boolean isStr() {
+ return token != null && token.startsWith("'") && token.endsWith("'");
Review comment:
Are tokens allowed to be empty (i.e., '')?
##########
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 {
+ private final List<Ast> children = new ArrayList<>();
+ private final String token;
+
+ public Ast(String token) {
+ this.token = token;
+ }
+
+ public void addChild(Ast child) {
+ children.add(child);
+ }
+
+ @Override
+ public String toString() {
+ return isAtom() ? token : children.toString();
+ }
+
+ public String token() {
+ return token;
+ }
+
+ public boolean isStr() {
+ return token != null && token.startsWith("'") && token.endsWith("'");
+ }
+
+ public boolean isNumber() {
+ if (token == null) {
Review comment:
I personally prefer single return statements whenever possible.
```suggestion
boolean result = false;
if (token !=null) {
try {
numValue();
result = true;
} catch (NumberFormatException e) {
// NaN
}
}
return result;
##########
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 {
+ private final List<Ast> children = new ArrayList<>();
+ private final String token;
+
+ public Ast(String token) {
+ this.token = token;
+ }
+
+ public void addChild(Ast child) {
+ children.add(child);
+ }
+
+ @Override
+ public String toString() {
+ return isAtom() ? token : children.toString();
+ }
+
+ public String token() {
+ return token;
+ }
+
+ public boolean isStr() {
+ return token != null && token.startsWith("'") && token.endsWith("'");
+ }
+
+ public boolean isNumber() {
+ if (token == null) {
+ return false;
+ }
+ try {
+ numValue();
+ return true;
+ } catch (NumberFormatException e) {
+ return false;
+ }
+ }
+
+ public Number numValue() {
+ try {
+ return Long.parseLong(token);
+ } catch (NumberFormatException e) {
+ return Double.parseDouble(token);
+ }
+ }
+
+ public String strValue() {
+ return token.substring(1, token.length() -1);
Review comment:
Would it be overkill to wrap this in a call to isStr() for validation?
##########
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:
I would still like to see this class name spelled out as AbstractSyntaxtTree.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811854091
##########
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:
AST stands for Abstract Syntax Tree, it is language implementation terminology. The parser produces the AST, which is a tree structure that represents the source code. In our cases it's list of lists (which is also a tree).
Fr example this is the text:
```
(or true (and false true))
```
This is the AST emitted by the parser. `See the AST>>toString`
```
[or, true, [and, false, true]]
```
It's a homoiconic language so there is 1 to 1 mapping between the text and the AST. It has the exact same structure, we just put everything into Java types, like `java.util.ArrayLists` and `java.lang.Booleans`.
We could have represented the AST as arbitrarily nested a `java.util.List`, but the Java's type system doesn't handle this nesting very well: `List<List<List<..> or Atom> or Atom>`, that's why we have a dedicated class.
The interpreter gets the AST and evaluates it. In our case the interpreter is a simple tree walking interpreter so it just recursively evaluates the tree.
In the filter we store the AST not the text, so every time a request comes in, we just need to interpret, without using the parser.
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r811834368
##########
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:
Few parsing error message examples:
* unterminated string (when someone forgets the closing ')
* Unexpected closing (when there are more closing parentheses then open parentheses)
* Missing closing ) (when there are more open parentheses then closing parentheses)
--
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
[GitHub] [knox] pzampino commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r813186144
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,34 @@
*/
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 = "Invalid mapping parameter name: Missing required group name.")
+ void missingVirtualGroupName();
+
+ @Message( level = MessageLevel.WARN, text = "Invalid mapping, 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 = "Checking user: {0} (with groups: {1}) whether to add virtualGroup: {2} using predicate: {3}")
Review comment:
Suggestion: "Checking whether user {0} (with group(s) {1}) should be added to group {2} based on predicate {3}"
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,34 @@
*/
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 = "Invalid mapping parameter name: Missing required group name.")
+ void missingVirtualGroupName();
+
+ @Message( level = MessageLevel.WARN, text = "Invalid mapping, 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:
Suggestion: "Invalid result: {2}. Expected boolean when evaluating group {0} mapping value {1}."
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,34 @@
*/
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 = "Invalid mapping parameter name: Missing required group name.")
+ void missingVirtualGroupName();
+
+ @Message( level = MessageLevel.WARN, text = "Invalid mapping, 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}")
Review comment:
Suggestion: "Adding user {0} to group {1} based on predicate {2}"
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,34 @@
*/
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 = "Invalid mapping parameter name: Missing required group name.")
+ void missingVirtualGroupName();
+
+ @Message( level = MessageLevel.WARN, text = "Invalid mapping, parse error: {2}. At {0}={1}")
Review comment:
Suggestion: "Invalid mapping {0}={1}, Parse error: {2}"
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/IdentityAsserterMessages.java
##########
@@ -17,12 +17,34 @@
*/
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 = "Invalid mapping parameter name: Missing required group name.")
+ void missingVirtualGroupName();
+
+ @Message( level = MessageLevel.WARN, text = "Invalid mapping, 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 = "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.DEBUG, text = "User: {0} (with groups: {1}) added to virtual groups: {2}")
Review comment:
Suggestion: "User {0} (with group(s) {1}) added to group(s) {2}"
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java
##########
@@ -17,30 +17,45 @@
*/
package org.apache.knox.gateway.identityasserter.common.filter;
+import java.io.IOException;
+import java.security.AccessController;
+import java.util.ArrayList;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
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 org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
import org.apache.knox.gateway.IdentityAsserterMessages;
import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Parser;
+import org.apache.knox.gateway.plang.SyntaxException;
+import org.apache.knox.gateway.security.GroupPrincipal;
import org.apache.knox.gateway.security.principal.PrincipalMappingException;
import org.apache.knox.gateway.security.principal.SimplePrincipalMapper;
-import java.io.IOException;
-import java.security.AccessController;
-
public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilter {
+ public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "virtual.group.mapping.";
Review comment:
I thought we agreed to remove the "virtual" language from the configuration. Did we revert that agreement?
I think it's fine in the code, but from a configuration-perspective, is there anything preventing mapping a user to a "real" group with this mechanism?
##########
File path: gateway-provider-identity-assertion-common/src/test/java/org/apache/knox/gateway/identityasserter/common/filter/VirtualGroupMapperTest.java
##########
@@ -0,0 +1,120 @@
+/*
+ * 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 static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+import static org.junit.Assert.assertEquals;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Parser;
+import org.junit.Test;
+
+@SuppressWarnings("PMD.NonStaticInitializer")
+public class VirtualGroupMapperTest {
Review comment:
Would some negative test cases add value here?
--
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
[GitHub] [knox] zeroflag commented on a change in pull request #537: KNOX-2707 - Virtual Group Mapping Provider
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #537:
URL: https://github.com/apache/knox/pull/537#discussion_r813791683
##########
File path: gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java
##########
@@ -17,30 +17,45 @@
*/
package org.apache.knox.gateway.identityasserter.common.filter;
+import java.io.IOException;
+import java.security.AccessController;
+import java.util.ArrayList;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
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 org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
import org.apache.knox.gateway.IdentityAsserterMessages;
import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.plang.Ast;
+import org.apache.knox.gateway.plang.Parser;
+import org.apache.knox.gateway.plang.SyntaxException;
+import org.apache.knox.gateway.security.GroupPrincipal;
import org.apache.knox.gateway.security.principal.PrincipalMappingException;
import org.apache.knox.gateway.security.principal.SimplePrincipalMapper;
-import java.io.IOException;
-import java.security.AccessController;
-
public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilter {
+ public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "virtual.group.mapping.";
Review comment:
I removed the virtual prefix, now it's just "group.mapping"
The provider supports the following:
* group.principal.mapping -> kminder=group1;lmccay=mrgroup,mrducks
* principal.mapping -> lmccay,kminder=hdfs;newuser=mapred
* group.mapping -> (username 'admin')
The 3rd one is the new one.
--
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