Merge lp:~maxiberta/canonical-identity-provider/max-emails-per-account into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Prevent adding new emails to addresses that have reached a certain threshold.

Prevent adding new emails to addresses that have reached a certain threshold (settings.MAX_EMAILS_PER_ACCOUNT, default 30, based on the current max # of emails in all accounts in production); and show a warning in email management views. E.g. (with MAX_EMAILS_PER_ACCOUNT=3):

https://screenshots.firefox.com/cUqIwUu0mMpDKcQn/10.51.30.184
https://screenshots.firefox.com/F4rYwCKpZU5hQice/10.51.30.184

There's no API to add emails to an account afaict.

[Daniel Manrique (roadmr)]

LGTM, thanks!

A couple of suggestions below.

review: Approve

[Daniel Manrique (roadmr)]

Thanks! Let's merge!

review: Approve

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/138/

=== modified file 'django_project/settings_base.py'
--- django_project/settings_base.py	2018-03-06 08:47:52 +0000
+++ django_project/settings_base.py	2018-05-09 20:50:38 +0000
@@ -378,6 +378,7 @@
 MACAROON_TTL = 365 * 24 * 3600  # seconds
 MACAROON_SERVICE_LOCATION = HOSTNAME
 MANAGERS = []
+MAX_EMAILS_PER_ACCOUNT = 30
 MAX_FAILED_LOGIN_ATTEMPTS = 20
 MAX_PASSWORD_RESET_TOKENS = 5
 MEDIA_ROOT = ''

=== modified file 'src/identityprovider/forms.py'
--- src/identityprovider/forms.py	2018-02-08 16:34:57 +0000
+++ src/identityprovider/forms.py	2018-05-09 20:50:38 +0000
@@ -5,6 +5,7 @@
 import re
 
 from django import forms
+from django.conf import settings
 from django.core.urlresolvers import reverse
 from django.forms import Form, fields, widgets
 from django.utils import translation
@@ -373,6 +374,12 @@
         data = self.cleaned_data['newemail']
         try:
             if self.account is not None:
+                if (self.account.emailaddress_set.count() >=
+                        settings.MAX_EMAILS_PER_ACCOUNT):
+                    raise forms.ValidationError(_(
+                        'Account reached the maximum allowed number of email '
+                        'addresses (%d).') % settings.MAX_EMAILS_PER_ACCOUNT)
+
                 # check email was not previously invalidated for this account
                 invalidated = self.account.invalidatedemailaddress_set.filter(
                     email__iexact=data)
@@ -382,13 +389,9 @@
                     ))
             email = EmailAddress.objects.get(email__iexact=data)
             if email.status != EmailStatus.NEW:
-                raise forms.ValidationError(_(
-                    "Please verify your email."
-                ))
+                raise forms.ValidationError(_("Please verify your email."))
             else:
-                raise forms.ValidationError(_(
-                    "Email already registered."
-                ))
+                raise forms.ValidationError(_("Email already registered."))
         except EmailAddress.DoesNotExist:
             return data
         return data

=== modified file 'src/identityprovider/tests/test_forms.py'
--- src/identityprovider/tests/test_forms.py	2018-02-16 13:29:05 +0000
+++ src/identityprovider/tests/test_forms.py	2018-05-09 20:50:38 +0000
@@ -5,6 +5,7 @@
 import urllib2
 
 from django import forms
+from django.conf import settings
 from django.http import HttpRequest
 from django.test.utils import override_settings
 from django.utils.timezone import now
@@ -770,6 +771,23 @@
         self.assertEqual(['Email already registered.'],
                          form.errors['newemail'])
 
+    @override_settings(MAX_EMAILS_PER_ACCOUNT=5)
+    def test_max_emails_per_account(self):
+        account = self.factory.make_account()
+
+        for _ in range(settings.MAX_EMAILS_PER_ACCOUNT - 1):
+            email = self.factory.make_email_address()
+            form = NewEmailForm(dict(newemail=email), account=account)
+            self.assertTrue(form.is_valid())
+            self.factory.make_email_for_account(account,
+                                                status=EmailStatus.NEW)
+        self.assertEqual(
+            settings.MAX_EMAILS_PER_ACCOUNT, account.emails().count())
+
+        email = self.factory.make_email_address()
+        form = NewEmailForm({'newemail': email}, account=account)
+        self.assertFalse(form.is_valid())
+
 
 class UserAttribsRequestFormTest(SSOBaseTestCase):
 

=== modified file 'src/webui/constants.py'
--- src/webui/constants.py	2015-05-08 17:24:05 +0000
+++ src/webui/constants.py	2018-05-09 20:50:38 +0000
@@ -3,3 +3,7 @@
 
 EMAIL_EXISTS_ERROR = _('An Ubuntu One account already exists with this email.')
 VERIFY_EMAIL_MESSAGE = _('Please verify your email.')
+MAX_EMAILS_PER_ACCOUNT_EXCEEDED = _(
+    "You have reached the maximum allowed number of email addresses per "
+    "account (%d). Delete some of those email addresses to be able to add new "
+    "ones.")

=== modified file 'src/webui/tests/test_views_account.py'
--- src/webui/tests/test_views_account.py	2018-04-04 15:34:30 +0000
+++ src/webui/tests/test_views_account.py	2018-05-09 20:50:38 +0000
@@ -41,7 +41,7 @@
 )
 from identityprovider.views.testing import MockLaunchpad
 from webui import decorators
-from webui.constants import EMAIL_EXISTS_ERROR
+from webui.constants import EMAIL_EXISTS_ERROR, MAX_EMAILS_PER_ACCOUNT_EXCEEDED
 
 
 class AccountEmailsViewTestCase(AuthenticatedTestCase):
@@ -168,6 +168,27 @@
                                              token=ctx['token'])
         self.assert_add_email_action(response, token=ctx['token'])
 
+    @override_settings(MAX_EMAILS_PER_ACCOUNT=5)
+    def test_get_with_max_emails_per_account_exceeded(self):
+        for _ in range(settings.MAX_EMAILS_PER_ACCOUNT - 1):
+            r = self.client.get(reverse('account-emails'))
+            self.assertEqual(r.status_code, 200)
+            self.assertNotIn(
+                (MAX_EMAILS_PER_ACCOUNT_EXCEEDED %
+                    settings.MAX_EMAILS_PER_ACCOUNT),
+                [m.message for m in r.context['messages']])
+            email = self.factory.make_email_address()
+            self.factory.make_email_for_account(self.account, email=email)
+
+        self.assertEqual(
+            settings.MAX_EMAILS_PER_ACCOUNT, self.account.emails().count())
+
+        r = self.client.get(reverse('account-emails'))
+        self.assertEqual(r.status_code, 200)
+        self.assertIn(
+            MAX_EMAILS_PER_ACCOUNT_EXCEEDED % settings.MAX_EMAILS_PER_ACCOUNT,
+            [m.message for m in r.context['messages']])
+
 
 class AccountViewsUnauthenticatedTestCase(SSOBaseTestCase):
 
@@ -550,6 +571,52 @@
         self.assertIn('newemail', response.context['form'].errors)
         self.assertIn(unicode(EMAIL_EXISTS_ERROR), response.content)
 
+    @override_settings(MAX_EMAILS_PER_ACCOUNT=5)
+    def test_new_email_get_with_max_emails_per_account_exceeded(self):
+        for _ in range(settings.MAX_EMAILS_PER_ACCOUNT - 1):
+            r = self.client.get(reverse('new_email'))
+            self.assertEqual(r.status_code, 200)
+            self.assertNotIn(
+                (MAX_EMAILS_PER_ACCOUNT_EXCEEDED %
+                    settings.MAX_EMAILS_PER_ACCOUNT),
+                [m.message for m in r.context['messages']])
+            email = self.factory.make_email_address()
+            self.factory.make_email_for_account(self.account, email=email)
+
+        self.assertEqual(
+            settings.MAX_EMAILS_PER_ACCOUNT, self.account.emails().count())
+
+        r = self.client.get(reverse('new_email'))
+        self.assertEqual(r.status_code, 200)
+        self.assertIn(
+            MAX_EMAILS_PER_ACCOUNT_EXCEEDED % settings.MAX_EMAILS_PER_ACCOUNT,
+            [m.message for m in r.context['messages']])
+
+    @override_settings(MAX_EMAILS_PER_ACCOUNT=5)
+    def test_new_email_post_with_max_emails_per_account_exceeded(self):
+        for _ in range(settings.MAX_EMAILS_PER_ACCOUNT - 1):
+            email = self.factory.make_email_address()
+            r = self.client.post(reverse('new_email'), {'newemail': email})
+            self.assertEqual(r.status_code, 200)
+            self.assertNotIn(
+                (MAX_EMAILS_PER_ACCOUNT_EXCEEDED %
+                    settings.MAX_EMAILS_PER_ACCOUNT),
+                [m.message for m in r.context['messages']])
+
+        self.assertEqual(
+            settings.MAX_EMAILS_PER_ACCOUNT, self.account.emails().count())
+
+        email = self.factory.make_email_address()
+        r = self.client.post(reverse('new_email'), {'newemail': email})
+        self.assertEqual(r.status_code, 200)
+        self.assertFalse(r.context['form'].is_valid())
+        self.assertIn(
+            MAX_EMAILS_PER_ACCOUNT_EXCEEDED % settings.MAX_EMAILS_PER_ACCOUNT,
+            [m.message for m in r.context['messages']])
+        # No new email was added to the account.
+        self.assertEqual(
+            settings.MAX_EMAILS_PER_ACCOUNT, self.account.emails().count())
+
     def test_delete_account_link(self):
         response = self.client.get('/')
         self.assertContains(response, '/+delete')

=== modified file 'src/webui/views/account.py'
--- src/webui/views/account.py	2018-04-04 15:34:30 +0000
+++ src/webui/views/account.py	2018-05-09 20:50:38 +0000
@@ -58,7 +58,11 @@
     require_testing_enabled,
 )
 
-from webui.constants import EMAIL_EXISTS_ERROR, VERIFY_EMAIL_MESSAGE
+from webui.constants import (
+    EMAIL_EXISTS_ERROR,
+    MAX_EMAILS_PER_ACCOUNT_EXCEEDED,
+    VERIFY_EMAIL_MESSAGE,
+)
 from webui.decorators import check_readonly, sso_login_required
 from webui.forms import (
     ClaimSSHKeyForm,
@@ -253,6 +257,12 @@
     form = NewEmailForm()
     verified_emails = account.verified_emails()
     unverified_emails = account.unverified_emails()
+
+    if account.emailaddress_set.count() >= settings.MAX_EMAILS_PER_ACCOUNT:
+        messages.error(
+            request,
+            MAX_EMAILS_PER_ACCOUNT_EXCEEDED % settings.MAX_EMAILS_PER_ACCOUNT)
+
     context = {
         'current_section': 'emails',
         'account': account,
@@ -328,6 +338,12 @@
             if err == VERIFY_EMAIL_MESSAGE:
                 messages.error(request, EMAIL_EXISTS_ERROR)
 
+    if (request.user.emailaddress_set.count() >=
+            settings.MAX_EMAILS_PER_ACCOUNT):
+        messages.error(
+            request,
+            MAX_EMAILS_PER_ACCOUNT_EXCEEDED % settings.MAX_EMAILS_PER_ACCOUNT)
+
     context = {
         'form': form,
         'token': token,