Aman-Mittal commented on PR #39: URL: https://github.com/apache/fineract-consumer-facing/pull/39#issuecomment-4976883448
Critical Maintainability Issues Overly Large Monolithic PR Single commit with 122 changed files (5,754 additions, 485 deletions) mixing multiple independent concerns: beneficiaries feature, ABAC refactoring, authentication fixes, token rotation, device fingerprinting, and test infrastructure—difficult to review and cherry-pick if needed Consider breaking into separate PRs: (1) Beneficiaries feature, (2) ABAC centralization, (3) Authentication/token fixes, (4) Device fingerprint fixes Orphaned Exception Classes from Refactoring Old exception names persist in new code: LoanAccessDeniedException, LoanNotFoundException (renamed to command/query variants), SavingsAccountAccessDeniedException (renamed), creating confusion about which to use and risking new code using stale exception names Add a deprecation pass or update all remaining references to use the new canonical names Insufficient Inline Documentation on Complex Logic BeneficiariesCommandServiceImpl.resolveAccount() method (lines ~175-215) performs 4 different account type lookups with complex status checking but lacks comments explaining non-obvious logic like NON_CLOSED_STATUS_IDS set or why different APIs are called for loans vs. savings AuthenticationCommandServiceImpl refresh token rotation logic (lines ~140-170) is security-critical but the grace period handling is not documented inline—readers must infer intent from test cases Inconsistent Package Structure JWT classes moved to infrastructure.jwt.data and infrastructure.jwt.service but no docs explain this split—separating data models from service implementations is reasonable but undocumented conventions will confuse future contributors Similar pattern with access policy classes (infrastructure.access.data, infrastructure.access.service, infrastructure.access.filter) lacks a README or architectural guide Magic Constant Duplication Across Services NON_CLOSED_STATUS_IDS (lines ~66 in BeneficiariesCommandServiceImpl) defined locally without reference to equivalent logic elsewhere; if Fineract's status codes change, multiple places must be updated StepUpConstants.STEPUP_TTL and OtpConstants.OTP_TTL_SECONDS define TTLs but no centralized timeouts configuration document Insufficient Error Context in Exceptions Generic BeneficiaryAccountInvalidException thrown for multiple failure modes (account not found, closed, wrong office, wrong status), making it hard for API consumers to provide specific error messages or retry logic Consider exception hierarchy: BeneficiaryAccountNotFoundException, BeneficiaryAccountClosedException, BeneficiaryAccountOwnershipMismatchException Test Coverage for ABAC Centralization Incomplete New AccessPolicyEvaluator class has good unit tests but the device fingerprint filter (DeviceFingerprintFilterTest) only covers success/rejection—missing edge cases like: Requests with malformed fingerprint header values Fingerprints that are valid but differ only in whitespace Double-checking that filter runs after JWT validation (order matters) Type Conversions in Loan/Savings APIs Removed toBigDecimal() conversions (e.g., line ~300 in LoansQueryServiceImpl) assuming Fineract client now returns BigDecimal instead of Double, but no comment explaining this breaking change assumption—fragile if upstream reverts SQL Enum Type Creation Without Rollback Guard 003-create-beneficiaries.yaml uses CREATE TYPE beneficiary_account_type AS ENUM but doesn't check if it already exists (can fail on re-run); rollback also doesn't guard against cascade deletes if beneficiaries table has foreign keys Use Liquibase's createType ifNotExists attribute if available, or guard with schema inspection Centralized ABAC Caller Responsibility Shift Old code had requireAccess() methods called at operation start; new code calls accessPolicyEvaluator.authorize() in each controller/service method—if a developer adds a new endpoint, forgetting the authorization call silently bypasses ABAC Consider enforcing via @PreAuthorize Spring annotations or an interceptor instead of manual calls Inconsistent Cookie Security Configuration authentication.properties changed refreshCookieSecure → cookieSecure (one boolean for both tokens), but comments note "set true wherever TLS terminates in front"—no validation that this is true in production, and fallback to false for dev is risky if deployed with false accidentally Add runtime warning log if cookie-secure=false in production profiles <!-- /copilot:review-comments --> Additional Observations JWT Denylist expiry logic is non-obvious: Uses Caffeine's custom Expiry interface with clock-skew padding (60s). While tested, a code comment explaining why 60s is needed would help maintainers. Beneficiaries transfer limit enforcement is fragile: Limits are checked only if the target account is a registered beneficiary; users can still transfer to their own owned accounts without limits, creating potential for confusion if this policy ever needs to expand. Recommendation: Split this PR into logical units and add architecture decision records (ADRs) documenting the ABAC centralization, token rotation grace period, and JWT denylist approach. -- 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]
