From 3a45ada4fde584d6b8c39583acecbab3471384c0 Mon Sep 17 00:00:00 2001 From: joshuakrueger-dfx Date: Fri, 31 Jul 2026 13:44:22 +0200 Subject: [PATCH] fix(kyc): stop KycStep.update from clearing omitted fields update() built its payload from the raw parameters, so Object.assign wrote undefined onto the entity for every argument a caller left out. Callers use undefined to mean "leave unchanged", so two paths silently lost state: - updateFinancialData calls update(undefined, responses). The step lost its status, and KycStepMapper.toStepBase then mapped StepMap[undefined] to undefined. An incomplete draft submit returned HTTP 200 without status and without sequenceNumber, although KycStepBase declares both as required. - mergeUserData passes status undefined for every slave step that is not in the cancel list, COMPLETED included. The check below it - slave.kycSteps.some(k => (VIDEO || SUMSUB_VIDEO) && k.isCompleted) - was therefore never true, so identificationType = VIDEO_ID and bankTransactionVerification = UNNECESSARY were never applied on a merge. That rule was added in #1424 and has been inert since 6fb9d18536 moved the loop onto update(). The database payload is unchanged for every caller: TypeORM already drops undefined values from the update statement, so the defect was in memory only. status becomes optional, matching the two call sites that pass undefined, and addComment returns string because Array.prototype.join always does. --- .../__tests__/kyc-step.entity.spec.ts | 34 ++++++++ .../generic/kyc/entities/kyc-step.entity.ts | 19 +++-- .../services/__tests__/kyc.service.spec.ts | 78 +++++++++++++++++++ .../__tests__/user-data.service.spec.ts | 37 +++++++++ 4 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 src/subdomains/generic/kyc/entities/__tests__/kyc-step.entity.spec.ts diff --git a/src/subdomains/generic/kyc/entities/__tests__/kyc-step.entity.spec.ts b/src/subdomains/generic/kyc/entities/__tests__/kyc-step.entity.spec.ts new file mode 100644 index 0000000000..a52341823e --- /dev/null +++ b/src/subdomains/generic/kyc/entities/__tests__/kyc-step.entity.spec.ts @@ -0,0 +1,34 @@ +import { KycStep } from '../kyc-step.entity'; +import { KycStepName } from '../../enums/kyc-step-name.enum'; +import { ReviewStatus } from '../../enums/review-status.enum'; + +describe('KycStep.update', () => { + const buildStep = (overrides: Partial = {}): KycStep => + Object.assign(new KycStep(), { + id: 1, + name: KycStepName.FINANCIAL_DATA, + status: ReviewStatus.IN_PROGRESS, + sequenceNumber: 5, + ...overrides, + }); + + it('leaves status and sequenceNumber unchanged when they are omitted, and omits both from the returned partial', () => { + const step = buildStep(); + + const [, update] = step.update(undefined, [{ key: 'income', value: 'salary' }]); + + expect(step.status).toBe(ReviewStatus.IN_PROGRESS); + expect(step.sequenceNumber).toBe(5); + expect(update).not.toHaveProperty('status'); + expect(update).not.toHaveProperty('sequenceNumber'); + }); + + it('applies an explicit status change and includes it in the returned partial', () => { + const step = buildStep(); + + const [, update] = step.update(ReviewStatus.INTERNAL_REVIEW, [{ key: 'income', value: 'salary' }]); + + expect(step.status).toBe(ReviewStatus.INTERNAL_REVIEW); + expect(update.status).toBe(ReviewStatus.INTERNAL_REVIEW); + }); +}); diff --git a/src/subdomains/generic/kyc/entities/kyc-step.entity.ts b/src/subdomains/generic/kyc/entities/kyc-step.entity.ts index 8f96031946..aa4f01f99f 100644 --- a/src/subdomains/generic/kyc/entities/kyc-step.entity.ts +++ b/src/subdomains/generic/kyc/entities/kyc-step.entity.ts @@ -214,17 +214,20 @@ export class KycStep extends IEntity { } update( - status: ReviewStatus, + status?: ReviewStatus, result?: KycStepResult, comment?: string, sequenceNumber?: number, ): UpdateResult { - const update: Partial = { - status, - result: this.setResult(result), - comment: this.addComment(comment), - sequenceNumber, - }; + // Callers pass undefined to leave a field unchanged; Object.assign would overwrite those properties. + const update = Object.fromEntries( + Object.entries({ + status, + result: this.setResult(result), + comment: this.addComment(comment), + sequenceNumber, + }).filter(([, v]) => v !== undefined), + ) as Partial; Object.assign(this, update); @@ -365,7 +368,7 @@ export class KycStep extends IEntity { return this.result; } - addComment(comment: string): string | undefined { + addComment(comment: string): string { return [this.comment, comment].filter((c) => c).join(';'); } diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index 73d6fdacdc..2917e2dc7e 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -10,6 +10,7 @@ import { CountryService } from 'src/shared/models/country/country.service'; import { DfxLogger } from 'src/shared/services/dfx-logger'; import * as processServiceModule from 'src/shared/services/process.service'; import { createCustomUserData } from '../../../user/models/user-data/__mocks__/user-data.entity.mock'; +import { AccountType } from '../../../user/models/user-data/account-type.enum'; import { UserData } from '../../../user/models/user-data/user-data.entity'; import { RiskStatus, UserDataStatus } from '../../../user/models/user-data/user-data.enum'; import { UserDataService } from '../../../user/models/user-data/user-data.service'; @@ -17,6 +18,7 @@ import { UserStatus } from '../../../user/models/user/user.enum'; import { IdentDocument } from '../../dto/ident.dto'; import { KycError } from '../../dto/kyc-error.enum'; import { FileSubType, FileType, KycFileBlob } from '../../dto/kyc-file.dto'; +import { KycStepStatus } from '../../dto/output/kyc-info.dto'; import { SumSubLevelName } from '../../dto/sum-sub.dto'; import { KycFile } from '../../entities/kyc-file.entity'; import { KycStep } from '../../entities/kyc-step.entity'; @@ -795,3 +797,79 @@ describe('KycService checkDfxApproval step promotion', () => { expect(kycStepRepo.update).not.toHaveBeenCalled(); }); }); + +// updateFinancialData / updateFileData call KycStep.update with omitted status or sequenceNumber; +// the in-memory step (and thus the mapped response) must keep those fields. +describe('KycService updateFinancialData incomplete draft', () => { + let service: KycService; + let kycStepRepo: jest.Mocked; + + beforeEach(() => { + kycStepRepo = createMock(); + service = Object.create(KycService.prototype); + (service as any).kycStepRepo = kycStepRepo; + }); + + it('returns status InProgress and the existing sequenceNumber, and keeps the step status', async () => { + const kycStep = Object.assign(new KycStep(), { + id: 11, + name: KycStepName.FINANCIAL_DATA, + status: ReviewStatus.IN_PROGRESS, + sequenceNumber: 3, + }); + const user = createMock({ accountType: AccountType.PERSONAL }); + user.getPendingStepOrThrow.mockReturnValue(kycStep); + jest.spyOn(service as any, 'getUser').mockResolvedValue(user); + jest.spyOn(service as any, 'verify2fa').mockResolvedValue(undefined); + jest.spyOn(service as any, 'updateProgress').mockResolvedValue(user); + + const response = await service.updateFinancialData('hash', '1.2.3.4', 11, { responses: [] }); + + expect(response.status).toBe(KycStepStatus.IN_PROGRESS); + expect(response.sequenceNumber).toBe(3); + expect(kycStep.status).toBe(ReviewStatus.IN_PROGRESS); + expect(kycStep.sequenceNumber).toBe(3); + }); +}); + +describe('KycService updateFileData sequenceNumber', () => { + let service: KycService; + let kycStepRepo: jest.Mocked; + let documentService: jest.Mocked; + + beforeEach(() => { + kycStepRepo = createMock(); + documentService = createMock(); + service = Object.create(KycService.prototype); + (service as any).kycStepRepo = kycStepRepo; + (service as any).documentService = documentService; + }); + + it('returns the step sequenceNumber unchanged after a file upload', async () => { + const kycStep = Object.assign(new KycStep(), { + id: 22, + name: KycStepName.ADDITIONAL_DOCUMENTS, + status: ReviewStatus.IN_PROGRESS, + sequenceNumber: 7, + userData: { kycLevel: 50 }, + }); + const user = createMock(); + user.getPendingStepOrThrow.mockReturnValue(kycStep); + jest.spyOn(service as any, 'getUser').mockResolvedValue(user); + jest.spyOn(service as any, 'createStepLog').mockResolvedValue(undefined); + jest.spyOn(service as any, 'updateProgress').mockResolvedValue(user); + documentService.uploadUserFile.mockResolvedValue({ url: 'https://example.com/file.pdf' } as never); + + const filePayload = ['data:application/pdf;base64', Buffer.from('x').toString('base64')].join(','); + const response = await service.updateFileData( + 'hash', + 22, + KycStepName.ADDITIONAL_DOCUMENTS, + { fileName: 'doc.pdf', file: filePayload }, + FileType.ADDITIONAL_DOCUMENTS, + ); + + expect(response.sequenceNumber).toBe(7); + expect(kycStep.sequenceNumber).toBe(7); + }); +}); diff --git a/src/subdomains/generic/user/models/user-data/__tests__/user-data.service.spec.ts b/src/subdomains/generic/user/models/user-data/__tests__/user-data.service.spec.ts index 6da886d253..62dbd78be6 100644 --- a/src/subdomains/generic/user/models/user-data/__tests__/user-data.service.spec.ts +++ b/src/subdomains/generic/user/models/user-data/__tests__/user-data.service.spec.ts @@ -47,10 +47,13 @@ import { import { VirtualIban, VirtualIbanStatus } from 'src/subdomains/supporting/bank/virtual-iban/virtual-iban.entity'; import { VirtualIbanIssuanceIntentStatus } from 'src/subdomains/supporting/bank/virtual-iban/virtual-iban-issuance-intent-status.enum'; import { VirtualIbanIssuanceIntent } from 'src/subdomains/supporting/bank/virtual-iban/virtual-iban-issuance-intent.entity'; +import { CheckStatus } from 'src/subdomains/core/aml/enums/check-status.enum'; import { KycStep } from 'src/subdomains/generic/kyc/entities/kyc-step.entity'; import { KycStepName } from 'src/subdomains/generic/kyc/enums/kyc-step-name.enum'; +import { KycStepType } from 'src/subdomains/generic/kyc/enums/kyc.enum'; import { ReviewStatus } from 'src/subdomains/generic/kyc/enums/review-status.enum'; import { UserData } from '../user-data.entity'; +import { KycIdentificationType } from '../kyc-identification-type.enum'; import { KycType, UserDataStatus } from '../user-data.enum'; import { UserDataRepository } from '../user-data.repository'; import { @@ -717,6 +720,40 @@ describe('UserDataService', () => { } expect(new Set(assigned.map(([, s]) => s)).size).toBe(assigned.length); }); + + // COMPLETED video ident steps are not in the cancel list, so update() receives status undefined; + // that must not wipe status, or the VIDEO_ID / bankTransactionVerification branch never fires. + describe.each([KycStepType.VIDEO, KycStepType.SUMSUB_VIDEO])('completed %s slave ident step', (identType) => { + it('sets master identificationType to VIDEO_ID and bankTransactionVerification to UNNECESSARY', async () => { + const master = buildAccount(1000, 50); + const slave = buildAccount(2000, 20); + const videoStep = Object.assign(new KycStep(), { + id: 1, + name: KycStepName.IDENT, + type: identType, + status: ReviewStatus.COMPLETED, + sequenceNumber: 0, + }); + + userDataRepo.findOne.mockResolvedValueOnce(master).mockResolvedValueOnce(slave); + transactionService.getAllTransactionsForUserData.mockResolvedValue([]); + userRepo.find.mockResolvedValue([]); + bankDataService.getAllBankDatasForUser.mockResolvedValue([]); + virtualIbanService.getFrickVirtualIbansForAccount.mockResolvedValue([]); + kycAdminService.getKycSteps.mockResolvedValueOnce([]).mockResolvedValueOnce([videoStep]); + documentService.copyFiles.mockResolvedValue(undefined); + jest.spyOn(service, 'updateVolumes').mockResolvedValue(undefined); + jest + .spyOn(service as unknown as { updateBankTxTime: () => Promise }, 'updateBankTxTime') + .mockResolvedValue(undefined); + + await service.mergeUserData(master.id, slave.id); + + expect(master.identificationType).toBe(KycIdentificationType.VIDEO_ID); + expect(master.bankTransactionVerification).toBe(CheckStatus.UNNECESSARY); + expect(videoStep.status).toBe(ReviewStatus.COMPLETED); + }); + }); }); describe('mergeUserData virtual IBAN reassignment', () => {