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
