[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider
Daniel Fuchs
daniel.fuchs at oracle.com
Tue Oct 30 14:25:18 UTC 2018
Hi Rob,
LdapCtxFactory.java
187 for (String u : r.getEndpoints()) {
188 try {
189 ctx = getLdapCtxFromUrl(
190 r.getDomainName(), new LdapURL(u), env);
191 } catch (NamingException e) {
192 // try the next element
193 lastException = e;
194 }
195 }
is a break statement missing after line 190?
If not then can you add a comment explaining why only the last
context is retained (and returned?)
Alternatively, if a break is indeed missing, these three lines
could be moved into the for loop above:
206 // Record the URL that created the context
207 ctx.setProviderUrl(url);
208 return ctx;
and maybe lines 206-207 could be moved into the
getLdapCtxFromUrl() method?
LdapDnsProviderService.java:
Why is this class non final? If it can be made final then
the protected methods should probably become package.
LdapDnsProvider.java:
It is strange to see new APIs with Hashtable in the method
signature. I understand that our implementation will need
an Hashtable at some point to call javax.naming.spi.NamingProvider,
but how likely is it that a clean room implementation of
a LdapDnsProvider will need to do that?
Maybe we could have Map<?,?> in the signature instead - and
leave the burden to our implementation - somewhere in ServiceLocator,
to adapt back to Hashtable where it needs to?
best regards,
-- daniel
On 25/10/2018 17:34, Rob McKenna wrote:
> This recently received CSR approval, so it seems like a good time to pick
> the codereview up again:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.08/
>
> Referencing:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html
>
> 1) I'm copying the behaviour from
> LdapCtxFactory.getInitialContext(Hashtable<?,?>) where an empty String is
> the default value used when the provider url is null.
>
> I don't think HostnameChecker (by way of SNIHostname) will accept either
> empty or null values when doing the comparison.
>
> Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
> appears to pass the String "null" to
> StartTlsResponseImpl.setConnection(Connection, String), which attempts
> to substitute null values with the Connections host so there may be a bug
> here.
>
> I'm happy to allow null returns here if necessary. Sean, can you
> comment on the distinction between null & "" as far as hostname
> verification is concerned?
>
> 2) In the latest iteration lookupEndpoints() returns an
> Optional<LdapDnsResult>. Currently neither getEndpoints() nor
> getDomainName() can be null. (they can be an empty list and/or an empty
> String however)
>
> 3) Corrected.
>
> 4) See https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5
>
> -Rob
>
More information about the core-libs-dev
mailing list