You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks for picking this up — the diagnosis is right, and letting a domain-rule 403 through to the stacked-message branch is the correct shape for the fix. One thing to change before it lands, and it is only visible against a real platform.
The stacked branch prefers developerMessage
The branch this now routes 403s into already reads:
developerMessage first. Reproduced against a running Fineract just now:
DELETE /codes/15/codevalues/24 → 403
errors[0].developerMessage : "The request caused a data integrity issue to be fired by the database."
errors[0].defaultUserMessage : "This code value is in use"
So after this change that user stops seeing "You do not have permission to perform this action" and starts seeing "The request caused a data integrity issue to be fired by the database." — which is not an improvement, and is the same category of problem the comment left in place is warning about. The identical preference sits in the fallback a few lines below.
Flipping it to defaultUserMessage || developerMessage in both places gives "This code value is in use", which is the message the platform wrote for the user.
The new test does not catch this because its fixture has no developerMessage on the error entry. Real Fineract bodies always carry one — the group-closure example in the test looks like this in practice:
{
"errors": [
{
"developerMessage": "A loan with money outstanding cannot be closed",
"defaultUserMessage": "A loan with money outstanding cannot be closed",
"userMessageGlobalisationCode": "error.msg.loan.close.loan.has.money.outstanding",
"parameterName": "id"
}
]
}
A fixture where the two strings differ would pin the behaviour that matters.
Two smaller notes
isDomainRuleViolation collapses to hasValidationErrors. Since it is code === '…' || hasValidationErrors, and the FORBIDDEN branch is skipped whenever either holds, the globalisation-code half never changes an outcome. Worth dropping it, or keeping it and dropping the || — as written it reads like it does more than it does.
There is also a gap between the two: when the code matches but errors[] is empty, this skips FORBIDDEN, skips the stacked branch, and lands on the top-level developerMessage — "Request was understood but caused a domain rule violation."
A question rather than a claim: does the platform's genuine permission-denied 403 also carry an errors[] array? If it does, this change routes it into the stacked branch too, and the user would be shown the NOT_ALLOWED permission code that the original guard existed to hide. I could not construct that case here without a restricted user, so I have not asserted it either way — but it seems worth one test before merge, since it is the only path that could make things worse rather than better.
Happy to be wrong on any of this; the underlying change is one the app needs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Show Fineract's domain-rule explanation for 403 responses while keeping the generic permission message for authorization failures.
Closes #258
Verification
Screenshots
Not applicable.
Checklist
src/app/api/.