Clean LDAP group search impl async/await

2026-09-22 13:39:35 +00:00 · 2024-04-24 08:15:19 -07:00
parent 1c95b3abe7
commit 3ca596d4af
6 changed files with 138 additions and 144 deletions
--- a/backend/src/ee/routes/v1/ldap-router.ts
+++ b/backend/src/ee/routes/v1/ldap-router.ts
@@ -11,11 +11,11 @@ import { IncomingMessage } from "node:http";
 import { Authenticator } from "@fastify/passport";
 import fastifySession from "@fastify/session";
 import { FastifyRequest } from "fastify";
-import ldapjs from "ldapjs";
 import LdapStrategy from "passport-ldapauth";
 import { z } from "zod";

 import { LdapConfigsSchema, LdapGroupMapsSchema } from "@app/db/schemas";
+import { TLDAPConfig } from "@app/ee/services/ldap-config/ldap-config-types";
 import { searchGroups } from "@app/ee/services/ldap-config/ldap-fns";
 import { getConfig } from "@app/lib/config/env";
 import { logger } from "@app/lib/logger";
@@ -46,91 +46,36 @@ export const registerLdapRouter = async (server: FastifyZodProvider) => {
    });
  };

-  interface LDAPConfig {
-    id: string;
-    organization: string;
-    isActive: boolean;
-    url: string;
-    bindDN: string;
-    bindPass: string;
-    searchBase: string;
-    groupSearchBase: string;
-    groupSearchFilter: string;
-    caCert: string;
-  }
-
  passport.use(
    new LdapStrategy(
      getLdapPassportOpts as any,
      // eslint-disable-next-line
      async (req: IncomingMessage, user, cb) => {
        try {
-          const ldapConfig = (req as unknown as FastifyRequest).ldapConfig as LDAPConfig;
-
-          if (!ldapConfig.groupSearchFilter || !ldapConfig.groupSearchBase) {
-            // If group search values are not provided, proceed directly to LDAP login
-            return await server.services.ldap
-              .ldapLogin({
-                ldapConfigId: ldapConfig.id,
-                externalId: user.uidNumber,
-                username: user.uid,
-                firstName: user.givenName ?? user.cn ?? "",
-                lastName: user.sn ?? "",
-                emails: user.mail ? [user.mail] : [],
-                relayState: ((req as unknown as FastifyRequest).body as { RelayState?: string }).RelayState,
-                orgId: (req as unknown as FastifyRequest).ldapConfig.organization
-              })
-              .then(({ isUserCompleted, providerAuthToken }) => {
-                cb(null, { isUserCompleted, providerAuthToken });
-              })
-              .catch((err) => {
-                logger.error(err);
-                cb(err, false);
-              });
-          }
-
-          // query for groups
-          const ldapClient = ldapjs.createClient({
-            url: ldapConfig.url,
-            bindDN: ldapConfig.bindDN,
-            bindCredentials: ldapConfig.bindPass,
-            ...(ldapConfig.caCert !== ""
-              ? {
-                  tlsOptions: {
-                    ca: [ldapConfig.caCert]
-                  }
-                }
-              : {})
-          });
+          const ldapConfig = (req as unknown as FastifyRequest).ldapConfig as TLDAPConfig;

          const groupFilter = "(|(memberUid={{.Username}})(member={{.UserDN}})(uniqueMember={{.UserDN}}))";
          const searchFilter =
            ldapConfig.groupSearchFilter ||
            groupFilter.replace("{{.Username}}", user.uid).replace("{{.UserDN}}", user.dn);

-          searchGroups(ldapClient, searchFilter, ldapConfig.groupSearchBase)
-            .then((groups) => {
-              ldapClient.unbind();
-              return server.services.ldap.ldapLogin({
-                ldapConfigId: ldapConfig.id,
-                externalId: user.uidNumber,
-                username: user.uid,
-                firstName: user.givenName ?? user.cn ?? "",
-                lastName: user.sn ?? "",
-                emails: user.mail ? [user.mail] : [],
-                groups,
-                relayState: ((req as unknown as FastifyRequest).body as { RelayState?: string }).RelayState,
-                orgId: (req as unknown as FastifyRequest).ldapConfig.organization
-              });
-            })
-            .then(({ isUserCompleted, providerAuthToken }) => {
-              cb(null, { isUserCompleted, providerAuthToken });
-            })
-            .catch((err2) => {
-              ldapClient.unbind();
-              logger.error(err2);
-              cb(err2, false);
-            });
+          const shouldProcessGroups = ldapConfig.groupSearchFilter && ldapConfig.groupSearchBase;
+
+          const { isUserCompleted, providerAuthToken } = await server.services.ldap.ldapLogin({
+            ldapConfigId: ldapConfig.id,
+            externalId: user.uidNumber,
+            username: user.uid,
+            firstName: user.givenName ?? user.cn ?? "",
+            lastName: user.sn ?? "",
+            emails: user.mail ? [user.mail] : [],
+            groups: shouldProcessGroups
+              ? await searchGroups(ldapConfig, searchFilter, ldapConfig.groupSearchBase)
+              : undefined,
+            relayState: ((req as unknown as FastifyRequest).body as { RelayState?: string }).RelayState,
+            orgId: (req as unknown as FastifyRequest).ldapConfig.organization
+          });
+
+          return cb(null, { isUserCompleted, providerAuthToken });
        } catch (error) {
          logger.error(error);
          return cb(error, false);
@@ -220,8 +165,8 @@ export const registerLdapRouter = async (server: FastifyZodProvider) => {
        bindDN: z.string().trim(),
        bindPass: z.string().trim(),
        searchBase: z.string().trim(),
-        groupSearchBase: z.string().trim(),
-        groupSearchFilter: z.string().trim(),
+        groupSearchBase: z.string().trim().default(""),
+        groupSearchFilter: z.string().trim().default(""),
        caCert: z.string().trim().default("")
      }),
      response: {
--- a/backend/src/ee/services/ldap-config/ldap-config-service.ts
+++ b/backend/src/ee/services/ldap-config/ldap-config-service.ts
@@ -450,79 +450,84 @@ export const ldapConfigServiceFactory = ({
      });
    }

-    const user = await userDAL.findOne({ id: userAlias.userId });
+    const user = await userDAL.transaction(async (tx) => {
+      const newUser = await userDAL.findOne({ id: userAlias.userId }, tx);
+      if (groups) {
+        const ldapGroupIdsToBePartOf = (
+          await ldapGroupMapDAL.find({
+            ldapConfigId,
+            $in: {
+              ldapGroupCN: groups.map((group) => group.cn)
+            }
+          })
+        ).map((groupMap) => groupMap.groupId);

-    if (groups) {
-      const ldapGroupIdsToBePartOf = (
-        await ldapGroupMapDAL.find({
-          ldapConfigId,
+        const groupsToBePartOf = await groupDAL.find({
+          orgId,
          $in: {
-            ldapGroupCN: groups.map((group) => group.cn)
+            id: ldapGroupIdsToBePartOf
          }
-        })
-      ).map((groupMap) => groupMap.groupId);
+        });
+        const toBePartOfGroupIdsSet = new Set(groupsToBePartOf.map((groupToBePartOf) => groupToBePartOf.id));

-      const groupsToBePartOf = await groupDAL.find({
-        orgId,
-        $in: {
-          id: ldapGroupIdsToBePartOf
+        const allLdapGroupMaps = await ldapGroupMapDAL.find({
+          ldapConfigId
+        });
+
+        const ldapGroupIdsCurrentlyPartOf = (
+          await userGroupMembershipDAL.find({
+            userId: newUser.id,
+            $in: {
+              groupId: allLdapGroupMaps.map((groupMap) => groupMap.groupId)
+            }
+          })
+        ).map((userGroupMembership) => userGroupMembership.groupId);
+
+        const userGroupMembershipGroupIdsSet = new Set(ldapGroupIdsCurrentlyPartOf);
+
+        for await (const group of groupsToBePartOf) {
+          if (!userGroupMembershipGroupIdsSet.has(group.id)) {
+            // add user to group that they should be part of
+            await addUsersToGroupByUserIds({
+              group,
+              userIds: [newUser.id],
+              userDAL,
+              userGroupMembershipDAL,
+              orgDAL,
+              groupProjectDAL,
+              projectKeyDAL,
+              projectDAL,
+              projectBotDAL,
+              tx
+            });
+          }
        }
-      });
-      const toBePartOfGroupIdsSet = new Set(groupsToBePartOf.map((groupToBePartOf) => groupToBePartOf.id));

-      const allLdapGroupMaps = await ldapGroupMapDAL.find({
-        ldapConfigId
-      });
-
-      const ldapGroupIdsCurrentlyPartOf = (
-        await userGroupMembershipDAL.find({
-          userId: user.id,
+        const groupsCurrentlyPartOf = await groupDAL.find({
+          orgId,
          $in: {
-            groupId: allLdapGroupMaps.map((groupMap) => groupMap.groupId)
+            id: ldapGroupIdsCurrentlyPartOf
          }
-        })
-      ).map((userGroupMembership) => userGroupMembership.groupId);
+        });

-      const userGroupMembershipGroupIdsSet = new Set(ldapGroupIdsCurrentlyPartOf);
-
-      for await (const group of groupsToBePartOf) {
-        if (!userGroupMembershipGroupIdsSet.has(group.id)) {
-          // add user to group that they should be part of
-          await addUsersToGroupByUserIds({
-            group,
-            userIds: [user.id],
-            userDAL,
-            userGroupMembershipDAL,
-            orgDAL,
-            groupProjectDAL,
-            projectKeyDAL,
-            projectDAL,
-            projectBotDAL
-          });
+        for await (const group of groupsCurrentlyPartOf) {
+          if (!toBePartOfGroupIdsSet.has(group.id)) {
+            // remove user from group that they should no longer be part of
+            await removeUsersFromGroupByUserIds({
+              group,
+              userIds: [newUser.id],
+              userDAL,
+              userGroupMembershipDAL,
+              groupProjectDAL,
+              projectKeyDAL,
+              tx
+            });
+          }
        }
      }

-      const groupsCurrentlyPartOf = await groupDAL.find({
-        orgId,
-        $in: {
-          id: ldapGroupIdsCurrentlyPartOf
-        }
-      });
-
-      for await (const group of groupsCurrentlyPartOf) {
-        if (!toBePartOfGroupIdsSet.has(group.id)) {
-          // remove user from group that they should no longer be part of
-          await removeUsersFromGroupByUserIds({
-            group,
-            userIds: [user.id],
-            userDAL,
-            userGroupMembershipDAL,
-            groupProjectDAL,
-            projectKeyDAL
-          });
-        }
-      }
-    }
+      return newUser;
+    });

    const isUserCompleted = Boolean(user.isAccepted);

--- a/backend/src/ee/services/ldap-config/ldap-config-types.ts
+++ b/backend/src/ee/services/ldap-config/ldap-config-types.ts
@@ -1,5 +1,18 @@
 import { TOrgPermission } from "@app/lib/types";

+export type TLDAPConfig = {
+  id: string;
+  organization: string;
+  isActive: boolean;
+  url: string;
+  bindDN: string;
+  bindPass: string;
+  searchBase: string;
+  groupSearchBase: string;
+  groupSearchFilter: string;
+  caCert: string;
+};
+
 export type TCreateLdapCfgDTO = {
  orgId: string;
  isActive: boolean;
--- a/backend/src/ee/services/ldap-config/ldap-fns.ts
+++ b/backend/src/ee/services/ldap-config/ldap-fns.ts
@@ -1,11 +1,28 @@
-import ldap from "ldapjs";
+import ldapjs from "ldapjs";
+
+import { logger } from "@app/lib/logger";
+
+import { TLDAPConfig } from "./ldap-config-types";

 export const searchGroups = async (
-  ldapClient: ldap.Client,
+  ldapConfig: TLDAPConfig,
  filter: string,
  base: string
 ): Promise<{ dn: string; cn: string }[]> => {
  return new Promise((resolve, reject) => {
+    const ldapClient = ldapjs.createClient({
+      url: ldapConfig.url,
+      bindDN: ldapConfig.bindDN,
+      bindCredentials: ldapConfig.bindPass,
+      ...(ldapConfig.caCert !== ""
+        ? {
+            tlsOptions: {
+              ca: [ldapConfig.caCert]
+            }
+          }
+        : {})
+    });
+
    ldapClient.search(
      base,
      {
@@ -14,6 +31,11 @@ export const searchGroups = async (
      },
      (err, res) => {
        if (err) {
+          ldapClient.unbind((unbindError) => {
+            if (unbindError) {
+              logger.error("Error unbinding LDAP client:", unbindError);
+            }
+          });
          return reject(err);
        }

@@ -29,10 +51,19 @@ export const searchGroups = async (
          groups.push({ dn, cn });
        });
        res.on("error", (error) => {
-          console.error(`error: ${error.message}`);
+          ldapClient.unbind((unbindError) => {
+            if (unbindError) {
+              logger.error("Error unbinding LDAP client:", unbindError);
+            }
+          });
          reject(error);
        });
        res.on("end", () => {
+          ldapClient.unbind((unbindError) => {
+            if (unbindError) {
+              logger.error("Error unbinding LDAP client:", unbindError);
+            }
+          });
          resolve(groups);
        });
      }
--- a/backend/src/ee/services/ldap-config/ldap-group-map-dal.ts
+++ b/backend/src/ee/services/ldap-config/ldap-group-map-dal.ts
@@ -16,8 +16,8 @@ export const ldapGroupMapDALFactory = (db: TDbClient) => {
        .select(selectAllTableCols(TableName.LdapGroupMap))
        .select(
          db.ref("id").withSchema(TableName.Groups).as("groupId"),
-          db.ref("name").withSchema(TableName.Groups).as("groupSlug"),
-          db.ref("slug").withSchema(TableName.Groups).as("groupName")
+          db.ref("name").withSchema(TableName.Groups).as("groupName"),
+          db.ref("slug").withSchema(TableName.Groups).as("groupSlug")
        );

      return docs.map((doc) => {
--- a/frontend/src/views/Settings/OrgSettingsPage/components/OrgAuthTab/LDAPGroupMapModal.tsx
+++ b/frontend/src/views/Settings/OrgSettingsPage/components/OrgAuthTab/LDAPGroupMapModal.tsx
@@ -201,11 +201,11 @@ export const LDAPGroupMapModal = ({ popUp, handlePopUpOpen, handlePopUpToggle }:
            <TBody>
              {isLoading && <TableSkeleton columns={3} innerKey="ldap-group-maps" />}
              {!isLoading &&
-                groupMaps?.map(({ id, ldapGroupCN, group: { name } }) => {
+                groupMaps?.map(({ id, ldapGroupCN, group }) => {
                  return (
                    <Tr className="h-10 items-center" key={`ldap-group-map-${id}`}>
                      <Td>{ldapGroupCN}</Td>
-                      <Td>{name}</Td>
+                      <Td>{group.name}</Td>
                      <Td>
                        <IconButton
                          onClick={() => {