From c626e96f0fc3b1f9700c1b502fcf57e94bd4d541 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Fri, 29 Dec 2023 13:32:22 -0800 Subject: [PATCH] Reworks custom field value validation to check and return a 400 error code in more cases and support more URL looking items, not just some basic schemes --- src/documents/serialisers.py | 35 +- src/documents/tests/test_api_custom_fields.py | 134 ++++++- src/documents/utils.py | 378 ------------------ src/documents/validators.py | 29 ++ 4 files changed, 173 insertions(+), 403 deletions(-) create mode 100644 src/documents/validators.py diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index 0c36a32b5..c07b00f78 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -2,6 +2,7 @@ import datetime import math import re import zoneinfo +from decimal import Decimal import magic from celery import states @@ -9,7 +10,9 @@ from django.conf import settings from django.contrib.auth.models import Group from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType -from django.core.validators import URLValidator +from django.core.validators import DecimalValidator +from django.core.validators import MaxLengthValidator +from django.core.validators import integer_validator from django.utils.crypto import get_random_string from django.utils.text import slugify from django.utils.translation import gettext as _ @@ -41,7 +44,7 @@ from documents.models import UiSettings from documents.parsers import is_mime_type_supported from documents.permissions import get_groups_with_only_permission from documents.permissions import set_permissions_for_object -from documents.utils import ALL_URL_SCHEMES +from documents.validators import uri_validator # https://www.django-rest-framework.org/api-guide/serializers/#example @@ -490,17 +493,29 @@ class CustomFieldInstanceSerializer(serializers.ModelSerializer): def validate(self, data): """ - For some reason, URLField validation is not run against the value - automatically. Force it to run against the value + Probably because we're kind of doing it odd, validation from the model + doesn't run against the field "value", so we have to re-create it here. + + Don't like it, but it is better than returning an HTTP 500 when the database + hates the value """ data = super().validate(data) field: CustomField = data["field"] - if ( - field.data_type == CustomField.FieldDataType.URL - and data["value"] is not None - and len(data["value"]) > 0 - ): - URLValidator(schemes=ALL_URL_SCHEMES)(data["value"]) + if "value" in data and data["value"] is not None: + if ( + field.data_type == CustomField.FieldDataType.URL + and len(data["value"]) > 0 + ): + uri_validator(data["value"]) + elif field.data_type == CustomField.FieldDataType.INT: + integer_validator(data["value"]) + elif field.data_type == CustomField.FieldDataType.MONETARY: + DecimalValidator(max_digits=12, decimal_places=2)( + Decimal(str(data["value"])), + ) + elif field.data_type == CustomField.FieldDataType.STRING: + MaxLengthValidator(limit_value=128)(data["value"]) + return data def reflect_doclinks( diff --git a/src/documents/tests/test_api_custom_fields.py b/src/documents/tests/test_api_custom_fields.py index 15abcd053..690e92690 100644 --- a/src/documents/tests/test_api_custom_fields.py +++ b/src/documents/tests/test_api_custom_fields.py @@ -333,19 +333,17 @@ class TestCustomField(DirectoriesMixin, APITestCase): }, format="json", ) - from pprint import pprint - pprint(resp.json()) self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) self.assertEqual(CustomFieldInstance.objects.count(), 0) self.assertEqual(len(doc.custom_fields.all()), 0) - def test_custom_field_value_validation(self): + def test_custom_field_value_url_validation(self): """ GIVEN: - Document & custom field exist WHEN: - - API request to set a field value + - API request to set a field value to something which is or is not a link THEN: - HTTP 400 is returned - No field instance is created or attached to the document @@ -360,31 +358,62 @@ class TestCustomField(DirectoriesMixin, APITestCase): name="Test Custom Field URL", data_type=CustomField.FieldDataType.URL, ) - custom_field_int = CustomField.objects.create( - name="Test Custom Field INT", - data_type=CustomField.FieldDataType.INT, - ) + for value in ["not a url", "file:"]: + with self.subTest(f"Test value {value}"): + resp = self.client.patch( + f"/api/documents/{doc.id}/", + data={ + "custom_fields": [ + { + "field": custom_field_url.id, + "value": value, + }, + ], + }, + format="json", + ) + + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(CustomFieldInstance.objects.count(), 0) + self.assertEqual(len(doc.custom_fields.all()), 0) resp = self.client.patch( f"/api/documents/{doc.id}/", data={ "custom_fields": [ { "field": custom_field_url.id, - "value": "not a url", + "value": "tel:+1-816-555-1212", }, ], }, format="json", ) - self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(CustomFieldInstance.objects.count(), 0) - self.assertEqual(len(doc.custom_fields.all()), 0) + self.assertEqual(resp.status_code, status.HTTP_200_OK) - self.assertRaises( - Exception, - self.client.patch, + def test_custom_field_value_integer_validation(self): + """ + GIVEN: + - Document & custom field exist + WHEN: + - API request to set a field value to something not an integer + THEN: + - HTTP 400 is returned + - No field instance is created or attached to the document + """ + doc = Document.objects.create( + title="WOW", + content="the content", + checksum="123", + mime_type="application/pdf", + ) + custom_field_int = CustomField.objects.create( + name="Test Custom Field INT", + data_type=CustomField.FieldDataType.INT, + ) + + resp = self.client.patch( f"/api/documents/{doc.id}/", data={ "custom_fields": [ @@ -397,6 +426,81 @@ class TestCustomField(DirectoriesMixin, APITestCase): format="json", ) + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(CustomFieldInstance.objects.count(), 0) + self.assertEqual(len(doc.custom_fields.all()), 0) + + def test_custom_field_value_monetary_validation(self): + """ + GIVEN: + - Document & custom field exist + WHEN: + - API request to set a field value to something not a valid monetary decimal + THEN: + - HTTP 400 is returned + - No field instance is created or attached to the document + """ + doc = Document.objects.create( + title="WOW", + content="the content", + checksum="123", + mime_type="application/pdf", + ) + custom_field_money = CustomField.objects.create( + name="Test Custom Field MONETARY", + data_type=CustomField.FieldDataType.MONETARY, + ) + + resp = self.client.patch( + f"/api/documents/{doc.id}/", + data={ + "custom_fields": [ + { + "field": custom_field_money.id, + # Too many places past decimal + "value": 12.123, + }, + ], + }, + format="json", + ) + + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(CustomFieldInstance.objects.count(), 0) + self.assertEqual(len(doc.custom_fields.all()), 0) + + def test_custom_field_value_short_text_validation(self): + """ + GIVEN: + - Document & custom field exist + WHEN: + - API request to set a field value to a too long string + THEN: + - HTTP 400 is returned + - No field instance is created or attached to the document + """ + doc = Document.objects.create( + title="WOW", + content="the content", + checksum="123", + mime_type="application/pdf", + ) + custom_field_string = CustomField.objects.create( + name="Test Custom Field STRING", + data_type=CustomField.FieldDataType.STRING, + ) + + resp = self.client.patch( + f"/api/documents/{doc.id}/", + data={ + "custom_fields": [ + {"field": custom_field_string.id, "value": "a" * 129}, + ], + }, + format="json", + ) + + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) self.assertEqual(CustomFieldInstance.objects.count(), 0) self.assertEqual(len(doc.custom_fields.all()), 0) diff --git a/src/documents/utils.py b/src/documents/utils.py index 2206ea4b5..b84c9b53c 100644 --- a/src/documents/utils.py +++ b/src/documents/utils.py @@ -40,381 +40,3 @@ def copy_file_with_basic_stats( shutil.copy(source, dest) copy_basic_file_stats(source, dest) - - -ALL_URL_SCHEMES = [ - "aaa", - "aaas", - "about", - "acap", - "acct", - "acd", - "acr", - "adiumxtra", - "adt", - "afp", - "afs", - "aim", - "amss", - "android", - "appdata", - "apt", - "ar", - "ark", - "at", - "attachment", - "aw", - "barion", - "bb", - "beshare", - "bitcoin", - "bitcoincash", - "blob", - "bolo", - "brid", - "browserext", - "cabal", - "calculator", - "callto", - "cap", - "cast", - "casts", - "chrome", - "chrome-extension", - "cid", - "coap", - "coap+tcp", - "coap+ws", - "coaps", - "coaps+tcp", - "coaps+ws", - "com-eventbrite-attendee", - "content", - "content-type", - "crid", - "cstr", - "cvs", - "dab", - "dat", - "data", - "dav", - "dhttp", - "diaspora", - "dict", - "did", - "dis", - "dlna-playcontainer", - "dlna-playsingle", - "dns", - "dntp", - "doi", - "dpp", - "drm", - "drop", - "dtmi", - "dtn", - "dvb", - "dvx", - "dweb", - "ed2k", - "eid", - "elsi", - "embedded", - "ens", - "ethereum", - "example", - "facetime", - "fax", - "feed", - "feedready", - "fido", - "file", - "filesystem", - "finger", - "first-run-pen-experience", - "fish", - "fm", - "ftp", - "fuchsia-pkg", - "geo", - "gg", - "git", - "gitoid", - "gizmoproject", - "go", - "gopher", - "graph", - "grd", - "gtalk", - "h323", - "ham", - "hcap", - "hcp", - "http", - "https", - "hxxp", - "hxxps", - "hydrazone", - "hyper", - "iax", - "icap", - "icon", - "im", - "imap", - "info", - "iotdisco", - "ipfs", - "ipn", - "ipns", - "ipp", - "ipps", - "irc", - "irc6", - "ircs", - "iris", - "iris.beep", - "iris.lwz", - "iris.xpc", - "iris.xpcs", - "isostore", - "itms", - "jabber", - "jar", - "jms", - "keyparc", - "lastfm", - "lbry", - "ldap", - "ldaps", - "leaptofrogans", - "lid", - "lorawan", - "lpa", - "lvlt", - "magnet", - "mailserver", - "mailto", - "maps", - "market", - "matrix", - "message", - "microsoft.windows.camera", - "microsoft.windows.camera.multipicker", - "microsoft.windows.camera.picker", - "mid", - "mms", - "modem", - "mongodb", - "moz", - "ms-access", - "ms-appinstaller", - "ms-browser-extension", - "ms-calculator", - "ms-drive-to", - "ms-enrollment", - "ms-excel", - "ms-eyecontrolspeech", - "ms-gamebarservices", - "ms-gamingoverlay", - "ms-getoffice", - "ms-help", - "ms-infopath", - "ms-inputapp", - "ms-launchremotedesktop", - "ms-lockscreencomponent-config", - "ms-media-stream-id", - "ms-meetnow", - "ms-mixedrealitycapture", - "ms-mobileplans", - "ms-newsandinterests", - "ms-officeapp", - "ms-people", - "ms-project", - "ms-powerpoint", - "ms-publisher", - "ms-remotedesktop", - "ms-remotedesktop-launch", - "ms-restoretabcompanion", - "ms-screenclip", - "ms-screensketch", - "ms-search", - "ms-search-repair", - "ms-secondary-screen-controller", - "ms-secondary-screen-setup", - "ms-settings", - "ms-settings-airplanemode", - "ms-settings-bluetooth", - "ms-settings-camera", - "ms-settings-cellular", - "ms-settings-cloudstorage", - "ms-settings-connectabledevices", - "ms-settings-displays-topology", - "ms-settings-emailandaccounts", - "ms-settings-language", - "ms-settings-location", - "ms-settings-lock", - "ms-settings-nfctransactions", - "ms-settings-notifications", - "ms-settings-power", - "ms-settings-privacy", - "ms-settings-proximity", - "ms-settings-screenrotation", - "ms-settings-wifi", - "ms-settings-workplace", - "ms-spd", - "ms-stickers", - "ms-sttoverlay", - "ms-transit-to", - "ms-useractivityset", - "ms-virtualtouchpad", - "ms-visio", - "ms-walk-to", - "ms-whiteboard", - "ms-whiteboard-cmd", - "ms-word", - "msnim", - "msrp", - "msrps", - "mss", - "mt", - "mtqp", - "mumble", - "mupdate", - "mvn", - "news", - "nfs", - "ni", - "nih", - "nntp", - "notes", - "num", - "ocf", - "oid", - "onenote", - "onenote-cmd", - "opaquelocktoken", - "openid", - "openpgp4fpr", - "otpauth", - "p1", - "pack", - "palm", - "paparazzi", - "payment", - "payto", - "pkcs11", - "platform", - "pop", - "pres", - "prospero", - "proxy", - "pwid", - "psyc", - "pttp", - "qb", - "query", - "quic-transport", - "redis", - "rediss", - "reload", - "res", - "resource", - "rmi", - "rsync", - "rtmfp", - "rtmp", - "rtsp", - "rtsps", - "rtspu", - "sarif", - "secondlife", - "secret-token", - "service", - "session", - "sftp", - "sgn", - "shc", - "shttp (OBSOLETE)", - "sieve", - "simpleledger", - "simplex", - "sip", - "sips", - "skype", - "smb", - "smp", - "sms", - "smtp", - "snews", - "snmp", - "soap.beep", - "soap.beeps", - "soldat", - "spiffe", - "spotify", - "ssb", - "ssh", - "starknet", - "steam", - "stun", - "stuns", - "submit", - "svn", - "swh", - "swid", - "swidpath", - "tag", - "taler", - "teamspeak", - "tel", - "teliaeid", - "telnet", - "tftp", - "things", - "thismessage", - "tip", - "tn3270", - "tool", - "turn", - "turns", - "tv", - "udp", - "unreal", - "upt", - "urn", - "ut2004", - "uuid-in-package", - "v-event", - "vemmi", - "ventrilo", - "ves", - "videotex", - "vnc", - "view-source", - "vscode", - "vscode-insiders", - "vsls", - "w3", - "wais", - "web3", - "wcr", - "webcal", - "web+ap", - "wifi", - "wpid", - "ws", - "wss", - "wtai", - "wyciwyg", - "xcon", - "xcon-userid", - "xfire", - "xmlrpc.beep", - "xmlrpc.beeps", - "xmpp", - "xri", - "ymsgr", - "z39.50", - "z39.50r", - "z39.50s", -] # pragma: no cover diff --git a/src/documents/validators.py b/src/documents/validators.py new file mode 100644 index 000000000..c11c88523 --- /dev/null +++ b/src/documents/validators.py @@ -0,0 +1,29 @@ +from urllib.parse import urlparse + +from django.core.exceptions import ValidationError +from django.utils.translation import gettext_lazy as _ + + +def uri_validator(value) -> None: + """ + Raises a ValidationError if the given value does not parse as an + URI looking thing, which we're defining as a scheme and either network + location or path value + """ + try: + parts = urlparse(value) + if not parts.scheme: + raise ValidationError( + _(f"Unable to parse URI {value}, missing scheme"), + params={"value": value}, + ) + elif not parts.netloc and not parts.path: + raise ValidationError( + _(f"Unable to parse URI {value}, missing net loction or path"), + params={"value": value}, + ) + except Exception as e: + raise ValidationError( + _(f"Unable to parse URI {value}"), + params={"value": value}, + ) from e