KEYCLOAK-8688 LDAPSyncTest is failing in some environments

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

@@ -424,13 +424,14 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory<LD
 
         logger.infof("Sync all users from LDAP to local store: realm: %s, federation provider: %s", realmId, model.getName());
 
-        LDAPQuery userQuery = createQuery(sessionFactory, realmId, model);
+        try (LDAPQuery userQuery = createQuery(sessionFactory, realmId, model)) {
-        SynchronizationResult syncResult = syncImpl(sessionFactory, userQuery, realmId, model);
+            SynchronizationResult syncResult = syncImpl(sessionFactory, userQuery, realmId, model);
 
-        // TODO: Remove all existing keycloak users, which have federation links, but are not in LDAP. Perhaps don't check users, which were just added or updated during this sync?
+            // TODO: Remove all existing keycloak users, which have federation links, but are not in LDAP. Perhaps don't check users, which were just added or updated during this sync?
 
-        logger.infof("Sync all users finished: %s", syncResult.getStatus());
+            logger.infof("Sync all users finished: %s", syncResult.getStatus());
-        return syncResult;
+            return syncResult;
+        }
     }
 
     @Override
@@ -445,12 +446,13 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory<LD
         Condition modifyCondition = conditionsBuilder.greaterThanOrEqualTo(LDAPConstants.MODIFY_TIMESTAMP, lastSync);
         Condition orCondition = conditionsBuilder.orCondition(createCondition, modifyCondition);
 
-        LDAPQuery userQuery = createQuery(sessionFactory, realmId, model);
+        try (LDAPQuery userQuery = createQuery(sessionFactory, realmId, model)) {
-        userQuery.addWhereCondition(orCondition);
+            userQuery.addWhereCondition(orCondition);
-        SynchronizationResult result = syncImpl(sessionFactory, userQuery, realmId, model);
+            SynchronizationResult result = syncImpl(sessionFactory, userQuery, realmId, model);
 
-        logger.infof("Sync changed users finished: %s", result.getStatus());
+            logger.infof("Sync changed users finished: %s", result.getStatus());
-        return result;
+            return result;
+        }
     }
 
     protected void syncMappers(KeycloakSessionFactory sessionFactory, final String realmId, final ComponentModel model) {
@@ -486,7 +488,7 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory<LD
             while (nextPage) {
                 userQuery.setLimit(pageSize);
                 final List<LDAPObject> users = userQuery.getResultList();
-                nextPage = userQuery.getPaginationContext() != null;
+                nextPage = userQuery.getPaginationContext().hasNextPage();
                 SynchronizationResult currentPageSync = importLdapUsers(sessionFactory, realmId, fedModel, users);
                 syncResult.add(currentPageSync);
             }

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

@@ -250,7 +250,7 @@ public class LDAPUtils {
      * Load all LDAP objects corresponding to given query. We will load them paginated, so we allow to bypass the limitation of 1000
      * maximum loaded objects in single query in MSAD
      *
-     * @param ldapQuery
+     * @param ldapQuery LDAP query to be used. The caller should close it after calling this method
      * @param ldapProvider
      * @return
      */
@@ -268,7 +268,7 @@ public class LDAPUtils {
                 ldapQuery.setLimit(pageSize);
                 final List<LDAPObject> currentPageGroups = ldapQuery.getResultList();
                 result.addAll(currentPageGroups);
-                nextPage = ldapQuery.getPaginationContext() != null;
+                nextPage = ldapQuery.getPaginationContext().hasNextPage();
             }
 
             return result;

federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/query/internal/LDAPQuery.java

@@ -17,6 +17,7 @@
 
 package org.keycloak.storage.ldap.idm.query.internal;
 
+import org.jboss.logging.Logger;
 import org.keycloak.component.ComponentModel;
 import org.keycloak.models.ModelDuplicateException;
 import org.keycloak.models.ModelException;
@@ -26,7 +27,10 @@ import org.keycloak.storage.ldap.idm.query.Condition;
 import org.keycloak.storage.ldap.idm.query.Sort;
 import org.keycloak.storage.ldap.mappers.LDAPStorageMapper;
 
+import javax.naming.NamingException;
 import javax.naming.directory.SearchControls;
+import javax.naming.ldap.LdapContext;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -39,16 +43,19 @@ import static java.util.Collections.unmodifiableSet;
 /**
  * Default IdentityQuery implementation.
  *
+ * LDAPQuery should be closed after use in case that pagination was used (initPagination was called)
  *
  * @author Shane Bryzak
  */
-public class LDAPQuery {
+public class LDAPQuery implements AutoCloseable{
+
+    private static final Logger logger = Logger.getLogger(LDAPQuery.class);
 
     private final LDAPStorageProvider ldapFedProvider;
 
     private int offset;
     private int limit;
-    private byte[] paginationContext;
+    private PaginationContext paginationContext;
     private String searchDn;
     private final Set<Condition> conditions = new LinkedHashSet<Condition>();
     private final Set<Sort> ordering = new LinkedHashSet<Sort>();
@@ -144,7 +151,7 @@ public class LDAPQuery {
         return offset;
     }
 
-    public byte[] getPaginationContext() {
+    public PaginationContext getPaginationContext() {
         return paginationContext;
     }
 
@@ -197,8 +204,8 @@ public class LDAPQuery {
         return this;
     }
 
-    public LDAPQuery setPaginationContext(byte[] paginationContext) {
+    public LDAPQuery initPagination(LdapContext ldapContext) {
-        this.paginationContext = paginationContext;
+        this.paginationContext = new PaginationContext(ldapContext);
         return this;
     }
 
@@ -210,4 +217,47 @@ public class LDAPQuery {
         return ldapFedProvider;
     }
 
+
+    @Override
+    public void close() {
+        if (paginationContext != null) {
+            try {
+                paginationContext.ldapContext.close();
+            } catch (NamingException ne) {
+                logger.error("Could not close Ldap context.", ne);
+            }
+        }
+    }
+
+
+    public static class PaginationContext {
+
+        private final LdapContext ldapContext;
+        private byte[] cookie;
+
+        private PaginationContext(LdapContext ldapContext) {
+            if (ldapContext == null) {
+                throw new IllegalArgumentException("Bad usage. Ldap context must be not null");
+            }
+            this.ldapContext = ldapContext;
+        }
+
+
+        public LdapContext getLdapContext() {
+            return ldapContext;
+        }
+
+        public byte[] getCookie() {
+            return cookie;
+        }
+
+        public void setCookie(byte[] cookie) {
+            this.cookie = cookie;
+        }
+
+        public boolean hasNextPage() {
+            return this.cookie != null;
+        }
+    }
+
 }

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

@@ -286,13 +286,19 @@ public class LDAPOperationManager {
         final List<SearchResult> result = new ArrayList<SearchResult>();
         final SearchControls cons = getSearchControls(identityQuery.getReturningLdapAttributes(), identityQuery.getSearchScope());
 
+        // Very 1st page. Pagination context is not yet present
+        if (identityQuery.getPaginationContext() == null) {
+            LdapContext ldapContext = createLdapContext();
+            identityQuery.initPagination(ldapContext);
+        }
+
         try {
             return execute(new LdapOperation<List<SearchResult>>() {
 
                 @Override
                 public List<SearchResult> execute(LdapContext context) throws NamingException {
                     try {
-                        byte[] cookie = identityQuery.getPaginationContext();
+                        byte[] cookie = identityQuery.getPaginationContext().getCookie();
                         PagedResultsControl pagedControls = new PagedResultsControl(identityQuery.getLimit(), cookie, Control.CRITICAL);
                         context.setRequestControls(new Control[] { pagedControls });
 
@@ -310,7 +316,7 @@ public class LDAPOperationManager {
                                 if (respControl instanceof PagedResultsResponseControl) {
                                     PagedResultsResponseControl prrc = (PagedResultsResponseControl)respControl;
                                     cookie = prrc.getCookie();
-                                    identityQuery.setPaginationContext(cookie);
+                                    identityQuery.getPaginationContext().setCookie(cookie);
                                 }
                             }
                         }
@@ -335,7 +341,7 @@ public class LDAPOperationManager {
                             .toString();
                 }
 
-            });
+            }, identityQuery.getPaginationContext().getLdapContext(), null);
         } catch (NamingException e) {
             logger.errorf(e, "Could not query server using DN [%s] and filter [%s]", baseDN, filter);
             throw e;
@@ -565,7 +571,7 @@ public class LDAPOperationManager {
                 }
 
 
-            }, decorator);
+            }, null, decorator);
         } catch (NamingException e) {
             throw new ModelException("Could not modify attribute for DN [" + dn + "]", e);
         }
@@ -726,11 +732,13 @@ public class LDAPOperationManager {
     }
 
     private <R> R execute(LdapOperation<R> operation) throws NamingException {
-        return execute(operation, null);
+        return execute(operation, null, null);
     }
 
-    private <R> R execute(LdapOperation<R> operation, LDAPOperationDecorator decorator) throws NamingException {
+    private <R> R execute(LdapOperation<R> operation, LdapContext context, LDAPOperationDecorator decorator) throws NamingException {
-        LdapContext context = null;
+        // We won't manage LDAP context (create and close) in case that existing context was passed as an argument to this method
+        boolean manageContext = context == null;
+
         Long start = null;
 
         try {
@@ -738,14 +746,17 @@ public class LDAPOperationManager {
                 start = Time.currentTimeMillis();
             }
 
-            context = createLdapContext();
+            if (manageContext) {
+                context = createLdapContext();
+            }
+
             if (decorator != null) {
                 decorator.beforeLDAPOperation(context, operation);
             }
 
             return operation.execute(context);
         } finally {
-            if (context != null) {
+            if (context != null && manageContext) {
                 try {
                     context.close();
                 } catch (NamingException ne) {

federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java

@@ -350,8 +350,9 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements
 
     // Send LDAP query to retrieve all groups
     protected List<LDAPObject> getAllLDAPGroups(boolean includeMemberAttribute) {
-        LDAPQuery ldapGroupQuery = createGroupQuery(includeMemberAttribute);
+        try (LDAPQuery ldapGroupQuery = createGroupQuery(includeMemberAttribute)) {
-        return LDAPUtils.loadAllLDAPObjects(ldapGroupQuery, ldapProvider);
+            return LDAPUtils.loadAllLDAPObjects(ldapGroupQuery, ldapProvider);
+        }
     }
 
 

federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/role/RoleLDAPStorageMapper.java

@@ -124,24 +124,25 @@ public class RoleLDAPStorageMapper extends AbstractLDAPStorageMapper implements
         logger.debugf("Syncing roles from LDAP into Keycloak DB. Mapper is [%s], LDAP provider is [%s]", mapperModel.getName(), ldapProvider.getModel().getName());
 
         // Send LDAP query to load all roles
-        LDAPQuery ldapRoleQuery = createRoleQuery(false);
+        try (LDAPQuery ldapRoleQuery = createRoleQuery(false)) {
-        List<LDAPObject> ldapRoles = LDAPUtils.loadAllLDAPObjects(ldapRoleQuery, ldapProvider);
+            List<LDAPObject> ldapRoles = LDAPUtils.loadAllLDAPObjects(ldapRoleQuery, ldapProvider);
 
-        RoleContainerModel roleContainer = getTargetRoleContainer(realm);
+            RoleContainerModel roleContainer = getTargetRoleContainer(realm);
-        String rolesRdnAttr = config.getRoleNameLdapAttribute();
+            String rolesRdnAttr = config.getRoleNameLdapAttribute();
-        for (LDAPObject ldapRole : ldapRoles) {
+            for (LDAPObject ldapRole : ldapRoles) {
-            String roleName = ldapRole.getAttributeAsString(rolesRdnAttr);
+                String roleName = ldapRole.getAttributeAsString(rolesRdnAttr);
 
-            if (roleContainer.getRole(roleName) == null) {
+                if (roleContainer.getRole(roleName) == null) {
-                logger.debugf("Syncing role [%s] from LDAP to keycloak DB", roleName);
+                    logger.debugf("Syncing role [%s] from LDAP to keycloak DB", roleName);
-                roleContainer.addRole(roleName);
+                    roleContainer.addRole(roleName);
-                syncResult.increaseAdded();
+                    syncResult.increaseAdded();
-            } else {
+                } else {
-                syncResult.increaseUpdated();
+                    syncResult.increaseUpdated();
+                }
             }
-        }
 
-        return syncResult;
+            return syncResult;
+        }
     }
 
 
@@ -165,32 +166,33 @@ public class RoleLDAPStorageMapper extends AbstractLDAPStorageMapper implements
         logger.debugf("Syncing roles from Keycloak into LDAP. Mapper is [%s], LDAP provider is [%s]", mapperModel.getName(), ldapProvider.getModel().getName());
 
         // Send LDAP query to see which roles exists there
-        LDAPQuery ldapQuery = createRoleQuery(false);
+        try (LDAPQuery ldapQuery = createRoleQuery(false)) {
-        List<LDAPObject> ldapRoles = LDAPUtils.loadAllLDAPObjects(ldapQuery, ldapProvider);
+            List<LDAPObject> ldapRoles = LDAPUtils.loadAllLDAPObjects(ldapQuery, ldapProvider);
 
-        Set<String> ldapRoleNames = new HashSet<>();
+            Set<String> ldapRoleNames = new HashSet<>();
-        String rolesRdnAttr = config.getRoleNameLdapAttribute();
+            String rolesRdnAttr = config.getRoleNameLdapAttribute();
-        for (LDAPObject ldapRole : ldapRoles) {
+            for (LDAPObject ldapRole : ldapRoles) {
-            String roleName = ldapRole.getAttributeAsString(rolesRdnAttr);
+                String roleName = ldapRole.getAttributeAsString(rolesRdnAttr);
-            ldapRoleNames.add(roleName);
+                ldapRoleNames.add(roleName);
-        }
-
-
-        RoleContainerModel roleContainer = getTargetRoleContainer(realm);
-        Set<RoleModel> keycloakRoles = roleContainer.getRoles();
-
-        for (RoleModel keycloakRole : keycloakRoles) {
-            String roleName = keycloakRole.getName();
-            if (ldapRoleNames.contains(roleName)) {
-                syncResult.increaseUpdated();
-            } else {
-                logger.debugf("Syncing role [%s] from Keycloak to LDAP", roleName);
-                createLDAPRole(roleName);
-                syncResult.increaseAdded();
             }
-        }
 
-        return syncResult;
+
+            RoleContainerModel roleContainer = getTargetRoleContainer(realm);
+            Set<RoleModel> keycloakRoles = roleContainer.getRoles();
+
+            for (RoleModel keycloakRole : keycloakRoles) {
+                String roleName = keycloakRole.getName();
+                if (ldapRoleNames.contains(roleName)) {
+                    syncResult.increaseUpdated();
+                } else {
+                    logger.debugf("Syncing role [%s] from Keycloak to LDAP", roleName);
+                    createLDAPRole(roleName);
+                    syncResult.increaseAdded();
+                }
+            }
+
+            return syncResult;
+        }
     }
 
     // TODO: Possible to merge with GroupMapper and move to common class