priyam99 opened a new pull request, #172:
URL: https://github.com/apache/fineract-loan-origination/pull/172
## Description
When a customer's JWT is used to call a staff-only endpoint, the request is
correctly blocked, but the API answers with **500 Internal Server Error**.
Nothing is broken, so that status is wrong; the caller is simply not allowed to
do this. The right answer is **403 Forbidden**.
The cause is in `GlobalExceptionHandler`. Spring Security raises
`AuthorizationDeniedException` (a subclass of `AccessDeniedException`) when a
`@PreAuthorize` check fails, and the handler had no entry for it, so it fell
through to the catch-all `Exception` handler that returns 500.
What I changed:
- Added an `AccessDeniedException` handler to `GlobalExceptionHandler` that
returns 403 in the project's standard error format. Catching the parent class
also covers `AuthorizationDeniedException`.
- The handler returns a fixed message
(`LosErrorConstants.MSG_ACCESS_DENIED`) rather than the exception text, so no
authorization detail leaks to the caller.
- Added tests. `MethodSecurityAuthorizationTest` sends real requests through
the Spring Security filter chain, and `GlobalExceptionHandlerTest` covers the
handler directly.
The denial still happens before the protected method runs, so no business
logic executes and no Fineract call is made. I did not touch the existing
`AuthorizationException` handler, so current behaviour for that path is
unchanged.
I checked that the tests actually catch the bug: with the new handler
removed, 6 of the 7 tests fail with the unhandled
`AuthorizationDeniedException` escaping as a 500. With it in place they all
pass.
Closes #159
## Verification
- [x] `./mvnw spotless:apply`
- [x] `./mvnw clean verify` — 196 unit tests and 7 Cucumber tests pass,
`BUILD SUCCESS`
- [x] `./mvnw apache-rat:check` — runs as part of `clean verify`
- [ ] Frontend changes: not applicable, this is a backend-only change
Tests added in this PR:
| Test | What it checks |
|---|---|
| `customerCannotStartReview` | customer JWT on `POST /{ref}/start-review`
gets 403 |
| `customerCannotDisburse` | customer JWT on `POST /{ref}/disburse` gets 403
|
| `staffWithoutAdminRoleCannotListStaff` | staff without the admin role gets
403 |
| `authorizationDeniedReturnsForbidden` | `AuthorizationDeniedException`
maps to 403 |
| `accessDeniedDoesNotExposeMessage` | the response carries the fixed
message, not the exception text |
## AI assistance (optional)
- Tool / model: Claude (Anthropic)
- How it was used: Helped me check the branch was based on `main` rather
than on my open PR #170, ran the build and test suite, and reviewed the wording
of this description. I wrote and verified the change, and I am responsible for
every line.
## Checklist
- [x] This PR is one logical change, not a code dump.
- [x] I added or updated tests, or explained why tests are not needed.
- [x] New files carry the ASF license header.
- [x] Docs (and `AGENTS.md`, if commands, paths, or architecture rules
changed) are updated — no doc changes needed. `docs/api/rest-api.adoc` already
lists 403 as a standard error status, and this change makes method-security
denials use it too.
- [x] All commits are signed.
- [x] I followed the [AI
Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy)
and the [ASF Generative Tooling
Guidance](https://www.apache.org/legal/generative-tooling.html).
---
Note on overlap with #170: that PR changes
`CustomerLoanApplicationController` to throw `ApplicationNotFoundException`
(404) for cross-customer access. This PR only adds a handler in
`GlobalExceptionHandler` and a constant, so the two touch no common files. This
branch is based on `main`, not on #170.
--
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]
