FINERACT-2688: Fix loan disbursement for checker-only users via maker-checker permission check#6128
Conversation
30e1794 to
f830f34
Compare
Aman-Mittal
left a comment
Aman-Mittal
left a comment
There was a problem hiding this comment.
Need a integration test for this change before merge
Which explain whole process for this fix for verification
Enable maker-checker for loan disbursement
Maker submits → queued as AWAITING_APPROVAL
Maker submits again → HTTP 403 error.msg.maker.checker.duplicate.pending.submission
Checker-only user (has DISBURSE_LOAN_CHECKER) submits → auto-approves and disburses
Checker-only with no pending entry → HTTP 403 error.msg.maker.checker.checker.only.cannot.initiate
Existing /makerchecker/{id} approve/reject flow unaffected
| this.context.authenticatedUser(wrapper).validateHasPermissionTo(wrapper.getTaskPermissionName()); | ||
| final String taskPermission = wrapper.getTaskPermissionName(); | ||
| final boolean hasBasePermission = !currentUser.hasNotPermissionForAnyOf(taskPermission); | ||
| final boolean hasCheckerPermission = !currentUser.hasNotPermissionForAnyOf("CHECKER_SUPER_USER", taskPermission + "_CHECKER"); |
There was a problem hiding this comment.
This Logic can be simplified more
|
@aya-abdallah-FOO also please amend your commit to exclude AI co-author |
Aman-Mittal
added
bug
Needs Functional Review
b71128f to
62ec993
Compare
|
@aya-abdallah-FOO Also please sign your commits |
549814c to
b0bd55d
Compare
…-checker permission check When maker-checker is enabled, a user holding only the <ACTION>_CHECKER permission (e.g. DISBURSE_LOAN_CHECKER) was rejected with a permission error instead of being routed to the pending approval queue. - logCommandSource(): detect checker-only users, look up the pending AWAITING_APPROVAL command for the same action/entity/resource, and auto-approve it; block makers from duplicate pending submissions - validateMakerChecker(): broaden approval condition from isCheckerSuperUser() to also accept role-specific _CHECKER permissions - Add CommandSourceRepository.findPendingByActionAndEntityAndResource() native query (searches all resource-id columns) - Add MakerCheckerCheckerOnlyInitiationException and MakerCheckerDuplicatePendingSubmissionException Signed-off-by: aya.abdallah <aya.abdallah@foo.mobi>
…ix MakercheckerTest The previous change replaced user.isCheckerSuperUser() with a broader userHasCheckerPermission check that also matched task-specific _CHECKER permissions. This caused makers who hold both base and _CHECKER permissions to have their commands auto-approved instead of routed to AWAITING_APPROVAL, breaking MakercheckerTest across all three databases. The checker-only flow introduced by FINERACT-2688 is handled upstream in logCommandSource, so validateMakerChecker never sees checker-only users and needs no change. Signed-off-by: aya.abdallah <aya.abdallah@foo.mobi>
b0bd55d to
89d50b9
Compare
@Aman-Mittal can you please check if all is good? |
yes now properly signed |
…t maker-checker flow
Covers all five scenarios:
- Maker submits disbursement → queued as AWAITING_APPROVAL
- Maker submits again → HTTP 403 duplicate pending submission
- Checker-only user with no pending entry → HTTP 403 cannot initiate
- Checker-only user auto-approves pending entry and disburses
- Traditional /makerchecker/{id}?command=approve flow unaffected
Signed-off-by: aya.abdallah <aya.abdallah@foo.mobi>
89d50b9 to
106bac4
Compare
Two fixes for the flaky CI failures in MySQL/MariaDB shards: 1. Move global config updates (MAKER_CHECKER, ENABLE_SAME_MAKER_CHECKER) inside the try block so the finally cleanup is guaranteed even if the second update throws — preventing MAKER_CHECKER=true from leaking to subsequent tests in the same shard and causing unexpected maker-checker queuing. 2. Narrow findPendingByActionAndEntityAndResource to match only resource_id, loan_id, and savings_account_id. The previous query also matched client_id, office_id, group_id, product_id, and other contextual columns. When a loan ID coincidentally equalled a contextual ID stored on another pending entry, the wrong command was returned — causing Scenario 3 (checker-only no-entry → 403) to auto-approve the wrong loan instead of throwing, breaking the assertion.
|
@Aman-Mittal can you please run the CI and if you know why would any of them still fail? |
|
CI Results — Shard 6 passed; Shard 4 and Kafka failures explained below Shard 6 (our test shard) - Passed on MySQL, MariaDB, PostgreSQL LoanDisbursementMakerCheckerIntegrationTest lands in Shard 6 of 15 (confirmed via the round-robin sort order). All three DB variants passed on the latest commit (b1fc327). Shard 4 failures (MySQL, MariaDB, PostgreSQL) — test-suite reshuffling, not a logic bug Adding LoanDisbursementMakerCheckerIntegrationTest inserts a new class alphabetically between LoanDisbursalDateValidationTest and LoanDisbursementDetailsIntegrationTest, shifting the round-robin shard assignment of every test class that sorts after it. As a result, tests that were previously in Shard 3 on develop (e.g. LoanMultipleDisbursementRepaymentScheduleTest, UndoRepaymentWithDownPaymentIntegrationTest) now run in Shard 4 after the original Shard-4 tests, creating a new execution ordering. Our production code changes are guarded behind the global MAKER_CHECKER configuration (defaults to false/disabled), so the new logCommandSource paths and CommandSourceRepository query are never exercised in any of the Shard 4 tests. The failures are not in our code's execution path. One specific pre-existing fragility we found: UndoRepaymentWithDownPaymentIntegrationTest enables ENABLE_BUSINESS_DATE at the top of its @test method without a try/finally — if the test fails mid-way, the config leaks into subsequent tests. This issue exists on develop too but didn't surface before because the test ran in Shard 3 with different predecessors. Could a maintainer check the artifact test-results-*-shard-4-attempt-2 to confirm which specific test is the first to fail? That would let us pinpoint the root cause and file a targeted fix for those tests. Kafka Smoke Test — transient Docker infrastructure failure The failure is in step 8 "Build the image" (jibDockerBuild), before any Kafka tests run. This is a Docker daemon / image-pull issue in the CI runner, unrelated to our Java changes (Shard 6 compilation and startup succeeded). Could a maintainer re-run just the Kafka smoke test job? @Aman-Mittal |
|
@aya-abdallah-FOO Please review the failing tests. |
@adamsaghy FYI |
…tWithDownPaymentIntegrationTest
3 participants
Summary
Fixes a bug where a user holding only a role-specific
_CHECKERpermission (e.g.DISBURSE_LOAN_CHECKER) was rejected with a permission error instead of being routed to thepending maker-checker approval queue.
Root causes:
PortfolioCommandSourceWritePlatformServiceImpl.logCommandSource()calledvalidateHasPermissionTo(taskPermission)unconditionally, rejecting checker-only usersCommandSourceService.validateMakerChecker()only marked a command as checked forCHECKER_SUPER_USER, ignoring role-specific_CHECKERpermissionsChanges:
logCommandSource(): detect checker-only users (has_CHECKERbut not base permission), auto-approve the pending AWAITING_APPROVAL entry; block makers from duplicate pendingsubmissions
validateMakerChecker(): replaceisCheckerSuperUser()withhasNotPermissionForAnyOf("CHECKER_SUPER_USER", permission + "_CHECKER")CommandSourceRepository.findPendingByActionAndEntityAndResource()native SQL queryMakerCheckerCheckerOnlyInitiationExceptionandMakerCheckerDuplicatePendingSubmissionException(both → HTTP 403)JIRA
https://issues.apache.org/jira/browse/FINERACT-2688
Test plan
error.msg.maker.checker.duplicate.pending.submissionDISBURSE_LOAN_CHECKER) submits → auto-approves and disburseserror.msg.maker.checker.checker.only.cannot.initiate/makerchecker/{id}approve/reject flow unaffected