feat: validate the document notify field (#4856)

* feat: validate the document notify field

* chore: fix spelling in comment

* feat: be more explicit in notify validation, preserving input order.

* fix: restrict notify duplicate check to nameaddrs. Add tests.

ietf/doc/forms.py

@@ -6,6 +6,7 @@ import datetime
 import debug #pyflakes:ignore
 from django import forms
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
+from django.core.validators import validate_email
 
 from ietf.doc.fields import SearchableDocAliasesField, SearchableDocAliasField
 from ietf.doc.models import RelatedDocument, DocExtResource
@@ -84,8 +85,42 @@ class NotifyForm(forms.Form):
     )
 
     def clean_notify(self):
-        addrspecs = [x.strip() for x in self.cleaned_data["notify"].split(',')]
+        # As long as the widget is a Textarea, users will separate addresses with newlines, whether that matches the instructions or not
-        return ', '.join(addrspecs)
+        # We have been allowing nameaddrs for a long time (there are many Documents with namaddrs in their notify field)
+        # python set doesn't preserve order, so in an attempt to mostly preserve the order of what was entered, we'll use
+        # a dict (whose keys are guaranteed to be ordered) to cull out duplicates
+
+        nameaddrs=dict()
+        duplicate_addrspecs = set()
+        bad_nameaddrs = []
+        for nameaddr in self.cleaned_data["notify"].replace("\n", ",").split(","):
+            stripped = nameaddr.strip()
+            if stripped == "":
+                continue
+            if "<" in stripped:
+                if stripped[-1] != ">":
+                    bad_nameaddrs.append(nameaddr)
+                    continue
+                addrspec = stripped[stripped.find("<")+1:-1]
+            else:
+                addrspec = stripped
+            try:
+                validate_email(addrspec)
+            except ValidationError:
+                bad_nameaddrs.append(nameaddr)
+            if addrspec in nameaddrs:
+                duplicate_addrspecs.add(addrspec)
+                continue
+            else:
+                nameaddrs[addrspec] = stripped
+        error_messages = []
+        if len(duplicate_addrspecs) != 0:
+            error_messages.append(f'Duplicate addresses: {", ".join(duplicate_addrspecs)}')
+        if len(bad_nameaddrs) != 0:
+            error_messages.append(f'Invalid addresses: {", ".join(bad_nameaddrs)}')
+        if len(error_messages) != 0:
+            raise ValidationError(" and ".join(error_messages))
+        return ", ".join(nameaddrs.values())
 
 class ActionHoldersForm(forms.Form):
     action_holders = SearchablePersonsField(required=False)

ietf/doc/tests.py

@@ -41,6 +41,7 @@ from ietf.doc.factories import ( DocumentFactory, DocEventFactory, CharterFactor
     IndividualRfcFactory, StateDocEventFactory, BallotPositionDocEventFactory, 
     BallotDocEventFactory, DocumentAuthorFactory, NewRevisionDocEventFactory,
     StatusChangeFactory, BofreqFactory)
+from ietf.doc.forms import NotifyForm
 from ietf.doc.fields import SearchableDocumentsField
 from ietf.doc.utils import create_ballot_if_not_open, uppercase_std_abbreviated_name
 from ietf.doc.views_search import ad_dashboard_group, ad_dashboard_group_type, shorten_group_name # TODO: red flag that we're importing from views in tests. Move these to utils.
@@ -2926,3 +2927,43 @@ class PdfizedTests(TestCase):
             for ext in ('pdf','txt','html','anythingatall'):
                 self.should_succeed(dict(name=rfc.name,rev=f'{r:02d}',ext=ext))
         self.should_404(dict(name=rfc.name,rev='02'))
+
+class NotifyValidationTests(TestCase):
+    def test_notify_validation(self):
+        valid_values = [
+            "foo@example.com, bar@example.com",
+            "Foo Bar <foobar@example.com>, baz@example.com",
+            "foo@example.com, ,bar@example.com,", # We're ignoring extra commas
+            "foo@example.com\nbar@example.com", # Yes, we're quietly accepting a newline as a comma
+        ]
+        bad_nameaddr_values = [
+            "@example.com",
+            "foo",
+            "foo@",
+            "foo bar foobar@example.com",
+        ]
+        duplicate_values = [
+            "foo@bar.com, bar@baz.com, foo@bar.com",
+            "Foo <foo@bar.com>, foobar <foo@bar.com>",
+        ]
+        both_duplicate_and_bad_values = [
+            "foo@example.com, bar@, Foo <foo@example.com>",
+            "Foo <@example.com>, Bar <@example.com>",
+        ]
+        for v in valid_values:
+            self.assertTrue(NotifyForm({"notify": v}).is_valid())
+        for v in bad_nameaddr_values:
+            f = NotifyForm({"notify": v})
+            self.assertFalse(f.is_valid())
+            self.assertTrue("Invalid addresses" in f.errors["notify"][0])
+            self.assertFalse("Duplicate addresses" in f.errors["notify"][0])
+        for v in duplicate_values:
+            f = NotifyForm({"notify": v})
+            self.assertFalse(f.is_valid())
+            self.assertFalse("Invalid addresses" in f.errors["notify"][0])
+            self.assertTrue("Duplicate addresses" in f.errors["notify"][0])
+        for v in both_duplicate_and_bad_values:
+            f = NotifyForm({"notify": v})
+            self.assertFalse(f.is_valid())
+            self.assertTrue("Invalid addresses" in f.errors["notify"][0])
+            self.assertTrue("Duplicate addresses" in f.errors["notify"][0])