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

    
https://github.com/apache/incubator-guacamole-client/pull/202#discussion_r149181098
  
    --- Diff: 
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java
 ---
    @@ -0,0 +1,80 @@
    +/*
    + * 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.ldap;
    +
    +import com.google.inject.Inject;
    +import com.novell.ldap.LDAPAuthHandler;
    +import com.novell.ldap.LDAPAuthProvider;
    +import com.novell.ldap.LDAPConnection;
    +import java.io.UnsupportedEncodingException;
    +import org.apache.guacamole.GuacamoleException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +/**
    + * Class that implements the necessary authentication handling
    + * for following referrals in LDAP connections.
    + */
    +public class ReferralAuthHandler implements LDAPAuthHandler {
    +
    +    /**
    +     * Logger for this class.
    +     */
    +    private final Logger logger = 
LoggerFactory.getLogger(ReferralAuthHandler.class);
    +
    +    /**
    +     * The LDAPAuthProvider object that will be set and returned to the 
referral handler.
    +     */
    +    private final LDAPAuthProvider ldapAuth;
    +
    +    /**
    +     * Creates a ReferralAuthHandler object to handle authentication when
    +     * following referrals in a LDAP connection, using the provided dn and
    +     * password.
    +     *
    +     * @throws GuacamoleException
    +     *     If exceptions are caught while converting the password from a 
string
    +     *     into a byte array.
    +     */
    +    public ReferralAuthHandler(String dn, String password) throws 
GuacamoleException {
    +        byte[] passwordBytes;
    +        try {
    +
    +            // Convert password into corresponding byte array
    +            if (password != null)
    +                passwordBytes = password.getBytes("UTF-8");
    +            else
    +                passwordBytes = null;
    +
    +        }   
    +        catch (UnsupportedEncodingException e) {
    +            logger.error("Unexpected lack of support for UTF-8: {}", 
e.getMessage());
    +            logger.debug("Support for UTF-8 (as required by Java spec) not 
found.", e); 
    +            throw new GuacamoleException("Could not set password due to 
missing UTF-8 support.");
    --- End diff --
    
    In general:
    
    1. Rather than throw the base `GuacamoleException`, we should be throwing 
an appropriate subclass. `GuacamoleServerException` would be a reasonable 
choice in this case.
    2. For any exception caused by another, [the constructor which includes the 
causing 
exception](http://guacamole.incubator.apache.org/doc/guacamole-common/org/apache/guacamole/GuacamoleServerException.html#GuacamoleServerException-java.lang.String-java.lang.Throwable-)
 should be used (rather than `new GuacamoleServerException("message")`, use 
`new GuacamoleServerException("message", e)`).
    
    That said, since UTF-8 support is guaranteed by Java, there's no need for 
checked exceptions here. Something like `UnsupportedOperationException` might 
be more appropriate. For example:
    
    
https://github.com/apache/incubator-guacamole-client/blob/0611fe8fff694dff7e3cb6a62918f6c90668728c/extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/form/CASTicketField.java#L80-L83


---

Reply via email to