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

Reply via email to