From 1222951af6f305d7a91be4f6796b8b5686865cab Mon Sep 17 00:00:00 2001 From: Schalli Date: Wed, 9 Sep 2026 07:48:37 +0200 Subject: [PATCH] fix(quick-260909-ab3): kollidierende AD-Konten werden angelegt, nur ohne Adresse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - User.email auf optional gestellt (Migration geschrieben, NICHT ausgefuehrt); Eindeutigkeitsindex unangetastet, NULL bleibt in Postgres je verschieden - Neuer Kollisionsentscheider (resolveEmailForWrite) in ldap.service.ts: eine bereits vergebene Adresse wird nie umgehaengt (T-Q3-01) — das zuerst angelegte Konto behaelt sie, jedes weitere Konto entsteht ohne Adresse (gesperrte Nutzerentscheidung 2026-09-09, WINDOWS #15) - Entscheider in upsertMappedUser (Sync) UND importUsersByDn (Handimport) verdrahtet, damit der zweite Anlageweg nicht als Luecke bestehen bleibt - LdapSyncResult um emailConflicts/skippedNoLogin/entryFailures erweitert; rohe ORM-Ausnahmetexte gehen nur noch an logger.error, nie in den Bericht (T-Q3-02) - UserService.create nimmt die Adresse optional entgegen; Tender-Digest und Instant-Alert ueberspringen Empfaenger ohne Adresse (continue) - Fuenf neue Testfaelle vorab gegen den unveraenderten Bestand rot gelaufen (erwartete Ursachen bestaetigt); 651/651 API-Tests gruen, prisma validate und type-check sauber Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FYZcd3SSmo14QTqWx2KKzU --- .../migration.sql | 18 ++ apps/api/prisma/schema.prisma | 2 +- apps/api/src/ldap/ldap.service.spec.ts | 172 ++++++++++++++++++ apps/api/src/ldap/ldap.service.ts | 135 ++++++++++++-- .../src/tenders/tender-digest.scheduler.ts | 5 +- .../src/tenders/tender-matching.service.ts | 5 +- apps/api/src/user/user.service.ts | 2 +- 7 files changed, 324 insertions(+), 15 deletions(-) create mode 100644 apps/api/prisma/migrations/20260909120000_user_email_optional/migration.sql diff --git a/apps/api/prisma/migrations/20260909120000_user_email_optional/migration.sql b/apps/api/prisma/migrations/20260909120000_user_email_optional/migration.sql new file mode 100644 index 0000000..b2d0cf0 --- /dev/null +++ b/apps/api/prisma/migrations/20260909120000_user_email_optional/migration.sql @@ -0,0 +1,18 @@ +-- WINDOWS #15: der AD-Sync liess Konten, deren Adresse bereits einem anderen +-- Konto gehoert (vier Funktionskonten mit geteilter mail-Adresse gemessen), +-- wegen der Pflicht auf "User.email" komplett unimportiert liegen -- ohne +-- verstaendliche Rueckmeldung. +-- +-- Gesperrte Nutzerentscheidung vom 2026-09-09 (nicht verhandelbar): ein +-- Konto mit bereits belegter Adresse wird trotzdem angelegt, nur eben OHNE +-- Adresse. Die Anmeldung laeuft ueber den Benutzernamen, nicht ueber die +-- Adresse -- das Konto bleibt voll funktionsfaehig, lediglich +-- Benachrichtigungen per E-Mail erreichen es nicht. Wer eine Adresse zuerst +-- belegt, behaelt sie unveraendert. +-- +-- Der Eindeutigkeitsindex auf "email" wird NICHT angefasst: PostgreSQL +-- behandelt NULL-Werte in einer Eindeutigkeitsregel als jeweils verschieden, +-- mehrere Konten ohne Adresse sind also weiterhin erlaubt. Bestandszeilen +-- bleiben unangetastet -- jedes bereits angelegte Konto behaelt seine +-- heutige Adresse. +ALTER TABLE "User" ALTER COLUMN "email" DROP NOT NULL; diff --git a/apps/api/prisma/schema.prisma b/apps/api/prisma/schema.prisma index 4c0e2bc..118ee07 100644 --- a/apps/api/prisma/schema.prisma +++ b/apps/api/prisma/schema.prisma @@ -29,7 +29,7 @@ enum Role { model User { id String @id @default(uuid()) username String @unique - email String @unique + email String? @unique passwordHash String? displayName String? role Role @default(USER) diff --git a/apps/api/src/ldap/ldap.service.spec.ts b/apps/api/src/ldap/ldap.service.spec.ts index 52c7b21..630cf19 100644 --- a/apps/api/src/ldap/ldap.service.spec.ts +++ b/apps/api/src/ldap/ldap.service.spec.ts @@ -218,6 +218,9 @@ describe('LdapService.syncUsersForTenant — empty base DN no-op', () => { groupsRenamed: 0, groupsDeleted: 0, defaultMarkerMoved: 0, + emailConflicts: [], + skippedNoLogin: [], + entryFailures: [], errors: [], }); expect(mockSearch).not.toHaveBeenCalled(); @@ -348,6 +351,9 @@ describe('LdapService — individual user search & import (dedup)', () => { prisma = { user: { findFirst: vi.fn().mockResolvedValue(null), + // Consulted by importUsersByDn's WINDOWS #15 email-collision decider + // whenever an entry carries a mapped `mail` attribute. + findUnique: vi.fn().mockResolvedValue(null), findMany: vi.fn().mockResolvedValue([]), update: vi.fn().mockResolvedValue({}), }, @@ -473,6 +479,172 @@ describe('LdapService — individual user search & import (dedup)', () => { }); }); +describe('LdapService.syncUsersForTenant — E-Mail-Kollision bei der Kontoanlage (WINDOWS #15)', () => { + let service: LdapService; + let prisma: any; + let userService: any; + + const cfg = { + id: 'cfg1', + tenantId: 't1', + serverUrl: 'ldap://example', + baseDn: 'dc=example,dc=com', + searchFilter: '(objectClass=person)', + groupFilterDns: [] as string[], + userExcludeList: [] as string[], + fieldMappings: [ + { ldapField: 'sAMAccountName', tesseraField: 'username' }, + { ldapField: 'mail', tesseraField: 'email' }, + ], + }; + + beforeEach(() => { + vi.clearAllMocks(); + mockBind.mockResolvedValue(undefined); + mockUnbind.mockResolvedValue(undefined); + prisma = { + user: { + findFirst: vi.fn().mockResolvedValue(null), + // The collision decider queries this — NOT mocked in the older + // describe blocks above, added here because this is the first + // block that exercises it (see PLAN.md Task 2 behavior note). + findUnique: vi.fn().mockResolvedValue(null), + findMany: vi.fn().mockResolvedValue([]), + update: vi.fn().mockResolvedValue({}), + }, + group: { findMany: vi.fn().mockResolvedValue([]) }, + groupMembership: { + createMany: vi.fn().mockResolvedValue({ count: 0 }), + deleteMany: vi.fn().mockResolvedValue({ count: 0 }), + }, + ldapConfig: { update: vi.fn().mockResolvedValue({}) }, + }; + userService = { create: vi.fn().mockResolvedValue({ id: 'created-user' }) }; + service = new LdapService(prisma, userService, {} as any); + }); + + it('legt ein Konto mit belegter Adresse trotzdem an, nur ohne Adresse', async () => { + mockSearch.mockResolvedValue({ + searchEntries: [ + { + dn: 'cn=konto1,dc=example,dc=com', + sAMAccountName: 'konto1', + mail: 'geteilt@example.com', + }, + { + dn: 'cn=konto2,dc=example,dc=com', + sAMAccountName: 'konto2', + mail: 'geteilt@example.com', + }, + ], + }); + // The first entry's address is free (findUnique -> null). By the time + // the second entry is processed, the address already belongs to the + // first-created account. + prisma.user.findUnique + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: 'konto1-id', email: 'geteilt@example.com' }); + + const result = await service.syncUsersForTenant(cfg as any, 't1'); + + expect(userService.create).toHaveBeenCalledTimes(2); + expect(userService.create).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ username: 'konto1', email: 'geteilt@example.com' }), + ); + const secondCallArgs = userService.create.mock.calls[1][0]; + expect(secondCallArgs.email).toBeUndefined(); + expect(result.created).toBe(2); + }); + + it('meldet die Kollision strukturiert und nicht als Fehler', async () => { + mockSearch.mockResolvedValue({ + searchEntries: [ + { + dn: 'cn=konto1,dc=example,dc=com', + sAMAccountName: 'konto1', + mail: 'geteilt@example.com', + }, + { + dn: 'cn=konto2,dc=example,dc=com', + sAMAccountName: 'konto2', + mail: 'geteilt@example.com', + }, + ], + }); + prisma.user.findUnique + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: 'konto1-id', email: 'geteilt@example.com' }); + + const result = await service.syncUsersForTenant(cfg as any, 't1'); + + expect(result.emailConflicts).toEqual([ + { account: 'konto2', email: 'geteilt@example.com' }, + ]); + expect(result.errors).toEqual([]); + }); + + it('nimmt einem bestehenden Konto seine Adresse nicht weg', async () => { + mockSearch.mockResolvedValue({ + searchEntries: [ + { + dn: 'cn=konto3,dc=example,dc=com', + sAMAccountName: 'konto3', + mail: 'fremd@example.com', + }, + ], + }); + // konto3 already exists locally (matched by ldapDn); the address AD now + // reports for it belongs to a THIRD, unrelated user. + prisma.user.findFirst.mockResolvedValue({ id: 'konto3-id', email: 'alt@example.com' }); + prisma.user.findUnique.mockResolvedValue({ id: 'dritter-user-id', email: 'fremd@example.com' }); + + const result = await service.syncUsersForTenant(cfg as any, 't1'); + + expect(prisma.user.update).toHaveBeenCalledWith( + expect.objectContaining({ + where: { id: 'konto3-id' }, + data: expect.not.objectContaining({ email: expect.anything() }), + }), + ); + expect(result.emailConflicts).toEqual([ + { account: 'konto3', email: 'fremd@example.com' }, + ]); + }); + + it('meldet Eintraege ohne Anmeldenamen getrennt und nicht als Fehler', async () => { + mockSearch.mockResolvedValue({ + searchEntries: [{ dn: 'cn=kontakt1,dc=example,dc=com' }], + }); + + const result = await service.syncUsersForTenant(cfg as any, 't1'); + + expect(result.skippedNoLogin).toEqual(['cn=kontakt1,dc=example,dc=com']); + expect(result.errors).toEqual([]); + }); + + it('reicht keinen rohen Datenbanktext an den Bericht durch', async () => { + mockSearch.mockResolvedValue({ + searchEntries: [ + { + dn: 'cn=konto5,dc=example,dc=com', + sAMAccountName: 'konto5', + mail: 'konto5@example.com', + }, + ], + }); + const ormMessage = + 'Invalid `prisma.user.create()` invocation: Unique constraint failed on the fields: (`email`)'; + userService.create.mockRejectedValueOnce(new Error(ormMessage)); + + const result = await service.syncUsersForTenant(cfg as any, 't1'); + + expect(result.entryFailures).toEqual(['cn=konto5,dc=example,dc=com']); + expect(result.errors).not.toContain(ormMessage); + expect(JSON.stringify(result)).not.toContain(ormMessage); + }); +}); + describe('LdapService.testConnection — TLS verification opt-out (ldaps)', () => { let service: LdapService; diff --git a/apps/api/src/ldap/ldap.service.ts b/apps/api/src/ldap/ldap.service.ts index 46d0ba8..7aed337 100644 --- a/apps/api/src/ldap/ldap.service.ts +++ b/apps/api/src/ldap/ldap.service.ts @@ -30,9 +30,31 @@ export interface LdapSyncResult { groupsRenamed: number; groupsDeleted: number; defaultMarkerMoved: number; + // WINDOWS #15, gesperrte Nutzerentscheidung 2026-09-09: ein AD-Konto, + // dessen mail-Adresse bereits einem ANDEREN Konto gehoert, wird trotzdem + // angelegt/aktualisiert -- nur eben OHNE diese Adresse. Wer die Adresse + // zuerst hatte, behaelt sie unveraendert (T-Q3-01). Diese Liste macht die + // Kollision im Bericht sichtbar statt sie still zu verwerfen. + emailConflicts: LdapEmailConflict[]; + // Verzeichniseintraege ohne sAMAccountName (Kontakte, Verteiler, + // Ressourcen) -- ein normaler, erwarteter Vorgang, kein Fehler. Traegt die + // Kennung (dn) des uebersprungenen Eintrags. + skippedNoLogin: string[]; + // Ein unerwarteter Fehler bei genau diesem Eintrag. Nur die Kennung (dn) + // steht hier -- der technische Wortlaut (z. B. eine rohe Prisma/ORM- + // Ausnahme) geht AUSSCHLIESSLICH ueber this.logger.error ins + // Serverprotokoll und erreicht diesen Bericht nie (T-Q3-02). + entryFailures: string[]; errors: string[]; } +/** Ein Konto, das wegen einer bereits vergebenen Adresse ohne diese Adresse + * angelegt oder aktualisiert wurde (WINDOWS #15). */ +export interface LdapEmailConflict { + account: string; + email: string; +} + /** * LDAP configuration shape as stored in the database. */ @@ -378,6 +400,32 @@ export class LdapService { return { username: mappedData['username']?.toLowerCase(), mappedData }; } + /** + * Decide whether `desiredEmail` may be written to the account identified by + * `ownRecordId` (null when the account does not exist yet, i.e. a create). + * Queries the unique `email` column via findUnique (deliberately NOT + * findFirst — the column is unique, and this keeps the call unambiguously + * distinguishable in tests from the identity findFirst lookups in + * upsertMappedUser/importUsersByDn) and returns the address only when + * nobody holds it yet, or the holder IS this same account. Otherwise the + * address is withheld and reported as a collision instead — an address is + * NEVER handed from one account to another (T-Q3-01): a directory entry + * could otherwise take over a real person's address and receive their + * password-reset mail. + */ + private async resolveEmailForWrite( + desiredEmail: string, + ownRecordId: string | null, + ): Promise<{ email?: string; collides: boolean }> { + const holder = await this.prisma.user.findUnique({ + where: { email: desiredEmail }, + }); + if (!holder || holder.id === ownRecordId) { + return { email: desiredEmail, collides: false }; + } + return { collides: true }; + } + /** * Find-or-upsert one LDAP user by (ldapDn, then username) within a tenant. * @@ -385,13 +433,22 @@ export class LdapService { * identity resolution: a user imported one way is NEVER duplicated by the * other. A manually-imported user (ldapDn set) is matched by ldapDn on a * later department/group sync and updated in place, not re-created. + * + * WINDOWS #15: an account whose mapped `mail` address is already held by a + * DIFFERENT account is still created/updated, just without that address + * (locked user decision, 2026-09-09) — see resolveEmailForWrite() above. + * The `${username}@ldap.local` fallback for entries with NO mail attribute + * at all is untouched: it is not part of this defect. */ private async upsertMappedUser( dn: string, username: string, mappedData: Record, tenantId: string, - ): Promise<'created' | 'updated'> { + ): Promise<{ + status: 'created' | 'updated'; + emailConflict?: LdapEmailConflict; + }> { const existingByDn = await this.prisma.user.findFirst({ where: { ldapDn: dn, tenantId }, }); @@ -401,30 +458,60 @@ export class LdapService { const existing = existingByDn || existingByUsername; if (existing) { + let emailToWrite: string | undefined; + let emailConflict: LdapEmailConflict | undefined; + if (mappedData['email']) { + const decision = await this.resolveEmailForWrite( + mappedData['email'], + existing.id, + ); + if (decision.collides) { + emailConflict = { account: username, email: mappedData['email'] }; + } else { + emailToWrite = decision.email; + } + } + await this.prisma.user.update({ where: { id: existing.id }, data: { ...(mappedData['displayName'] && { displayName: mappedData['displayName'], }), - ...(mappedData['email'] && { email: mappedData['email'] }), + ...(emailToWrite && { email: emailToWrite }), ...(mappedData['username'] && { username }), ldapDn: dn, isActive: true, }, }); - return 'updated'; + return { status: 'updated', emailConflict }; + } + + let createEmail: string | undefined = + mappedData['email'] || `${username}@ldap.local`; + let emailConflict: LdapEmailConflict | undefined; + if (mappedData['email']) { + const decision = await this.resolveEmailForWrite( + mappedData['email'], + null, + ); + if (decision.collides) { + createEmail = undefined; + emailConflict = { account: username, email: mappedData['email'] }; + } else { + createEmail = decision.email; + } } await this.userService.create({ username, - email: mappedData['email'] || `${username}@ldap.local`, + ...(createEmail && { email: createEmail }), displayName: mappedData['displayName'], role: 'USER', tenantId, ldapDn: dn, }); - return 'created'; + return { status: 'created', emailConflict }; } /** @@ -599,9 +686,24 @@ export class LdapService { continue; } + // WINDOWS #15: same collision decider as upsertMappedUser — an + // address already held by a DIFFERENT account is withheld, never + // reassigned (T-Q3-01). No new LdapUserImportResult field: the + // account is still created and counted, this manual-import path's + // display stays as-is. + let createEmail: string | undefined = + mappedData['email'] || `${username}@ldap.local`; + if (mappedData['email']) { + const decision = await this.resolveEmailForWrite( + mappedData['email'], + null, + ); + createEmail = decision.collides ? undefined : decision.email; + } + await this.userService.create({ username, - email: mappedData['email'] || `${username}@ldap.local`, + ...(createEmail && { email: createEmail }), displayName: mappedData['displayName'], role: 'USER', tenantId, @@ -775,6 +877,9 @@ export class LdapService { groupsRenamed: 0, groupsDeleted: 0, defaultMarkerMoved: 0, + emailConflicts: [], + skippedNoLogin: [], + entryFailures: [], errors: [], }; @@ -858,15 +963,15 @@ export class LdapService { syncedDns.push(dn); if (!username) { - result.errors.push( - `Entry ${dn}: no username mapped (check sAMAccountName mapping)`, - ); + // Normal for contacts/resources/distribution entries without a + // sAMAccountName — a neutral hint, not an error (WINDOWS #15). + result.skippedNoLogin.push(dn); continue; } // Find-or-upsert by (ldapDn, then username) via the shared helper so // sync and manual import dedupe identically (never a duplicate row). - const status = await this.upsertMappedUser( + const { status, emailConflict } = await this.upsertMappedUser( dn, username, mappedData, @@ -877,12 +982,20 @@ export class LdapService { } else { result.updated++; } + if (emailConflict) { + result.emailConflicts.push(emailConflict); + } } catch (entryError: unknown) { const msg = entryError instanceof Error ? entryError.message : 'Unknown error processing entry'; - result.errors.push(`Entry ${entry.dn}: ${msg}`); + // T-Q3-02: the raw exception text (ORM table/column/call names) + // goes to the server log only — never to the report an + // administrator reads. Only the entry's identifier (dn) is + // reported. + this.logger.error(`LDAP sync entry ${entry.dn} failed: ${msg}`); + result.entryFailures.push(entry.dn); } } diff --git a/apps/api/src/tenders/tender-digest.scheduler.ts b/apps/api/src/tenders/tender-digest.scheduler.ts index 3bbfe72..d955ac4 100644 --- a/apps/api/src/tenders/tender-digest.scheduler.ts +++ b/apps/api/src/tenders/tender-digest.scheduler.ts @@ -134,7 +134,10 @@ export class TenderDigestScheduler implements OnModuleInit { if (!matches.length) continue; const user = await this.prisma.user.findUnique({ where: { id: userId } }); - if (!user) continue; + // Kein Konto, oder ein Konto ohne Adresse (WINDOWS #15, kollidierte + // AD-Adresse) -- die Zugehoerigkeit funktioniert, nur der + // Mailversand wird uebersprungen (zugesagtes Verhalten). + if (!user || !user.email) continue; const sections = groupMatchesByProfile(matches); const sent = await this.mail.sendDigest({ email: user.email }, user.tenantId, sections); diff --git a/apps/api/src/tenders/tender-matching.service.ts b/apps/api/src/tenders/tender-matching.service.ts index b7dd804..628f03c 100644 --- a/apps/api/src/tenders/tender-matching.service.ts +++ b/apps/api/src/tenders/tender-matching.service.ts @@ -125,7 +125,10 @@ export class TenderMatchingService { if (!fresh.length) continue; // nothing new for this profile this tick const user = await this.prisma.user.findUnique({ where: { id: profile.userId } }); - if (!user) continue; + // Kein Konto, oder ein Konto ohne Adresse (WINDOWS #15, kollidierte + // AD-Adresse) -- die Zugehoerigkeit funktioniert, nur der + // Mailversand wird uebersprungen (zugesagtes Verhalten). + if (!user || !user.email) continue; const sent = await this.mail.sendInstant( { email: user.email }, diff --git a/apps/api/src/user/user.service.ts b/apps/api/src/user/user.service.ts index 57444c8..a58bc7b 100644 --- a/apps/api/src/user/user.service.ts +++ b/apps/api/src/user/user.service.ts @@ -48,7 +48,7 @@ export class UserService { */ async create(data: { username: string; - email: string; + email?: string; password?: string; displayName?: string; role?: 'SUPER_ADMIN' | 'ADMIN' | 'USER';