Aman-Mittal opened a new pull request, #212:
URL: https://github.com/apache/fineract-consumer-facing/pull/212
## What and why
Two logic flaws in the BFF's own authentication design, found by reviewing
the auth, OTP, ABAC,
open-banking and audit paths end to end. Both are in scope under
SECURITY.md: neither is a
"prototype lacks production hardening" finding.
**One-time codes were not bound to a purpose.** The pending-code slot was
keyed on the user alone
(`otp:pending:{publicId}`), so the six ceremonies that mint codes —
registration, login 2FA,
password change, password reset, transfer step-up, beneficiary step-up —
shared one slot, and
whichever code was minted last satisfied whichever challenge was presented.
The weakest challenge
in the system therefore issued a credential for the strongest one, and `POST
/api/v1/user/password/reset`
is public and takes nothing but an email address, a code and a new password.
`OtpPurpose` now joins
both the Valkey key and the hashed material, so neither a slot read nor a
lifted hash crosses
ceremonies, and the delivery email states what the code authorises instead
of being purpose-free.
**The registration submit endpoint answered two different ways.** It
verified the applicant's
identity document before checking whether the Fineract client was already
registered, so a correct
government ID returned 409 and a wrong one 403 — on a public endpoint,
against a sequential client
id, with nothing counting the attempts. The already-registered check now
runs first, so both cases
answer identically, and `IdentityVerificationAttemptCounter` caps failures
at five per client per
fifteen minutes and reports a lockout as the same generic mismatch, so the
refusal itself tells an
attacker nothing. Per-IP throttling stays delegated to the deployment
environment, as SECURITY.md
and the docs already say.
Two smaller things in the same area:
- `POST /api/v1/registration/otp/send` performed no status check, so any
public id presented to it
got a fresh code emailed — including one belonging to a fully onboarded
account. Both OTP steps
now require `PENDING_OTP`.
- `PendingOtp` carried the plaintext code and the destination address under
a bare `@ToString`,
against the project's own rule for secret-bearing data classes. Nothing
logs it today; one
careless `log.debug` would have.
The review covered more than it changed. The ABAC enforcement across all
thirty feature request
mappings, the open-banking authorization server (redirect-URI matching,
PKCE, consent-to-TPP-and-user
binding, revocation, scope handling, token separation), the refresh-token
lifecycle, the JWT denylist,
the audit submission path and the error envelope all held up; no change was
needed there, and the
docs now record what was verified.
## Verification
- `./gradlew test` — 561 unit tests, 0 failures. Includes the 36 ArchUnit
and Spring Modulith checks,
which cover the module boundaries and naming of the new classes.
- `./gradlew cucumber` against the Docker stack (real Fineract, PostgreSQL,
Valkey, Mailpit) —
83 scenarios / 367 steps, all passing.
- New Cucumber coverage: purpose binding in both directions (a login
two-factor code refused at the
password reset, a password reset code refused as a login second factor —
both would have been
accepted before), an already-registered client refused identically for a
matching and a
non-matching identifier, the identity lockout, and the OTP-send status
guard.
- New unit coverage: `RegistrationCommandServiceImplTest` (the order of the
three submit checks, the
lockout short-circuit, the `PENDING_OTP` guard on both OTP steps),
`IdentityVerificationAttemptCounterTest` (cap, window, fail-open when
Valkey is unreachable), and
additions to `OtpServiceImplTest` / `OtpRepositoryTest` (a hash minted
under one purpose does not
validate under another, the same code hashes differently per purpose, one
slot per purpose, key
segments unique and `:`-free).
- `node scripts/check-i18n.mjs` — passes; the new error code is translated
in `en` and `hi`.
- One existing scenario changed meaning and was retargeted rather than
worked around: with the
`PENDING_OTP` guard, replaying a verified registration code is refused by
the state guard (409)
before the spent code is looked at (400). It now asserts that refusal, and
says where the
single-use property it used to stand for is pinned instead
(`OtpServiceImplTest`, `login.feature`).
## Screenshots
Not applicable — no UI change beyond one new translated error code.
## Checklist
- [x] I did not hand-edit generated files under
`frontend/src/openapi-client/`.
- [x] Backend code follows the feature and CQRS layout in AGENTS.md; the new
counter is an
`infrastructure` component beside `RateLimitCounter` and
`ClientStandingChecker`, the new
exception lives on the registration command side, and the architecture
tests pass unchanged.
- [x] I did not add entries to a lint or architecture baseline.
- [x] User-facing strings use translation keys
(`error.msg.consumer.registration.not.pending`, en + hi).
- [x] I added or updated tests appropriate to this change.
- [x] UI workflow changes include suitable e2e coverage, including
real-backend testing where
relevant — the behaviour changes are all server-side and covered by
Cucumber against real Fineract.
- [x] Commits are signed.
- [x] I followed the AI-assisted contributions guidance.
- [x] This is a bug fix, so the dev-list-first rule for features does not
apply.
--
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]
