Aman-Mittal opened a new pull request, #368:
URL: https://github.com/apache/fineract-backoffice-ui/pull/368
Closes #367
## Summary
An authenticated user could reach any screen in the application by knowing
its URL. `authGuard`
answered *"is this user signed in?"*; nothing answered *"may this user open
this feature?"*. All
324 routes carried authentication and nothing else, so a user holding
`READ_CLIENT` could type
`/accounting/chart-of-accounts` into the address bar and the screen opened.
The sidebar hid the
entry; the URL handed it straight back.
This PR closes that, brings navigation visibility into agreement with it,
gates the privileged
actions behind it, and turns on the release gate that has been failing
without anyone seeing it.
> **The Angular permission guard is defence-in-depth and does not replace
server-side
> authorization. Fineract Core remains the authoritative security
boundary.** Every screen here is
> backed by an API that performs its own permission check. What this buys is
that a user is not led
> into a screen whose every request will 403, and that the URLs a user can
reach agree with the
> navigation they are shown. The backend E2E spec exists to demonstrate
that, not to obscure it.
## Changes
**Route authorization** — `core/guards/permission.guard.ts`. Reads
`data.permissions` (and
`data.permissionsMatchAll`) from the route and refuses to `/forbidden`.
Evaluation is delegated
entirely to the existing `AuthService.hasPermission()`, so there is no
second permission
implementation and `ALL_FUNCTIONS` / `ALL_FUNCTIONS_READ` work with no new
logic.
Deliberately a `CanActivateFn`, not a `CanMatchFn`: a failing `canMatch`
makes the router carry on
looking and land on `path: '**' → redirectTo: ''`, silently depositing the
user on the dashboard.
That reads as a broken link rather than a decision, and cannot be asserted
on.
**Access Denied** — `features/errors/access-denied.component.ts` at
`/forbidden`, inside the
authenticated shell but carrying no permission of its own (a refusal that
could itself be refused
would loop). Single `<h1>` that takes focus on arrival — the user did not
ask for this navigation —
a polite `aria-live` region, keyboard-reachable return to the dashboard,
i18n keys in `en`/`hi`/`ko`.
It does not name the missing permission: a code is not something an end user
can act on, and
telling an unauthorized visitor exactly which grant unlocks a screen is a
hint worth withholding.
**Route requirements** — 263 of 298 screens now declare one, across all 23
feature route files.
Lists and detail views take `READ_*`, create routes `CREATE_*`, edit routes
`UPDATE_*`. That split
is not stylistic: `ALL_FUNCTIONS_READ` admits a request only when *every*
required code is a read
one, so declaring `READ_CLIENT` on the create route would hand a read-only
user a form they cannot
submit.
**Navigation parity** — `requiredPermissions` declarations go from 21 to
100, covering every one
of the 115 routed entries whose route is gated. `filterNavItems()` is
unchanged and no second
navigation permission system was introduced; this is data.
**Anti-drift check** — `scripts/check-route-permissions.mjs`, in the style of
`check-internal-endpoints.mjs`. Static by necessity: every feature is behind
`loadChildren`, so a
lazy feature's routes are absent from `Router.config` until something
navigates into them — long
after the navigation is built at login.
**Action-level authorization** — the shared `app-data-table` gains a
`createPermission` input, and
all 58 list screens with a create button now name the permission their own
create route declares.
`loan-view` had 4 gated actions out of 33; the remaining 29 (approve,
disburse, reject, write-off,
charge-off, waive, repayment, re-age, re-amortize, refunds, …) are now gated
with the code Fineract
requires for each. The existing `*appHasPermission` directive is the
mechanism throughout.
**Four permission codes that do not exist** were being used, and so could
never be satisfied by any
role — the controls behind them were invisible to everyone except
`ALL_FUNCTIONS` holders:
| Used | Actual Fineract code | Where |
|---|---|---|
| `CREATE_/UPDATE_/DELETE_CLIENTFAMILYMEMBER` | `..._FAMILYMEMBERS` |
`client-family-members-list` |
| `CREATE_/UPDATE_/DELETE_NOTE` | `..._CLIENTNOTE` | `client-notes-list` |
| `CREATE_/UPDATE_/DELETE_DELINQUENCYBUCKET` / `...RANGE` |
`..._DELINQUENCY_BUCKET` / `..._RANGE` | `delinquency-management` |
| `READ_LOANCOLLATERAL` | `READ_COLLATERAL` | `loans-list` |
**A session with no `permissions` field crashed login.** `setSession` called
`normalizePermissions(session.permissions)` unguarded while
`getStoredSession` used `?? []`; a
truncated or malformed response threw inside `login()` and the user could
not sign in at all.
**GA gate** — `npm run ga:check` has existed and failed without CI ever
running it. It now runs as
a blocking job, alongside `npm run api:surface`. No gate was weakened and no
blocking failure was
downgraded; the adapter-boundary backlog stays advisory and its count is
unchanged at 420.
The API surface manifest was re-recorded after reviewing all 21 reported
entries individually: 4
services genuinely removed upstream (MIX ×3, CashierJournals — retirements
already established),
and 17 operations with real non-spec call sites in merged work. One further
stale entry surfaced,
`RunReportsService.getRunreportsReportName` —
`report-execution.service.ts:244` builds that URL by
hand, so nothing calls the generated operation.
**Documentation** — `DOCS/RBAC.md`, with a pointer from `security.md`. The
"not release-ready"
notice in `security.md` stays; that comes out when the gates justify it, not
because this landed.
## RBAC
- **Source of truth**: `AuthService.hasPermission()`. Route guard,
navigation service and the
`*appHasPermission` directive all call it. Nothing re-implements the
semantics.
- **Super user**: `ALL_FUNCTIONS` admits everything, everywhere.
- **Read-only**: `ALL_FUNCTIONS_READ` admits a requirement only when every
code in it starts with
`READ_`. A requirement mixing a read code with a write one falls through
to the ordinary check.
- **OR / AND**: several codes are OR by default; `permissionsMatchAll: true`
on a route (or
`requiredAllPermissions: true` on a nav entry) makes them AND.
- **Ordering**: always `canActivate: [authGuard, permissionGuard]`. An
unauthenticated visitor must
reach `/login`, never `/forbidden`. The drift check enforces the order.
- **Navigation is not the boundary**: every hidden entry is also refused by
URL. That is the whole
point — hiding alone was the old behaviour.
- **`rbacEnabled: false`** restores pre-RBAC behaviour across all three
layers, for deployments
mid-rollout. It changes nothing server-side.
### Permission codes came from the platform, not from memory
Every code was checked against `GET /v1/permissions` on a running Fineract
(698 codes). Nothing was
invented. Where Fineract defines no read code, the screen is recorded in the
check's `UNRESTRICTED`
list with that reason rather than given a fabricated gate that no role could
satisfy — tellers,
share products and accounts, provisioning, ad-hoc queries, entity mapping,
group levels, external
asset owners and the SPM screens are all in this category. Where write codes
*do* exist for the
same feature, the write routes are gated even though the read route is not;
that asymmetry is
deliberate and documented.
One subtlety worth recording: Fineract seeds a duplicate
`STANDINGINSTRUCTION ` family whose codes
carry a **trailing space**, and `READ_STANDINGINSTRUCTION` exists *only* in
that padded form. The
existing session normalisation in `AuthService` is what makes that gate
satisfiable.
## Testing
All run locally against a fresh stack (`bash scripts/e2e-stack.sh`), in this
order:
| Command | Result |
|---|---|
| `npm run lint:prune` | pass — no new suppressions |
| `npm run format:check` | pass |
| `npm run i18n:check` | pass — 1522 keys, none missing |
| `npm run check:icons` | pass |
| `npm run check:internal-endpoints` | pass |
| `npm run check:route-permissions` | pass — 298 screens, 115 nav entries
agree |
| `npm run test` | **977 SUCCESS**, 0 failed (grepped for `✘`/`FAILED`, not
the tail line) |
| `npm run test:mfe` | 2 SUCCESS |
| `npm run build` | pass — production bundle generated |
| `npm run api:surface` | pass — 142 services, 555 operations |
| `npm run ga:check` | **8/9, 0 blocking failures** (was 7/9 with 1
blocking) |
| `./scripts/check-license.sh` | pass |
| `npx playwright test --project=mocked rbac-route-protection.spec.ts` |
**15 passed** |
| `npx playwright test --project=backend
rbac-backend-restricted-user.spec.ts` | **7 passed** |
| `npm run test:e2e:local -- --workers=1` | full suite (314 tests) — running
at time of writing; result added below when it completes |
The drift check was verified to fail, not merely to pass: a removed route
permission, a nav/route
mismatch, a reversed guard order and a stale allow-list entry were each
injected and each produced
a specific error naming the route, the expected value and the actual one.
## E2E scenarios
**Mocked** (`e2e/rbac-route-protection.spec.ts`, 15 tests) — the permission
set is injected by
mocking the authentication response, so combinations that would be tedious
to seed are covered on
every PR:
- `ALL_FUNCTIONS` superuser — every sampled route reachable, full navigation
- `READ_CLIENT` only — `/clients` reachable, accounting/security/loans
refused by URL
- `ALL_FUNCTIONS_READ` — lists reachable, `/clients/create` and
`/clients/edit/1` refused
- empty permission list — refused everywhere, navigation collapses,
self-service still reachable, no crash
- **missing** `permissions` field — treated as none, not as a crash and not
as a wildcard
- unrecognised permission code — grants nothing
- unauthenticated, direct URL — `/login`, **not** `/forbidden`
- `rbacEnabled: false` — pre-RBAC behaviour restored
- Access Denied — single heading, focus lands on it, polite announcement,
dashboard return, reachable by keyboard alone
- navigation parity — a hidden entry is also refused by URL; the create
button is withheld where the create route would refuse
**Real backend** (`e2e/rbac-backend-restricted-user.spec.ts`, 7 tests) —
`seedRole()` and
`seedRestrictedUser()` create a real Fineract role and user through `POST
/roles`,
`PUT /roles/{id}/permissions` and `POST /users`, with no hardcoded ids:
- the seeded user holds exactly `READ_CLIENT` (asserted, so the rest of the
spec cannot pass vacuously)
- reaches `/clients`
- refused `/accounting/chart-of-accounts` — **and** Fineract answers 403 to
`GET /glaccounts` and `GET /offices`
- refused `/clients/create` — **and** Fineract answers 403 to `POST /clients`
- the create button and the unauthorized nav entries are absent
- the superuser the rest of the backend suite signs in as is unaffected
The paired backend assertion is the point. Proving only that the client
refused something would
quietly invite the conclusion that the client is the boundary.
## Release readiness
| Item | Status |
|---|---|
| Build (production) | **PASS** |
| Unit tests | **PASS** — 977 + 2 |
| E2E — mocked RBAC | **PASS** — 15/15 |
| E2E — real-backend RBAC | **PASS** — 7/7 |
| E2E — full regression suite | see Testing table |
| RBAC — route protection | **FIXED** — 263/298 screens gated, 35 documented
|
| RBAC — navigation protection | **FIXED** — 21 → 100 gates, full parity,
checked in CI |
| RBAC — action authorization | **FIXED** — 58 create buttons, 29 loan
actions, 4 wrong codes corrected |
| Access Denied UX | **FIXED** |
| Accessibility (new surface) | **PASS** — heading, focus, `aria-live`,
keyboard |
| i18n | **PASS** — en/hi/ko, `i18n:check` green |
| Security — session robustness | **FIXED** — missing `permissions` no
longer breaks login |
| License / ASF checks | **PASS** |
| GA gate | **FIXED** — 0 blocking failures, and now blocking in CI |
| API surface manifest | **FIXED** |
| Adapter-boundary backlog | **NOT APPLICABLE** — advisory, out of scope,
count unchanged |
### Remaining blockers
None introduced by this PR. The pre-existing ones are unchanged and out of
scope: the advisory
adapter-boundary backlog (420 sites), and whatever else stands behind the
"not release-ready"
notice in `security.md`, which this PR deliberately leaves in place.
## Known limitations
- **Client-side RBAC is defence-in-depth.** It is not a security boundary
and must not be relied on
as one. A user with a patched bundle reaches whatever Fineract lets them
reach, and no more.
- **35 screens are unrestricted because Fineract defines no read permission
for them.** Their
write routes are gated where write codes exist, but the reads are open to
any authenticated
user and the backend is the only thing deciding. Each is listed with its
reason; several are
arguably upstream gaps in the permission catalogue.
- **`/products/:accountType/:accountId/action/:command`** is a dispatch
route whose requirement
depends on runtime parameters, so no static declaration would be right for
most of the commands
it serves. It is unrestricted and documented as such.
- **Action-level gating is not exhaustive across all 291 components.** The
privileged surfaces the
audit identified are covered; the routes behind everything else are gated,
so an ungated control
leads to Access Denied rather than to an unauthorized operation.
- The mocked matrix proves the client is self-consistent, not that Fineract
agrees. Only the
backend spec does that, and it covers one restricted role rather than a
matrix of them.
## Breaking changes
**Yes.** Previously, any authenticated user could reach certain screens
directly by URL. Protected
routes now require the corresponding permission, and a user without it is
sent to `/forbidden`.
Concretely:
- Users whose roles lack a permission lose URL access to the matching
screens. Deployments that
relied — knowingly or not — on screens being reachable regardless of role
will see access change
for real users. Review role grants before upgrading.
- Navigation entries that were visible to everyone are now filtered by
permission, so menus will
look smaller for non-superusers. This is the intended behaviour, not a
regression.
- `/transfers/account-transfer` moves from `READ_ACCOUNTTRANSFER` to
`CREATE_ACCOUNTTRANSFER`. The
screen is a form that posts a transfer; offering it to a user who can only
read led to a refusal.
- Deployments not ready for this can set `rbacEnabled: false` in
`config.json` to restore the
previous behaviour exactly, at all three layers.
No API contract changed, no existing test was deleted, and no E2E
infrastructure was broken. Three
navigation specs asserting the old, looser behaviour were **updated to the
new contract** rather
than removed, and their intent — per-item gating, no leakage between
siblings — is preserved and
strengthened.
--
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]
