Skip to content
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.

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

fix internal server error on too long input for transaction create and update mutation #13608

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,8 @@ Shipping methods can be removed by the user after it has been assigned to a chec
- Add missing descriptions to payment module - #13546 by @devilsautumn
- Fix `NOTIFY_USER` allow to create webhook with only one event - #13584 by @Air-t
- Add Index for 'Created' field of the Order Model - #13682 by @ritanjandawn
- Fix transaction create mutation's psp_reference max length issue - #12696 by @ssuraliya
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would have to move this entry to the newest version section - 3.18.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already moved forward with releases so when you fix this please move this to 3.19 (or just the newest unreleased version in the CHANGELOG.md)



# 3.14.0

Expand Down
38 changes: 36 additions & 2 deletions saleor/graphql/payment/mutations/transaction/transaction_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,24 @@ class Meta:
error_type_class = common_types.TransactionCreateError
permissions = (PaymentPermissions.HANDLE_PAYMENTS,)

@classmethod
def validate_psp_reference(
cls,
psp_reference: Optional[str],
field_name: str,
error_code: str,
):
if psp_reference is None:
return
if len(psp_reference) > 512:
raise ValidationError(
{
field_name: ValidationError(
"Maximum length for `pspReference` is 512.", code=error_code,
)
}
)

@classmethod
def validate_external_url(cls, external_url: Optional[str], error_code: str):
if external_url is None:
Expand Down Expand Up @@ -214,10 +232,24 @@ def validate_money_input(

@classmethod
def validate_input(
cls, instance: Union[checkout_models.Checkout, order_models.Order], transaction
cls,
instance: Union[checkout_models.Checkout, order_models.Order],
transaction,
transaction_event,
) -> Union[checkout_models.Checkout, order_models.Order]:
currency = instance.currency

cls.validate_psp_reference(
transaction.get("psp_reference"),
field_name="transaction",
error_code=TransactionCreateErrorCode.INVALID.value,
)
if transaction_event is not None:
cls.validate_psp_reference(
transaction_event.get("psp_reference"),
field_name="transactionEvent",
error_code=TransactionCreateErrorCode.INVALID.value,
)
cls.validate_money_input(
transaction,
currency,
Expand Down Expand Up @@ -335,7 +367,9 @@ def perform_mutation( # type: ignore[override]
order_or_checkout_instance, id
)
order_or_checkout_instance = cls.validate_input(
order_or_checkout_instance, transaction=transaction
order_or_checkout_instance,
transaction=transaction,
transaction_event=transaction_event,
)
transaction_data = {**transaction}
transaction_data["currency"] = order_or_checkout_instance.currency
Expand Down
59 changes: 38 additions & 21 deletions saleor/graphql/payment/mutations/transaction/transaction_update.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,33 +97,49 @@ def check_can_update(

@classmethod
def validate_transaction_input(
cls, instance: payment_models.TransactionItem, transaction_data
cls,
instance: payment_models.TransactionItem,
transaction_data,
transaction_event_data
Comment on lines +113 to +114
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's just a small nitpick - the first mutation uses transaction and transaction_event names, while this one has different ones. It would be nice to use the same pattern for consistency as I think these are the same objects.

):
currency = instance.currency
if transaction_data.get("available_actions") is not None:
transaction_data["available_actions"] = list(
set(transaction_data.get("available_actions", []))
)

cls.validate_money_input(
transaction_data,
currency,
TransactionUpdateErrorCode.INCORRECT_CURRENCY.value,
)
cls.validate_metadata_keys(
transaction_data.get("metadata", []),
field_name="metadata",
error_code=TransactionUpdateErrorCode.METADATA_KEY_REQUIRED.value,
)
cls.validate_metadata_keys(
transaction_data.get("private_metadata", []),
field_name="privateMetadata",
error_code=TransactionUpdateErrorCode.METADATA_KEY_REQUIRED.value,
)
cls.validate_external_url(
transaction_data.get("external_url"),
error_code=TransactionCreateErrorCode.INVALID.value,
)

if transaction_data:
cls.validate_psp_reference(
transaction_data.get("psp_reference"),
field_name="transaction",
error_code=TransactionUpdateErrorCode.INVALID.value,
)
cls.validate_money_input(
transaction_data,
currency,
TransactionUpdateErrorCode.INCORRECT_CURRENCY.value,
)
cls.validate_metadata_keys(
transaction_data.get("metadata", []),
field_name="metadata",
error_code=TransactionUpdateErrorCode.METADATA_KEY_REQUIRED.value,
)
cls.validate_metadata_keys(
transaction_data.get("private_metadata", []),
field_name="privateMetadata",
error_code=TransactionUpdateErrorCode.METADATA_KEY_REQUIRED.value,
)
cls.validate_external_url(
transaction_data.get("external_url"),
error_code=TransactionCreateErrorCode.INVALID.value,
)
if transaction_event_data:
cls.validate_psp_reference(
transaction_event_data.get("psp_reference"),
field_name="transactionEvent",
error_code=TransactionUpdateErrorCode.INVALID.value,
)

@classmethod
def update_transaction(
Expand Down Expand Up @@ -200,8 +216,9 @@ def perform_mutation( # type: ignore[override]
previous_charged_value = instance.charged_value
previous_refunded_value = instance.refunded_value

cls.validate_transaction_input(instance, transaction, transaction_event)

if transaction:
cls.validate_transaction_input(instance, transaction)
cls.assign_app_to_transaction_data_if_missing(instance, transaction, app)
cls.cleanup_metadata_data(transaction)
money_data = cls.get_money_data_from_input(transaction)
Expand Down
26 changes: 26 additions & 0 deletions saleor/graphql/payment/tests/mutations/test_transaction_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -1865,3 +1865,29 @@ def test_transaction_create_for_order_triggers_webhook_when_partially_refunded(
assert not mock_order_fully_refunded.called
mock_order_updated.assert_called_once_with(order_with_lines)
mock_order_refunded.assert_called_once_with(order_with_lines)


def test_transaction_create_psp_reference_length_exceed(
order_with_lines, permission_manage_payments, app_api_client
):
psp_reference = "a" * 513

variables = {
"id": graphene.Node.to_global_id("Order", order_with_lines.pk),
"transaction": {
"pspReference": psp_reference,
},
}

# when
response = app_api_client.post_graphql(
MUTATION_TRANSACTION_CREATE, variables, permissions=[permission_manage_payments]
)

# then
content = get_graphql_content(response)
data = content["data"]["transactionCreate"]
assert data["errors"][0]["field"] == "transaction"
assert (
data["errors"][0]["code"] == TransactionCreateErrorCode.INVALID.name
)
29 changes: 29 additions & 0 deletions saleor/graphql/payment/tests/mutations/test_transaction_update.py
Original file line number Diff line number Diff line change
Expand Up @@ -2790,3 +2790,32 @@ def test_transaction_update_by_app_assign_app_owner(
assert transaction.app_identifier == app_api_client.app.identifier
assert transaction.app == app_api_client.app
assert transaction.user is None


def test_transaction_update_psp_reference_length_exceed(
transaction_item_created_by_app, permission_manage_payments, app_api_client
):
# given
transaction = transaction_item_created_by_app

psp_reference = "a" * 513

variables = {
"id": graphene.Node.to_global_id("TransactionItem", transaction.token),
"transaction": {
"pspReference": psp_reference,
},
}

# when
response = app_api_client.post_graphql(
MUTATION_TRANSACTION_UPDATE, variables, permissions=[permission_manage_payments]
)

# then
content = get_graphql_content(response)
data = content["data"]["transactionUpdate"]
assert data["errors"][0]["field"] == "transaction"
assert (
data["errors"][0]["code"] == TransactionUpdateErrorCode.INVALID.name
)
26 changes: 26 additions & 0 deletions saleor/payment/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,32 @@ def test_parse_transaction_action_data_with_missing_mandatory_event_fields():
# then
assert parsed_data is None

def test_parse_transaction_action_data_with_psp_reference_max_length_exceed():
# given
expected_psp_reference = "a" * 513
event_amount = 12.00
event_type = TransactionEventType.CHARGE_SUCCESS
event_time = "2022-11-18T13:25:58.169685+00:00"
event_url = "http://localhost:3000/event/ref123"
event_cause = "No cause"

response_data = {
"pspReference": expected_psp_reference,
"amount": event_amount,
"result": event_type.upper(),
"time": event_time,
"externalUrl": event_url,
"message": event_cause,
}

# when
parsed_data, error_msg = parse_transaction_action_data(
response_data, TransactionEventType.CHARGE_REQUEST
)
# then
assert parsed_data is None
assert isinstance(error_msg, str)


def test_create_failed_transaction_event(transaction_item_generator):
# given
Expand Down
6 changes: 6 additions & 0 deletions saleor/payment/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,12 @@ def parse_transaction_action_data(
logger.error(msg)
return None, msg

if psp_reference:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could make it as a single condition:

if psp_reference and len(psp_reference) > 512:
    ...

if len(psp_reference) > 512:
msg: str = "Maximum length for `pspReference` is 512."
logger.error(msg)
return None, msg

available_actions = response_data.get("actions", None)
if available_actions is not None:
possible_actions = {
Expand Down