Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245747651
  
    --- Diff: 
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java
 ---
    @@ -148,12 +169,25 @@
     
                     // Set protocol
                     GuacamoleConfiguration config = new 
GuacamoleConfiguration();
    +                
    +                GuacamoleProxyConfiguration proxyConfig;
    +                try {
    +                    proxyConfig = new 
LocalEnvironment().getDefaultGuacamoleProxyConfiguration();
    --- End diff --
    
    The `LocalEnvironment` should not be repeatedly recreated if it can be 
avoided.
    
    A singleton instance of it is actually already bound during startup of the 
LDAP extension, so that would be the instance of choice:
    
    
https://github.com/apache/guacamole-client/blob/78f1ae1b4eac25501d532ddee94fd1d8588e56dc/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPAuthenticationProviderModule.java#L74
    
    That singleton instance can be injected within this class if needed, for 
example:
    
    
https://github.com/apache/guacamole-client/blob/78f1ae1b4eac25501d532ddee94fd1d8588e56dc/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java#L41-L45
    
    Since this is configuration, though, I'd think it would make more sense to 
provide this via a function within `ConfigurationService`.


---

Reply via email to