You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2021/03/23 19:35:37 UTC
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #511: GUACAMOLE-1025: Allow QuickConnect module to configure allowed and/or denied parameters
mike-jumper commented on a change in pull request #511:
URL: https://github.com/apache/guacamole-client/pull/511#discussion_r599873743
##########
File path: extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java
##########
@@ -118,5 +136,54 @@ public String create(GuacamoleConfiguration config) throws GuacamoleException {
return newConnectionId;
}
+
+ /**
+ * Checks the provided GuacamoleConfiguration to make sure the parameters
+ * in the configuration are allowed by the configuration. If the
+ * allowed parameter configuration option is set, all parameters not
+ * specified will be removed from the config. If the denied parameter
+ * configuration option is set, any parameters specified by that will be
+ * removed from the configuration. If neither are set then all parameters
+ * are allowed.
+ *
+ * @param config
+ * The GuacamoleConfiguration containing parameters to check against
+ * configured allowed and denied values.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ private void checkConfigParameters(GuacamoleConfiguration config) throws GuacamoleException {
Review comment:
I think `checkConfigParameters()` here is a misnomer. The function doesn't just check whether parameters are allowed, but rather strips away any parameters that are denied or not allowed.
##########
File path: extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/conf/StringListProperty.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.quickconnect.conf;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.regex.Pattern;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.properties.GuacamoleProperty;
+
+/**
+ * A GuacamoleProperty whose value is a List of Strings. The string value
+ * parsed to produce this list is a comma-delimited list. Duplicate values are
+ * ignored, as is any whitespace following delimiters. To maintain
+ * compatibility with the behavior of Java properties in general, only
+ * whitespace at the beginning of each value is ignored; trailing whitespace
+ * becomes part of the value.
+ */
+public abstract class StringListProperty implements GuacamoleProperty<List<String>> {
Review comment:
I believe this is now part of guacamole-ext.
##########
File path: extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/conf/ConfigurationService.java
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.quickconnect.conf;
+
+import com.google.inject.Inject;
+import java.util.List;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+
+/**
+ * Configuration options to control the QuickConnect module.
+ */
+public class ConfigurationService {
+
+ @Inject
+ private Environment environment;
Review comment:
Please add JavaDoc.
##########
File path: extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java
##########
@@ -118,5 +136,54 @@ public String create(GuacamoleConfiguration config) throws GuacamoleException {
return newConnectionId;
}
+
+ /**
+ * Checks the provided GuacamoleConfiguration to make sure the parameters
+ * in the configuration are allowed by the configuration. If the
+ * allowed parameter configuration option is set, all parameters not
+ * specified will be removed from the config. If the denied parameter
+ * configuration option is set, any parameters specified by that will be
+ * removed from the configuration. If neither are set then all parameters
+ * are allowed.
+ *
+ * @param config
+ * The GuacamoleConfiguration containing parameters to check against
+ * configured allowed and denied values.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ private void checkConfigParameters(GuacamoleConfiguration config) throws GuacamoleException {
+
+ // Get configuration items.
+ List<String> allowedParams = confService.getAllowedParameters();
+ List<String> deniedParams = confService.getDeniedParameters();
+
+ // Loop through parameters and remove them if they are not allowed.
+ Set<String> setParams = new HashSet<>(config.getParameterNames());
+ for (String key : setParams) {
+
+ if (allowedParams != null
+ && !allowedParams.isEmpty()
+ && !allowedParams.contains(key)) {
+
+ logger.debug("Parameter \"{}\" is not allowed and will be removed.",
+ key);
+ config.unsetParameter(key);
+
+ }
+
+ if (deniedParams != null
+ && !deniedParams.isEmpty()
+ && deniedParams.contains(key)) {
+
+ logger.debug("Parameter \"{}\" has been denied and will be removed.",
+ key);
+ config.unsetParameter(key);
+
+ }
+ }
Review comment:
`config.getParameters().keySet().retainAll()` and `config.getParameters().keySet().removeAll()` would be easier than manually looping through the map.
##########
File path: extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/utility/QCParser.java
##########
@@ -184,26 +178,39 @@ public static GuacamoleConfiguration getConfiguration(String uri)
* The GuacamoleConfiguration object to store the username
* and password in.
*
- * @throws UnsupportedEncodingException
- * If Java lacks UTF-8 support.
+ * @throws GuacamoleException
+ * If an error occurs checking if the parameter is allowed, or if
+ * Java unexpectedly lacks UTF-8 support.
*/
public static void parseUserInfo(String userInfo,
GuacamoleConfiguration config)
- throws UnsupportedEncodingException {
+ throws GuacamoleException {
Matcher userinfoMatcher = userinfoPattern.matcher(userInfo);
if (userinfoMatcher.matches()) {
String username = userinfoMatcher.group(USERNAME_GROUP);
String password = userinfoMatcher.group(PASSWORD_GROUP);
- if (username != null && !username.isEmpty())
+ if (username != null && !username.isEmpty()) {
+ try {
config.setParameter("username",
URLDecoder.decode(username, "UTF-8"));
+ }
Review comment:
Indentation here somehow became wonky.
##########
File path: extensions/guacamole-auth-quickconnect/pom.xml
##########
@@ -191,6 +191,18 @@
<version>1.2.0</version>
<scope>provided</scope>
</dependency>
+
+ <!-- Guice -->
+ <dependency>
+ <groupId>com.google.inject</groupId>
+ <artifactId>guice</artifactId>
+ <version>3.0</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.inject.extensions</groupId>
+ <artifactId>guice-multibindings</artifactId>
+ <version>3.0</version>
+ </dependency>
Review comment:
Please be sure to add the licenses for these new dependencies to the set of licenses bundled with the extension.
##########
File path: extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java
##########
@@ -118,5 +136,54 @@ public String create(GuacamoleConfiguration config) throws GuacamoleException {
return newConnectionId;
}
+
+ /**
+ * Checks the provided GuacamoleConfiguration to make sure the parameters
+ * in the configuration are allowed by the configuration. If the
+ * allowed parameter configuration option is set, all parameters not
+ * specified will be removed from the config. If the denied parameter
+ * configuration option is set, any parameters specified by that will be
+ * removed from the configuration. If neither are set then all parameters
+ * are allowed.
+ *
+ * @param config
+ * The GuacamoleConfiguration containing parameters to check against
+ * configured allowed and denied values.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ private void checkConfigParameters(GuacamoleConfiguration config) throws GuacamoleException {
Review comment:
Should this instead be part of `QCParser` (and tested via `QCParserTest`)? Comments within `QCParser` look like this may have originally been intended, and it feels like ensuring `QCParser` always abides by the set of allowed/denied parameters would be a safer approach
##########
File path: extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/conf/ConfigurationService.java
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.quickconnect.conf;
+
+import com.google.inject.Inject;
+import java.util.List;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+
+/**
+ * Configuration options to control the QuickConnect module.
+ */
+public class ConfigurationService {
+
+ @Inject
+ private Environment environment;
+
+ /**
+ * A list of parameters that, if set, will limit the parameters allowed to
+ * be defined by connections created using the QuickConnect module to only
+ * the parameters defined in this list. Defaults to null (all parameters
+ * are allowed).
+ */
+ public static final StringListProperty QUICKCONNECT_ALLOWED_PARAMETERS = new StringListProperty() {
+
+ @Override
+ public String getName() { return "quickconnect-allowed-parameters"; }
+
+ };
+
+ /**
+ * A list of parameters that, if set, will limit the parameters allowed to
+ * be defined by connections created using the QuickConnect module to any
+ * except the ones defined in this list. Defaults to null (all parameters
+ * are allowed).
+ */
+ public static final StringListProperty QUICKCONNECT_DENIED_PARAMETERS = new StringListProperty() {
+
+ @Override
+ public String getName() { return "quickconnect-denied-parameters"; }
+
+ };
+
+ /**
+ * Return the list of allowed parameters to be set by connections created
+ * using the QuickConnect module, or null if none are defined (thereby
+ * allowing all parameters to be set).
+ *
+ * @return
+ * The list of allowed parameters to be set by connections crated using
+ * the QuickConnect module.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public List<String> getAllowedParameters() throws GuacamoleException {
+ return environment.getProperty(QUICKCONNECT_ALLOWED_PARAMETERS);
+ }
+
+ /**
+ * Return the list of denied parameters for connections created using the
+ * QuickConnect module, or null if none are defined (thereby allowing all
+ * parameters to be set).
+ *
+ * @return
+ * THe list of parameters that cannot be set by connections created
Review comment:
The*
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org