fix: handle ObjectGUID as Buffer correctly (#332)

Mixing little endian and big endian should be considered a warcrime.
https://joelitechlife.ca/2021/04/05/get-objectguid-of-microsoft-active-directory-using-ldapsearch/

src/database/database.ts

@@ -97,6 +97,7 @@ import SellerPayoutPdf from '../entity/file/seller-payout-pdf';
 import { UserTypeEnums1725196803203 } from '../migrations/1725196803203-user-type-enums';
 import { CustomInvoiceEntries1725388477226 } from '../migrations/1725388477226-custom-invoice-entries';
 import { SellerPayoutPdf1726066600389 } from '../migrations/1726066600389-seller-payout-pdf';
+import { LDAPObjectGUID1726689003147 } from '../migrations/1726689003147-ldap-objectguid';
 
 // We need to load the dotenv to prevent the env from being undefined.
 dotenv.config();
@@ -132,6 +133,7 @@ const options: DataSourceOptions = {
     UserTypeEnums1725196803203,
     CustomInvoiceEntries1725388477226,
     SellerPayoutPdf1726066600389,
+    LDAPObjectGUID1726689003147,
   ],
   extra: {
     authPlugins: {

src/entity/authenticator/ldap-authenticator.ts

@@ -23,14 +23,27 @@ import {
 } from 'typeorm';
 import AuthenticationMethod from './authentication-method';
 
+const bufferTransformer = {
+  to: (buffer: Buffer): string => {
+    // Convert Buffer to hex string when saving to DB
+    return buffer.toString('hex');
+  },
+  from: (hex: string): Buffer => {
+    // Convert hex string back to Buffer when retrieving from DB
+    return Buffer.from(hex, 'hex');
+  },
+};
+
 /**
  * @typedef {AuthenticationMethod} LDAPAuthenticator
  * @property {string} accountName.required - The associated AD account name
  */
 @Entity()
 export default class LDAPAuthenticator extends AuthenticationMethod {
   @Column({
+    type: 'varchar',
     length: 32,
+    transformer: bufferTransformer,
   })
-  public UUID: string;
+  public UUID: Buffer;
 }

src/helpers/ad.ts

@@ -38,7 +38,7 @@ export function getLDAPSettings() {
 }
 
 export interface LDAPResponse {
-  objectGUID: string,
+  objectGUID: Buffer,
   whenChanged: string,
 }
 
@@ -54,7 +54,7 @@ export interface LDAPUser {
   memberOfFlattened: string[],
   givenName: string,
   sn: string,
-  objectGUID: string,
+  objectGUID: Buffer,
   mail: string,
   mNumber: number | undefined;
 }
@@ -67,13 +67,11 @@ export interface LDAPUser {
  * @param user - The User to bind to.
  */
 export async function bindUser(manager: EntityManager,
-  ADUser: { objectGUID: string }, user: User): Promise<LDAPAuthenticator> {
-  const auth = Object.assign(new LDAPAuthenticator(), {
-    user,
+  ADUser: { objectGUID: Buffer }, user: User): Promise<LDAPAuthenticator> {
+  return manager.save(LDAPAuthenticator, {
+    user: user,
     UUID: ADUser.objectGUID,
-  }) as LDAPAuthenticator;
-  await manager.save(auth);
-  return auth;
+  });
 }
 
 /**
@@ -100,11 +98,22 @@ export async function getLDAPConnection(): Promise<Client> {
   return client;
 }
 
+export interface LDAPResult {
+  dn: string,
+  whenChanged: string,
+  memberOfFlattened: string[],
+  givenName: string,
+  sn: string,
+  objectGUID: Buffer,
+  mail: string,
+  employeeNumber: number | undefined;
+}
+
 /**
  * Wrapper for typing the untyped ldap result.
  * @param ldapResult - Search result to type
  */
-export function userFromLDAP(ldapResult: any): LDAPUser {
+export function userFromLDAP(ldapResult: LDAPResult): LDAPUser {
   const {
     dn, memberOfFlattened, givenName, sn,
     objectGUID, mail, employeeNumber, whenChanged,

src/migrations/1726689003147-ldap-objectguid.ts

@@ -0,0 +1,116 @@
+/**
+ *  SudoSOS back-end API service.
+ *  Copyright (C) 2024  Study association GEWIS
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU Affero General Public License as published
+ *  by the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU Affero General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Affero General Public License
+ *  along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ *
+ *  @license
+ */
+
+import { In, MigrationInterface, Not, QueryRunner } from 'typeorm';
+import { getLDAPConnection, LDAPResult } from '../helpers/ad';
+import ADService from '../service/ad-service';
+import LDAPAuthenticator from '../entity/authenticator/ldap-authenticator';
+import { UserType } from '../entity/user/user';
+
+export class LDAPObjectGUID1726689003147 implements MigrationInterface {
+
+  public async up(queryRunner: QueryRunner): Promise<void> {
+    // Check if ENV vars are set
+    if (!process.env.LDAP_BASE || !process.env.LDAP_USER_BASE || !process.env.LDAP_SHARED_ACCOUNT_FILTER) {
+      console.log('LDAP_BASE, LDAP_USER_BASE, LDAP_SHARED_ACCOUNT_FILTER must be for the migration to work, skipping.');
+      return;
+    }
+
+    // Get LDAP connection
+    const client = await getLDAPConnection();
+    const adService = new ADService();
+
+    const oldUsers = await client.search(process.env.LDAP_BASE, {
+      filter: `(&(objectClass=user)(objectCategory=person)(memberOf:1.2.840.113556.1.4.1941:=${process.env.LDAP_USER_BASE}))`,
+    });
+
+    const oldOrgans = await client.search(process.env.LDAP_SHARED_ACCOUNT_FILTER, {
+      filter: '(CN=*)',
+    });
+
+    const oldGUIDs = [...oldUsers.searchEntries, ...oldOrgans.searchEntries].map((entry: any) => ({
+      dn: entry.dn,
+      objectGUID: entry.objectGUID,
+    }));
+
+    // Create a map of cn to old objectGUID
+    const oldGUIDMap = new Map(oldGUIDs.map(({ dn, objectGUID }) => [dn, objectGUID]));
+
+    // Fetch new LDAP users to get new objectGUIDs
+    const newUsers = await adService.getLDAPGroupMembers(client, process.env.LDAP_USER_BASE);
+
+    const newOrgans = await adService.getLDAPGroups<LDAPResult>(client, process.env.LDAP_SHARED_ACCOUNT_FILTER);
+
+    const newGUIDs = [...newUsers.searchEntries, ...newOrgans].map((entry: any) => ({
+      dn: entry.dn,
+      objectGUID: entry.objectGUID,
+    }));
+
+    // Create a map of cn to new objectGUID
+    const newGUIDMap = new Map(newGUIDs.map(({ dn, objectGUID }) => [dn, objectGUID]));
+
+    const membersToFix = new Set<number>();
+    await queryRunner.manager.find(LDAPAuthenticator, { where: { user: { type: UserType.MEMBER } } }).then((auths) => {
+      auths.forEach((auth) => {
+        membersToFix.add(auth.userId);
+      });
+    });
+
+    const otherToFix = new Set<number>();
+    await queryRunner.manager.find(LDAPAuthenticator, { where: { user: { type: Not(UserType.MEMBER) } } }).then((auths) => {
+      auths.forEach((auth) => {
+        otherToFix.add(auth.userId);
+      });
+    });
+
+
+    for (const [dn, objectGUID] of oldGUIDMap) {
+      console.info(`Checking ${dn}`);
+      const auth = await queryRunner.manager.findOne(LDAPAuthenticator, {
+        where: { UUID: objectGUID },
+      });
+      if (!auth) throw new Error(`Could not find LDAPAuthenticator for ${dn}`);
+      const newObjectGUID = newGUIDMap.get(dn);
+      if (!newObjectGUID) throw new Error(`Could not find new objectGUID for ${dn}`);
+
+      console.error(auth.UUID, newObjectGUID);
+      membersToFix.delete(auth.userId);
+      otherToFix.delete(auth.userId);
+    }
+
+    if (membersToFix.size > 0) {
+      // These AD accounts have been unlinked and can be removed
+      const ids = Array.from(membersToFix.values());
+      console.error('Removing old LDAPAuthenticators:', ids);
+      await queryRunner.manager.delete(LDAPAuthenticator, { userId: In(ids) });
+    }
+
+    if (otherToFix.size > 0) {
+      console.error('Others to fix:', otherToFix);
+      throw new Error('Not all users have been fixed');
+    }
+  }
+
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  public async down(queryRunner: QueryRunner): Promise<void> {
+    // no-op
+  }
+
+}

src/service/ad-service.ts

@@ -18,13 +18,11 @@
  *  @license
  */
 
-import { Client } from 'ldapts';
+import { Client, SearchResult } from 'ldapts';
 import { In } from 'typeorm';
 import LDAPAuthenticator from '../entity/authenticator/ldap-authenticator';
 import User, { TermsOfServiceStatus, UserType } from '../entity/user/user';
-import {
-  bindUser, getLDAPConnection, LDAPGroup, LDAPResponse, LDAPUser, userFromLDAP,
-} from '../helpers/ad';
+import { bindUser, getLDAPConnection, LDAPGroup, LDAPResponse, LDAPResult, LDAPUser, userFromLDAP } from '../helpers/ad';
 import AuthenticationService from './authentication-service';
 import Bindings from '../helpers/bindings';
 import RoleManager from '../rbac/role-manager';
@@ -94,11 +92,15 @@ export default class ADService extends WithManager {
    */
   private async filterUnboundGUID(ldapResponses: LDAPResponse[]) {
     const ids = ldapResponses.map((s) => s.objectGUID);
-    const auths = (await this.manager.find(LDAPAuthenticator, { where: { UUID: In(ids) }, relations: ['user'] }));
+    const auths = await this.manager.find(LDAPAuthenticator, { where: { UUID: In(ids) }, relations: ['user'] });
     const existing = auths.map((l: LDAPAuthenticator) => l.UUID);
 
-    return ldapResponses
-      .filter((response) => existing.indexOf(response.objectGUID) === -1);
+    // Use Buffer.compare to filter out existing GUIDs
+    const filtered = ldapResponses.filter((response) =>
+      !existing.some((uuid) => Buffer.compare(response.objectGUID, uuid) === 0),
+    );
+
+    return filtered;
   }
 
   /**
@@ -218,7 +220,9 @@ export default class ADService extends WithManager {
   public getLDAPGroupMembers(client: Client, dn: string) {
     return client.search(process.env.LDAP_BASE, {
       filter: `(&(objectClass=user)(objectCategory=person)(memberOf:1.2.840.113556.1.4.1941:=${dn}))`,
-    });
+      explicitBufferAttributes: ['objectGUID'],
+    // This is because `search` returns the most generic response and we want to narrow it down.
+    }) as any as Promise<Pick<SearchResult, 'searchReferences'> & { searchEntries: LDAPResult[] }>;
   }
 
   /**
@@ -230,6 +234,7 @@ export default class ADService extends WithManager {
     try {
       const { searchEntries } = await client.search(baseDN, {
         filter: '(CN=*)',
+        explicitBufferAttributes: ['objectGUID'],
       });
       return searchEntries.map((e) => (e as any) as T);
     } catch (error) {

src/service/authentication-service.ts

@@ -32,7 +32,7 @@ import RoleManager from '../rbac/role-manager';
 import LDAPAuthenticator from '../entity/authenticator/ldap-authenticator';
 import MemberAuthenticator from '../entity/authenticator/member-authenticator';
 import {
-  bindUser, getLDAPConnection, getLDAPSettings, LDAPUser, userFromLDAP,
+  bindUser, getLDAPConnection, getLDAPSettings, LDAPResult, LDAPUser, userFromLDAP,
 } from '../helpers/ad';
 import { parseUserToResponse } from '../helpers/revision-to-response';
 import HashBasedAuthenticationMethod from '../entity/authenticator/hash-based-authentication-method';
@@ -268,9 +268,10 @@ export default class AuthenticationService extends WithManager {
       const { searchEntries } = await client.search(ldapSettings.base, {
         scope: 'sub',
         filter: filterstr,
+        explicitBufferAttributes: ['objectGUID'],
       });
       if (searchEntries[0]) {
-        ADUser = userFromLDAP(searchEntries[0]);
+        ADUser = userFromLDAP(searchEntries[0] as any as LDAPResult);
       } else {
         logger.trace(`User ${uid} not found in DB`);
         return undefined;

test/unit/controller/authentication-controller.ts

@@ -377,7 +377,7 @@ describe('AuthenticationController', async (): Promise<void> => {
       ],
       givenName: 'Sudo',
       sn: 'SOS',
-      objectGUID: '1',
+      objectGUID: Buffer.from('11', 'hex'),
       sAMAccountName: 'm4141',
       mail: '[email protected]',
     };

test/unit/gewis/controller/gewis-authentication-controller.ts

@@ -237,7 +237,7 @@ describe('GewisAuthenticationController', async (): Promise<void> => {
       ],
       givenName: 'Sudo',
       sn: 'SOS',
-      objectGUID: '1',
+      objectGUID: Buffer.from('11', 'hex'),
       employeeNumber: '4141',
       sAMAccountName: 'm4141',
       mail: '[email protected]',

test/unit/gewis/gewis.ts

@@ -89,7 +89,7 @@ describe('GEWIS Helper functions', async (): Promise<void> => {
       ],
       givenName: 'Sudo',
       sn: 'SOS',
-      objectGUID: '1',
+      objectGUID: Buffer.from('11', 'hex'),
       employeeNumber: '4141',
       sAMAccountName: 'm4141',
       mail: '[email protected]',
@@ -172,7 +172,9 @@ describe('GEWIS Helper functions', async (): Promise<void> => {
     it('should bind to GEWIS User if already exists', async () => {
       await inUserContext(await (await UserFactory()).clone(1), async (user: User) => {
         const ADuser = {
-          ...ctx.validADUser, givenName: `Sudo #${user.firstName}`, sn: `SOS #${user.lastName}`, objectGUID: user.id, employeeNumber: `${user.id}`,
+          ...ctx.validADUser, givenName: `Sudo #${user.firstName}`, sn: `SOS #${user.lastName}`,
+          objectGUID: Buffer.from(((user.id).toString().length % 2 ? '0' : '') + (user.id).toString(), 'hex'),
+          employeeNumber: `${user.id}`,
         };
         const userCount = await User.count();
 
@@ -207,7 +209,9 @@ describe('GEWIS Helper functions', async (): Promise<void> => {
     it('should login and create a user + GEWIS user using LDAP ', async () => {
       await inUserContext(await (await UserFactory()).clone(1), async (user: User) => {
         const ADuser = {
-          ...ctx.validADUser, givenName: `Sudo #${user.firstName}`, sn: `SOS #${user.lastName}`, objectGUID: user.id, employeeNumber: `${user.id}`,
+          ...ctx.validADUser, givenName: `Sudo #${user.firstName}`, sn: `SOS #${user.lastName}`,
+          objectGUID: Buffer.from(((user.id).toString().length % 2 ? '0' : '') + (user.id).toString(), 'hex'),
+          employeeNumber: `${user.id}`,
         };
         let DBUser = await User.findOne(
           { where: { firstName: ctx.validADUser.givenName, lastName: ctx.validADUser.sn } },