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
