Review feedback

federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPConfig.java

@@ -111,7 +111,7 @@ public class LDAPConfig {
     }
 
     public String getConnectionPooling() {
-        if(isTls()) {
+        if(isStartTls()) {
             return null;
         } else {
             return config.getFirst(LDAPConstants.CONNECTION_POOLING);
@@ -223,9 +223,8 @@ public class LDAPConfig {
         return null;
     }
 
-    public boolean isTls() {
+    public boolean isStartTls() {
-        String tls = config.getFirst(LDAPConstants.TLS);
+        return Boolean.parseBoolean(config.getFirst(LDAPConstants.START_TLS));
-        return Boolean.parseBoolean(tls);
     }
 
     public UserStorageProvider.EditMode getEditMode() {

federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java

@@ -128,7 +128,7 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory<LD
                 .type(ProviderConfigProperty.STRING_TYPE)
                 .defaultValue("simple")
                 .add()
-                .property().name(LDAPConstants.TLS)
+                .property().name(LDAPConstants.START_TLS)
                 .type(ProviderConfigProperty.BOOLEAN_TYPE)
                 .add()
                 .property().name(LDAPConstants.BIND_DN)
@@ -270,6 +270,9 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory<LD
             }
         }
 
+        if(cfg.isStartTls() && cfg.getConnectionPooling() != null) {
+            throw new ComponentValidationException("ldapErrorCantEnableStartTlsAndConnectionPooling");
+        }
     }
 
     @Override

federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPOperationManager.java

@@ -39,7 +39,14 @@ import javax.naming.directory.DirContext;
 import javax.naming.directory.ModificationItem;
 import javax.naming.directory.SearchControls;
 import javax.naming.directory.SearchResult;
-import javax.naming.ldap.*;
+import javax.naming.ldap.Control;
+import javax.naming.ldap.InitialLdapContext;
+import javax.naming.ldap.LdapContext;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.PagedResultsControl;
+import javax.naming.ldap.PagedResultsResponseControl;
+import javax.naming.ldap.StartTlsRequest;
+import javax.naming.ldap.StartTlsResponse;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -497,7 +504,7 @@ public class LDAPOperationManager {
             // Never use connection pool to prevent password caching
             env.put("com.sun.jndi.ldap.connect.pool", "false");
 
-            if(!this.config.isTls()) {
+            if(!this.config.isStartTls()) {
                 env.put(Context.SECURITY_AUTHENTICATION, this.config.getAuthType());
                 env.put(Context.SECURITY_PRINCIPAL, dn);
                 env.put(Context.SECURITY_CREDENTIALS, password);
@@ -527,7 +534,7 @@ public class LDAPOperationManager {
     }
 
     private void startTLS(LdapContext ldapContext, String authType, String bindDN, String bindCredentials) throws NamingException {
-        if(this.config.isTls()) {
+        if(this.config.isStartTls()) {
             try {
                 StartTlsResponse tls = (StartTlsResponse) ldapContext.extendedOperation(new StartTlsRequest());
                 tls.negotiate();
@@ -675,7 +682,7 @@ public class LDAPOperationManager {
     }
 
     private LdapContext createLdapContext() throws NamingException {
-        if(!config.isTls()) {
+        if(!config.isStartTls()) {
             return new InitialLdapContext(new Hashtable<Object, Object>(this.connectionProperties), null);
         } else {
             LdapContext ldapContext = new InitialLdapContext(new Hashtable<Object, Object>(this.connectionProperties), null);
@@ -689,7 +696,7 @@ public class LDAPOperationManager {
 
         env.put(Context.INITIAL_CONTEXT_FACTORY, this.config.getFactoryName());
 
-        if(!this.config.isTls()) {
+        if(!this.config.isStartTls()) {
             String authType = this.config.getAuthType();
 
             env.put(Context.SECURITY_AUTHENTICATION, authType);

server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java

@@ -82,7 +82,7 @@ public class LDAPConstants {
     // Custom user search filter
     public static final String CUSTOM_USER_SEARCH_FILTER = "customUserSearchFilter";
 
-    public static final String TLS = "tls";
+    public static final String START_TLS = "startTls";
 
     // Custom attributes on UserModel, which is mapped to LDAP
     public static final String LDAP_ID = "LDAP_ID";

services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java

@@ -930,14 +930,14 @@ public class RealmAdminResource {
     public Response testLDAPConnection(@FormParam("action") String action, @FormParam("connectionUrl") String connectionUrl,
                                        @FormParam("bindDn") String bindDn, @FormParam("bindCredential") String bindCredential,
                                        @FormParam("useTruststoreSpi") String useTruststoreSpi, @FormParam("connectionTimeout") String connectionTimeout,
-                                       @FormParam("componentId") String componentId, @FormParam("tls") String tls) {
+                                       @FormParam("componentId") String componentId, @FormParam("startTls") String startTls) {
         auth.realm().requireManageRealm();
 
         if (componentId != null && bindCredential.equals(ComponentRepresentation.SECRET_VALUE)) {
             bindCredential = realm.getComponent(componentId).getConfig().getFirst(LDAPConstants.BIND_CREDENTIAL);
         }
 
-        boolean result = new LDAPConnectionTestManager().testLDAP(action, connectionUrl, bindDn, bindCredential, useTruststoreSpi, connectionTimeout, tls);
+        boolean result = new LDAPConnectionTestManager().testLDAP(action, connectionUrl, bindDn, bindCredential, useTruststoreSpi, connectionTimeout, startTls);
         return result ? Response.noContent().build() : ErrorResponse.error("LDAP test error", Response.Status.BAD_REQUEST);
     }
 

themes/src/main/resources/theme/base/admin/messages/admin-messages_en.properties

@@ -968,7 +968,7 @@ ldap.connection-pooling.prefsize.tooltip=The string representation of an integer
 ldap.connection-pooling.protocol.tooltip=A list of space-separated protocol types of connections that may be pooled. Valid types are "plain" and "ssl".
 ldap.connection-pooling.timeout.tooltip=The string representation of an integer that represents the number of milliseconds that an idle connection may remain in the pool without being closed and removed from the pool.
 ldap.pagination.tooltip=Does the LDAP server support pagination.
-ldap.tls.tooltip=Does the LDAP server require STARTTLS.
+ldap.startTls.tooltip=Encrypts the connection to LDAP using STARTTLS, this will disable connection pooling.
 kerberos-integration=Kerberos Integration
 allow-kerberos-authentication=Allow Kerberos authentication
 ldap.allow-kerberos-authentication.tooltip=Enable/disable HTTP authentication of users with SPNEGO/Kerberos tokens. The data about authenticated users will be provisioned from this LDAP server
@@ -993,7 +993,6 @@ identity-provider-user-id.tooltip=Unique ID of the user on the Identity Provider
 identity-provider-username=Identity Provider Username
 identity-provider-username.tooltip=Username on the Identity Provider side
 pagination=Pagination
-tls=TLS
 browser-flow=Browser Flow
 browser-flow.tooltip=Select the flow you want to use for browser authentication.
 registration-flow=Registration Flow

themes/src/main/resources/theme/base/admin/messages/messages_en.properties

@@ -16,6 +16,7 @@ ldapErrorMissingClientId=Client ID needs to be provided in config when Realm Rol
 ldapErrorCantPreserveGroupInheritanceWithUIDMembershipType=Not possible to preserve group inheritance and use UID membership type together.
 ldapErrorCantWriteOnlyForReadOnlyLdap=Can't set write only when LDAP provider mode is not WRITABLE
 ldapErrorCantWriteOnlyAndReadOnly=Can't set write-only and read-only together
+ldapErrorCantEnableStartTlsAndConnectionPooling=Can't enable both StartTLS and connection pooling.
 
 clientRedirectURIsFragmentError=Redirect URIs must not contain an URI fragment
 clientRootURLFragmentError=Root URL must not contain an URL fragment

themes/src/main/resources/theme/base/admin/resources/js/controllers/users.js

@@ -1523,7 +1523,7 @@ module.controller('LDAPUserStorageCtrl', function($scope, $location, Notificatio
             bindCredential: ldapConfig.bindCredential,
             useTruststoreSpi: ldapConfig.useTruststoreSpi,
             connectionTimeout: ldapConfig.connectionTimeout,
-            tls: ldapConfig.tls,
+            startTls: ldapConfig.startTls,
             componentId: instance.id
         };
     };

themes/src/main/resources/theme/base/admin/resources/partials/user-storage-ldap.html

@@ -141,11 +141,11 @@
                 <kc-tooltip>{{:: 'ldap.authentication-type.tooltip' | translate}}</kc-tooltip>
             </div>
             <div class="form-group clearfix">
-                <label class="col-md-2 control-label" for="tls">{{:: 'tls' | translate}}</label>
+                <label class="col-md-2 control-label" for="startTls">{{:: 'enable-start-tls' | translate}}</label>
                 <div class="col-md-6">
-                    <input ng-model="instance.config['tls'][0]" name="tls" id="tls" onoffswitchvalue on-text="{{:: 'onText' | translate}}" off-text="{{:: 'offText' | translate}}" />
+                    <input ng-model="instance.config['startTls'][0]" name="startTls" id="startTls" onoffswitchvalue on-text="{{:: 'onText' | translate}}" off-text="{{:: 'offText' | translate}}" />
                 </div>
-                <kc-tooltip>{{:: 'ldap.tls.tooltip' | translate}}</kc-tooltip>
+                <kc-tooltip>{{:: 'ldap.startTls.tooltip' | translate}}</kc-tooltip>
             </div>
             <div class="form-group clearfix" data-ng-hide="instance.config['authType'][0] == 'none'">
                 <label class="col-md-2 control-label" for="ldapBindDn"><span class="required">*</span> {{:: 'bind-dn' | translate}}</label>