Release: develop -> main

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 () => {

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));
       }
     }

src/subdomains/core/payment-link/repositories/payment-link.repository.ts

@@ -24,6 +24,13 @@ export class PaymentLinkRepository extends BaseRepository<PaymentLink> {
     });
   }
 
+  async getAllPaymentLinksByExternalPaymentId(externalPaymentId: string): Promise<PaymentLink[]> {
+    return this.find({
+      where: { payments: { externalId: Equal(externalPaymentId) } },
+      relations: { route: { user: { userData: { organization: true } } } },
+    });
+  }
+
   async getHistoryByStatus(
     userId: number,
     paymentStatus: PaymentLinkPaymentStatus[],

src/subdomains/core/payment-link/services/payment-link-payment.service.ts

@@ -118,16 +118,12 @@ export class PaymentLinkPaymentService {
     });
   }
 
-  async getPaymentByExternalId(externalPaymentId: string): Promise<PaymentLinkPayment | null> {
+  // 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<PaymentLinkPayment | null> {
     return this.paymentLinkPaymentRepo.findOne({
-      where: { externalId: externalPaymentId },
-    });
-  }
-
-  async getAllPaymentsByExternalLinkId(externalPaymentId: string): Promise<PaymentLinkPayment[]> {
-    return this.paymentLinkPaymentRepo.find({
-      where: { externalId: externalPaymentId },
-      relations: { link: { route: { user: { userData: true } } } },
+      where: { externalId: externalPaymentId, link: { id: linkId } },
     });
   }
 

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');