You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2019/01/02 16:41:58 UTC

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

GitHub user necouchman opened a pull request:

    https://github.com/apache/guacamole-client/pull/353

    GUACAMOLE-577: Support for configuring the Guacamole Proxy in LDAP Connections

    This pull request implements the changes necessary to support the `GuacamoleProxyConfiguration` in LDAP connections (and any future extensions that make use of the `SimpleConnection` class.  Not entirely sure this is the right/best way to go, but took a stab at it.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/necouchman/guacamole-client jira/577

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/guacamole-client/pull/353.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #353
    
----
commit 60189b1870ffe368893e24cef5c0ed830b108329
Author: Nick Couchman <vn...@...>
Date:   2019-01-02T16:33:57Z

    GUACAMOLE-577: Support for configuring the Guacamole Proxy settings in LDAP connections.

commit 366804344f1d83ec68243a566ffa3fe3265dcb79
Author: Nick Couchman <vn...@...>
Date:   2019-01-02T16:39:25Z

    GUACAMOLE-577: Fix up comments, remove unnecessary code.

commit f3a7e7f241ebdb237a1f53eeeb4b58f135f308bc
Author: Nick Couchman <vn...@...>
Date:   2019-01-02T16:41:22Z

    GUACAMOLE-577: Remove unused code.

----


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
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`.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858199
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    Okay, created a new LDAP attribute for the proxy configuration, under the existing OID string.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245779470
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    We could switch over to the ASF one: https://cwiki.apache.org/confluence/display/DIRxPMGT/OID+Assignment+Scheme
    
    Probably a good idea to do so. I'll still locate the ancient scrap of paper to server as a reference for assigning numbers from that, though - we need to use some consistent method for assigning the numbers within the Guacamole part of the ASF space (once that exists).


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858656
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java ---
    @@ -68,21 +72,34 @@ public SimpleConnection() {
          * @param identifier The identifier to associate with this connection.
          * @param config The configuration describing how to connect to this
          *               connection.
    +     * 
    +     * @throws GuacamoleException
    +     *     If the default proxy configuration cannot be retrieved.
          */
         public SimpleConnection(String name, String identifier,
    -            GuacamoleConfiguration config) {
    +            GuacamoleConfiguration config) throws GuacamoleException {
    --- End diff --
    
    Backed this change out and just kept things in the `connect()` method.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245789239
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    Cool.  Do we need to open an INFRA ticket to get that OID assigned under ASF?  Or is there some documentation somewhere on the ASF site that documents this?


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245755153
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java ---
    @@ -68,21 +72,34 @@ public SimpleConnection() {
          * @param identifier The identifier to associate with this connection.
          * @param config The configuration describing how to connect to this
          *               connection.
    +     * 
    +     * @throws GuacamoleException
    +     *     If the default proxy configuration cannot be retrieved.
          */
         public SimpleConnection(String name, String identifier,
    -            GuacamoleConfiguration config) {
    +            GuacamoleConfiguration config) throws GuacamoleException {
             
             // Set name
    -        setName(name);
    +        super.setName(name);
     
             // Set identifier
    -        setIdentifier(identifier);
    +        super.setIdentifier(identifier);
     
             // Set config
    -        setConfiguration(config);
    +        super.setConfiguration(config);
             this.config = config;
    +        this.proxyConfig = new LocalEnvironment().getDefaultGuacamoleProxyConfiguration();
     
         }
    +    
    +    public SimpleConnection(String name, String identifier,
    --- End diff --
    
    Please document this constructor.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245754954
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java ---
    @@ -68,21 +72,34 @@ public SimpleConnection() {
          * @param identifier The identifier to associate with this connection.
          * @param config The configuration describing how to connect to this
          *               connection.
    +     * 
    +     * @throws GuacamoleException
    +     *     If the default proxy configuration cannot be retrieved.
          */
         public SimpleConnection(String name, String identifier,
    -            GuacamoleConfiguration config) {
    +            GuacamoleConfiguration config) throws GuacamoleException {
    --- End diff --
    
    If possible, we should avoid adding new exceptions to an existing constructor, as downstream users of that constructor will suddenly have new things to catch.
    
    Perhaps retrieving the configuration from the local environment could still be put off until `connect()`, being the default behavior if no `GuacamoleProxyConfiguration` is provided?


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858704
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java ---
    @@ -68,21 +72,34 @@ public SimpleConnection() {
          * @param identifier The identifier to associate with this connection.
          * @param config The configuration describing how to connect to this
          *               connection.
    +     * 
    +     * @throws GuacamoleException
    +     *     If the default proxy configuration cannot be retrieved.
          */
         public SimpleConnection(String name, String identifier,
    -            GuacamoleConfiguration config) {
    +            GuacamoleConfiguration config) throws GuacamoleException {
             
             // Set name
    -        setName(name);
    +        super.setName(name);
     
             // Set identifier
    -        setIdentifier(identifier);
    +        super.setIdentifier(identifier);
     
             // Set config
    -        setConfiguration(config);
    +        super.setConfiguration(config);
             this.config = config;
    +        this.proxyConfig = new LocalEnvironment().getDefaultGuacamoleProxyConfiguration();
     
         }
    +    
    +    public SimpleConnection(String name, String identifier,
    --- End diff --
    
    Documented.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245818609
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    Cool.  I'm thinking the right way to do this is to continue with the current OID in this PR, and then open a separate JIRA issue (and PR) for converting from the old OID to the ASF OID?


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245757512
  
    --- 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();
    +                }
    +                catch (GuacamoleException e) {
    +                    return null;
    --- End diff --
    
    Got it.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245752774
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connection.java ---
    @@ -125,5 +125,17 @@
          *     connection.
          */
         public Set<String> getSharingProfileIdentifiers() throws GuacamoleException;
    +    
    +    /**
    +     * Get the GuacamoleProxyConfiguration for this connection.
    +     * 
    +     * @return
    +     *     The GuacamoleProxyConfiguration for this connection.
    +     * 
    +     * @throws GuacamoleException 
    +     *     If configuration information cannot be retrieved or parsed.
    +     */
    +    public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
    --- End diff --
    
    We have historically avoided making the assumption that extensions will define a means of connecting to guacd at the `Connection` level. The connection to an underlying Guacamole proxy, whether such a proxy is used at all, etc. is all kept abstract behind the `GuacamoleTunnel` returned by a call to `connect()`.
    
    I think it would be better to continue avoiding that assumption.
    
    If we will be providing such a getter for convenience (which is OK), there needs to be well-defined semantics allowing implementations to choose not to define it at that level (returning `null` perhaps).


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245753521
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connection.java ---
    @@ -125,5 +125,17 @@
          *     connection.
          */
         public Set<String> getSharingProfileIdentifiers() throws GuacamoleException;
    +    
    +    /**
    +     * Get the GuacamoleProxyConfiguration for this connection.
    --- End diff --
    
    There should be more to the documentation of `X.getFoo()` than "Gets the Foo for this X". There are semantics which apply here that should be noted.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858747
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connection.java ---
    @@ -125,5 +125,17 @@
          *     connection.
          */
         public Set<String> getSharingProfileIdentifiers() throws GuacamoleException;
    +    
    +    /**
    +     * Get the GuacamoleProxyConfiguration for this connection.
    +     * 
    +     * @return
    +     *     The GuacamoleProxyConfiguration for this connection.
    +     * 
    +     * @throws GuacamoleException 
    +     *     If configuration information cannot be retrieved or parsed.
    --- End diff --
    
    Pulled this out.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858540
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connection.java ---
    @@ -125,5 +125,17 @@
          *     connection.
          */
         public Set<String> getSharingProfileIdentifiers() throws GuacamoleException;
    +    
    +    /**
    +     * Get the GuacamoleProxyConfiguration for this connection.
    +     * 
    +     * @return
    +     *     The GuacamoleProxyConfiguration for this connection.
    +     * 
    +     * @throws GuacamoleException 
    +     *     If configuration information cannot be retrieved or parsed.
    +     */
    +    public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
    --- End diff --
    
    Okay, I've removed this entirely from the interfaces and abstracts.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245791722
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    According to https://cwiki.apache.org/confluence/display/DIRxPMGT/OID+Assignment+Scheme:
    
    > Each contact person is the authority for assigning unique OID values and ranges to projects or persons. Contact that person for more assignments.
    
    The contact person listed for the main ASF OID is Alex Karasulu. Feels weird to me to not use a mailing list, but I'll shoot him an email. ;)


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245756503
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    Sounds good.  I hesitated to modify the LDAP schema, but I'm happy to go that route if that's the best way to go.  Do you think separate LDAP attributes specifically for proxying, or just a generic Guacamole Attribute that is handled similar to the parameters?
    
    For the OID, should we use the Guacamole one, or does ASF have one we should go with?


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245799935
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    Depending on OID assignment, it looks like we should be fine with the above system. There is no standard limit imposed on OID length, and longer OIDs are already actively used.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245757432
  
    --- 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 --
    
    I originally went this route, but thought it might be a little overkill :-).  I'll go back to this method...


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245760171
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java ---
    @@ -68,21 +72,34 @@ public SimpleConnection() {
          * @param identifier The identifier to associate with this connection.
          * @param config The configuration describing how to connect to this
          *               connection.
    +     * 
    +     * @throws GuacamoleException
    +     *     If the default proxy configuration cannot be retrieved.
          */
         public SimpleConnection(String name, String identifier,
    -            GuacamoleConfiguration config) {
    +            GuacamoleConfiguration config) throws GuacamoleException {
    --- End diff --
    
    Okay, will refactor.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858609
  
    --- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connection.java ---
    @@ -125,5 +125,17 @@
          *     connection.
          */
         public Set<String> getSharingProfileIdentifiers() throws GuacamoleException;
    +    
    +    /**
    +     * Get the GuacamoleProxyConfiguration for this connection.
    --- End diff --
    
    Removed.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858320
  
    --- 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 --
    
    Okay, switched back to this method.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245744066
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    These should probably be separate, dedicated LDAP attributes, rather than reusing `guacConfigParameter` (which is meant for connection parameters). This would mean changes to the LDAP schema, which would mean new OID values.
    
    I have the OID number assignment scheme written down on a scrap of paper from years past ... I'll locate my notes and copy them here. There's an OID space which was reserved for Guacamole ages ago (see 38971 within https://www.iana.org/assignments/enterprise-numbers/enterprise-numbers), but there is a system for consistently assigning numbers within that space.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245747944
  
    --- 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();
    +                }
    +                catch (GuacamoleException e) {
    +                    return null;
    --- End diff --
    
    If retrieval of the proxy config fails, that failure should be logged rather than silently ignored.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245858363
  
    --- 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();
    +                }
    +                catch (GuacamoleException e) {
    +                    return null;
    --- End diff --
    
    Logged.


---

[GitHub] guacamole-client pull request #353: GUACAMOLE-577: Support for configuring t...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-client/pull/353#discussion_r245786503
  
    --- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
    @@ -167,18 +201,34 @@
                                 // Parse name
                                 String name = parameter.substring(0, equals);
                                 String value = parameter.substring(equals+1);
    -
    -                            config.setParameter(name, value);
    +                            
    +                            // Pull out and set proxy parameters, if present
    +                            // Otherwise set the parameter.
    +                            switch(name) {
    +                                case PROXY_HOST_PARAMETER:
    --- End diff --
    
    Found it!
    
    ![ancient-guac-ldap-oid-assignment-scheme](https://user-images.githubusercontent.com/4632905/50790691-7c27cc00-1274-11e9-8baa-182f9e2fb50c.jpg)
    
    Quoting the contents here for reference:
    
    >     1.3.6.1.4.1.38971        = guac
    >                      .1      = LDAP schema OID's
    >                        .1    = attribute types
    >                          .1  = guacConfigProtocol
    >                          .2  = guacConfigParameter
    >                        .2    = object classes
    >                          .1  = guacConfigGroup
    
    Using the ASF OID, assuming we end up with "1.3.6.1.4.1.18060.18", we could redefine things beneath that. Presumably:
    
        1.3.6.1.4.1.18060.18        = Apache Guacamole
                            .1      = LDAP schema
                              .1    = attribute types
                                .1  = guacConfigProtocol
                                .2  = guacConfigParameter
                              .2    = object classes
                                .1  = guacConfigGroup
    
    I'm looking around to see if there is a limit on the number of numeric groups in an LDAP OID, or at least if there are examples in the wild of LDAP schemas having OIDs at least as long as those proposed above.


---