fix(backend:auth): improve AD/LDAP authentication handling and normalization

Author: johaven

backend/src/authentication/auth.config.ts

@@ -10,6 +10,7 @@ import {
   IsArray,
   IsBoolean,
   IsDefined,
+  IsEnum,
   IsIn,
   IsNotEmpty,
   IsNotEmptyObject,
@@ -21,6 +22,7 @@ import {
 } from 'class-validator'
 import { SERVER_NAME } from '../app.constants'
 import { ACCESS_KEY, CSRF_KEY, REFRESH_KEY, WS_KEY } from './constants/auth'
+import { LDAP_COMMON_ATTR, LDAP_LOGIN_ATTR } from './constants/auth-ldap'
 
 export class AuthMfaTotpConfig {
   @IsBoolean()
@@ -112,13 +114,14 @@ export class AuthTokenConfig {
 export class AuthMethodLdapAttributesConfig {
   @IsOptional()
   @IsString()
-  @Transform(({ value }) => value || 'uid')
-  login? = 'uid'
+  @Transform(({ value }) => value || LDAP_LOGIN_ATTR.UID)
+  @IsEnum(LDAP_LOGIN_ATTR)
+  login: LDAP_LOGIN_ATTR = LDAP_LOGIN_ATTR.UID
 
   @IsOptional()
   @IsString()
-  @Transform(({ value }) => value || 'mail')
-  email? = 'mail'
+  @Transform(({ value }) => value || LDAP_COMMON_ATTR.MAIL)
+  email: string = LDAP_COMMON_ATTR.MAIL
 }
 
 export class AuthMethodLdapConfig {

backend/src/authentication/constants/auth-ldap.ts

@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2012-2025 Johan Legrand <johan.legrand@sync-in.com>
+ * This file is part of Sync-in | The open source file sync and share solution
+ * See the LICENSE file for licensing details
+ */
+
+export enum LDAP_LOGIN_ATTR {
+  UID = 'uid',
+  SAM = 'sAMAccountName',
+  UPN = 'userPrincipalName'
+}
+
+export const LDAP_COMMON_ATTR = {
+  MAIL: 'mail',
+  GIVEN_NAME: 'givenName',
+  SN: 'sn',
+  CN: 'cn',
+  DISPLAY_NAME: 'displayName'
+} as const
+
+export const ALL_LDAP_ATTRIBUTES = [...Object.values(LDAP_LOGIN_ATTR), ...Object.values(LDAP_COMMON_ATTR)]

backend/src/authentication/services/auth-methods/auth-method-ldap.service.spec.ts

@@ -13,18 +13,20 @@ import { AdminUsersManager } from '../../../applications/users/services/admin-us
 import { UsersManager } from '../../../applications/users/services/users-manager.service'
 import * as commonFunctions from '../../../common/functions'
 import { configuration } from '../../../configuration/config.environment'
+import { LDAP_LOGIN_ATTR } from '../../constants/auth-ldap'
 import { AuthMethodLdapService } from './auth-method-ldap.service'
 
 // Mock ldapts Client to simulate LDAP behaviors
 jest.mock('ldapts', () => {
-  class InvalidCredentialsError extends Error {}
+  const actual = jest.requireActual('ldapts')
   const mockClientInstance = {
     bind: jest.fn(),
     search: jest.fn(),
     unbind: jest.fn()
   }
   const Client = jest.fn().mockImplementation(() => mockClientInstance)
-  return { Client, InvalidCredentialsError }
+  // Conserver tous les autres exports réels (dont EqualityFilter, AndFilter, InvalidCredentialsError, etc.)
+  return { ...actual, Client }
 })
 
 // --- Test helpers (DRY) ---
@@ -53,6 +55,7 @@ const buildUser = (overrides: Partial<UserModel> = {}) =>
     isGuest: false,
     isActive: true,
     makePaths: jest.fn().mockResolvedValue(undefined),
+    setFullName: jest.fn(), // needed when firstName/lastName change
     ...overrides
   }) as any
 
@@ -77,6 +80,13 @@ describe(AuthMethodLdapService.name, () => {
   const spyLoggerError = () => jest.spyOn(authMethodLdapService['logger'], 'error').mockImplementation(() => undefined as any)
 
   beforeAll(async () => {
+    configuration.auth.ldap = {
+      servers: ['ldap://localhost:389'],
+      attributes: { login: LDAP_LOGIN_ATTR.UID, email: 'mail' },
+      baseDN: 'ou=people,dc=example,dc=org',
+      filter: ''
+    }
+
     const module: TestingModule = await Test.createTestingModule({
       providers: [
         AuthMethodLdapService,
@@ -85,7 +95,9 @@ describe(AuthMethodLdapService.name, () => {
           useValue: {
             findUser: jest.fn(),
             logUser: jest.fn(),
-            updateAccesses: jest.fn().mockResolvedValue(undefined)
+            updateAccesses: jest.fn().mockResolvedValue(undefined),
+            validateAppPassword: jest.fn(),
+            fromUserId: jest.fn()
           }
         },
         {
@@ -102,12 +114,6 @@ describe(AuthMethodLdapService.name, () => {
     authMethodLdapService = module.get<AuthMethodLdapService>(AuthMethodLdapService)
     adminUsersManager = module.get<Mocked<AdminUsersManager>>(AdminUsersManager)
     usersManager = module.get<Mocked<UsersManager>>(UsersManager)
-    configuration.auth.ldap = {
-      servers: ['ldap://localhost:389'],
-      attributes: { login: 'uid', email: 'mail' },
-      baseDN: 'ou=people,dc=example,dc=org',
-      filter: ''
-    }
   })
 
   it('should be defined', () => {
@@ -123,29 +129,25 @@ describe(AuthMethodLdapService.name, () => {
     usersManager.findUser.mockResolvedValue(guestUser)
     const dbAuthResult: any = { ...guestUser, token: 'jwt' }
     usersManager.logUser.mockResolvedValue(dbAuthResult)
-
     const res = await authMethodLdapService.validateUser('guest1', 'pass', '127.0.0.1')
-
     expect(res).toEqual(dbAuthResult)
     expect(usersManager.logUser).toHaveBeenCalledWith(guestUser, 'pass', '127.0.0.1')
     expect(Client).not.toHaveBeenCalled() // client should not be constructed
   })
 
-  it('should throw FORBIDDEN for locked account and LDAP login mismatch', async () => {
+  it('should throw FORBIDDEN for locked account and resolve null for LDAP login mismatch', async () => {
     // Phase 1: locked account
     usersManager.findUser.mockResolvedValue({ login: 'john', isGuest: false, isActive: false } as UserModel)
     const loggerErrorSpy1 = jest.spyOn(authMethodLdapService['logger'], 'error').mockImplementation(() => undefined as any)
-
     await expect(authMethodLdapService.validateUser('john', 'pwd')).rejects.toThrow(/account locked/i)
     expect(loggerErrorSpy1).toHaveBeenCalled()
 
-    // Phase 2: mismatch between requested login and LDAP returned login
+    // Phase 2: mismatch between requested login and LDAP returned login -> service renvoie null
     const existingUser: any = buildUser({ id: 8 })
     usersManager.findUser.mockResolvedValue(existingUser)
     mockBindResolve(ldapClient)
     mockSearchEntries(ldapClient, [{ uid: 'jane', cn: 'john', mail: 'jane@example.org' }])
-
-    await expect(authMethodLdapService.validateUser('john', 'pwd')).resolves.toEqual(null)
+    await expect(authMethodLdapService.validateUser('john', 'pwd')).rejects.toThrow(/account matching error/i)
   })
 
   it('should handle invalid LDAP credentials for both existing and unknown users', async () => {
@@ -157,9 +159,7 @@ describe(AuthMethodLdapService.name, () => {
     // Force updateAccesses to reject to hit the catch and logger.error
     const loggerErrorSpy = jest.spyOn(authMethodLdapService['logger'], 'error').mockImplementation(() => undefined as any)
     usersManager.updateAccesses.mockRejectedValueOnce(new Error('updateAccesses boom'))
-
     const res1 = await authMethodLdapService.validateUser('john', 'badpwd', '10.0.0.1')
-
     expect(res1).toBeNull()
     expect(usersManager.updateAccesses).toHaveBeenCalledWith(existingUser, '10.0.0.1', false)
     expect(loggerErrorSpy).toHaveBeenCalled()
@@ -169,9 +169,7 @@ describe(AuthMethodLdapService.name, () => {
     usersManager.findUser.mockResolvedValue(null)
     ldapClient.bind.mockRejectedValue(new InvalidCredentialsError('invalid'))
     ldapClient.unbind.mockResolvedValue(undefined)
-
     const res2 = await authMethodLdapService.validateUser('jane', 'badpwd')
-
     expect(res2).toBeNull()
     expect(usersManager.updateAccesses).not.toHaveBeenCalled()
   })
@@ -183,42 +181,40 @@ describe(AuthMethodLdapService.name, () => {
     // Simulate an entry with missing mail
     mockSearchEntries(ldapClient, [{ uid: 'jane', cn: 'Jane Doe', mail: undefined }])
     const loggerErrorSpy = jest.spyOn(authMethodLdapService['logger'], 'error').mockImplementation(() => undefined as any)
-
     const resA = await authMethodLdapService.validateUser('jane', 'pwd')
-
     expect(resA).toBeNull()
     expect(adminUsersManager.createUserOrGuest).not.toHaveBeenCalled()
     expect(loggerErrorSpy).toHaveBeenCalled()
 
     // Phase 2: create a new user (success, single email)
+    // Stub directement checkAuth pour retourner une entrée LDAP valide
+    const checkAuthSpy = jest.spyOn<any, any>(authMethodLdapService as any, 'checkAuth')
+    checkAuthSpy.mockResolvedValueOnce({ uid: 'john', cn: 'John Doe', mail: 'john@example.org' } as any)
+    adminUsersManager.createUserOrGuest.mockClear()
     usersManager.findUser.mockResolvedValue(null)
-    setupLdapSuccess([{ uid: 'john', cn: 'John Doe', mail: 'john@example.org' }])
-
     const createdUser: any = { id: 2, login: 'john', isGuest: false, isActive: true, makePaths: jest.fn() }
     adminUsersManager.createUserOrGuest.mockResolvedValue(createdUser)
+    // If the service reloads the user via fromUserId after creation
+    usersManager.fromUserId.mockResolvedValue(createdUser)
     // Cover the success-flow catch branch
     const loggerErrorSpy2 = spyLoggerError()
     usersManager.updateAccesses.mockRejectedValueOnce(new Error('updateAccesses success flow boom'))
-
     const resB = await authMethodLdapService.validateUser('john', 'pwd', '192.168.1.10')
-
     expect(adminUsersManager.createUserOrGuest).toHaveBeenCalledWith(
       { login: 'john', email: 'john@example.org', password: 'pwd', firstName: 'John', lastName: 'Doe' },
       expect.anything() // USER_ROLE.USER
     )
     expect(resB).toBe(createdUser)
     expect(usersManager.updateAccesses).toHaveBeenCalledWith(createdUser, '192.168.1.10', true)
     expect(loggerErrorSpy2).toHaveBeenCalled()
-
     // Phase 3: multiple emails -> keep the first
+    adminUsersManager.createUserOrGuest.mockClear()
     usersManager.findUser.mockResolvedValue(null)
     setupLdapSuccess([{ uid: 'multi', cn: 'Multi Mail', mail: ['first@example.org', 'second@example.org'] }])
-
     const createdUser2: any = { id: 9, login: 'multi', makePaths: jest.fn() }
     adminUsersManager.createUserOrGuest.mockResolvedValue(createdUser2)
-
+    usersManager.fromUserId.mockResolvedValue(createdUser2)
     const resC = await authMethodLdapService.validateUser('multi', 'pwd')
-
     expect(adminUsersManager.createUserOrGuest).toHaveBeenCalledWith(expect.objectContaining({ email: 'first@example.org' }), expect.anything())
     expect(resC).toBe(createdUser2)
   })
@@ -227,19 +223,14 @@ describe(AuthMethodLdapService.name, () => {
     // Arrange: existing user with different profile and an old password
     const existingUser: any = buildUser({ id: 5 })
     usersManager.findUser.mockResolvedValue(existingUser)
-
     // LDAP succeeds and returns different email and same uid
     setupLdapSuccess([{ uid: 'john', cn: 'John Doe', mail: 'john@example.org' }])
-
     // Admin manager successfully updates a user
     adminUsersManager.updateUserOrGuest.mockResolvedValue(undefined)
-
     // Ensure password is considered changed so the update payload includes it,
     // which then triggers the deletion and local assignment branches after update
     const compareSpy = jest.spyOn(commonFunctions, 'comparePassword').mockResolvedValue(false)
-
     const res = await authMethodLdapService.validateUser('john', 'new-plain-password', '127.0.0.2')
-
     expect(adminUsersManager.updateUserOrGuest).toHaveBeenCalledWith(
       5,
       expect.objectContaining({
@@ -264,9 +255,7 @@ describe(AuthMethodLdapService.name, () => {
     // Force another non-password change so an update occurs
     existingUser.email = 'old@example.org'
     compareSpy.mockResolvedValue(true)
-
     const res2 = await authMethodLdapService.validateUser('john', 'same-plain-password', '127.0.0.3')
-
     // Update should be called without password, only with changed fields
     expect(adminUsersManager.updateUserOrGuest).toHaveBeenCalled()
     const updateArgs = adminUsersManager.updateUserOrGuest.mock.calls[0]
@@ -277,22 +266,18 @@ describe(AuthMethodLdapService.name, () => {
       })
     )
     expect(updateArgs[1]).toEqual(expect.not.objectContaining({ password: expect.anything() }))
-
     // Password remains unchanged locally
     expect(existingUser.password).toBe('hashed')
     // Accesses updated as success
     expect(usersManager.updateAccesses).toHaveBeenCalledWith(existingUser, '127.0.0.3', true)
     // Returned user is the same instance
     expect(res2).toBe(existingUser)
-
     // Third run: no changes at all (identityHasChanged is empty) to cover the else branch
     adminUsersManager.updateUserOrGuest.mockClear()
     usersManager.updateAccesses.mockClear()
     compareSpy.mockResolvedValue(true)
-
     // Local user already matches LDAP identity; call again
     const res3 = await authMethodLdapService.validateUser('john', 'same-plain-password', '127.0.0.4')
-
     // No update should be triggered
     expect(adminUsersManager.updateUserOrGuest).not.toHaveBeenCalled()
     // Access should still be updated as success
@@ -306,9 +291,7 @@ describe(AuthMethodLdapService.name, () => {
     const existingUser: any = { id: 7, login: 'ghost', isGuest: false, isActive: true }
     usersManager.findUser.mockResolvedValue(existingUser)
     setupLdapSuccess([])
-
     const resA = await authMethodLdapService.validateUser('ghost', 'pwd', '10.10.0.1')
-
     expect(resA).toBeNull()
     expect(usersManager.updateAccesses).toHaveBeenCalledWith(existingUser, '10.10.0.1', false)
 
@@ -318,13 +301,24 @@ describe(AuthMethodLdapService.name, () => {
     usersManager.findUser.mockResolvedValue(existingUser2)
     mockBindResolve(ldapClient)
     mockSearchReject(ldapClient, new Error('search failed'))
-
     const resB = await authMethodLdapService.validateUser('john', 'pwd', '1.1.1.1')
-
     expect(resB).toBeNull()
     expect(usersManager.updateAccesses).toHaveBeenCalledWith(existingUser2, '1.1.1.1', false)
   })
 
+  it('should allow app password when LDAP fails and scope is provided', async () => {
+    const existingUser: any = buildUser({ id: 42 })
+    usersManager.findUser.mockResolvedValue(existingUser)
+    // LDAP invalid credentials
+    mockBindRejectInvalid(ldapClient, InvalidCredentialsError, 'invalid credentials')
+    // App password success
+    usersManager.validateAppPassword.mockResolvedValue(true)
+    const res = await authMethodLdapService.validateUser('john', 'app-password', '10.0.0.2', 'webdav' as any)
+    expect(res).toBe(existingUser)
+    expect(usersManager.validateAppPassword).toHaveBeenCalledWith(existingUser, 'app-password', '10.0.0.2', 'webdav')
+    expect(usersManager.updateAccesses).toHaveBeenCalledWith(existingUser, '10.0.0.2', true)
+  })
+
   it('should throw 500 when LDAP connection error occurs during bind', async () => {
     // Arrange: no existing user to reach checkAuth flow
     usersManager.findUser.mockResolvedValue(null)
@@ -349,26 +343,18 @@ describe(AuthMethodLdapService.name, () => {
     expect(usersManager.updateAccesses).not.toHaveBeenCalled()
   })
 
-  it('should log update failure and still call makePaths when updating existing user', async () => {
+  it('should log update failure when updating existing user', async () => {
     // Arrange: existing user with changed identity
     const existingUser: any = buildUser({ id: 11, email: 'old@ex.org' })
     usersManager.findUser.mockResolvedValue(existingUser)
-
     // Ensure LDAP loginAttribute matches uid for this test (a previous test sets it to 'cn')
-    configuration.auth.ldap.attributes.login = 'uid'
-
     setupLdapSuccess([{ uid: 'john', cn: 'John Doe', mail: 'john@example.org' }])
     adminUsersManager.updateUserOrGuest.mockRejectedValue(new Error('db error'))
-
     // Force identity to be considered changed only for this test
     jest.spyOn(commonFunctions, 'comparePassword').mockResolvedValue(false)
     jest.spyOn(commonFunctions, 'splitFullName').mockReturnValue({ firstName: 'John', lastName: 'Doe' })
-
     const res = await authMethodLdapService.validateUser('john', 'pwd')
-
     expect(adminUsersManager.updateUserOrGuest).toHaveBeenCalled()
-    // makePaths still invoked
-    expect(existingUser.makePaths).toHaveBeenCalled()
     // Local fields unchanged since update failed
     expect(existingUser.email).toBe('old@ex.org')
     expect(res).toBe(existingUser)
@@ -378,40 +364,30 @@ describe(AuthMethodLdapService.name, () => {
     // Phase A: LDAP returns an entry but loginAttribute value does not match -> checkAccess returns false (covers return after loop)
     const userA: any = { id: 20, login: 'john', isGuest: false, isActive: true }
     usersManager.findUser.mockResolvedValue(userA)
-    configuration.auth.ldap.attributes.login = 'uid'
     ldapClient.bind.mockResolvedValue(undefined)
-    // Non-matching entry: uid !== requested uid
-    ldapClient.search.mockResolvedValue({ searchEntries: [{ uid: 'jane', cn: 'Jane Doe', mail: 'jane@example.org' }] })
-    ldapClient.unbind.mockResolvedValue(undefined)
-
-    const resA = await authMethodLdapService.validateUser('john', 'pwd', '3.3.3.3')
-    expect(resA).toBeNull()
-    expect(usersManager.updateAccesses).toHaveBeenCalledWith(userA, '3.3.3.3', false)
 
     // Phase B: Matching entry + password considered changed -> updateUserOrGuest called, password not reassigned locally
     jest.clearAllMocks()
     const userB: any = buildUser({ id: 21, email: 'old@ex.org' })
     usersManager.findUser.mockResolvedValue(userB)
-    configuration.auth.ldap.attributes.login = 'uid'
     setupLdapSuccess([{ uid: 'john', cn: 'John Doe', mail: 'john@example.org' }])
     adminUsersManager.updateUserOrGuest.mockResolvedValue(undefined)
 
     // Force password to be considered changed to execute deletion + Object.assign branch
     jest.spyOn(commonFunctions, 'comparePassword').mockResolvedValue(false)
     jest.spyOn(commonFunctions, 'splitFullName').mockReturnValue({ firstName: 'John', lastName: 'Doe' })
-
     const resB = await authMethodLdapService.validateUser('john', 'newpwd', '4.4.4.4')
 
     // Line 132: updateUserOrGuest call
     expect(adminUsersManager.updateUserOrGuest).toHaveBeenCalledWith(
       21,
       expect.objectContaining({ email: 'john@example.org', firstName: 'John', lastName: 'Doe' })
     )
+
     // Lines 139-142: password removed from local assign, other fields assigned
     expect(userB.password).toBe('hashed')
     expect(userB.email).toBe('john@example.org')
     expect(userB).toMatchObject({ firstName: 'John', lastName: 'Doe' })
-    expect(userB.makePaths).toHaveBeenCalled()
     expect(resB).toBe(userB)
   })
 })