diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java index 25b268f90..592a66d32 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java @@ -423,7 +423,13 @@ public class CashierDepositRequestApiResource { // even for current accounts (submitted before the frontend fix), and FD/RD accounts share // the very same m_savings_account table as a plain savings account, distinguished only by // deposit_type_enum -- so this is looked up fresh rather than trusted from the request. - final String depositCategory = resolveDepositAccountCategory(savingsAccountId); + // The account_no is also checked -- m_current_account and m_savings_account each keep + // their own identity sequence, so the same numeric id can legitimately belong to a + // current account AND an unrelated savings account at once. An id match alone once sent + // a real deposit into a stranger's current account instead of the savings account the + // request actually named; account_no is the one value that can't collide between them. + final String requestAccountNo = (String) record.get("account_no"); + final String depositCategory = resolveDepositAccountCategory(savingsAccountId, requestAccountNo); final boolean isCurrentAccount = "CURRENT".equals(depositCategory); txnId = switch (depositCategory) { case "CURRENT" -> executeCurrentAccountDeposit(savingsAccountId, amount, ptId, txnDateStr, description); @@ -578,7 +584,8 @@ public class CashierDepositRequestApiResource { final String txnDateStr = txnDateObj != null ? Iterables.get(Splitter.on(' ').split(txnDateObj.toString()), 0) : null; final String description = (String) record.get("description"); - if (savingsAccountId == null || !isCurrentAccountById(savingsAccountId)) { + final String requestAccountNo = (String) record.get("account_no"); + if (savingsAccountId == null || !isCurrentAccountById(savingsAccountId, requestAccountNo)) { throw apiError(400, "This account is not a current account. Reprocess is only needed for current account deposits."); } @@ -863,11 +870,30 @@ public class CashierDepositRequestApiResource { return result.getResourceId() != null ? result.getResourceId().toString() : null; } - /** Returns true if this account ID belongs to m_current_account (not m_savings_account). */ - private boolean isCurrentAccountById(final Long accountId) { + /** + * Returns true if this account ID belongs to m_current_account (not m_savings_account). + * + * m_current_account and m_savings_account each keep their own identity sequence, so id=1 (say) + * can be a real row in BOTH tables at once — two completely unrelated accounts, possibly two + * different clients, sharing nothing but a coincidental number. An id-only check here once sent + * a savings deposit into a stranger's current account: id=1 existed in m_current_account, so + * this returned true, even though the request was for m_savings_account id=1. + * + * expectedAccountNo — the account_no the caller actually meant — closes that gap: an id match is + * only trusted once the account_no on that current-account row confirms it is truly the same + * account, not a same-numbered stranger. Pass null to skip the check (only safe where the caller + * has no account_no to compare against, e.g. an id already known to be unambiguous). + */ + private boolean isCurrentAccountById(final Long accountId, final String expectedAccountNo) { try { + if (expectedAccountNo == null || expectedAccountNo.isBlank()) { + final Integer count = jdbcTemplate.queryForObject( + "SELECT COUNT(*) FROM m_current_account WHERE id = ?", Integer.class, accountId); + return count != null && count > 0; + } final Integer count = jdbcTemplate.queryForObject( - "SELECT COUNT(*) FROM m_current_account WHERE id = ?", Integer.class, accountId); + "SELECT COUNT(*) FROM m_current_account WHERE id = ? AND account_no = ?", + Integer.class, accountId, expectedAccountNo); return count != null && count > 0; } catch (Exception e) { return false; @@ -880,12 +906,15 @@ public class CashierDepositRequestApiResource { * m_savings_account table as a plain savings account (deposit_type_enum 200/300 vs 100), so a * plain "not current → savings" check would post an FD/RD deposit through the generic savings * deposit command, which does not accept a term-deposit account id. + * + * expectedAccountNo guards the current-account check against an id collision with + * m_savings_account — see {@link #isCurrentAccountById}. */ - private String resolveDepositAccountCategory(final Long accountId) { + private String resolveDepositAccountCategory(final Long accountId, final String expectedAccountNo) { if (accountId == null) { return "SAVINGS"; } - if (isCurrentAccountById(accountId)) { + if (isCurrentAccountById(accountId, expectedAccountNo)) { return "CURRENT"; } final List depositTypes = jdbcTemplate.queryForList( diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java index b8cc344d9..b94837355 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java @@ -551,8 +551,11 @@ public class ClerkDisbursementRequestApiResource { final BigDecimal entryAmt = new BigDecimal(entry.get("amount").getAsString()); final String acNo = entry.has("accountNo") ? entry.get("accountNo").getAsString() : "?"; final Long accountId = entry.get("savingsAccountId").getAsLong(); - final boolean isCurrent = "CURRENT".equals( - entry.has("accountType") ? entry.get("accountType").getAsString() : "SAVINGS"); + // Not trusted from entry.accountType alone -- see isCurrentAccountById: an id can + // exist in both m_current_account and m_savings_account at once, and a bank + // transfer posts and moves money immediately, with no maker-checker step to catch + // a wrong label before it does. + final boolean isCurrent = isCurrentAccountById(accountId, acNo); final Long ctrlGl = isCurrent ? getCurrentControlGlId(accountId) : getSavingsControlGlId(accountId); @@ -660,7 +663,7 @@ public class ClerkDisbursementRequestApiResource { } final Long accountId = entry.get("savingsAccountId").getAsLong(); - final boolean isCurrentAccount = isCurrentAccountById(accountId); + final boolean isCurrentAccount = isCurrentAccountById(accountId, acNo); String txnId = null; if ("Cr".equals(dbCr)) { @@ -959,8 +962,11 @@ public class ClerkDisbursementRequestApiResource { final Long accountId = e.get("savingsAccountId").getAsLong(); final BigDecimal amount = new BigDecimal(e.get("amount").getAsString()); final String acNo = e.has("accountNo") ? e.get("accountNo").getAsString() : "?"; - final String acTyp = e.has("accountType") ? e.get("accountType").getAsString() : "SAVINGS"; - final boolean isCurrent = "CURRENT".equals(acTyp); + // Not trusted from entry.accountType alone -- see isCurrentAccountById: an id can + // exist in both m_current_account and m_savings_account at once, and a bank transfer + // posts and moves money immediately, with no maker-checker step to catch a wrong + // label before it does. + final boolean isCurrent = isCurrentAccountById(accountId, acNo); final Long ctrlGl = isCurrent ? getCurrentControlGlId(accountId) : getSavingsControlGlId(accountId); @@ -986,8 +992,8 @@ public class ClerkDisbursementRequestApiResource { final Long accountId = e.get("savingsAccountId").getAsLong(); final BigDecimal amount = new BigDecimal(e.get("amount").getAsString()); final String acNo = e.has("accountNo") ? e.get("accountNo").getAsString() : "?"; - final String acTyp = e.has("accountType") ? e.get("accountType").getAsString() : "SAVINGS"; - final boolean isCurrent = "CURRENT".equals(acTyp); + // Not trusted from entry.accountType alone -- see isCurrentAccountById. + final boolean isCurrent = isCurrentAccountById(accountId, acNo); final Long ctrlGl = isCurrent ? getCurrentControlGlId(accountId) : getSavingsControlGlId(accountId); @@ -1361,10 +1367,25 @@ public class ClerkDisbursementRequestApiResource { commandsSourceWritePlatformService.logCommandSource(cmd); } - private boolean isCurrentAccountById(final Long accountId) { + /** + * m_current_account and m_savings_account each keep their own identity sequence, so the same + * numeric id can be a real row in both at once -- two unrelated accounts, possibly two different + * clients, sharing nothing but a coincidental number. An id-only check here has already sent a + * real deposit into a stranger's current account instead of the savings account the entry + * actually named. expectedAccountNo -- the entry's own accountNo -- closes that gap: an id match + * is only trusted once the account_no on that current-account row confirms it. Pass null to skip + * the check where the caller has no account_no to compare against. + */ + private boolean isCurrentAccountById(final Long accountId, final String expectedAccountNo) { try { + if (expectedAccountNo == null || expectedAccountNo.isBlank() || "?".equals(expectedAccountNo)) { + final Integer count = jdbcTemplate.queryForObject( + "SELECT COUNT(*) FROM m_current_account WHERE id = ?", Integer.class, accountId); + return count != null && count > 0; + } final Integer count = jdbcTemplate.queryForObject( - "SELECT COUNT(*) FROM m_current_account WHERE id = ?", Integer.class, accountId); + "SELECT COUNT(*) FROM m_current_account WHERE id = ? AND account_no = ?", + Integer.class, accountId, expectedAccountNo); return count != null && count > 0; } catch (Exception e) { return false; diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkWithdrawalRequestApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkWithdrawalRequestApiResource.java index 8e2891297..57f9cb070 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkWithdrawalRequestApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkWithdrawalRequestApiResource.java @@ -470,7 +470,7 @@ public class ClerkWithdrawalRequestApiResource { String txnId = null; if (savingsAccountId != null) { - final boolean isCurrentAccount = isCurrentAccountById(savingsAccountId); + final boolean isCurrentAccount = isCurrentAccountById(savingsAccountId, (String) record.get("account_no")); txnId = isCurrentAccount ? executeCurrentAccountWithdrawal(savingsAccountId, amount, ptId, txnDateStr, description) : executeSavingsWithdrawal(savingsAccountId, amount, ptId, txnDateStr, description); @@ -872,10 +872,25 @@ public class ClerkWithdrawalRequestApiResource { return result.getResourceId() != null ? result.getResourceId().toString() : null; } - private boolean isCurrentAccountById(final Long accountId) { + /** + * m_current_account and m_savings_account each keep their own identity sequence, so the same + * numeric id can be a real row in both at once -- two unrelated accounts, possibly two different + * clients, sharing nothing but a coincidental number. An id-only check here can send a real + * withdrawal against a stranger's current account instead of the savings account the request + * actually named. expectedAccountNo -- the request's own account_no -- closes that gap: an id + * match is only trusted once the account_no on that current-account row confirms it. Pass null + * to skip the check where the caller has no account_no to compare against. + */ + private boolean isCurrentAccountById(final Long accountId, final String expectedAccountNo) { try { + if (expectedAccountNo == null || expectedAccountNo.isBlank()) { + final Integer count = jdbcTemplate.queryForObject( + "SELECT COUNT(*) FROM m_current_account WHERE id = ?", Integer.class, accountId); + return count != null && count > 0; + } final Integer count = jdbcTemplate.queryForObject( - "SELECT COUNT(*) FROM m_current_account WHERE id = ?", Integer.class, accountId); + "SELECT COUNT(*) FROM m_current_account WHERE id = ? AND account_no = ?", + Integer.class, accountId, expectedAccountNo); return count != null && count > 0; } catch (Exception e) { return false;