You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "elnafateh (via GitHub)" <gi...@apache.org> on 2026/09/08 15:43:49 UTC
[PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
elnafateh opened a new pull request, #6420:
URL: https://github.com/apache/fineract/pull/6420
## Description
Adds centralized office-hierarchy authorization checks for account transfers across savings and loan accounts, including transfer creation, refunds, top-ups, and undo operations.
Adds unit and integration coverage for unauthorized cross-office transfers.
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [x] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [x] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [x] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
- [x] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [x] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [x] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on code in PR #6420:
URL: https://github.com/apache/fineract/pull/6420#discussion_r3988510386
##########
README.md:
##########
@@ -41,6 +41,19 @@ If you believe you have found a new vulnerability, [let us know privately](https
For details about security during development and deployment, see the documentation [here](https://fineract.apache.org/docs/stable/#_security).
+OFFICE HIERARCHY ACCESS
+=======================
+
Review Comment:
ok
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy for loan and savings write operations [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy merged PR #6420:
URL: https://github.com/apache/fineract/pull/6420
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5606960168
> @adamsaghy kindly review this when u get a chance
I am traveling this week. The earliest i can review is next week. :(
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5666478672
@IOhacker Please review based on functional requirements
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-6495051808
@adamsaghy kindly review
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5620196850
@Aman-Mittal These concerns are addressed.
Unit tests and Integration-tests have been added to cover all the hierarchy expansion scenarios, along with a supporting docs.
Kindly rerun tests and Review when u get a chance!
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy for loan and savings write operations [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-6632783044
@elnafateh I have rebased and squashed you commits... its easier to review this way ;)
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5629774111
@elnafateh Check the failing ci check
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5617595992
@elnafateh
Hi Reviewed this with the help of Copilot
While with prompt "Help me review this pull request" recommends for merge
but i dig a little deeper, As i am not familier with office hierarchy
So it is like this from what i find
Office Hierarchy is a string-based path representation of an organization's office structure in Fineract. It's used to enforce authorization constraints - ensuring users can only access resources belonging to their office or subordinate offices.
something like
Hierarchy String Format: ".parentOfficeId.childOfficeId."
Example:
- Root office (Head Office): "."
- Branch A (child of root): ".1."
- Sub-branch A1 (child of A): ".1.2."
- Sub-branch A2 (child of A): ".1.3."
Code wise there is no red flags but there are some coverage it needs to expand on
- Parent office can access child office resources (hierarchy prefix working correctly)
- Child office cannot access parent office resources (negative case)
- Same office transfer (positive case)
- Multi-level hierarchies (.1.2.3. scenarios)
Also i think we can also add documentation.
If that can get covered then i can review more.
@adamsaghy @IOhacker fyi
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5600954148
@adamsaghy kindly review this when u get a chance
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5609139339
> > @adamsaghy kindly review this when u get a chance
>
> I am traveling this week. The earliest i can review is next week. :(
Safe travels
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-5666302677
@Aman-Mittal kindly review the added e2e test and .adoc adjustment
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub) via issues" <is...@fineract.apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-6631702508
Thanks @adamsaghy . I’ve addressed the remaining points, Kindly review whenever u have the time.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-6502938750
@elnafateh Please rebase
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on code in PR #6420:
URL: https://github.com/apache/fineract/pull/6420#discussion_r3988718494
##########
README.md:
##########
@@ -41,6 +41,19 @@ If you believe you have found a new vulnerability, [let us know privately](https
For details about security during development and deployment, see the documentation [here](https://fineract.apache.org/docs/stable/#_security).
+OFFICE HIERARCHY ACCESS
+=======================
+
Review Comment:
@elnafateh Can you also add e2e tests for these too?
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub) via issues" <is...@fineract.apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-6535487027
> Hi @elnafateh, thanks for the PR and sorry it took this long to get to it.
>
> Unfortunately the timing is bad here. [FINERACT-2639](https://issues.apache.org/jira/browse/FINERACT-2639) was already implemented and merged yesterday via #6698: `LoanRepositoryWrapper.findOneInUserScope` and `SavingsAccountRepositoryWrapper.findOneInUserScope` now validate the office hierarchy, `create()` uses the `InUserScope` assemblers for every transfer type, `undo()` checks both offices, and there's `AccountTransferOfficeHierarchyAccessIntegrationTest` covering the sibling-office rejection. So the `create()` and `refundByTransfer()` parts of this PR just run the same check a second time. There's also #6260 open (draft) on the same ticket and the same files. Let's not have three PRs for one ticket.
>
> The bigger problem is that the rebase went wrong somewhere. The PR deletes 3 step definitions from `LoanRepaymentStepDef` that `LoanTransactionAdjustment.feature` and `LoanRepayment-Part4.feature` still use on develop, and re-adds 3 old steps nothing uses. It also rewrites `AccountTransfersWritePlatformServiceImplTest` and drops the existing undo tests that came with #6698. None of that is related to the office hierarchy, and as it stands the E2E suite would fail with undefined steps.
>
> What's actually new compared to develop is the check inside `transferFunds()` / `repayLoanWithTopup()` and the one hidden in `checkClientOrGroupActive`. I'm not convinced about the first one, see inline. Could you rebase on the latest develop, drop everything #6698 already covers, restore the unrelated files, and only keep what's actually missing? Thoughts?
>
> ### fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:147
> ```java
> final SavingsAccount toSavingsAccount = this.savingsAccountAssembler.assembleFromInUserScope(toSavingsId,
> backdatedTxnsAllowedTill);
> validateTransferAccess(null, null, fromSavingsAccount, toSavingsAccount);
> ```
>
> > Both accounts were just loaded via `assembleFromInUserScope`, which since #6698 already calls `context.validateAccessRights(account.office().getHierarchy())`. This checks the same thing twice. Same for line 174, 205 and for `refundByTransfer` (line 653).
>
> ### fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:326
> ```java
> toLoanAccount = accountTransferDetails.toLoanAccount();
> }
> validateTransferAccess(toLoanAccount, null, fromSavingsAccount, null);
> ```
>
> > `transferFunds` is the internal path, not a user-initiated transfer. It's called from standing instructions, the deposit maturity / transfer-interest flows, loan charge payment from a linked savings account and `GuarantorDomainServiceImpl` ("Payment from guarantor savings"). The guarantor's savings account can easily be in a different branch than the borrower's loan, so with this check a branch user acting on their own loan would get a 403 because of the guarantor's office. Is that intended? I think this would break legit cross-office flows. If we really want it here, I'd like to see tests for those flows, not only the REST transfer.
>
> ### fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:600
> ```java
> private void validateTransferAccess(final Loan fromLoanAccount, final Loan toLoanAccount, final SavingsAccount fromSavingsAccount,
> final SavingsAccount toSavingsAccount) {
> ```
>
> > There's already a `validateAccessRights(AccountTransferDetails)` a few lines above that does the same thing. Four nullable params where every call site passes two `null`s isn't easy to read either. If anything survives the rebase, let's reuse what's there.
>
> ### fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java:2190
> ```java
> private void checkClientOrGroupActive(final Loan loan) {
> validateLoanAccess(loan);
> ```
>
> > Hiding an authorization check in a method called `checkClientOrGroupActive` is surprising. Nobody reading a call site will guess that it also does an office check. Most callers already load the loan via `assembleFromInUserScope`, so it's mostly a duplicate anyway. If there are call sites that load the loan outside the user scope and should be checked, let's switch those to the `InUserScope` loader the way #6698 did. Same for `SavingsAccountWritePlatformServiceJpaRepositoryImpl:1012`.
>
> ### fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/stepdef/loan/LoanRepaymentStepDef.java:721
> ```java
> - @When("Customer adjust {int}th {string} transaction made on {string} with {string} payment type on {string} with {double} EUR transaction amount")
> - public void adjustTransactionWithAutoIdempotencyKey(...)
> - @Then("Customer is forbidden to adjust {string}th {string} transaction made on {string} with {double} EUR transaction amount due to a validation error:")
> - @Then("Adjusted transaction is created with {double} amount and {string} payment type")
> ```
>
> > Why removing these? `LoanTransactionAdjustment.feature` and `LoanRepayment-Part4.feature` use them on develop. Same question for `makeRepaymentByExternalId`, `makeLoanRepaymentFails` and `makeLoanRepaymentUndoAfterChargeOff` at line 175+, which nothing calls. Looks like an old version of the file came back during conflict resolution. Pls restore it to develop and only add the 1 new step.
>
> ### fineract-provider/src/test/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImplTest.java:73
> ```java
> - void givenLoanToSavingsRefund_whenUndo_thenSavingsUndoneLoanAdjustedAndTransferReversed() {
> - void givenLoanToSavingsDisbursement_whenUndo_thenRejectedBeforeAnythingIsReversed() {
> - void givenAlreadyReversedTransfer_whenUndo_thenThrowsGeneralPlatformDomainRuleException() {
> +@MockitoSettings(strictness = Strictness.LENIENT)
> ```
>
> > Same here, the existing undo tests are gone. Pls add the new tests next to them instead of replacing the file. Also, why `LENIENT`? It's usually a sign that the stubbing is broader than what the test needs.
>
> Thanks.
Thanks for the review. I’ve updated the PR accordingly, Pls review the update whenever you can.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-6523365933
@adamsaghy rerun checks pls
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on code in PR #6420:
URL: https://github.com/apache/fineract/pull/6420#discussion_r3986097147
##########
README.md:
##########
@@ -41,6 +41,19 @@ If you believe you have found a new vulnerability, [let us know privately](https
For details about security during development and deployment, see the documentation [here](https://fineract.apache.org/docs/stable/#_security).
+OFFICE HIERARCHY ACCESS
+=======================
+
Review Comment:
It should be in .adoc file in fineract doc
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub)" <gi...@apache.org>.
elnafateh commented on code in PR #6420:
URL: https://github.com/apache/fineract/pull/6420#discussion_r3988866597
##########
README.md:
##########
@@ -41,6 +41,19 @@ If you believe you have found a new vulnerability, [let us know privately](https
For details about security during development and deployment, see the documentation [here](https://fineract.apache.org/docs/stable/#_security).
+OFFICE HIERARCHY ACCESS
+=======================
+
Review Comment:
sure thing
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2639: Enforce office hierarchy checks for account transfers [fineract]
Posted by "elnafateh (via GitHub) via issues" <is...@fineract.apache.org>.
elnafateh commented on PR #6420:
URL: https://github.com/apache/fineract/pull/6420#issuecomment-6574953072
@galovics Pls review the update when u get the chance
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org