Skip to content

chore: remove primary EC check #327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jacopocarlini wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

chore: remove primary EC check #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2,698 changes: 1,267 additions & 1,431 deletions openapi/openapi_aca_v1.json
View file
Open in desktop

Large diffs are not rendered by default.

2,942 changes: 1,383 additions & 1,559 deletions openapi/openapi_external_v1.json
View file
Open in desktop

Large diffs are not rendered by default.

1,721 changes: 807 additions & 914 deletions openapi/openapi_external_v2.json
View file
Open in desktop

Large diffs are not rendered by default.

2,465 changes: 1,156 additions & 1,309 deletions openapi/openapi_external_v3.json
View file
Open in desktop

Large diffs are not rendered by default.

4,590 changes: 2,166 additions & 2,424 deletions openapi/openapi_internal_v1.json
View file
Open in desktop

Large diffs are not rendered by default.

4,989 changes: 2,357 additions & 2,632 deletions openapi/openapi_internal_v2.json
View file
Open in desktop

Large diffs are not rendered by default.

1,104 changes: 523 additions & 581 deletions openapi/openapi_send_v1.json
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 4 additions & 7 deletions src/main/java/it/gov/pagopa/debtposition/exception/AppError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,10 @@ public enum AppError {
"The caller does not have proper authorization to access or modify the IUVs in the payment"
+ " position. [Organization Fiscal Code=%s, IUPD=%s]"),
PAYMENT_OPTION_RESERVED_METADATA(
HttpStatus.CONFLICT,
"The payment option contains reserved metadata",
"The caller should not add or modify reserved payment option metadata. Reserved metadata list = {NOTIFICATION_FEE}."),
HttpStatus.CONFLICT,
"The payment option contains reserved metadata",
"The caller should not add or modify reserved payment option metadata. Reserved metadata list"
+ " = {NOTIFICATION_FEE}."),
ORGANIZATION_NOT_FOUND(
HttpStatus.NOT_FOUND,
"Not found the organization",
Expand Down Expand Up @@ -122,10 +123,6 @@ public enum AppError {
"Not in unpaid state for notification fee update",
"The payment option with Organization Fiscal Code %s and NAV %s is not in unpaid state and"
+ " the notification fee cannot be updated."),
PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_TRANSFER_NOT_FOUND(
HttpStatus.UNPROCESSABLE_ENTITY,
"No valid transfer found for notification fee update",
"No transfer found for payment option with IUV %s related to Organization Fiscal Code %s"),
PAYMENT_OPTION_PAY_FAILED(
HttpStatus.INTERNAL_SERVER_ERROR,
"The pay call for a payment option is failed",
Expand Down
19 changes: 8 additions & 11 deletions src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@
public static void updateAmountsWithNotificationFee(
PaymentOption paymentOption, String organizationFiscalCode, long notificationFeeAmount) {
// Get the first valid transfer to add the fee
Transfer validTransfer = findPrimaryTransfer(paymentOption, organizationFiscalCode);
Optional<Transfer> validTransfer = findPrimaryTransfer(paymentOption, organizationFiscalCode);

/*
Retrieving the old notification fee. It MUST BE SUBTRACTED from the various amount in order due to the fact that
Expand All @@ -271,8 +271,11 @@
paymentOption.setAmount(paymentOption.getAmount() + notificationFeeAmount);

// Subtracting the old value and adding the new one
validTransfer.setAmount(validTransfer.getAmount() - oldNotificationFee);
validTransfer.setAmount(validTransfer.getAmount() + notificationFeeAmount);
validTransfer.ifPresent(
elem -> {
elem.setAmount(elem.getAmount() - oldNotificationFee);
elem.setAmount(elem.getAmount() + notificationFeeAmount);

Check failure

Code scanning / CodeQL

User-controlled data in arithmetic expression High

This arithmetic expression depends on a
user-provided value
Loading
, potentially causing an overflow.

Copilot Autofix

AI 6 months ago

To fix this issue, we need to ensure that any arithmetic operation involving user-controlled data (here, notificationFeeAmount) does not result in an overflow or underflow. The best way is to validate the input before performing the arithmetic, ensuring that the result of the addition or subtraction will not exceed the bounds of the long type. We should also ensure that the notification fee is not negative, as that would likely be invalid in the business context.

Specifically, in updateAmountsWithNotificationFee, before performing:

  • paymentOption.setAmount(paymentOption.getAmount() + notificationFeeAmount)
  • elem.setAmount(elem.getAmount() + notificationFeeAmount)

We should check that the sum will not exceed Long.MAX_VALUE and will not be less than Long.MIN_VALUE. Similarly, for subtraction, we should check for underflow. If the check fails, we should throw an exception (e.g., AppException) indicating invalid input.

We will add a helper method to perform safe addition and subtraction with overflow checks, and use it in place of the direct arithmetic. We will also validate that notificationFeeAmount is non-negative.

All changes are within src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java.

Suggested changeset 1
src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java b/src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java
--- a/src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java
+++ b/src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java
@@ -35,6 +35,32 @@
 
 @Service
 @Slf4j
+
+  /**
+   * Safely adds two long values, throwing an AppException if overflow would occur.
+   */
+  private static long safeAdd(long a, long b, String organizationFiscalCode) {
+    if (b > 0L && a > Long.MAX_VALUE - b) {
+      throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Addition would cause overflow");
+    }
+    if (b < 0L && a < Long.MIN_VALUE - b) {
+      throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Addition would cause underflow");
+    }
+    return a + b;
+  }
+
+  /**
+   * Safely subtracts two long values, throwing an AppException if overflow would occur.
+   */
+  private static long safeSubtract(long a, long b, String organizationFiscalCode) {
+    if (b > 0L && a < Long.MIN_VALUE + b) {
+      throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Subtraction would cause underflow");
+    }
+    if (b < 0L && a > Long.MAX_VALUE + b) {
+      throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Subtraction would cause overflow");
+    }
+    return a - b;
+  }
 public class PaymentsService {
 
   private final PaymentOptionRepository paymentOptionRepository;
@@ -255,6 +281,10 @@
 
   public static void updateAmountsWithNotificationFee(
       PaymentOption paymentOption, String organizationFiscalCode, long notificationFeeAmount) {
+    // Validate notificationFeeAmount is non-negative
+    if (notificationFeeAmount < 0) {
+      throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Notification fee cannot be negative");
+    }
     // Get the first valid transfer to add the fee
     Optional<Transfer> validTransfer = findPrimaryTransfer(paymentOption, organizationFiscalCode);
 
@@ -267,14 +297,14 @@
     // Setting the new value of the notification fee, updating the amount of the payment option and
     // the last updated date fee
     paymentOption.setNotificationFee(notificationFeeAmount);
-    paymentOption.setAmount(paymentOption.getAmount() - oldNotificationFee);
-    paymentOption.setAmount(paymentOption.getAmount() + notificationFeeAmount);
+    paymentOption.setAmount(safeSubtract(paymentOption.getAmount(), oldNotificationFee, organizationFiscalCode));
+    paymentOption.setAmount(safeAdd(paymentOption.getAmount(), notificationFeeAmount, organizationFiscalCode));
 
     // Subtracting the old value and adding the new one
     validTransfer.ifPresent(
         elem -> {
-          elem.setAmount(elem.getAmount() - oldNotificationFee);
-          elem.setAmount(elem.getAmount() + notificationFeeAmount);
+          elem.setAmount(safeSubtract(elem.getAmount(), oldNotificationFee, organizationFiscalCode));
+          elem.setAmount(safeAdd(elem.getAmount(), notificationFeeAmount, organizationFiscalCode));
         });
   }
 
EOF
@@ -35,6 +35,32 @@

@Service
@Slf4j

/**
* Safely adds two long values, throwing an AppException if overflow would occur.
*/
private static long safeAdd(long a, long b, String organizationFiscalCode) {
if (b > 0L && a > Long.MAX_VALUE - b) {
throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Addition would cause overflow");
}
if (b < 0L && a < Long.MIN_VALUE - b) {
throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Addition would cause underflow");
}
return a + b;
}

/**
* Safely subtracts two long values, throwing an AppException if overflow would occur.
*/
private static long safeSubtract(long a, long b, String organizationFiscalCode) {
if (b > 0L && a < Long.MIN_VALUE + b) {
throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Subtraction would cause underflow");
}
if (b < 0L && a > Long.MAX_VALUE + b) {
throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Subtraction would cause overflow");
}
return a - b;
}
public class PaymentsService {

private final PaymentOptionRepository paymentOptionRepository;
@@ -255,6 +281,10 @@

public static void updateAmountsWithNotificationFee(
PaymentOption paymentOption, String organizationFiscalCode, long notificationFeeAmount) {
// Validate notificationFeeAmount is non-negative
if (notificationFeeAmount < 0) {
throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Notification fee cannot be negative");
}
// Get the first valid transfer to add the fee
Optional<Transfer> validTransfer = findPrimaryTransfer(paymentOption, organizationFiscalCode);

@@ -267,14 +297,14 @@
// Setting the new value of the notification fee, updating the amount of the payment option and
// the last updated date fee
paymentOption.setNotificationFee(notificationFeeAmount);
paymentOption.setAmount(paymentOption.getAmount() - oldNotificationFee);
paymentOption.setAmount(paymentOption.getAmount() + notificationFeeAmount);
paymentOption.setAmount(safeSubtract(paymentOption.getAmount(), oldNotificationFee, organizationFiscalCode));
paymentOption.setAmount(safeAdd(paymentOption.getAmount(), notificationFeeAmount, organizationFiscalCode));

// Subtracting the old value and adding the new one
validTransfer.ifPresent(
elem -> {
elem.setAmount(elem.getAmount() - oldNotificationFee);
elem.setAmount(elem.getAmount() + notificationFeeAmount);
elem.setAmount(safeSubtract(elem.getAmount(), oldNotificationFee, organizationFiscalCode));
elem.setAmount(safeAdd(elem.getAmount(), notificationFeeAmount, organizationFiscalCode));
});
}

Copilot is powered by AI and may make mistakes. Always verify output.
});
}

/**
Expand All @@ -282,7 +285,7 @@
* @param organizationFiscalCode EC
* @return the transfer of the primary Creditor Institution
*/
public static Transfer findPrimaryTransfer(
public static Optional<Transfer> findPrimaryTransfer(
PaymentOption paymentOption, String organizationFiscalCode) {
List<Transfer> transfers = paymentOption.getTransfer();
return transfers.stream()
Expand All @@ -291,13 +294,7 @@
transfer ->
transfer.getOrganizationFiscalCode() == null
|| organizationFiscalCode.equals(transfer.getOrganizationFiscalCode()))
.findFirst()
.orElseThrow(
() ->
new AppException(
AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_TRANSFER_NOT_FOUND,
paymentOption.getIuv(),
organizationFiscalCode));
.findFirst();
}

public List<OrganizationModelQueryBean> getOrganizationsToAdd(@NotNull LocalDate since) {
Expand Down
20 changes: 4 additions & 16 deletions src/main/java/it/gov/pagopa/debtposition/service/pd/crud/PaymentPositionCRUDService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,13 +248,6 @@ public PaymentPosition update(
} catch (ValidationException e) {
throw new AppException(AppError.DEBT_POSITION_REQUEST_DATA_ERROR, e.getMessage());
} catch (AppException e) {
if (AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_TRANSFER_NOT_FOUND.title.equals(
e.getTitle())) {
throw new AppException(
AppError.DEBT_POSITION_UPDATE_FAILED_NO_TRANSFER_FOR_NOTIFICATION_FEE,
organizationFiscalCode,
ppToUpdate.getIupd());
}
throw e;
} catch (Exception e) {
log.error(String.format(ERROR_UPDATE_LOG_MSG, e.getMessage()), e);
Expand All @@ -280,8 +273,10 @@ private static void updateAmounts(String organizationFiscalCode, PaymentPosition

if (!po.getTransfer().isEmpty()) {
// update amount in the Transfer
Transfer primaryTransfer = findPrimaryTransfer(po, organizationFiscalCode);
primaryTransfer.setAmount(primaryTransfer.getAmount() + po.getNotificationFee());
Optional<Transfer> primaryTransfer =
findPrimaryTransfer(po, organizationFiscalCode);
primaryTransfer.ifPresent(
elem -> elem.setAmount(elem.getAmount() + po.getNotificationFee()));
}
}
});
Expand Down Expand Up @@ -371,13 +366,6 @@ public List<PaymentPosition> updateMultipleDebtPositions(
} catch (ValidationException e) {
throw new AppException(AppError.DEBT_POSITION_REQUEST_DATA_ERROR, e.getMessage());
} catch (AppException e) {
if (AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_TRANSFER_NOT_FOUND.title.equals(
e.getTitle())) {
throw new AppException(
AppError.DEBT_POSITION_UPDATE_FAILED_NO_TRANSFER_FOR_NOTIFICATION_FEE,
organizationFiscalCode,
e.getMessage());
}
throw e;
} catch (Exception e) {
// Logga l'errore completo per il debug
Expand Down
46 changes: 0 additions & 46 deletions src/test/java/it/gov/pagopa/debtposition/controller/DebtPositionControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package it.gov.pagopa.debtposition.controller;

import static org.hamcrest.CoreMatchers.containsString;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.when;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
Expand All @@ -14,8 +12,6 @@
import it.gov.pagopa.debtposition.client.NodeClient;
import it.gov.pagopa.debtposition.dto.*;
import it.gov.pagopa.debtposition.mock.DebtPositionMock;
import it.gov.pagopa.debtposition.model.checkposition.NodeCheckPositionModel;
import it.gov.pagopa.debtposition.model.checkposition.response.NodeCheckPositionResponse;
import it.gov.pagopa.debtposition.model.enumeration.DebtPositionStatus;
import it.gov.pagopa.debtposition.model.enumeration.PaymentOptionStatus;
import it.gov.pagopa.debtposition.model.enumeration.TransferStatus;
Expand All @@ -42,7 +38,6 @@
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.http.MediaType;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
import org.springframework.test.web.servlet.result.MockMvcResultMatchers;

@SpringBootTest(classes = DebtPositionApplication.class)
Expand Down Expand Up @@ -2082,47 +2077,6 @@ void updateDebtPosition_PAID_409() throws Exception {
.andExpect(status().isConflict());
}

@Test
void updateDebtPosition_noValidTransfer_422() throws Exception {

PaymentPositionDTO paymentPositionDTO =
DebtPositionMock.paymentPositionForNotificationUpdateMock1();

when(nodeClient.getCheckPosition(any(NodeCheckPositionModel.class)))
.thenReturn(NodeCheckPositionResponse.builder().outcome("OK").build());

// creo una posizione debitoria e recupero la payment option associata
mvc.perform(
post("/organizations/UPD422_novalidtransfer_12345678901/debtpositions")
.content(TestUtil.toJson(paymentPositionDTO))
.contentType(MediaType.APPLICATION_JSON))
.andExpect(status().isCreated());

// effettuo un aggiornamento della notification fee (si continua ad utilizzare lo IUV e non il
// NAV)
mvc.perform(
MockMvcRequestBuilders.put(
"/organizations/UPD422_novalidtransfer_12345678901/paymentoptions/123456IUVMOCK1/notificationfee")
.content(TestUtil.toJson(DebtPositionMock.createNotificationFeeMock(150L)))
.contentType(MediaType.APPLICATION_JSON))
.andExpect(status().isOk());

// effettuo la chiamata di modifica del codice fiscale dell'EC del transfer ma non posso
// procedere perche eliminerei tutti i transfer associabili per la fee
paymentPositionDTO
.getPaymentOption()
.get(0)
.getTransfer()
.get(0)
.setOrganizationFiscalCode("acreditorinstitution");
mvc.perform(
MockMvcRequestBuilders.put(
"/organizations/UPD422_novalidtransfer_12345678901/debtpositions/12345678901IUPDMOCK1")
.content(TestUtil.toJson(paymentPositionDTO))
.contentType(MediaType.APPLICATION_JSON))
.andExpect(status().isUnprocessableEntity());
}

/** UPDATE IBAN ON TRANSFERS */
@Test
void updateTransferIbanMassive_200() throws Exception {
Expand Down
36 changes: 0 additions & 36 deletions src/test/java/it/gov/pagopa/debtposition/controller/PaymentsControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1908,42 +1908,6 @@ void updateNotificationFee_paymentOptionAlreadyPaid_422() throws Exception {
verify(nodeClient, times(0)).getCheckPosition(any(NodeCheckPositionModel.class));
}

@Test
void updateNotificationFee_noValidTransfer_422() throws Exception {

PaymentPositionDTO paymentPositionDTO =
DebtPositionMock.paymentPositionForNotificationUpdateMock1();

// creo una posizione debitoria e recupero la payment option associata
mvc.perform(
post("/organizations/PO422_notificationfee_invalidtransfer_12345678901/debtpositions")
.content(TestUtil.toJson(paymentPositionDTO))
.contentType(MediaType.APPLICATION_JSON))
.andExpect(status().isCreated());

mvc.perform(
MockMvcRequestBuilders.put(
"/organizations/PO422_notificationfee_invalidtransfer_12345678901/paymentoptions/123456IUVMOCK1/notificationfee")
.content(TestUtil.toJson(DebtPositionMock.createNotificationFeeMock(150L)))
.contentType(MediaType.APPLICATION_JSON))
.andExpect(status().is(209));

// effettuo la chiamata ma non posso continuare perche non esiste un transfer correlata all'EC
// in input
paymentPositionDTO
.getPaymentOption()
.get(0)
.getTransfer()
.get(0)
.setOrganizationFiscalCode("hfdkjshfkdha");
mvc.perform(
MockMvcRequestBuilders.put(
"/organizations/PO422_notificationfee_invalidtransfer_12345678901/debtpositions/12345678901IUPDMOCK1")
.content(TestUtil.toJson(paymentPositionDTO))
.contentType(MediaType.APPLICATION_JSON))
.andExpect(status().isUnprocessableEntity());
}

@Test
void updateNotificationFee_209() throws Exception {

Expand Down
Loading