adamsaghy commented on PR #6698: URL: https://github.com/apache/fineract/pull/6698#issuecomment-6294835473
### Findings **1. Collection-sheet savings deposits skip the check (confirmed by tracing the code)** - **Path:** `CollectionSheetWritePlatformServiceJpaRepositoryImpl.updateBulkMandatorySavingsDuePayments` calls `DepositAccountWritePlatformServiceJpaRepositoryImpl.mandatorySavingsAccountDeposit`, which loads the account with `depositAccountAssembler.assembleFrom(...)`. There is no office check. - **No other guard:** the `savingsId` comes straight from the `bulkSavingsDueTransactions` JSON (`DepositAccountAssembler:471`). Nothing checks that the account belongs to the group. - **Effect:** a sibling-branch user can still deposit into another branch's savings account through `saveCollectionSheet` or `saveIndividualCollectionSheet`. The same request's loan repayments and disbursals are blocked, because lines 768 and 1207 were changed. - **Why it matters:** the description says deposits are covered. **2. Fixed and recurring deposit accounts are not covered at all** - Every method in `DepositAccountWritePlatformServiceJpaRepositoryImpl` still uses `assembleFrom`. That includes `depositToRDAccount`, `depositToFDAccount`, `withdrawal`, `undoFD/RDTransaction`, `adjustFD/RDTransaction`, `prematureCloseFD/RDAccount` and `closeFD/RDAccount`. - These are the same kinds of operation the PR blocks on plain savings. Either guard them or say in the description that they are out of scope. **3. Undoing a loan disbursal can now fail with 403 (plausible; traced, not run)** - `undoLoanDisbursal` calls `reverseAllTransactions(loanId, LOAN)` at line 935. That undoes every account transfer linked to the loan through `savingsAccountWritePlatformService.undoTransaction`, which now goes through `assembleFromInUserScope` (line 795). - Two kinds of transfer can involve a savings account in another branch. Standing instructions accept a `toOfficeId`. Guarantors have no office restriction, so a guarantor recovery can pull from another branch. - **Effect:** a branch-A user who can see and act on a branch-A loan gets a 403 on undo-disbursal, and the whole undo rolls back. Before this PR it worked. - The original transfer ran unchecked, as the system user or through `transferFunds`. So the user can't reverse something they were never blocked from creating. - **Suggested fix:** in internal reversal paths, check the account that triggered the reversal (the loan), not each counterparty account. ### Smaller gaps against the stated scope (non-blocking) - **Loan transactions outside `LoanWritePlatformServiceJpaRepositoryImpl`:** capitalized income, buy-down fee, re-age, re-amortize, contract termination and interest pause still use `assembleFrom`. Capitalized income and buy-down fee create transactions with an amount, so they count as monetary. - **Working-capital loans** (`fineract-working-capital-loan`) have their own repayment, write-off and charge-off services. None of them is guarded. - **Transfers:** the account-transfer API takes explicit `fromOfficeId` and `toOfficeId` parameters. Requiring both sides to be in scope is a deliberate breaking change for clients that do cross-branch transfers as non-head-office users. It should be called out in the release notes. ### Tests The integration tests give the sibling user the relevant permission (for example `REPAYMENT_LOAN`), so the 403 really comes from the hierarchy check and not from a missing permission. There is no test for the collection-sheet path, FD/RD, or undo-disbursal across branches. Those are the three findings above. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
