[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Vyom Tewari]

Hi Rob,

Most of the code changes looks fine except new code will throw NPE if 
"ServiceLocator.mapDnToDomainName(ldapUrl.getDN())" is not able to map. 
you have to check for null in LdapCtxFactory.java class as follows.

if(ServiceLocator.mapDnToDomainName(ldapUrl.getDN())!=null){
                 ((LdapCtx) ctx).setDomainName(
ServiceLocator.mapDnToDomainName(ldapUrl.getDN()));
                 }


##############################

Exception in thread "main" java.lang.NullPointerException
     at java.base/java.util.Hashtable.put(Hashtable.java:474)
     at 
java.naming/com.sun.jndi.ldap.LdapCtx.setDomainName(LdapCtx.java:2346)
     at 
java.naming/com.sun.jndi.ldap.LdapCtxFactory.getUsingURLs(LdapCtxFactory.java:210)
################################

Thanks,

Vyom


On Friday 03 February 2017 03:44 AM, Rob McKenna wrote:
> Thanks Daniel,
>
> New webrev at http://cr.openjdk.java.net/~robm/8160768/webrev.04/
>
> In addition to your comments below I've added the package access check
> we discussed.
>
> Note: when a provider returns multiple results we create an LdapCtx
> based on the first of those. I think it might be useful to extend
> LdapClient to accept a list of addresses which can be iterated over in the
> event of a connection failure but I'll tackle that separately.
>
>      -Rob
>
> On 02/02/17 11:01, Daniel Fuchs wrote:
>> Hi Rob,
>>
>> This is not a code I know well - but here are a couple
>> of comments:
>>
>> LdapCtxFactory.java:
>>
>> 168         NamingException ne = new NamingException();
>> 232         NamingException ne = new NamingException();
>>
>> Creating an exception is somewhat costly as it records the
>> backtrace in the Throwable constructor. I don't know if
>> it's an issue there but I thought I'd mention it. It might
>> be better to create the exception only if you're actually
>> going to throw it.
>>
>>   186         } catch (Exception e) {
>>   187             ne.setRootCause(e);
>>   188         }
>>
>> This will catch the NamingException thrown at line 173
>> and set it at the cause of another NamingException that
>> has no message. Maybe it would be better to have two
>> catch clauses:
>>
>>   186         } catch (NamingException e) {
>>   187             throw e;
>>   186         } catch (Exception e) {
>>                   ... transform it to NamingException ...
>>
>> In getDnsUrls:
>>
>>   198         if (env.containsKey(LdapCtx.DNS_PROVIDER)
>>   199                 && env.get(LdapCtx.DNS_PROVIDER) != null
>>   200                 && !env.get(LdapCtx.DNS_PROVIDER).equals(""))
>>
>> I'd suggest to simplify this and make a single lookup:
>>
>>               String providerClass = env.get(LdapCtx.DNS_PROVIDER);
>>               if (providerClass != null && !providerClass.isEmpty()) {
>>                   ...
>>                   Class<?> cls = Class.forName(providerClass, true, cl);
>>
>> best regards,
>>
>> -- daniel
>>
>>
>> On 01/02/17 20:05, Rob McKenna wrote:
>>> New webrev with updated test here:
>>>
>>> http://cr.openjdk.java.net/~robm/8160768/webrev.03/
>>>
>>>     -Rob
>>>
>>> On 26/01/17 04:03, Rob McKenna wrote:
>>>> Ack, apologies, thats what I get for rushing. (which I suppose I'm doing
>>>> again) That code was actually supposed to be in the getDnsUrls method.
>>>> Updated in place:
>>>>
>>>> http://cr.openjdk.java.net/~robm/8160768/webrev.02/
>>>>
>>>> I'll add a couple of tests for the SecurityManager along with some input
>>>> checking tests too.
>>>>
>>>>     -Rob
>>>>
>>>> On 25/01/17 05:50, Daniel Fuchs wrote:
>>>>> Hi Rob,
>>>>>
>>>>> I believe you should move the security check to before
>>>>> the class is actually loaded, before the call to
>>>>> 171             List<String> urls = getDnsUrls(url, env);
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>> On 25/01/17 17:44, Rob McKenna wrote:
>>>>>> I neglected to include a security check so I've cribbed the one from
>>>>>> OBJECT_FACTORIES (NamingManager.setObjectFactoryBuilder()) - see:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~robm/8160768/webrev.02/
>>>>>>
>>>>>> Note, this wraps the SecurityException in a NamingException. Presumably
>>>>>> its better to throw something than simply leave the default value in
>>>>>> place, but feedback appreciated!
>>>>>>
>>>>>>    -Rob
>>>>>>
>>>>>> On 25/01/17 04:31, Rob McKenna wrote:
>>>>>>> Hi folks,
>>>>>>>
>>>>>>> I'm looking for feedback on this suggested fix for the following bug:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-816076
>>>>>>> http://cr.openjdk.java.net/~robm/8160768/webrev.01/
>>>>>>>
>>>>>>> This is something that has come up a few times. Basically in certain
>>>>>>> environments (e.g. MS Active Directory) there is a dependence on
>>>>>>> DNS records to provide a pointer to the actual ldap server to be used
>>>>>>> for a given LdapCtx.PROVIDER_URL where the url itself simply points to the
>>>>>>> domain name.
>>>>>>>
>>>>>>> This fix add a new Ldap context property which allows a user to specify a
>>>>>>> class (implementing BiFunction) which can perform any necessary extra steps
>>>>>>> to derive the ldap servers hostname/port from the PROVIDER_URL.
>>>>>>>
>>>>>>>    -Rob
>>>>>>>