Aman-Mittal opened a new pull request, #692:
URL: https://github.com/apache/fineract-backoffice-ui/pull/692
## The defect
**The role permission matrix could not save. Not for some roles — for any
role, by any administrator, whatever was ticked.**
Fineract publishes five permission codes with a trailing space, and
`READ_STANDINGINSTRUCTION` exists in *no other form*:
```
"CREATE_STANDINGINSTRUCTION " "DELETE_STANDINGINSTRUCTION "
"READ_STANDINGINSTRUCTION " "UPDATE_STANDINGINSTRUCTION "
"READ_ClientSummary "
```
`role-form.component.ts` trimmed each code on the way in. That part is
right, and deliberate: every route's `data.permissions` uses the trimmed
spelling, and `DOCS/RBAC.md` already records that a gate on
`READ_STANDINGINSTRUCTION` is unsatisfiable without the trim. It then sent the
trimmed keys back, and since the payload is the whole code-to-boolean map,
every save failed:
```
PUT /roles/153/permissions → 404
"Permission with Code READ_STANDINGINSTRUCTION does not exist"
```
Three things kept it hidden, and they are the same three ADR 0006 names:
- the component's spec mocks `putRolesRoleIdPermissions`;
- its fixture contained no padded code, so no test could have noticed;
- every RBAC e2e spec grants permissions over HTTP through `seed-api.ts`
rather than through the screen, so the one place in the application that writes
this endpoint had never been driven against a real platform.
### The fix
`RolePermission` carries both spellings and fixes which is used where:
```ts
export interface RolePermission {
/** Fineract's spelling, trailing space and all. The key setPermissions
must be given. */
readonly code: string;
/** code trimmed. What the matrix shows and filters on. */
readonly label: string;
…
}
```
`setPermissions` takes `readonly PermissionSelection[]` rather than a
`Record<code, boolean>`. A map leaves the caller to choose which spelling to
key by, and choosing the readable one is exactly the bug; taking rows built
from what `permissions()` answered makes the wrong spelling unreachable rather
than merely discouraged.
Trimming still happens, in one place, on the read side only:
`destinationsFor()` trims before asking `NavigationConfigService` what screens
a selection opens, because that comparison is against the codes routes declare.
The two directions are now different jobs with different code, which is the
general lesson — recorded in ADR 0006 — that a contract earns most where the
application was already normalising a platform quirk.
## The boundary work
`ROLE_API` and `USER_API` put roles and users behind contracts, clearing six
files and dropping `local/no-generated-api-import` from **435 to 429** (total
suppressions 716 → 710).
Three casts came out that the generated types never needed:
| Was | Reality |
| --- | --- |
| `permissionUsageData as unknown as Record<string, unknown>[]` |
`GetRolesRoleIdPermissionsResponsePermissionData` declares `code`, `grouping`,
`selected` |
| `allowedOffices as unknown as Record<string, unknown>[]` |
`Array<OfficeData>`, with `id` and `name` declared — the cast is how
`office['id']` escaped `strictTemplates` |
| `selectedRoles?.map((r) => r.id!)` | `id?: number`; an `undefined` in that
list preselects nothing, and an unticked box is indistinguishable from a role
not held, so the next save silently revoked it |
`create()` now requires the `resourceId` instead of falling back to the
list. The id *is* the next screen — the new role's permission matrix — and a
role holding nothing is not something to send an administrator back to the list
to find again.
## The spec that found it
`e2e/loan-flows-non-admin-roles.spec.ts` takes one loan from application to
active across four separated duties — officer, approver, disburser, repayment —
with **every role, permission grant and account created by filling the forms**.
No `seed-api.ts` import.
That is the point. Driving the administrative screens is what exposed the
404, and it covers Fineract's `transaction_loan` grouping (`APPROVE_LOAN`,
`DISBURSE_LOAN`, `REPAYMENT_LOAN`) — the codes a lender actually separates
duties along, and which the suite had no coverage of at all. The existing
`rbac-*` specs vary what a session may *read*.
`seed-api.ts` opens by explaining that these prerequisites used to be built
through the UI and were moved to HTTP because the chain was flaky. That
reasoning is right for a spec whose subject is something else; here the setup
*is* the subject, so it is driven through the forms deliberately, and the one
genuinely racy step reuses the `toPass` retry `loan-lifecycle.spec.ts` already
has for it.
Every claim about what an account may do is checked twice — against what the
application offers, and against what Fineract answers the same account.
`platformAllows()` does the second half **without performing the operation**:
Fineract authorises before it validates, so an empty body separates 403
(refused) from 400 (authorised, bad body). That distinction is the whole
assertion and costs no state.
All nine cases pass against a real instance.
## What it confirmed: #691
Two of those cases document a defect rather than asserting correct
behaviour. `:loanId/transactions/:type` is gated on `UPDATE_LOAN`, which is
**neither necessary nor sufficient**:
| Account holds | Platform, `disburse` | The route |
| --- | --- | --- |
| `DISBURSE_LOAN` | **400** — authorised | **refuses** → `/forbidden` |
| `UPDATE_LOAN` | **403** — refused | **admits** → a form that can only 403 |
The action layer is right; the route contradicts it. A disburser is offered
an enabled button whose only destination is Access Denied. Left as-is here and
filed as #691, since the fix is a routing decision — each assertion carries a
message saying that a pass after the fix means it needs inverting, so the spec
will fail loudly rather than silently assert the old behaviour.
## Two corrections this made to my own assumptions
- **A refused loan action is disabled, not hidden.** My first draft expected
an absent button. `appRequiresPermission` disables and names the missing code;
`appHasPermission` removes. The directive documents why, and the reasoning
holds — an action on the record on screen is a fact about the user's role they
need in order to act on it, so hiding it would leave them concluding the
feature is missing. The assertions check for the refusal marker instead;
asserting `toHaveCount(0)` would have been asserting the wrong design.
- **`products/:accountType/:accountId/action/:command` is *not* a missing
gate.** It has no `permissions` declaration, which looks like an oversight
until you find it recorded in `check-route-permissions.mjs`'s `UNRESTRICTED`
with a sound reason: the code depends on `:accountType` and `:command` at
runtime. #691 is the narrower case where the type *is* in the path and so is
knowable.
## Locator hazards worth recording
Both found by running this, both the same shape as the folded accessible
name `ionSelect()` already documents:
- **`ion-button` hoists host `aria-*` into its shadow root**, so the reason
the directive binds as `aria-label` is not on the element a `data-testid`
resolves to. Read off `title` instead.
- **`ion-checkbox` does not reflect `name` onto the host** — it goes into a
hidden `input.aux-input` — so `ion-checkbox[name=…]` matches nothing. The
permission matrix is addressed by accessible name with `exact`, which also
stops `READ_LOAN` from matching `READ_LOANPRODUCT`.
Adds `data-testid="loan-disburse-action"`, which the button lacked while
Approve and Repayment had theirs. It matters beyond tests: with the name
replaced by the reason, there is otherwise no stable locator for a refused
Disburse.
## Verification
- **290 unit test files, 1987 tests, all passing** — +12 for
`FineractRoleApi`, +15 for `FineractUserApi`, and `role-form.component.test.ts`
rewritten with a padded code in its fixture plus a regression test asserting
the padded spelling goes out.
- `npm run lint` clean, suppressions re-pruned, `prettier --check` clean,
`tsc` clean for both the app and the e2e project.
- `e2e/loan-flows-non-admin-roles.spec.ts` — 9/9 against the local stack.
- Full e2e suite re-run and green; a first pass showed 12 failures, all of
which passed single-worker, with signatures (240 s timeouts, `ECONNRESET`,
`Network.getResponseBody`) of one Fineract under too much concurrent load
rather than of defects.
## Follow-up
`audit-logs-list.component.ts` is the remaining `security/` file on the
generated client — a long positional `getAudits()` call and a `Record<string,
unknown>[]` model. Left out deliberately: it is the largest single piece of the
area, nothing in this PR exercises it, and it would have buried the fix.
--
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]
