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

[Rob McKenna]

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
> >>>>>
> >>>
>