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

Make IP address optional when reporting transactions #141

Merged
merged 5 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions HISTORY.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@
History
-------

2.11.0
+++++++++++++++++++

* Updated the validation for the Report Transactions API to make the
``ip_address`` parameter optional. Now the ``tag`` and at least one of the
following parameters must be supplied: ``ip_address``, ``maxmind_id``,
``minfraud_id``, ``transaction_id``.

2.10.0 (2024-04-16)
+++++++++++++++++++

Expand Down
3 changes: 2 additions & 1 deletion README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ The method takes a dictionary representing the report to be sent to the web
service. The structure of this dictionary should be in `the format specified
in the REST API documentation
<https://dev.maxmind.com/minfraud/report-a-transaction?lang=en>`__. The
``ip_address`` and ``tag`` fields are required. All other fields are optional.
required fields are ``tag`` and one or more of the following: ``ip_address``,
``maxmind_id``, ``minfraud_id``, ``transaction_id``.

Request Validation (for all request methods)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
44 changes: 39 additions & 5 deletions minfraud/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from typing import Optional

from email_validator import validate_email # type: ignore
from voluptuous import All, Any, In, Match, Range, Required, Schema
from voluptuous import All, Any, In, Match, MultipleInvalid, Range, Required, Schema
from voluptuous.error import UrlInvalid

# Pylint doesn't like the private function type naming for the callable
Expand Down Expand Up @@ -379,14 +379,48 @@ def _uuid(s: str) -> str:
raise ValueError


validate_report = Schema(
NIL_UUID = str(uuid.UUID(int=0))


def _non_empty_uuid(s: str) -> str:
if _uuid(s) == NIL_UUID:
raise ValueError
return s


def _transaction_id(s: Optional[str]) -> str:
if isinstance(s, str) and len(s) > 0:
return s
raise ValueError


_validate_report_schema = Schema(
{
"chargeback_code": str,
Required("ip_address"): _ip_address,
"ip_address": _ip_address,
"maxmind_id": _maxmind_id,
"minfraud_id": _uuid,
"minfraud_id": _non_empty_uuid,
"notes": str,
Required("tag"): _tag,
"transaction_id": str,
"transaction_id": _transaction_id,
},
)


def _validate_at_least_one_identifier_field(report):
optional_fields = ["ip_address", "maxmind_id", "minfraud_id", "transaction_id"]
if not any(field in report for field in optional_fields):
Comment on lines +411 to +412
Copy link
Member

Choose a reason for hiding this comment

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

This is probably bikeshedding, but expressing it as a set operation seems a bit simpler to me:

Suggested change
optional_fields = ["ip_address", "maxmind_id", "minfraud_id", "transaction_id"]
if not any(field in report for field in optional_fields):
optional_fields = {"ip_address", "maxmind_id", "minfraud_id", "transaction_id"}
if not optional_fields & report.keys():

Copy link
Member

Choose a reason for hiding this comment

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

That said, maybe we should handle the empty string for these too like we do for the other APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The individual field validation that exists should already disallow empty strings for all of these fields. The only one we might need to check is minfraid_id to disallow the zero UUID

# We return MultipleInvalid instead of ValueError to be consistent with what
# voluptuous returns.
raise MultipleInvalid(
"The report must contain at least one of the following fields: "
"'ip_address', 'maxmind_id', 'minfraud_id', 'transaction_id'."
)
return True


def validate_report(report):
"""Validate minFraud Transaction Report fields."""
_validate_report_schema(report)
_validate_at_least_one_identifier_field(report)
return True
17 changes: 17 additions & 0 deletions tests/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,17 @@ def setup_report(self, report):

def check_invalid_report(self, report):
self.setup_report(report)
self.check_invalid_report_no_setup(report)

def check_invalid_report_no_setup(self, report):
with self.assertRaises(MultipleInvalid, msg=f"{report} is invalid"):
validate_report(report)

def check_report(self, report):
self.setup_report(report)
self.check_report_no_setup(report)

def check_report_no_setup(self, report):
try:
validate_report(report)
except MultipleInvalid as e:
Expand Down Expand Up @@ -414,6 +420,7 @@ def test_minfraud_id(self):
"12345678-123412341234-12345678901",
"12345678-1234-1234-1234-1234567890123",
"12345678-1234-1234-1234-12345678901g",
"00000000-0000-0000-0000-000000000000",
"",
):
self.check_invalid_report({"minfraud_id": bad})
Expand All @@ -431,3 +438,13 @@ def test_tag(self):
self.check_report({"tag": good})
for bad in ("risky_business", "", None):
self.check_invalid_report({"tag": bad})

def test_report_valid_identifier(self):
self.check_invalid_report_no_setup({"tag": "chargeback"})

self.check_report_no_setup({"tag": "chargeback", "ip_address": "1.1.1.1"})
self.check_report_no_setup(
{"tag": "chargeback", "minfraud_id": "58fa38d8-4b87-458b-a22b-f00eda1aa20d"}
)
self.check_report_no_setup({"tag": "chargeback", "maxmind_id": "12345678"})
self.check_report_no_setup({"tag": "chargeback", "transaction_id": "abc123"})
Loading