Repository: unt-libraries/django-invite

Issue:
Normalize emails everywhere.
@htarver reported an issue where duplicate invitations were accidentally sent because submitted emails are not being normalized properly _all_ the time. I suggest we clean the emails submitted in the forms by lower-casing and stripping them and then make sure we do the same on any emails being submitted by users for comparisons. Changes would need to be made in both the [forms](https://github.com/unt-libraries/django-invite/blob/master/invite/forms.py) and [views](https://github.com/unt-libraries/django-invite/blob/master/invite/views.py).

Discussion:

Patch:
diff --git a/README.md b/README.md
index 8f063e2..fbb233a 100644
--- a/README.md
+++ b/README.md
@@ -71,3 +71,4 @@ Contributors:
 - [Lauren Ko](http://github.com/ldko)
 - [Gio Gottardi](http://github.com/somexpert)
 - [Madhulika Bayyavarapu](http://github.com/madhulika95b)
+- [Trey Clark](http://github.com/clarktr1)
\ No newline at end of file
diff --git a/invite/forms.py b/invite/forms.py
index 9f1e78d..22a1d93 100644
--- a/invite/forms.py
+++ b/invite/forms.py
@@ -1,28 +1,30 @@
-from invite.models import InviteItem
+from invite.models import Invitation, InviteItem
 from django import forms
 from django.contrib.auth import authenticate
 from django.contrib.auth.models import User
-from django.core import validators
 from django.core.exceptions import ValidationError
 
 
 def validate_username(value):
-    if value in User.objects.all().values_list('username', flat=True):
-        raise ValidationError('Username taken, choose another')
+    if User.objects.filter(username__iexact=value).exists():
+        raise ValidationError(f'Username {value} taken, choose another')
+
+
+def validate_username_invitation(value):
+    if Invitation.objects.filter(username__iexact=value).exists():
+        raise ValidationError(f'Username {value} taken, choose another')
 
 
 def validate_user_email(value):
-    insensitive_emails = (
-        [e.lower() for e in User.objects.all().values_list('email', flat=True)]
-    )
-    assert False, insensitive_emails
-    if value.lower() not in insensitive_emails:
+    if not User.objects.filter(email__iexact=value).exists():
         raise ValidationError('The email provided doesn\'t belong to any user')
 
 
-def validate_user_email_exists(value):
-    if value in User.objects.all().values_list('email', flat=True):
-        raise ValidationError('The email provided already belongs to a user')
+def validate_new_email(value):
+    if User.objects.filter(email__iexact=value).exists():
+        raise ValidationError(f'{value} already belongs to a user')
+    if Invitation.objects.filter(email__iexact=value).exists():
+        raise ValidationError(f'{value} already belongs to a pending invitation')
 
 
 class SignupForm(forms.Form):
@@ -116,12 +118,41 @@ class SignupForm(forms.Form):
         return self.cleaned_data
 
 
+class BaseInviteItemFormSet(forms.BaseFormSet):
+    def clean(self):
+        """Validate that there are no duplicate email addresses across forms."""
+        super().clean()
+        emails = []
+        users = []
+        errors = []
+
+        for form in self.forms:
+            email = form.cleaned_data.get('email')
+            user = form.cleaned_data.get('username')
+            if email:
+                if email in emails:
+                    errors.append(ValidationError(
+                            f'Email: {email} is duplicated within this form',
+                            code='duplicate'))
+                else:
+                    emails.append(email)
+            if user:
+                if user in users:
+                    errors.append(ValidationError(
+                            f'Username: {user} is duplicated within this form',
+                            code='duplicate'))
+                else:
+                    users.append(user)
+        if errors:
+            raise ValidationError(errors)
+
+
 class InviteItemForm(forms.ModelForm):
     # Construct group choices list because many to many fields do not have
     # an order
 
     username = forms.CharField(
-        validators=[validate_username],
+        validators=[validate_username, validate_username_invitation],
         max_length=30,
         widget=forms.TextInput(
             attrs={
@@ -133,7 +164,7 @@ class InviteItemForm(forms.ModelForm):
         ),
     )
     email = forms.EmailField(
-        validators=[validators.validate_email, validate_user_email_exists],
+        validators=[validate_new_email],
         widget=forms.TextInput(
             attrs={
                 'onkeydown': 'if (event.keyCode == 13) { this.form.submit(); return false; }',
@@ -144,6 +175,12 @@ class InviteItemForm(forms.ModelForm):
         ),
     )
 
+    def clean_email(self):
+        email = self.cleaned_data.get('email')
+        if email:
+            email = email.strip().lower()
+        return email
+
     class Meta:
         model = InviteItem
         fields = '__all__'
@@ -225,7 +262,7 @@ class LoginForm(forms.Form):
 
 class IForgotForm(forms.Form):
     email = forms.EmailField(
-        validators=[validators.validate_email],
+        validators=[validate_user_email],
         widget=forms.TextInput(
             attrs={
                 'class': 'form-control',
@@ -235,17 +272,6 @@ class IForgotForm(forms.Form):
         ),
     )
 
-    def clean_email(self):
-        insensitive_emails = [e.lower() for e in User.objects.all().values_list('email', flat=True)]  # noqa
-        if self.data['email'].lower() not in insensitive_emails:
-            raise ValidationError(
-                'The email provided doesn\'t belong to any user')
-        return self.data['email']
-
-    def clean(self, *args, **kwargs):
-        self.clean_email()
-        return self.cleaned_data
-
 
 class ResetForm(forms.Form):
     password = forms.CharField(
diff --git a/invite/templates/invite/invite.html b/invite/templates/invite/invite.html
index 404371b..1c7b059 100644
--- a/invite/templates/invite/invite.html
+++ b/invite/templates/invite/invite.html
@@ -151,7 +151,7 @@
             var $row = $input.closest(".control-group");
             var type = $input.attr("placeholder").toLowerCase();
             var usernameError = $("<li></li>").text("Username: Username taken, choose another");
-            var emailError = $("<li></li>").text("Email: The email provided already belongs to a user");
+            var emailError = $("<li></li>").text(`Email: ${$input.val()} already belongs to a User or pending invite `);
             var typeToError = {"email":emailError, "username":usernameError};
             var error = typeToError[type];
             xhttp.onreadystatechange = function() {
@@ -165,18 +165,21 @@
                             disableSubmit();
                         }
                         // Add the error to the error list, if it isn't there already
-                        if ($row.prev().find(":contains('" + error.text() + "')").length === 0) {
+                        if ($row.prev().find(":contains('Email:'), :contains('Username:')").length === 0) {
                             $row.prev().append(error);
                         }
                     } else {
-                        // Remove the previous error/s, if it exists
+                        // Remove previous errors, if they exist
                         $row.prev()
                             .children()
-                            .filter(function() {return $(this).text().toLowerCase().indexOf(type) > -1;})
+                            .filter(function () {
+                                const text = $(this).text().toLowerCase();
+                                return text.indexOf("email") > -1 || text.indexOf("username:") > -1;
+                            })
                             .remove();
-                        // Remove error div, if it contains no more errors
+                        // Remove error div if it contains no more errors
                         $row.prev("div.alert-error:not(:has(li))").remove();
-                        // Enable submit button if all errors are gone from all rows
+                        // Enable submit button if no errors are present in all rows
                         if ($("div.alert-error").length === 0) {
                             enableSubmit();
                         }
@@ -197,6 +200,15 @@
 {% block content %}
     {% if perms.invite.add_invitation %}
         <h3>Whom would you like to invite, {{ user|title }}?</h3>
+        {% if errors %}
+        <div class="alert" style="background-color: #f2dede; color: #b94a48; border-color: #eed3d7;">
+            <ul>
+                {% for error in errors %}
+                        <li>{{ error }}</li>
+                {% endfor %}
+            </ul>
+        </div>
+        {% endif %}
         <form id='invite' class="navbar-form form-horizontal" method="POST">{% csrf_token %}
             {{ invite_item_formset.management_form }}
             <div class='well well-small' id="invitees">
@@ -271,4 +283,4 @@
     </h3>
     {% endif %}
 
-{% endblock %}
+{% endblock %}
\ No newline at end of file
diff --git a/invite/views.py b/invite/views.py
index 0a775af..0499f43 100644
--- a/invite/views.py
+++ b/invite/views.py
@@ -1,6 +1,6 @@
 from django.shortcuts import render
 from django.urls import reverse, reverse_lazy
-from django.forms.formsets import formset_factory, BaseFormSet
+from django.forms.formsets import formset_factory
 from django.http import HttpResponseRedirect, JsonResponse, HttpResponseServerError
 from django.contrib.auth import authenticate, login
 from django.contrib.auth.decorators import login_required, permission_required
@@ -288,11 +288,12 @@ def revoke(request, code):
 def invite(request):
     InviteItemFormSet = formset_factory(
         forms.InviteItemForm,
-        formset=BaseFormSet,
+        formset=forms.BaseInviteItemFormSet,
     )
     if request.method == 'POST':
         # Create a formset from the submitted data
         invite_item_formset = InviteItemFormSet(request.POST, request.FILES)
+
         if invite_item_formset.is_valid():
             for form in invite_item_formset.forms:
                 try:
@@ -338,6 +339,15 @@ def invite(request):
                     )
                 i.save()
             return HttpResponseRedirect(reverse('invite:index'))
+        else:
+            return render(
+                request,
+                'invite/invite.html',
+                {
+                    'invite_item_formset': invite_item_formset,
+                    'errors': invite_item_formset.non_form_errors(),
+                }
+            )
     else:
         invite_item_formset = InviteItemFormSet()
     return render(
@@ -439,9 +449,14 @@ def check(request):
     elif request.GET.get('email', None):
         try:
             User.objects.get(email__iexact=request.GET['email'].strip())
-            result = True
+            user_result = True
         except User.DoesNotExist:
-            result = False
+            user_result = False
+        if Invitation.objects.filter(email__iexact=request.GET['email'].strip()).exists():
+            invitation_result = True
+        else:
+            invitation_result = False
+        result = user_result or invitation_result
     else:
         result = False
     return JsonResponse({'taken': result})


Test patch:
diff --git a/tests/test_all.py b/tests/test_all.py
index c5b1577..2f6e74d 100644
--- a/tests/test_all.py
+++ b/tests/test_all.py
@@ -21,7 +21,7 @@ settings.SITE_ID = 1
 class TestOperations(TestCase):
 
     def test_invite_creation(self):
-        """Creates an invite and check to see if we can send the email"""
+        """Creates an invite and check to see if we can send the email."""
         i = Invitation.objects.create(
             email='joeyliechty@supergreatmail.com',
         )
@@ -33,7 +33,7 @@ class TestOperations(TestCase):
         """
         Since we trust the django mailer, and we don't want to do time wasting
         tests that do actual network activity, we'll use mock to ensure that
-        the mailer is called with the correct arguments by the model
+        the mailer is called with the correct arguments by the model.
         """
         i = Invitation.objects.create(
             email='1234testemail@noone.ghost',
@@ -65,7 +65,7 @@ class TestOperations(TestCase):
         """
         Since we trust the django mailer, and we don't want to do time wasting
         tests that do actual network activity, we'll use mock to ensure that
-        the mailer is called with the correct arguments by the model
+        the mailer is called with the correct arguments by the model.
         """
         i = PasswordResetInvitation.objects.create(
             email='1234testemail@noone.ghost',
@@ -344,7 +344,8 @@ class TestViews(TestCase):
         )
         self.assertIn('Email exists on other user', response.content.decode())
 
-    def test_invite_submit_same_email(self):
+    def test_invite_user_duplicate_email(self):
+        """Test for Invitation email matching current User email."""
         self.client.login(username='superuser', password='superuser')
         response = self.client.post(
             reverse('invite:invite'),
@@ -362,6 +363,62 @@ class TestViews(TestCase):
         self.assertIn('already belongs to a user', response.content.decode())
         self.assertEqual(len(mail.outbox), 0)
 
+    def test_invite_invitation_duplicate_email_and_username(self):
+        """"Test for duplicate emails and usernames in pending Invitations."""
+        self.client.login(username='superuser', password='superuser')
+        self.client.post(
+            reverse('invite:invite'),
+            {
+                'form-MAX_NUM_FORMS': [''],
+                'form-INITIAL_FORMS': ['0'],
+                'form-TOTAL_FORMS': ['1'],
+                'form-0-username': ['bobby1'],
+                'form-0-email': ['test1@test.com'],
+                'form-0-last_name': ['test1'],
+                'form-0-first_name': ['test1'],
+                'form-0-greeting': [''],
+            }
+        )
+        response = self.client.post(
+            reverse('invite:invite'),
+            {
+                'form-MAX_NUM_FORMS': [''],
+                'form-INITIAL_FORMS': ['0'],
+                'form-TOTAL_FORMS': ['1'],
+                'form-0-username': ['bobby1'],
+                'form-0-email': ['test1@test.com'],
+                'form-0-last_name': ['test1'],
+                'form-0-first_name': ['test1'],
+                'form-0-greeting': ['']
+            },
+        )
+        self.assertIn('test1@test.com already belongs', response.content.decode())
+        self.assertIn('Username bobby1 taken, choose another', response.content.decode())
+
+    def test_invitation_duplicates_and_clean(self):
+        """"Test for duplicate email/username in InviteForm and clean_email method."""
+        self.client.login(username='superuser', password='superuser')
+        response = self.client.post(
+            reverse('invite:invite'),
+            {
+                'form-MAX_NUM_FORMS': [''],
+                'form-INITIAL_FORMS': ['0'],
+                'form-TOTAL_FORMS': ['2'],
+                'form-0-username': ['test1'],
+                'form-0-email': ['EMAIL@EMAIL.com'],
+                'form-0-last_name': ['test1'],
+                'form-0-first_name': ['test1'],
+                'form-0-greeting': [''],
+                'form-1-username': ['test1'],
+                'form-1-email': ['email@email.com'],
+                'form-1-last_name': ['test2'],
+                'form-1-first_name': ['test2'],
+                'form-1-greeting': [''],
+            }
+        )
+        self.assertIn('Email: email@email.com is duplicated', response.content.decode())
+        self.assertIn('Username: test1 is duplicated', response.content.decode())
+
     def test_reset_submit(self):
         psi = PasswordResetInvitation.objects.create(
             email='normal@normal.normal',
@@ -455,7 +512,7 @@ class TestViews(TestCase):
         self.assertEqual(len(mail.outbox), 0)
 
     def test_forgotten_password(self):
-        """User forgets his password test"""
+        """User forgets their password test."""
         response = self.client.post(
             reverse('invite:amnesia'),
             {'email': 'normal@normal.normal'},
@@ -686,6 +743,26 @@ class TestViews(TestCase):
         response = self.client.get(reverse('invite:check'))
         self.assertFalse(json.loads(response.content)['taken'])
 
+    def test_check_invite_duplicate_exists(self):
+        self.client.login(username='superuser', password='superuser')
+        self.client.post(
+            reverse('invite:invite'),
+            {
+                'form-MAX_NUM_FORMS': [''],
+                'form-INITIAL_FORMS': ['0'],
+                'form-TOTAL_FORMS': ['1'],
+                'form-0-username': ['bobby'],
+                'form-0-email': ['test1@test.com'],
+                'form-0-last_name': ['test1'],
+                'form-0-first_name': ['test1'],
+                'form-0-greeting': [''],
+            }
+        )
+        response = self.client.get(
+            reverse('invite:check'), {'email': 'test1@test.com'}
+        )
+        self.assertTrue(json.loads(response.content)['taken'])
+
 
 if __name__ == '__main__':
     unittest.main()