diff --git a/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts index 7db1104b78..0a10f59724 100644 --- a/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts +++ b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts @@ -530,7 +530,7 @@ describe('CustodyAccountService', () => { ); }); - it('keeps the legacy entry when the only other visible account is read-only', async () => { + it('drops the empty legacy entry when the only other visible account is read-only', async () => { const foreignAccount = foreignCustodyAccount(); const readOnlyGrant = accessGrant({ account: foreignAccount, @@ -541,14 +541,37 @@ describe('CustodyAccountService', () => { const custodyUserId = 55; userDataService.getUserData.mockResolvedValue(ownerUserData({ users: [custodyRoleUser(custodyUserId)] })); mockFindActiveGrants([readOnlyGrant]); + custodyService.hasNonZeroCustodyBalance.mockResolvedValue(false); + + const result = await service.getCustodyAccountsForUser(ownerId); + + // The operator-side viewer: read access on someone else's account, own Safe empty. The + // empty entry goes, and with it the only writable one — see the note on the branch. + expect(result).toHaveLength(1); + expect(result.some((dto) => dto.isLegacy)).toBe(false); + expect(custodyService.hasNonZeroCustodyBalance).toHaveBeenCalledWith([custodyUserId]); + }); + + it('keeps the legacy entry when the only other visible account is read-only but the Safe holds a balance', async () => { + const foreignAccount = foreignCustodyAccount(); + const readOnlyGrant = accessGrant({ + account: foreignAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + const custodyUserId = 55; + userDataService.getUserData.mockResolvedValue(ownerUserData({ users: [custodyRoleUser(custodyUserId)] })); + mockFindActiveGrants([readOnlyGrant]); + custodyService.hasNonZeroCustodyBalance.mockResolvedValue(true); const result = await service.getCustodyAccountsForUser(ownerId); expect(result).toHaveLength(2); expect(result.some((dto) => dto.isLegacy)).toBe(true); - // A read-only grant elsewhere cannot substitute for the legacy Safe — checkAccess() - // rejects any write on a Read account, so the balance is never even queried here. - expect(custodyService.hasNonZeroCustodyBalance).not.toHaveBeenCalled(); + // Asserting the call is what makes this test about the fix: without it the case would + // pass under the old write-access rule too, where the balance was never consulted here. + expect(custodyService.hasNonZeroCustodyBalance).toHaveBeenCalledWith([custodyUserId]); }); it('drops the empty legacy entry when the user has write access elsewhere', async () => { diff --git a/src/subdomains/core/custody/services/custody-account.service.ts b/src/subdomains/core/custody/services/custody-account.service.ts index bf4c540b90..e4a3da3f48 100644 --- a/src/subdomains/core/custody/services/custody-account.service.ts +++ b/src/subdomains/core/custody/services/custody-account.service.ts @@ -87,17 +87,21 @@ export class CustodyAccountService { ]; // Legacy Safe = absence of any owned account row; independent of shared grants. Hidden - // only when it is both empty AND the customer already has write access on another entry - // — a Read-only grant elsewhere cannot substitute for it: checkAccess() rejects any write - // on a Read account, so someone with no writable account left would lose their only path - // to a deposit. A non-empty legacy Safe always stays, regardless of what else is writable. + // when it is both empty AND at least one other entry is visible, so an empty "Custody" + // never sits next to real accounts in the selector. The access level of those other + // entries deliberately does NOT matter: a viewer holding only Read grants is exactly the + // case this hides — an operator-side account with an empty Safe of its own. The cost is + // that such a viewer keeps no writable entry in the list (checkAccess() rejects writes on + // a Read account), so the selector offers them no deposit path. Whenever another entry is + // visible, the balance check below has the last word: a legacy Safe that holds something + // keeps its entry. With nothing else visible the check never runs and the entry stays. if (allOwnedAccounts.length === 0) { const custodyUserIds = account.users.filter((u) => u.role === UserRole.CUSTODY).map((u) => u.id); if (custodyUserIds.length > 0) { - // The balance check only runs when a legacy entry is actually in play AND the - // customer already has write access elsewhere — every other case stays cheap, no query. - const hasWriteElsewhere = custodyAccounts.some((ca) => ca.accessLevel === CustodyAccessLevel.WRITE); - const hideLegacy = hasWriteElsewhere && !(await this.custodyService.hasNonZeroCustodyBalance(custodyUserIds)); + // The balance check only runs when a legacy entry is actually in play AND something + // else is visible next to it — every other case stays cheap, no query. + const hasOtherAccounts = custodyAccounts.length > 0; + const hideLegacy = hasOtherAccounts && !(await this.custodyService.hasNonZeroCustodyBalance(custodyUserIds)); if (!hideLegacy) custodyAccounts.push(CustodyAccountDtoMapper.toLegacyDto(account)); } } diff --git a/src/subdomains/core/payment-link/repositories/payment-link.repository.ts b/src/subdomains/core/payment-link/repositories/payment-link.repository.ts index cffda11b93..cb28937c10 100644 --- a/src/subdomains/core/payment-link/repositories/payment-link.repository.ts +++ b/src/subdomains/core/payment-link/repositories/payment-link.repository.ts @@ -24,6 +24,13 @@ export class PaymentLinkRepository extends BaseRepository { }); } + async getAllPaymentLinksByExternalPaymentId(externalPaymentId: string): Promise { + return this.find({ + where: { payments: { externalId: Equal(externalPaymentId) } }, + relations: { route: { user: { userData: { organization: true } } } }, + }); + } + async getHistoryByStatus( userId: number, paymentStatus: PaymentLinkPaymentStatus[], diff --git a/src/subdomains/core/payment-link/services/payment-link-payment.service.ts b/src/subdomains/core/payment-link/services/payment-link-payment.service.ts index 7161152200..e7f223f2d9 100644 --- a/src/subdomains/core/payment-link/services/payment-link-payment.service.ts +++ b/src/subdomains/core/payment-link/services/payment-link-payment.service.ts @@ -118,16 +118,12 @@ export class PaymentLinkPaymentService { }); } - async getPaymentByExternalId(externalPaymentId: string): Promise { + // externalPaymentId is a merchant-supplied reconciliation identifier and is NOT unique across + // merchants; scope the lookup to a link the caller has already been authorized against, or the + // response leaks foreign merchants' payment records (BUG-1289). + async getPaymentByExternalId(linkId: number, externalPaymentId: string): Promise { return this.paymentLinkPaymentRepo.findOne({ - where: { externalId: externalPaymentId }, - }); - } - - async getAllPaymentsByExternalLinkId(externalPaymentId: string): Promise { - return this.paymentLinkPaymentRepo.find({ - where: { externalId: externalPaymentId }, - relations: { link: { route: { user: { userData: true } } } }, + where: { externalId: externalPaymentId, link: { id: linkId } }, }); } diff --git a/src/subdomains/core/payment-link/services/payment-link.service.ts b/src/subdomains/core/payment-link/services/payment-link.service.ts index d7f9426f21..e728cae428 100644 --- a/src/subdomains/core/payment-link/services/payment-link.service.ts +++ b/src/subdomains/core/payment-link/services/payment-link.service.ts @@ -71,7 +71,7 @@ export class PaymentLinkService { if (loadPayments) { const payment = externalPaymentId - ? await this.paymentLinkPaymentService.getPaymentByExternalId(externalPaymentId) + ? await this.paymentLinkPaymentService.getPaymentByExternalId(link.id, externalPaymentId) : await this.paymentLinkPaymentService.getMostRecentPayment(link.uniqueId); if (payment) link.payments.push(payment); } @@ -626,11 +626,13 @@ export class PaymentLinkService { } if (externalPaymentId) { - const payments = await this.paymentLinkPaymentService.getAllPaymentsByExternalLinkId(externalPaymentId); - const payment = payments.find((p) => p.link.configObj.accessKeys?.includes(key)); - if (!payment) throw new NotFoundException('No payment found'); + // Resolve the access-key against payment links first, then look up the payment scoped to + // those links — externalPaymentId is not unique across merchants (BUG-1289). + const candidateLinks = await this.paymentLinkRepo.getAllPaymentLinksByExternalPaymentId(externalPaymentId); + const paymentLink = candidateLinks.find((pl) => pl.configObj.accessKeys?.includes(key)); + if (!paymentLink) throw new NotFoundException('No payment found'); - return this.getOrThrow(payment.link.route.user.id, payment.link.id, payment.link.externalId, payment.externalId); + return this.getOrThrow(paymentLink.route.user.id, paymentLink.id, paymentLink.externalId, externalPaymentId); } throw new BadRequestException('Either externalLinkId or externalPaymentId must be provided');