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 2020/11/24 09:17:09 UTC
[GitHub] [guacamole-client] mike-jumper opened a new pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
mike-jumper opened a new pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578
Previously, the underlying protocol of an active connection was available only via the connection definition itself. That information is not available if the user is connecting through a connection group or a shared connection, causing handling of the `argv` and `required` instructions to fail in those cases.
These changes establish a REST API endpoint for retrieving underlying protocol unformation from the tunnel, independent of whether a connection, connection group, or shared connection was used. This involved:
* Adding a `onuuid` event to `Guacamole.Tunnel` to allow implementations to reliably wait for assignment of the tunnel UUID.
* Adding a `getProtocol()` function to the `GuacamoleSocket` interface, defaulting to unknown (`null`). This required upgrading the guacamole-common build to Java 1.8.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#issuecomment-733166788
Converted this PR to a draft - I'd like to investigate the protocol-driven approach.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman commented on a change in pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#discussion_r530354598
##########
File path: guacamole-common/pom.xml
##########
@@ -57,14 +57,14 @@
<build>
<plugins>
- <!-- Written for 1.6 -->
+ <!-- Written for 1.8 -->
Review comment:
<snif, snif>
Alas, poor Java 6.
##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AbstractGuacamoleTunnelService.java
##########
@@ -201,87 +201,54 @@ protected abstract void release(RemoteAuthenticatedUser user,
ModeledConnectionGroup connectionGroup);
/**
- * Returns a guacamole configuration containing the protocol and parameters
- * from the given connection. If the ID of an active connection is
- * provided, that connection will be joined instead of starting a new
- * primary connection. If tokens are used in the connection parameter
- * values, credentials from the given user will be substituted
- * appropriately.
- *
- * @param user
- * The user whose credentials should be used if necessary.
+ * Returns a GuacamoleConfiguration which connects to the given connection.
+ * If the connection ID of an active is provided, that active connection
+ * will be joined rather than establishing an entirely new connection. If
+ * a sharing profile is provided, the parameters associated with that
+ * sharing profile will be used to define the access provided to the user
+ * accessing the shared connection. If tokens are used in the connection
+ * parameter values of the sharing profile, credentials from the given user
+ * will be substituted appropriately.
Review comment:
A couple of minor issues with the wording, here:
* This makes it sounds like tokens will only be substituted for sharing profiles and not for normal connections. But, looking at the code, it looks like this is true of either case?
* It also makes it sound like tokens would only apply to credentials? But this would be true of any connection parameter or token (date/time, LDAP attributes, etc.), 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] knacktim commented on pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#issuecomment-732997116
Cherry-picked these into my staging/1.3.0 testing branch and all seems well.
Just for my own knowledge -- is there a reason that we could not just send the protocol and form information through the websocket-tunnel? Could that cause race conditions?
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#discussion_r530603678
##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AbstractGuacamoleTunnelService.java
##########
@@ -201,87 +201,54 @@ protected abstract void release(RemoteAuthenticatedUser user,
ModeledConnectionGroup connectionGroup);
/**
- * Returns a guacamole configuration containing the protocol and parameters
- * from the given connection. If the ID of an active connection is
- * provided, that connection will be joined instead of starting a new
- * primary connection. If tokens are used in the connection parameter
- * values, credentials from the given user will be substituted
- * appropriately.
- *
- * @param user
- * The user whose credentials should be used if necessary.
+ * Returns a GuacamoleConfiguration which connects to the given connection.
+ * If the connection ID of an active is provided, that active connection
+ * will be joined rather than establishing an entirely new connection. If
+ * a sharing profile is provided, the parameters associated with that
+ * sharing profile will be used to define the access provided to the user
+ * accessing the shared connection. If tokens are used in the connection
+ * parameter values of the sharing profile, credentials from the given user
+ * will be substituted appropriately.
Review comment:
Right - that wording is wrong as of commit 1210d5624c4eb173417cab8358eca4cc3b6c0ebe (which deprecated the `StandardTokens` class). This function used to perform token substitution, but no longer needs to and hasn't since that commit.
I've now removed that "If tokens are used ..." sentence.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#issuecomment-733162615
> Just for my own knowledge -- is there a reason that we could not just send the protocol and form information through the websocket-tunnel? Could that cause race conditions?
No, the reason that forms cannot be exposed automaticlaly as part of the tunnel is separation of concerns. The tunnel exists as a low-level component defined by guacamole-common and guacamole-common-js, whereas the concept of fields and forms are specific to the Guacamole webapp and its extension API (guacamole-ext). Only the protocol name and arbitrary parameter name/value pairs exist at that low a level.
Before deciding on the current implementation, I also considered:
* **Allowing `ConnectionGroup` and `ActiveConnection` to expose a `GuacamoleConfiguration`, possibly by moving the relevant function from `Connection` to `Connectable`.** This would have required more substantial changes, and would added further complexity to the part of the client-side code that pulls this information.
* **Modifying the `argv` and `required` instructions to expose the protocol name.** This feels redundant and would break backward compatibility for the established `argv` instruction.
Adding a new instruction that simply exposes the protocol name could be another approach.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#discussion_r530603923
##########
File path: guacamole-common/pom.xml
##########
@@ -57,14 +57,14 @@
<build>
<plugins>
- <!-- Written for 1.6 -->
+ <!-- Written for 1.8 -->
Review comment:
😿
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman commented on a change in pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#discussion_r530614189
##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AbstractGuacamoleTunnelService.java
##########
@@ -201,87 +201,52 @@ protected abstract void release(RemoteAuthenticatedUser user,
ModeledConnectionGroup connectionGroup);
/**
- * Returns a guacamole configuration containing the protocol and parameters
- * from the given connection. If the ID of an active connection is
- * provided, that connection will be joined instead of starting a new
- * primary connection. If tokens are used in the connection parameter
- * values, credentials from the given user will be substituted
- * appropriately.
- *
- * @param user
- * The user whose credentials should be used if necessary.
+ * Returns a GuacamoleConfiguration which connects to the given connection.
+ * If the connection ID of an active is provided, that active connection
Review comment:
> If the connection ID of an active *connection* is provided
Although, the term "active connection" is used several times in rapid succession, so maybe:
> If the ID of an active connection is provided
would make it a little more succinct.
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleConfiguration.java
##########
@@ -103,15 +102,34 @@ public void setConnectionID(String connectionID) {
/**
* Returns the name of the protocol to be used.
- * @return The name of the protocol to be used.
+ *
+ * @return
+ * The name of the protocol to be used.
*/
public String getProtocol() {
return protocol;
}
/**
- * Sets the name of the protocol to be used.
- * @param protocol The name of the protocol to be used.
+ * Sets the name of the protocol to be used. If no connection is being
+ * joined (a new connection is being established), this value must be set.
+ *
+ * <p>If a connection is being joined, <strong>this value should still be
+ * set</strong> to ensure that protocol-specific responses like the
+ * "required" and "argv" instructions can be understood in their proper
Review comment:
Extra space slipped in here...
##########
File path: guacamole/src/main/webapp/app/rest/services/tunnelService.js
##########
@@ -79,6 +79,36 @@ angular.module('rest').factory('tunnelService', ['$injector',
};
+ /**
+ * Makes a request to the REST API to retrieve the underlying protocol of
+ * the connection associated with a particular tunnel, returning a promise
+ * that provides a @link{Protocol} object if successful.
+ *
+ * @param {String} tunnel
+ * The UUID of the tunnel associated with the Guacamole connection
+ * whose underlying protocol is being retrieved.
+ *
+ * @returns {Promise.<Protocol>}
+ * A promise which will resolve with a @link{Protocol} object upon
+ * success.
+ */
+ service.getProtocol = function getProtocol(tunnel) {
+
+ // Build HTTP parameters set
+ var httpParameters = {
+ token : authenticationService.getCurrentToken()
+ };
+
+ // Retrieve all associated sharing profiles
Review comment:
Copy pasta? Don't think this one is retrieving sharing profiles...
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman merged pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
necouchman merged pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#issuecomment-733278190
I think this PR is still the best approach. In the general case of usage of guacamole-common and guacamole-common-js, it should be up to the implementation to decide whether the protocol name should be exposed.
In the Apache Guacamole web application, we do not consider the underlying protocol name to be secret, and we _do_ need the protocol name to process `argv` and `required`, but we can't assume that to be the case for all implementations.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#discussion_r530691919
##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AbstractGuacamoleTunnelService.java
##########
@@ -201,87 +201,52 @@ protected abstract void release(RemoteAuthenticatedUser user,
ModeledConnectionGroup connectionGroup);
/**
- * Returns a guacamole configuration containing the protocol and parameters
- * from the given connection. If the ID of an active connection is
- * provided, that connection will be joined instead of starting a new
- * primary connection. If tokens are used in the connection parameter
- * values, credentials from the given user will be substituted
- * appropriately.
- *
- * @param user
- * The user whose credentials should be used if necessary.
+ * Returns a GuacamoleConfiguration which connects to the given connection.
+ * If the connection ID of an active is provided, that active connection
Review comment:
Sure. Change made via rebase.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#discussion_r530691223
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleConfiguration.java
##########
@@ -103,15 +102,34 @@ public void setConnectionID(String connectionID) {
/**
* Returns the name of the protocol to be used.
- * @return The name of the protocol to be used.
+ *
+ * @return
+ * The name of the protocol to be used.
*/
public String getProtocol() {
return protocol;
}
/**
- * Sets the name of the protocol to be used.
- * @param protocol The name of the protocol to be used.
+ * Sets the name of the protocol to be used. If no connection is being
+ * joined (a new connection is being established), this value must be set.
+ *
+ * <p>If a connection is being joined, <strong>this value should still be
+ * set</strong> to ensure that protocol-specific responses like the
+ * "required" and "argv" instructions can be understood in their proper
Review comment:
Removed via rebase.
##########
File path: guacamole/src/main/webapp/app/rest/services/tunnelService.js
##########
@@ -79,6 +79,36 @@ angular.module('rest').factory('tunnelService', ['$injector',
};
+ /**
+ * Makes a request to the REST API to retrieve the underlying protocol of
+ * the connection associated with a particular tunnel, returning a promise
+ * that provides a @link{Protocol} object if successful.
+ *
+ * @param {String} tunnel
+ * The UUID of the tunnel associated with the Guacamole connection
+ * whose underlying protocol is being retrieved.
+ *
+ * @returns {Promise.<Protocol>}
+ * A promise which will resolve with a @link{Protocol} object upon
+ * success.
+ */
+ service.getProtocol = function getProtocol(tunnel) {
+
+ // Build HTTP parameters set
+ var httpParameters = {
+ token : authenticationService.getCurrentToken()
+ };
+
+ // Retrieve all associated sharing profiles
Review comment:
Yep. Fixed via rebase.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] knacktim commented on pull request #578: GUACAMOLE-221: Expose underlying protocol at tunnel level.
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #578:
URL: https://github.com/apache/guacamole-client/pull/578#issuecomment-734019053
Thanks for the quick turn-around guys! Happy Turkey week 🦃
----------------------------------------------------------------
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