Am 2020-06-30 um 14:22 schrieb Alexey Bakhtin:
Hello Daniel,
Thank you for review.
I have updated LdapCBPropertiesTest according to your comments.
Updated webrev at http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v9/
A few notes
+ throw new
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+ " property requires " +
+ "com.sun.jndi.ldap.connect.timeout property is set.");
"...requires ... to be set." The current text reads strange in English.
+ env.get("jdk.internal.sasl.tlschannelbinding") != null) {
and alike. Any reason to use the literal instead of the static symbol?
Module import issues?
In TlsChannelBinding.java:
+ // TLS channel binding type property
+ public static final String CHANNEL_BINDING_TYPE =
+ "com.sun.jndi.ldap.tls.cbtype";
and
+ public static TlsChannelBindingType parseType(String prop) throws
NamingException {
TLS channel binding is not tied to LDAP, it can be used with other
protocols, even custom ones. I see no good reason to have the property
contain jndi.ldap or use NamingException. IllegalArgumentException would
be approriate here.
Also on this class you have the wonderful enum TlsChannelBindingType,
but still use litrals throughout for the possible values. Why?
Why isn't this class part of the Sun SASL impl?
> #parseType()
I would not use the term 'prop'. I miss the loose coupling here.
Ideally, the class does not even know that is might be passed around
with a env property from LDAP, SMTP, foo protocol to SASL. It is merely
a record to contain the calculcated data. I'd remove all props from here.
TlsChannelBindingImpl.java:
I like this idea. This basically solves the problem with the same
approach I have proposed from the first time, a specialized subimpl, but
this time not for the address, but for the binding type. Nice!
I consider this webrev version much much better than v1. I really miss
the TLS Channel Binding abstraction from LDAP. The current situation
bascially means that no one with Java can implement TLS-CB with anything
else than the Sun JNDI LDAP provider over SASL. Kind of disappointing.
Michael
