GUACAMOLE-524: Update connect() API changes for backward compatibility with 1.0.0.#360
Conversation
…sions of connect() for sake of compatibility.
…uacamoleConfiguration within SimpleConnection. While raw, internal access to the GuacamoleConfiguration was originally present in older versions of SimpleConnection, this access was undocumented and could result in unexpected behavior if the default constructor was used, getConfiguration() was overridden, or setConfiguration() was called.
…deprecated connect() function to have the expected effect within subclasses of SimpleConnection.
…st automatic interpretation of parameter tokens. Do not enable by default. Previous implementations of SimpleConnection did not interpret parameter tokens automatically. Adding that behavior now could have security implications for downstream users of the class if parameter values may unexpectedly contain substrings which would be interpreted as tokens, particularly if parameter values are built from untrusted input.
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
Just a couple of quick questions, but looking pretty good.
| Map<String, String> tokens) throws GuacamoleException { | ||
|
|
||
| // Allow old implementations of Connectable to continue to work | ||
| return this.connect(info); |
There was a problem hiding this comment.
Is there a reason supplying a default of this particular instance of this method is necessary or valuable? Won't the old implementations already be calling the other default method, without the tokens parameter? And doesn't this introduce the possibility that an implementing class could end up recursively calling these parameters (if they don't actually implement either of them)? Or will that get caught at compile time?
There was a problem hiding this comment.
Is there a reason supplying a default of this particular instance of this method is necessary or valuable? Won't the old implementations already be calling the other default method, without the
tokensparameter?
Some sort of default is necessary to bridge the implementations, as it's the webapp that ultimately calls connect(), and in this case will be calling the version which has tokens.
In the general case, defaults are needed to bridge in both directions to allow extensions of different versions to interoperate. An older extension (compatible with 1.0.0) which decorates the connections of other extensions will need to be able to invoke connect(), even though it will be calling the older version. A newer extension and the webapp need the same ability but in the other direction.
And doesn't this introduce the possibility that an implementing class could end up recursively calling these parameters (if they don't actually implement either of them)?
Yes, and that's definitely a bad thing. I think I may have a route around this.
There was a problem hiding this comment.
OK - I'm rearranging things a bit such that the interface defined within guacamole-ext is still strictly as defined before: one connect() definition which accepts tokens.
Within the webapp, I'm adding another version of Connectable in the same package which defines both connect() versions as well as defaults, and thus an ABI-compatible redefinition of both versions of the interface. Because of the way servlet containers handle classes bundled with the webapp vs. within dependencies, the webapp version of Connectable takes priority.
|
|
||
| // Add as simple connection | ||
| Connection connection = new SimpleConnection(identifier, identifier, config); | ||
| Connection connection = new SimpleConnection(identifier, identifier, config, true); |
There was a problem hiding this comment.
Should this be true, or should this be passing the value of interpretTokens?
There was a problem hiding this comment.
Good catch - we should pass through interpretTokens.
…version. Defining an ABI-compatible version of Connectable at the guacamole-ext level is problematic as concrete implementations of the interface will suddenly compile despite having no implementation of connect() at all. We can instead rely on the web application to ensure binary compatibility, leaving guacamole-ext to define the interface that new code should use.
…retTokens is false.
|
Reading through things again, I think we might need to also update |
…) use the old connect() as their basis. Overriding the old connect() will not have the expected effect otherwise.
|
OK - I've gone through and made sure that any guacamole-ext class which provides a concrete implementation of |
2 participants
As discussed on dev@, these changes modify the
Connectableinterface such that implementations of the previous version of theconnect()function will continue to work, even if those implementations extendedSimpleConnectionand overrode the old, now-deprecated function. This is achieved through:connect()as the basis for the implementation.ThreadLocalto make the additionalMap<String, String>parameter available to the oldconnect()within a strict context.The
SimpleConnectionclass and related classes are also modified to provide the same behavior as past releases, particularly for downstream subclasses.With these changes, things are ABI-compatible with 1.0.0. Things are also API-compatible with 1.0.0, though continuing to use/override the old
connect()will result in compiler warnings. Behavior of the relevant classes should now be as follows:SimpleAuthenticationProviderSimpleUserContextSimpleConnectionOverall, behavior should be identical to 1.0.0 with the following exceptions:
SimpleAuthenticationProviderwill now handle additional tokens and apply those tokens in context of the connection, not at time of authentication.getConfiguration()on a connection accessed via a subclass ofSimpleAuthenticationProviderwill now contain any tokens in uninterpreted form.