mike-jumper commented on code in PR #885: URL: https://github.com/apache/guacamole-client/pull/885#discussion_r1225848813
########## extensions/guacamole-auth-json/src/test/java/org/apache/guacamole/auth/json/RequestValidationServiceTest.java: ########## @@ -0,0 +1,375 @@ +/* + * 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.guacamole.auth.json; + +import java.io.BufferedReader; +import java.security.Principal; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Enumeration; +import java.util.Locale; +import java.util.Map; +import java.util.regex.Pattern; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import javax.servlet.ServletInputStream; +import javax.servlet.RequestDispatcher; +import org.junit.Test; +import static org.junit.Assert.*; + +/** + * Unit test for RequestValidationService. Verifies that only requests + * from trusted hosts are allowed to authenticate. + */ +public class RequestValidationServiceTest { + + private class MockConfigurationService extends ConfigurationService { + + private Collection<String> trustedNetworks; + + public MockConfigurationService() { + trustedNetworks = Collections.<String>emptyList(); + } + + public MockConfigurationService(String trustedNetworks) { + this.trustedNetworks = Arrays.asList(Pattern.compile(",\\s*").split(trustedNetworks)); + } + + public Collection<String> getTrustedNetworks() { + return trustedNetworks; + } + + } + + private RequestValidationService requestService; + + private static HttpServletRequest mockHttpServletRequest(String remoteAddr) { Review Comment: Please document (all functions, classes, and member variables need corresponding JavaDoc, except for functions whose documentation is inherited from a superclass). ########## doc/licenses/spring-framework-3.0.5/README: ########## Review Comment: This is actually used by the (rather unmaintained) JRadius library, which is pulled in only if building of extensions with LGPL-licensed dependencies is enabled via: ``` mvn -Plgpl-extensions package ``` Building with the above will result in the following error: ``` Processing runtime dependencies to produce LICENSE and NOTICE. Output will be within "/home/mjumper/apache/guacamole/guacamole-client/extensions/guacamole-auth-radius/target/licenses". ERROR: License information missing for org.springframework:spring-context:jar:3.0.5.RELEASE ERROR: License information missing for org.springframework:spring-aop:jar:3.0.5.RELEASE ERROR: License information missing for org.springframework:spring-beans:jar:3.0.5.RELEASE ERROR: License information missing for org.springframework:spring-core:jar:3.0.5.RELEASE ERROR: License information missing for org.springframework:spring-expression:jar:3.0.5.RELEASE ERROR: License information missing for org.springframework:spring-asm:jar:3.0.5.RELEASE [ERROR] Command execution failed. org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) ``` ########## extensions/guacamole-auth-json/src/test/java/org/apache/guacamole/auth/json/RequestValidationServiceTest.java: ########## @@ -0,0 +1,375 @@ +/* + * 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.guacamole.auth.json; + +import java.io.BufferedReader; +import java.security.Principal; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Enumeration; +import java.util.Locale; +import java.util.Map; +import java.util.regex.Pattern; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import javax.servlet.ServletInputStream; +import javax.servlet.RequestDispatcher; +import org.junit.Test; +import static org.junit.Assert.*; + +/** + * Unit test for RequestValidationService. Verifies that only requests + * from trusted hosts are allowed to authenticate. + */ +public class RequestValidationServiceTest { + + private class MockConfigurationService extends ConfigurationService { + + private Collection<String> trustedNetworks; + + public MockConfigurationService() { + trustedNetworks = Collections.<String>emptyList(); + } + + public MockConfigurationService(String trustedNetworks) { + this.trustedNetworks = Arrays.asList(Pattern.compile(",\\s*").split(trustedNetworks)); + } + + public Collection<String> getTrustedNetworks() { + return trustedNetworks; + } Review Comment: Inherited and overridden functions should be annotated with `@Override`. ########## extensions/guacamole-auth-json/src/main/java/org/apache/guacamole/auth/json/RequestValidationService.java: ########## @@ -45,6 +45,10 @@ public class RequestValidationService { @Inject private ConfigurationService confService; + public RequestValidationService(ConfigurationService confService) { Review Comment: Please document (all functions must have corresponding JavaDoc). ########## extensions/guacamole-auth-json/src/test/java/org/apache/guacamole/auth/json/RequestValidationServiceTest.java: ########## @@ -0,0 +1,375 @@ +/* + * 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.guacamole.auth.json; + +import java.io.BufferedReader; +import java.security.Principal; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Enumeration; +import java.util.Locale; +import java.util.Map; +import java.util.regex.Pattern; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import javax.servlet.ServletInputStream; +import javax.servlet.RequestDispatcher; +import org.junit.Test; +import static org.junit.Assert.*; + +/** + * Unit test for RequestValidationService. Verifies that only requests + * from trusted hosts are allowed to authenticate. + */ +public class RequestValidationServiceTest { + + private class MockConfigurationService extends ConfigurationService { + + private Collection<String> trustedNetworks; + + public MockConfigurationService() { + trustedNetworks = Collections.<String>emptyList(); + } + + public MockConfigurationService(String trustedNetworks) { + this.trustedNetworks = Arrays.asList(Pattern.compile(",\\s*").split(trustedNetworks)); + } + + public Collection<String> getTrustedNetworks() { + return trustedNetworks; + } + + } + + private RequestValidationService requestService; + + private static HttpServletRequest mockHttpServletRequest(String remoteAddr) { + + return new HttpServletRequest() { + + public Object getAttribute(String name) { + return null; + } Review Comment: Here and elsewhere: `@Override` should be used for any function that overrides/implements a function from a superclass or interface. ########## extensions/guacamole-auth-json/src/test/java/org/apache/guacamole/auth/json/RequestValidationServiceTest.java: ########## @@ -0,0 +1,375 @@ +/* + * 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.guacamole.auth.json; + +import java.io.BufferedReader; +import java.security.Principal; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Enumeration; +import java.util.Locale; +import java.util.Map; +import java.util.regex.Pattern; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import javax.servlet.ServletInputStream; +import javax.servlet.RequestDispatcher; +import org.junit.Test; +import static org.junit.Assert.*; + +/** + * Unit test for RequestValidationService. Verifies that only requests + * from trusted hosts are allowed to authenticate. + */ +public class RequestValidationServiceTest { + + private class MockConfigurationService extends ConfigurationService { + + private Collection<String> trustedNetworks; + + public MockConfigurationService() { + trustedNetworks = Collections.<String>emptyList(); + } + + public MockConfigurationService(String trustedNetworks) { + this.trustedNetworks = Arrays.asList(Pattern.compile(",\\s*").split(trustedNetworks)); + } Review Comment: I'm OK with this as-is, but I suggest instead covering both these cases with just: `String... trustedNetworks`. ########## extensions/guacamole-auth-json/src/main/java/org/apache/guacamole/auth/json/RequestValidationService.java: ########## @@ -77,23 +81,18 @@ public boolean isAuthenticationAllowed(HttpServletRequest request) { return true; } - // Build matchers for each trusted network - Collection<IpAddressMatcher> matchers = new ArrayList<>(trustedNetworks.size()); - for (String network : trustedNetworks) - matchers.add(new IpAddressMatcher(network)); - - // Otherwise ensure at least one subnet matches - for (IpAddressMatcher matcher : matchers) { + // Otherwise ensure that the remote address is part of a trusted network + for (String network : trustedNetworks) { // Request is allowed if any subnet matches - if (matcher.matches(request)) { + if (new IPAddressString(network).contains(new IPAddressString(request.getRemoteAddr()))) { logger.debug("Authentication request from \"{}\" is ALLOWED (matched subnet).", request.getRemoteAddr()); return true; } } - // Otherwise request is denied - no subnets matched + // Otherwise request is denied Review Comment: Is this not 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
