Merge pull request #8026 from weseek/fix/128041-129379-large-ldap-group-tree-sync-error

Fix/128041 129379 large ldap group tree sync error

apps/app/src/features/external-user-group/server/service/external-user-group-sync.ts

@@ -65,7 +65,7 @@ abstract class ExternalUserGroupSyncService {
    * 2. For every element in node.userInfos, call getMemberUser and create an ExternalUserGroupRelation with ExternalUserGroup if it does not have one
    * 3. Retrun ExternalUserGroup
    * @param {string} node Node of external group tree
-   * @param {string} parentId Parent group id (id in GROWI) of the group we wan't to create/update
+   * @param {string} parentId Parent group id (id in GROWI) of the group we want to create/update
    * @returns {Promise<IExternalUserGroupHasId>} ExternalUserGroup that was created/updated
   */
   async createUpdateExternalUserGroup(node: ExternalUserGroupTreeNode, parentId?: string): Promise<IExternalUserGroupHasId> {

apps/app/src/features/external-user-group/server/service/ldap-user-group-sync.ts

@@ -1,6 +1,7 @@
 import { configManager } from '~/server/service/config-manager';
 import LdapService, { SearchResultEntry } from '~/server/service/ldap';
 import PassportService from '~/server/service/passport';
+import loggerFactory from '~/utils/logger';
 import { batchProcessPromiseAll } from '~/utils/promise';
 
 import {
@@ -9,6 +10,8 @@ import {
 
 import ExternalUserGroupSyncService from './external-user-group-sync';
 
+const logger = loggerFactory('growi:service:ldap-user-sync-service');
+
 // When d = max depth of group trees
 // Max space complexity of generateExternalUserGroupTrees will be:
 // O(TREES_BATCH_SIZE * d * USERS_BATCH_SIZE)
@@ -37,9 +40,11 @@ class LdapUserGroupSyncService extends ExternalUserGroupSyncService {
 
     let groupEntries: SearchResultEntry[];
     try {
+      await this.ldapService.bind();
       groupEntries = await this.ldapService.searchGroupDir();
     }
     catch (e) {
+      logger.error(e.message);
       throw Error('external_user_group.ldap.group_search_failed');
     }
 
@@ -123,6 +128,7 @@ class LdapUserGroupSyncService extends ExternalUserGroupSyncService {
       userEntries = await getUserEntries();
     }
     catch (e) {
+      logger.error(e.message);
       throw Error('external_user_group.ldap.user_search_failed');
     }
 

apps/app/src/server/service/ldap.ts

@@ -27,20 +27,38 @@ class LdapService {
 
   password?: string; // Necessary when bind type is user bind
 
+  client: ldap.Client;
+
+  searchBase: string;
+
   constructor(username?: string, password?: string) {
+    const serverUrl = configManager?.getConfig('crowi', 'security:passport-ldap:serverUrl');
+
     this.username = username;
     this.password = password;
+
+    // parse serverUrl
+    // see: https://regex101.com/r/0tuYBB/1
+    const match = serverUrl.match(/(ldaps?:\/\/[^/]+)\/(.*)?/);
+    if (match == null || match.length < 1) {
+      const urlInvalidMessage = 'serverUrl is invalid';
+      logger.error(urlInvalidMessage);
+      throw new Error(urlInvalidMessage);
+    }
+    const url = match[1];
+    this.searchBase = match[2] || '';
+
+    this.client = ldap.createClient({
+      url,
+    });
   }
 
   /**
-   * Execute search on LDAP server and return result
-   * @param {string} filter Search filter
-   * @param {string} base Base DN to execute search on
-   * @returns {SearchEntry[]} Search result. Default scope is set to 'sub'.
+   * Bind to LDAP server.
+   * This method is declared independently, so multiple operations can be requested to the LDAP server with a single bind.
    */
-  search(filter?: string, base?: string, scope: 'sub' | 'base' | 'one' = 'sub'): Promise<SearchResultEntry[]> {
+  bind(): Promise<void> {
     const isLdapEnabled = configManager?.getConfig('crowi', 'security:passport-ldap:isEnabled');
-
     if (!isLdapEnabled) {
       const notEnabledMessage = 'LDAP is not enabled';
       logger.error(notEnabledMessage);
@@ -49,41 +67,44 @@ class LdapService {
 
     // get configurations
     const isUserBind = configManager?.getConfig('crowi', 'security:passport-ldap:isUserBind');
-    const serverUrl = configManager?.getConfig('crowi', 'security:passport-ldap:serverUrl');
     const bindDN = configManager?.getConfig('crowi', 'security:passport-ldap:bindDN');
     const bindCredentials = configManager?.getConfig('crowi', 'security:passport-ldap:bindDNPassword');
 
-    // parse serverUrl
-    // see: https://regex101.com/r/0tuYBB/1
-    const match = serverUrl.match(/(ldaps?:\/\/[^/]+)\/(.*)?/);
-    if (match == null || match.length < 1) {
-      const urlInvalidMessage = 'serverUrl is invalid';
-      logger.error(urlInvalidMessage);
-      throw new Error(urlInvalidMessage);
-    }
-    const url = match[1];
-    const searchBase = match[2] || '';
-
     // user bind
     const fixedBindDN = (isUserBind)
       ? bindDN.replace(/{{username}}/, this.username)
       : bindDN;
     const fixedBindCredentials = (isUserBind) ? this.password : bindCredentials;
 
-    const client = ldap.createClient({
-      url,
+    return new Promise<void>((resolve, reject) => {
+      this.client.bind(fixedBindDN, fixedBindCredentials, (err) => {
+        if (err != null) {
+          reject(err);
+        }
+        resolve();
+      });
     });
+  }
 
+  /**
+   * Execute search on LDAP server and return result
+   * Execution of bind() is necessary before search
+   * @param {string} filter Search filter
+   * @param {string} base Base DN to execute search on
+   * @returns {SearchEntry[]} Search result. Default scope is set to 'sub'.
+   */
+  search(filter?: string, base?: string, scope: 'sub' | 'base' | 'one' = 'sub'): Promise<SearchResultEntry[]> {
     const searchResults: SearchResultEntry[] = [];
 
     return new Promise((resolve, reject) => {
-      client.bind(fixedBindDN, fixedBindCredentials, (err) => {
-        if (err != null) {
-          reject(err);
-        }
+      // reject on client connection error (occures when not binded or host is not found)
+      this.client.on('error', (err) => {
+        reject(err);
       });
 
-      client.search(base || searchBase, { scope, filter }, (err, res) => {
+      this.client.search(base || this.searchBase, {
+        scope, filter, paged: true, sizeLimit: 200,
+      }, (err, res) => {
         if (err != null) {
           reject(err);
         }

apps/app/test/integration/service/ldap-user-group-sync.test.ts

@@ -1,9 +1,12 @@
+import ldap, { Client } from 'ldapjs';
+
 import LdapUserGroupSyncService from '../../../src/features/external-user-group/server/service/ldap-user-group-sync';
 import { configManager } from '../../../src/server/service/config-manager';
 import LdapService from '../../../src/server/service/ldap';
 import PassportService from '../../../src/server/service/passport';
 import { getInstance } from '../setup-crowi';
 
+
 describe('LdapUserGroupSyncService.generateExternalUserGroupTrees', () => {
   let crowi;
   let ldapGroupSyncService: LdapUserGroupSyncService;
@@ -16,17 +19,25 @@ describe('LdapUserGroupSyncService.generateExternalUserGroupTrees', () => {
     'external-user-group:ldap:groupDescriptionAttribute': 'description',
     'external-user-group:ldap:groupMembershipAttributeType': 'DN',
     'external-user-group:ldap:groupSearchBase': 'ou=groups,dc=example,dc=org',
+    'security:passport-ldap:serverUrl': 'ldap://openldap:1389/dc=example,dc=org',
   };
 
   jest.mock('../../../src/server/service/ldap');
+  const mockBind = jest.spyOn(LdapService.prototype, 'bind');
   const mockLdapSearch = jest.spyOn(LdapService.prototype, 'search');
+  const mockLdapCreateClient = jest.spyOn(ldap, 'createClient');
 
   beforeAll(async() => {
     crowi = await getInstance();
+    await configManager.updateConfigsInTheSameNamespace('crowi', configParams, true);
+
+    mockBind.mockImplementation(() => {
+      return Promise.resolve();
+    });
+    mockLdapCreateClient.mockImplementation(() => { return {} as Client });
+
     const passportService = new PassportService(crowi);
     ldapGroupSyncService = new LdapUserGroupSyncService(passportService);
-
-    await configManager.updateConfigsInTheSameNamespace('crowi', configParams, true);
   });
 
   describe('When there is no circular reference in group tree', () => {