Aman-Mittal opened a new pull request, #695:
URL: https://github.com/apache/fineract-backoffice-ui/pull/695
Closes #691. Closes #665.
Six gates named a permission that cannot authorise what the control does.
Each one was measured against a running Fineract with an **empty body**, which
separates authorisation from validation without writing anything — the platform
authorises before it validates, so `403` means the code is wrong and `400`
means it is right and only the body was bad.
## 1. Dispatch routes declared one code for many commands (#691)
```
/loans/:loanId/transactions/:type UPDATE_LOAN (29
commands)
/products/savings-accounts/.../:command UPDATE_SAVINGSACCOUNT
```
Both are in Fineract's `portfolio` grouping; the commands they serve are in
`transaction_loan` and `transaction_savings`. So the declaration was wrong for
**every** command, in **both** directions:
| Account holds | Platform | The route |
| --- | --- | --- |
| `DISBURSE_LOAN` | 400 authorised | **refused** → `/forbidden` |
| `REPAYMENT_LOAN` | 400 authorised | **refused** |
| `DEPOSIT_SAVINGSACCOUNT` | 400 authorised | **refused** |
| `WITHDRAWAL_SAVINGSACCOUNT` | 400 authorised | **refused** |
| `UPDATE_LOAN` | 403 refused | **admitted** → a form that can only 403 |
| `UPDATE_SAVINGSACCOUNT` | 403 refused | **admitted** |
The action gates on the buttons were already right, so the two layers
contradicted each other: the only control the loan screen offered a disburser
led to Access Denied.
`data.permissions` now also accepts a function of the route's params, so the
code is resolved from `:type` or `:command`. The 26 entries in
`core/guards/command-permissions.ts` were each confirmed at the endpoint the
form actually posts to — they are **not** derivable from the command names:
| Command | Code |
| --- | --- |
| `withdrawnByClient` | `WITHDRAW_LOAN` |
| `waiveinterest` | `WAIVEINTERESTPORTION_LOAN` |
| `close-rescheduled` | `CLOSEASRESCHEDULED_LOAN` |
| `prepayLoan` | `REPAYMENT_LOAN` — a prepayment *is* a repayment; there is
no PREPAY code |
**Four commands are deliberately left unmapped.** The probe gave no clean
answer for `undoContractTermination` and `reAmortize` (403 at the endpoint the
form uses, 400 at the other URL shape), `undowriteoff` (503), or
`postInterestAsOn` (refused for both its apparent code *and* the one the route
declared). Those fall through to no client-side gate, the same position
`products/:accountType/:accountId/action/:command` is already documented in. A
guessed code refuses a user the platform would have allowed, which is the more
damaging and less visible half of this bug.
## 2. The savings cash-withdrawal button asked for the
application-withdrawal code
Both codes exist, one letter apart, and they are strictly
non-interchangeable:
```
transactions?command=withdrawal WITHDRAWAL_ 400 WITHDRAW_ 403
?command=withdrawnByApplicant WITHDRAWAL_ 403 WITHDRAW_ 400
```
`WITHDRAW_SAVINGSACCOUNT` withdraws the *application* — which the
Withdrawn-by-applicant button right above it correctly uses. This one withdraws
*cash*. A teller holding `WITHDRAWAL_SAVINGSACCOUNT` was shown a disabled
button; a holder of the application code was offered an enabled one the
platform refuses.
## 3. Two gates named codes that do not exist
Found by diffing every code the application names against a live `GET
/v1/permissions`:
- **`UNDOWITHDRAW_CLIENT`** → `UNDOWITHDRAWAL_CLIENT`. The same missing
"AL"; confirmed at 400 against `undoWithdrawal` while `WITHDRAW_CLIENT` answers
403.
- **`UPDATE_LOAN_AVAILABLE_DISBURSEMENT_AMOUNT`** → no such code, and **no
narrower one exists**. Found by bisecting the whole catalogue: a role granted
all 721 grantable codes is still refused `PUT
/loans/{id}/available-disbursement-amount`, while the superuser gets 400 on the
same request. It now declares `ALL_FUNCTIONS`, which is the true requirement
and makes the refusal hint name something an administrator can act on.
This is the failure `DOCS/RBAC.md` already warns about — "a gate on a code
that does not exist can never be satisfied by any role, which is worse than no
gate" — because the control is disabled forever and looks exactly like one the
viewer is not entitled to.
## 4. Office transactions needed the opposite of what #665 proposed
That issue recommended requiring `READ_OFFICETRANSACTION` **and**
`READ_OFFICE`. Measuring first showed the transaction code does nothing for
these endpoints at all:
```
READ_OFFICE alone → GET /officetransactions 200, /template 200
READ_OFFICETRANSACTION alone → 403, 403
READ_CLIENT alone (control) → 403, 403
```
So an AND would still refuse a `READ_OFFICE` holder the platform serves. The
list route and its nav entry now declare `READ_OFFICE`. The create route
declares both, since `/officetransactions/template` is also refused without
`READ_OFFICE` — a holder of the create code alone reached a form whose office
picker could never populate. Writing stays gated separately and correctly on
the controls (`CREATE_OFFICETRANSACTION`, `DELETE_OFFICETRANSACTION`), so
widening the read does not widen what can be done.
## Two checks so this class cannot come back
**`check-permission-codes.mjs`** (new, in CI) compares every code named in
an action gate, structural gate, route, nav entry, data table or command map
against a committed snapshot of the catalogue — so it runs with no Fineract
started. 311 codes checked. The snapshot drifts only as Fineract adds codes,
which is a false *pass* and never a false failure; `npm run
permissions:snapshot` refreshes it.
Its header states what it **cannot** catch: a code that exists but is wrong
for the operation, i.e. defect 2 above. Only a run against a real platform
separates those. Both halves are written down so nobody mistakes a green check
for proof the codes are right.
**`check-route-permissions.mjs`** learns the dynamic form, with a
`PER_COMMAND` registry mirroring `UNRESTRICTED`: a dispatch route must be
recorded with its reason to be accepted, and the script reads the map it names
and fails if that map is missing or empty. Worth noting the old check *passed*
on #691 — the route did declare a permission, so it had no way to know the
declaration named the wrong code.
## Verification
- **291 unit test files, 2006 tests passing.** 11 new cases for the dispatch
guard (including that `ALL_FUNCTIONS_READ` cannot reach a write command and
that an unmapped command admits rather than guesses) and 11 for the map itself.
- All nine `check:*` scripts pass, plus the new one. `npm run lint`,
`prettier --check .` and `tsc` clean for app and e2e.
- **Mutation-verified, five mutations, each producing a distinct failure**:
renaming a dynamic identifier, pointing the registry at a map that does not
exist, emptying the map, reintroducing `UNDOWITHDRAW_CLIENT`, and hollowing out
the snapshot. A sixth confirms the stated limitation is real — swapping the
savings code back to the wrong-but-existing one **passes** the code check.
- `e2e/loan-flows-non-admin-roles.spec.ts` rewritten to assert the fix: the
disburser holding only `DISBURSE_LOAN` drives the payout to Active, and the
repayment duty records a repayment — both end to end, because a gate that
admits the right user but a form that cannot submit would pass a narrower
assertion.
- `e2e/savings-flows-non-admin-roles.spec.ts` (new) covers
`transaction_savings`, which had no coverage at all: an account from
application to funded across officer, approver, activator and teller, every
role and user built by filling the forms. The cash-withdrawal code is asserted
from both sides.
## One ordering trap worth recording
The editor's test originally asserted the Disburse button was refused. That
broke once the disburser actually completed the payout: Disburse is gated on
`@if (isLoanApproved)`, so on an active loan it is absent for everyone
including a superuser. The assertion would have passed for a reason unrelated
to permissions — which is how an assertion keeps passing after the thing it
checked has broken. It now asserts Repayment, which *is* offered on an active
loan and which that account genuinely lacks the code for.
## Not changed
**#666** (the Offices screen asking for `includeAllOffices=true`) is left
alone. That issue is explicit that it could not establish which behaviour is
intended, and the platform answers either way — so it is a product decision,
not a defect, and not mine to make inside a bug-fix PR.
--
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]
