Aman-Mittal commented on code in PR #155: URL: https://github.com/apache/fineract-consumer-facing/pull/155#discussion_r4359198337
########## consumer/src/main/java/org/apache/fineract/consumer/savings/command/data/SubmitSavingsApplicationCommandRequest.java: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.savings.command.data; + +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; +import java.time.LocalDate; +import lombok.Builder; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.ToString; + +@Getter +@RequiredArgsConstructor +@Builder +@ToString(onlyExplicitlyIncluded = true) +public final class SubmitSavingsApplicationCommandRequest { + + @NotNull + @Positive + private final Long productId; + + @NotNull + private final LocalDate submittedOnDate; Review Comment: Still open from the last round: `submittedOnDate` has `@NotNull` but no bounds, here and on modify. Fineract only rejects future dates and dates before the client's activation, so a consumer can backdate an application. The new Cucumber modify step does exactly that (`MODIFIED_LOCAL_DATE = FIXED_LOCAL_DATE.minusDays(1)`) and it succeeds. This is one of the dev-list questions. My suggestion is to derive the date on the server (today, in the tenant's timezone) and drop it from the request. Either way, the outcome should be decided once for savings and loans. ########## consumer/src/test/resources/features/savings.feature: ########## @@ -44,3 +44,43 @@ Feature: Consumer savings reads When I get the other client's savings account Then the savings request is denied as forbidden + Scenario: Submitting the same savings application twice with one idempotency key creates one account + When I submit a savings application twice with the same idempotency key + Then both savings submissions return the same savings id + And I have exactly one more savings account + + Scenario: Submitting a savings application without an idempotency key is rejected + When I submit a savings application without an idempotency key + Then the savings submission is rejected for the missing idempotency key + + Scenario: Modifying a pending savings application + Given I have submitted a savings application + When I modify my pending savings application + Then the savings application is updated successfully + And the savings application reflects the modified values + + Scenario: Withdrawing a pending savings application + Given I have submitted a savings application + When I withdraw my pending savings application + Then the savings application is withdrawn successfully + And the savings account status is withdrawn by applicant + + Scenario: Modifying another client's savings account is forbidden Review Comment: Good to see the IDOR scenarios for modify and withdraw, asserting both 403 and the command error code. Please add: 1. **The foreign account is unchanged after the denied call.** One `Then` that reads the other client's account as that client (or via the seeder) and checks its status didn't move. A 403 alone doesn't prove nothing happened upstream. 2. **Open Banking scope is denied on submit.** The policy is `CONSUMER_ONLY`, so a TPP token calling `POST /api/v1/savings` should be rejected with the scope error. `abac-denial.feature` / `openbanking.feature` have the token setup. 3. **Idempotent replay of modify and withdraw.** The same key twice should return the same result, with no second transition and no 400 on the replayed withdraw. Not needed in Cucumber: the FD/RD deposit-type rejection is fine as unit-only, since the seeder doesn't create deposit accounts. ########## consumer/src/test/java/org/apache/fineract/consumer/cucumber/steps/SavingsSteps.java: ########## @@ -138,12 +167,160 @@ public void savingsDeniedForbidden() { assertThat(errorStatus).isEqualTo(FORBIDDEN); } + @When("I submit a savings application twice with the same idempotency key") + public void submitSavingsApplicationTwiceWithSameKey() { + savingsCountBeforeSubmit = savingsApi.listSavingsAccounts().size(); + String idempotencyKey = UUID.randomUUID().toString(); + firstSubmission = savingsCommandApi.submitSavingsApplication(idempotencyKey, savingsApplicationRequest()); + secondSubmission = savingsCommandApi.submitSavingsApplication(idempotencyKey, savingsApplicationRequest()); + } + + @Then("both savings submissions return the same savings id") + public void bothSavingsSubmissionsReturnSameSavingsId() { + assertThat(firstSubmission.getSavingsId()).isNotNull(); + assertThat(secondSubmission.getSavingsId()).isEqualTo(firstSubmission.getSavingsId()); + } + + @Then("I have exactly one more savings account") + public void exactlyOneMoreSavingsAccount() { + assertThat(savingsApi.listSavingsAccounts()).hasSize(savingsCountBeforeSubmit + 1); + } + + @When("I submit a savings application without an idempotency key") + public void submitSavingsApplicationWithoutKey() { + lastError = captureError(() -> savingsCommandApi.submitSavingsApplication(null, savingsApplicationRequest())); + } + + @Then("the savings submission is rejected for the missing idempotency key") + public void savingsSubmissionRejectedForMissingKey() { + assertThat(lastError.status()).isEqualTo(BAD_REQUEST); + assertThat(readCode(lastError.contentUTF8())).isEqualTo(MissingRequestHeaderExceptionHandler.CODE); + } + + @Given("I have submitted a savings application") + public void submittedSavingsApplication() { + String idempotencyKey = UUID.randomUUID().toString(); + SavingsApplicationCommandData submission = savingsCommandApi.submitSavingsApplication( + idempotencyKey, savingsApplicationRequest()); + pendingSavingsId = submission.getSavingsId(); + assertThat(pendingSavingsId).isNotNull(); + } + + @When("I modify my pending savings application") + public void modifyPendingSavingsApplication() { + String idempotencyKey = UUID.randomUUID().toString(); + commandResult = savingsCommandApi.modifySavingsApplication( + idempotencyKey, pendingSavingsId, modifySavingsApplicationRequest()); + } + + @Then("the savings application is updated successfully") + public void savingsApplicationUpdatedSuccessfully() { + assertThat(commandResult).isNotNull(); + assertThat(commandResult.getSavingsId()).isEqualTo(pendingSavingsId); + } + + @Then("the savings application reflects the modified values") + public void savingsApplicationReflectsModifiedValues() { Review Comment: **Cucumber: this step still can't fail.** It re-reads the account but only asserts `id == pendingSavingsId`, which is true whether or not the modify changed anything. `SavingsAccountQueryData` doesn't expose `submittedOnDate`, so the only observable change is the product. Seed a second savings product in `FineractSeeder` and modify the application to it. Then assert `account.getProductName()` changed, and ideally that `active` is still false and the status is still pending. That proves modify works end to end, and it also shows a product change is allowed, which is relevant to the allow-list question. ########## consumer/src/main/java/org/apache/fineract/consumer/savings/command/service/SavingsCommandServiceImpl.java: ########## @@ -0,0 +1,168 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.savings.command.service; + +import java.util.UUID; +import java.util.function.Supplier; +import lombok.RequiredArgsConstructor; +import org.apache.fineract.consumer.infrastructure.access.data.ConsumerAction; +import org.apache.fineract.consumer.infrastructure.access.service.AccessPolicyEvaluator; +import org.apache.fineract.consumer.infrastructure.access.service.OwnedAccountsCache; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.EnumOptionData; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsAccountIdRequest; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsAccountIdResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsRequest; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PutSavingsAccountsAccountIdRequest; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PutSavingsAccountsAccountIdResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.SavingsAccountData; +import org.apache.fineract.consumer.infrastructure.fineractclient.service.FineractCaller; +import org.apache.fineract.consumer.infrastructure.fineractclient.service.SavingsAccountApiClient; +import org.apache.fineract.consumer.infrastructure.idempotency.service.IdempotencyKeyDeriver; +import org.apache.fineract.consumer.infrastructure.idempotency.service.IdempotencyKeyHolder; +import org.apache.fineract.consumer.savings.command.data.ModifySavingsApplicationCommand; +import org.apache.fineract.consumer.savings.command.data.SavingsApplicationCommandData; +import org.apache.fineract.consumer.savings.command.data.SubmitSavingsApplicationCommand; +import org.apache.fineract.consumer.savings.command.data.WithdrawSavingsApplicationCommand; +import org.apache.fineract.consumer.savings.command.exception.SavingsApplicationInvalidException; +import org.apache.fineract.consumer.savings.command.exception.SavingsCommandAccessDeniedException; +import org.apache.fineract.consumer.savings.command.exception.SavingsCommandInProgressException; +import org.apache.fineract.consumer.savings.command.exception.SavingsCommandNotFoundException; +import org.apache.fineract.consumer.savings.command.exception.SavingsCommandUpstreamUnavailableException; +import org.apache.fineract.consumer.user.query.data.UserQueryData; +import org.apache.fineract.consumer.user.query.service.UserQueryService; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.stereotype.Service; + +@Service +@RequiredArgsConstructor +public class SavingsCommandServiceImpl implements SavingsCommandService { + + private static final String LOCALE = "en"; + private static final String DATE_FORMAT = "yyyy-MM-dd"; + private static final Long SAVINGS_DEPOSIT_TYPE_ID = 100L; + + private final UserQueryService userQueryService; + private final AccessPolicyEvaluator accessPolicyEvaluator; + private final OwnedAccountsCache ownedAccountsCache; + private final IdempotencyKeyHolder idempotencyKeyHolder; + private final SavingsAccountApiClient savingsAccountApiClient; + + @Override + public SavingsApplicationCommandData submitApplication(Jwt jwt, SubmitSavingsApplicationCommand command) { + accessPolicyEvaluator.authorize(jwt, ConsumerAction.SAVINGS_APPLICATION_SUBMIT); + Long clientId = resolveClientId(jwt); + PostSavingsAccountsRequest request = buildSubmitRequest(command, clientId); + PostSavingsAccountsResponse response = callWithIdempotency(jwt, command.getIdempotencyKey(), + () -> savingsAccountApiClient.submitSavingsApplication(request)); + ownedAccountsCache.evict(clientId); + return SavingsApplicationCommandData.builder() + .savingsId(response.getSavingsId()) + .resourceId(response.getResourceId()) + .clientId(clientId) + .build(); + } + + @Override + public SavingsApplicationCommandData modifyApplication(Jwt jwt, ModifySavingsApplicationCommand command) { + accessPolicyEvaluator.authorize(jwt, ConsumerAction.SAVINGS_APPLICATION_MODIFY, command.getSavingsId(), + SavingsCommandAccessDeniedException::new); + validateSavingsDepositType(command.getSavingsId()); + Long clientId = resolveClientId(jwt); + PutSavingsAccountsAccountIdRequest request = buildModifyRequest(command); + PutSavingsAccountsAccountIdResponse response = callWithIdempotency(jwt, command.getIdempotencyKey(), + () -> savingsAccountApiClient.updateSavingsAccount(command.getSavingsId(), request, null)); + return SavingsApplicationCommandData.builder() + .savingsId(command.getSavingsId()) + .resourceId(response.getResourceId()) + .clientId(clientId) + .build(); + } + + @Override + public SavingsApplicationCommandData withdrawApplication(Jwt jwt, WithdrawSavingsApplicationCommand command) { + accessPolicyEvaluator.authorize(jwt, ConsumerAction.SAVINGS_APPLICATION_WITHDRAW, command.getSavingsId(), + SavingsCommandAccessDeniedException::new); + validateSavingsDepositType(command.getSavingsId()); + Long clientId = resolveClientId(jwt); + PostSavingsAccountsAccountIdRequest request = new PostSavingsAccountsAccountIdRequest() + .withdrawnOnDate(command.getWithdrawnOnDate().toString()) + .note(command.getNote()) + .dateFormat(DATE_FORMAT) + .locale(LOCALE); + PostSavingsAccountsAccountIdResponse response = callWithIdempotency(jwt, command.getIdempotencyKey(), + () -> savingsAccountApiClient.handleCommandsSavingsAccount(command.getSavingsId(), request, WITHDRAW_COMMAND)); + return SavingsApplicationCommandData.builder() + .savingsId(command.getSavingsId()) + .resourceId(response.getResourceId()) + .clientId(clientId) + .build(); + } + + private void validateSavingsDepositType(Long savingsId) { + SavingsAccountData account = call(() -> savingsAccountApiClient.retrieveSavingsAccount(savingsId)); + EnumOptionData depositType = account.getDepositType(); + if (depositType != null && depositType.getId() != null Review Comment: **Security: this guard fails open.** If `depositType` or its `id` is missing, the request is allowed. Against current Fineract that doesn't happen: `SavingsAccountReadPlatformServiceImpl.retrieveOne` always maps `deposit_type_enum`, including for FD/RD accounts. But a guard against acting on fixed/recurring deposits shouldn't depend on a field always being present. A spec or serializer change that drops it would silently re-open the hole. Please invert it: allow only when `depositType.getId()` equals `100`, and reject when it is null, e.g. ```java Long typeId = account.getDepositType() == null ? null : account.getDepositType().getId(); if (!SAVINGS_DEPOSIT_TYPE_ID.equals(typeId)) { throw new SavingsApplicationInvalidException(); } ``` Then add a unit case for a null `depositType` next to `modifyRejectsNonSavingsDepositType` / `withdrawRejectsNonSavingsDepositType`. The `savingsAccount(null)` helper already exists. ########## consumer/src/main/java/org/apache/fineract/consumer/infrastructure/fineractclient/service/SavingsAccountApiClient.java: ########## @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.infrastructure.fineractclient.service; + +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsAccountIdRequest; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsAccountIdResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsRequest; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PutSavingsAccountsAccountIdRequest; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PutSavingsAccountsAccountIdResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.SavingsAccountData; + +public interface SavingsAccountApiClient { Review Comment: Non-blocking: this seam satisfies the ArchUnit rule, but it mirrors `SavingsAccountApi` one-to-one, including the generated request and response models and the raw `command` string. So `SavingsCommandServiceImpl` still builds Fineract shapes (`dateFormat`, `locale`, `PostSavingsAccountsRequest`). #211 went a step further with intent-level methods (`getLoanPayoff`, `getClientOfficeId`). Something like `submitApplication(clientId, productId, date)`, `modifyApplication(...)`, `withdrawApplication(id, date, note)` and `depositType(id)` would keep Fineract specifics in infrastructure. That's the direction #188 is weighing; fine as a follow-up. -- 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]
