Merge lp:~matiasb/canonical-identity-provider/prevent-preferring-unvalidated-email into lp:canonical-identity-provider/release

[Matias Bordese]

Updated checks and validation for users and (un)verified email addresses:
  - Prevent user changing preferred email address to an unverified email address.
  - Prevent password reset for users without a verified email address.

Updated checks and validation for users and (un)verified email addresses:
  - Prevent user changing preferred email address to an unverified email address.
  - Prevent password reset for users without a verified email address.

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

=== modified file 'identityprovider/forms.py'
--- identityprovider/forms.py	2012-12-18 18:37:03 +0000
+++ identityprovider/forms.py	2013-01-25 22:16:21 +0000
@@ -154,12 +154,6 @@
     displayname = fields.CharField(
         error_messages=default_errors,
         widget=ROAwareTextInput(attrs={'class': 'textType', 'size': '20'}))
-    preferred_email = PreferredEmailField(
-        error_messages=email_errors,
-        queryset=EmailAddress.objects.none(),
-        widget=ROAwareSelect,
-        empty_label=None,
-    )
     password = fields.CharField(
         required=False,
         help_text=PASSWORD_POLICY_HELP_TEXT,
@@ -193,16 +187,21 @@
         else:
             preferredemail_id = self.instance.preferredemail.id
 
-        # Override the field to set initial value (current preferred email)
-        emails = EmailAddress.objects.filter(account=self.instance)
-        self.fields['preferred_email'] = PreferredEmailField(
-            queryset=emails.exclude(status=EmailStatus.NEW).order_by('-status',
-                                                                     'email'),
-            initial=preferredemail_id,
-            widget=ROAwareSelect,
-            error_messages=email_errors,
-            empty_label=None
-        )
+        # check for emails that can be preferred
+        validated_emails = self.instance.verified_emails()
+
+        if validated_emails.count() > 0:
+            # add and display a dropdown with the valid choices
+            self.fields['preferred_email'] = PreferredEmailField(
+                queryset=validated_emails.order_by('email'),
+                initial=preferredemail_id,
+                widget=ROAwareSelect,
+                error_messages=email_errors,
+                empty_label=None,
+                help_text=_(
+                    'Only verified email addresses are listed. '
+                    'You can add and verify emails through the link below.'),
+            )
 
     class Meta:
         fields = ('displayname', 'warn_about_backup_device',
@@ -257,7 +256,8 @@
             new_password = encrypt_launchpad_password(password)
             self.instance.accountpassword.password = new_password
             self.instance.accountpassword.save()
-        self.instance.preferredemail = self.cleaned_data['preferred_email']
+        if 'preferred_email' in self.cleaned_data:
+            self.instance.preferredemail = self.cleaned_data['preferred_email']
 
         super(EditAccountForm, self).save()
 

=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2013-01-22 18:28:03 +0000
+++ identityprovider/models/account.py	2013-01-25 22:16:21 +0000
@@ -225,6 +225,8 @@
         return self._preferredemail
 
     def _set_preferredemail(self, email):
+        if not email.is_verified():
+            raise ValidationError('Email must be verified')
         current = self.preferredemail
         if current is not None:
             current.status = EmailStatus.VALIDATED
@@ -298,7 +300,8 @@
 
     @property
     def can_reset_password(self):
-        return (self.is_active or self.can_reactivate)
+        return ((self.is_active or self.can_reactivate) and
+                self.verified_emails().count() > 0)
 
     @property
     def is_active(self):

=== modified file 'identityprovider/models/emailaddress.py'
--- identityprovider/models/emailaddress.py	2013-01-22 18:28:03 +0000
+++ identityprovider/models/emailaddress.py	2013-01-25 22:16:21 +0000
@@ -39,6 +39,9 @@
     def is_preferred(self):
         return self.status == EmailStatus.PREFERRED
 
+    def is_verified(self):
+        return self.status in (EmailStatus.VALIDATED, EmailStatus.PREFERRED)
+
     def account_admin_link(self):
         if self.account is None:
             return "None"

=== modified file 'identityprovider/tests/unit/test_forms.py'
--- identityprovider/tests/unit/test_forms.py	2012-12-17 16:27:17 +0000
+++ identityprovider/tests/unit/test_forms.py	2013-01-25 22:16:21 +0000
@@ -9,6 +9,7 @@
 from openid.extensions.sreg import SRegRequest
 
 from identityprovider.models.account import Account
+from identityprovider.models.const import EmailStatus
 from identityprovider.forms import (
     DeviceRenameForm,
     EditAccountForm,
@@ -49,6 +50,40 @@
 
         self.assertEqual(form.initial.get('displayname', ''), "Sample Person")
 
+    def test_account_without_validated_emails_no_field(self):
+        self.account.emailaddress_set.update(status=EmailStatus.NEW)
+        form = EditAccountForm(instance=self.account)
+        self.assertNotIn('preferred_email', form.fields)
+
+    def test_account_with_validated_emails_editable_field(self):
+        form = EditAccountForm(instance=self.account)
+        self.assertIn('preferred_email', form.fields)
+        choices = form.fields['preferred_email'].queryset.all()
+        valid_choices = self.account.verified_emails()
+        self.assertEqual(choices.count(), valid_choices.count())
+        for email in choices:
+            self.assertIn(email, valid_choices)
+
+    def test_account_without_validated_emails_post_preferred_email(self):
+        self.account.emailaddress_set.update(status=EmailStatus.NEW)
+        original_value = self.account.preferredemail
+        invalid_email = self.account.unverified_emails().exclude(
+            email=original_value)[0]
+        form = EditAccountForm(instance=self.account,
+                               data={'displayname': self.account.displayname,
+                                     'preferred_email': invalid_email.email})
+        self.assertTrue(form.is_valid())
+        form.save()
+        account = Account.objects.get(id=self.account.id)
+        self.assertEqual(account.preferredemail, original_value)
+
+    def test_account_with_validated_email_changing_to_unvalidate(self):
+        invalid_email = self.account.unverified_emails()[0]
+        form = EditAccountForm(instance=self.account,
+                               data={'displayname': self.account.displayname,
+                                     'preferred_email': invalid_email.id})
+        self.assertFalse(form.is_valid())
+
     def test_displayname_validation(self):
         form = EditAccountForm(instance=self.account,
                                data={'displayname': ' '})

=== modified file 'identityprovider/tests/unit/test_models_account.py'
--- identityprovider/tests/unit/test_models_account.py	2013-01-14 12:31:37 +0000
+++ identityprovider/tests/unit/test_models_account.py	2013-01-25 22:16:21 +0000
@@ -209,6 +209,15 @@
         account = self.factory.make_account(email_validated=False)
         self.assertIsNone(account.preferredemail)
 
+    def test_set_preferred_with_unverified_email(self):
+        account = self.factory.make_account(email_validated=False)
+        with self.assertRaises(ValidationError):
+            account.preferredemail = account.emailaddress_set.get()
+
+    def test_cant_reset_password_without_verified_email(self):
+        account = self.factory.make_account(email_validated=False)
+        self.assertFalse(account.can_reset_password)
+
     def test_validated_email_address_is_preferred_email_by_default(self):
         account = self.factory.make_account(email_validated=False)
         email_address = account.emailaddress_set.create(

=== modified file 'identityprovider/tests/unit/test_models_emailaddress.py'
--- identityprovider/tests/unit/test_models_emailaddress.py	2012-10-24 20:04:50 +0000
+++ identityprovider/tests/unit/test_models_emailaddress.py	2013-01-25 22:16:21 +0000
@@ -1,6 +1,7 @@
 from mock import patch
 
 from identityprovider.models import Account, EmailAddress
+from identityprovider.models.const import EmailStatus
 from identityprovider.tests.utils import SSOBaseTestCase
 
 
@@ -12,6 +13,15 @@
         email = EmailAddress.objects.get(email='[e-mail]')
         self.assertEqual(account, email.account)
 
+    def test_emailaddress_is_verified(self):
+        email = EmailAddress.objects.get(email='[e-mail]')
+        assert email.status == EmailStatus.PREFERRED
+        self.assertTrue(email.is_verified())
+        email.status = EmailStatus.VALIDATED
+        self.assertTrue(email.is_verified())
+        email.status = EmailStatus.NEW
+        self.assertFalse(email.is_verified())
+
     @patch('identityprovider.models.emailaddress.reverse')
     def test_account_admin_link(self, mock_reverse):
         mock_reverse.return_value = '/foo'

=== modified file 'ubuntu_sso_saml/tests.py'
--- ubuntu_sso_saml/tests.py	2013-01-15 18:20:50 +0000
+++ ubuntu_sso_saml/tests.py	2013-01-25 22:16:21 +0000
@@ -176,7 +176,7 @@
         alternate_email = '[e-mail]'
         alternate, _ = EmailAddress.objects.get_or_create(
             email=alternate_email, defaults={'account': account,
-                                             'status': EmailStatus.NEW})
+                                             'status': EmailStatus.VALIDATED})
         # and make the alternate email the preferred
         account = change_preferred_email(self.login_email, alternate_email)
         self.assertEqual(account.preferredemail, alternate)

=== modified file 'webui/templates/account/edit.html'
--- webui/templates/account/edit.html	2012-12-04 18:51:42 +0000
+++ webui/templates/account/edit.html	2013-01-25 22:16:21 +0000
@@ -95,13 +95,22 @@
               </span>
             </label>
             <br />
-            {% if form.preferred_email.errors %}
-                <span class="error">
-                    {{ form.preferred_email.errors|first }}
-                </span>
-                <br />
+            {% if form.preferred_email %}
+                {% if form.preferred_email.errors %}
+                    <span class="error">
+                        {{ form.preferred_email.errors|first }}
+                    </span>
+                    <br />
+                {% endif %}
+                {{ form.preferred_email }}
+                <span class="formHelp">{{ form.preferred_email.help_text }}</span>
+            {% else %}
+                {# Manually added, it will be ignored by the django form #}
+                {% with form.instance.preferredemail as unvalidated_preferred %}
+                    <input type="text" value="{{ unvalidated_preferred }}" disabled="true" />
+                    <span class="formHelp">You still need to <a href="{% url verify_email %}?id={{ unvalidated_preferred.id }}">verify</a> your email address.</span>
+                {% endwith %}
             {% endif %}
-            {{ form.preferred_email }}
         </p>
         {% if not embedded %}
         <p><a href="/+emails">{% trans "Manage email addresses" %}</a></p>

=== modified file 'webui/tests/test_views_account.py'
--- webui/tests/test_views_account.py	2013-01-17 20:27:23 +0000
+++ webui/tests/test_views_account.py	2013-01-25 22:16:21 +0000
@@ -414,6 +414,8 @@
     def test_deactivate_account(self):
         # make sure we have a preferredemail
         email = self.account.emailaddress_set.all()[0]
+        email.status = EmailStatus.VALIDATED
+        email.save()
         self.account.preferredemail = email
 
         # deactivate account

=== modified file 'webui/tests/test_views_ui.py'
--- webui/tests/test_views_ui.py	2013-01-22 18:28:03 +0000
+++ webui/tests/test_views_ui.py	2013-01-25 22:16:21 +0000
@@ -273,26 +273,17 @@
         self.assertContains(response, "_gaq.push(['_setAccount', '12345']);")
 
     def test_claim_token_for_password_recovery_no_preferredemail(self):
-        """ According to bug #524582 this should be handled gracefully by
-        verifying the email address and resetting the password. """
+        """Prevent password reset for unvalidated emails."""
         account = Account.objects.get_by_email('[e-mail]')
-        _preferred_email = account.preferredemail
         for email_obj in account.emailaddress_set.all():
             email_obj.status = EmailStatus.NEW
             email_obj.save()
 
-        r = self.client.post(reverse('forgot_password'),
-                             {'email': '[e-mail]'})
-
-        # reset password
-        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
-        r = self.client.post(self._token_url(), data)
-        self.assertRedirects(r, reverse('account-index'))
-
-        for email_obj in account.emailaddress_set.all():
-            email_obj.status = EmailStatus.VALIDATED
-            email_obj.save()
-        account.preferredemail = _preferred_email
+        self.client.post(reverse('forgot_password'),
+                         {'email': '[e-mail]'})
+
+        # since there is verified email address, no email was sent
+        self.assertEqual(self.mock_send_email.call_count, 0)
 
 
 class EnterTokenTestCase(UIViewsBaseTestCase):
@@ -638,8 +629,7 @@
         self.assertRedirects(r, reverse('account-index'))
 
     def test_reset_password_when_account_deactivated(self):
-        """ Deactivated accounts can be reactivated using the reset password
-        functionality. Bug #556878 """
+        """Prevent password reset for unvalidated emails."""
         r = self.client.post(reverse('forgot_password'),
                              {'email': '[e-mail]'})
 
@@ -656,38 +646,41 @@
     def test_reset_password_when_account_deactivated_no_preferred_email(self):
         account = Account.objects.get_by_email('[e-mail]')
         # make sure the account is deactivated and preferred email is removed
-        _preferred_email = account.preferredemail
         account.status = AccountStatus.DEACTIVATED
         # Blow out the cached value
         account.save()
-        for email_obj in account.emailaddress_set.all():
-            email_obj.status = EmailStatus.NEW
-            email_obj.save()
+        account.emailaddress_set.update(status=EmailStatus.NEW)
 
         account = Account.objects.get_by_email('[e-mail]')
 
         if gargoyle.is_active('ALLOW_UNVALIDATED'):
-            self.assertEqual(account.preferredemail.email, '[e-mail]')
+            self.assertEqual(account.preferredemail.email,
+                             '[e-mail]')
         else:
             self.assertEqual(account.preferredemail, None)
 
-        r = self.client.post(reverse('forgot_password'),
-                             {'email': '[e-mail]'})
-
-        self.assertEqual(self.mock_send_email.call_count, 1)
-
-        # reset password
-        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
-        r = self.client.post(self._token_url(), data)
-        self.assertRedirects(r, reverse('account-index'))
-
-        account = Account.objects.get_by_email('[e-mail]')
-        self.assertEqual(account.preferredemail.email, '[e-mail]')
-
-        for email_obj in account.emailaddress_set.all():
-            email_obj.status = EmailStatus.VALIDATED
-            email_obj.save()
-        account.preferredemail = _preferred_email
+        self.client.post(reverse('forgot_password'),
+                         {'email': '[e-mail]'})
+
+        # since there is verified email address, no email was sent
+        self.assertEqual(self.mock_send_email.call_count, 0)
+
+    def test_reset_password_when_account_no_verified_emails(self):
+        account = Account.objects.get_by_email('[e-mail]')
+        account.emailaddress_set.update(status=EmailStatus.NEW)
+        self.client.post(reverse('forgot_password'),
+                         {'email': '[e-mail]'})
+        # since there is verified email address, no email was sent
+        self.assertEqual(self.mock_send_email.call_count, 0)
+
+    def test_reset_password_unverified_email_but_account_verified_email(self):
+        account = Account.objects.get_by_email('[e-mail]')
+        verified = account.verified_emails()
+        unverified = account.unverified_emails()[0]
+        self.client.post(reverse('forgot_password'),
+                         {'email': unverified.email})
+        # email sent to verified email addresses instead
+        self.assertEqual(self.mock_send_email.call_count, len(verified))
 
     def test_reset_password_when_account_noaccount(self):
         r = self.client.post(reverse('forgot_password'),

=== modified file 'webui/views/ui.py'
--- webui/views/ui.py	2013-01-22 18:28:03 +0000
+++ webui/views/ui.py	2013-01-25 22:16:21 +0000
@@ -768,30 +768,36 @@
 
 
 def _handle_reset_request(request, email, token):
-    # options for actions to be taken
-    NONE, RESET, CREATE = range(3)
-    # assume invalid
-    send_email = NONE
     account = Account.objects.get_by_email(email)
     if account is not None:
+        verified_emails = account.verified_emails()
         if account.can_reset_password:
-            send_email = RESET
+            redirection_url = redirection_url_for_token(token)
+            if not verified_emails.filter(email=email).count() > 0:
+                # provided email is not verified
+                # send email to verified addresses instead
+                for email_obj in verified_emails:
+                    emailutils.send_password_reset_email(account,
+                                                         email_obj.email,
+                                                         redirection_url)
+            else:
+                emailutils.send_password_reset_email(account, email,
+                                                     redirection_url)
+            set_session_email(request.session, email)
+        elif verified_emails.count() == 0:
+            # user does not have any verified email address
+            # he should contact support
+            condition = ("account '%s' has no verified email address" %
+                         account.displayname)
+            logger.debug("In view 'forgot_password' email was not "
+                         "sent out because %s" % condition)
         else:
-            send_email = NONE
             # log why email was not sent
             condition = ("account '%s' is not active" %
                          account.displayname)
             logger.debug("In view 'forgot_password' email was not "
                          "sent out because %s" % condition)
     else:
-        send_email = CREATE
-
-    # we should know now the action we need to take
-    if send_email == RESET:
-        redirection_url = redirection_url_for_token(token)
-        emailutils.send_password_reset_email(account, email, redirection_url)
-        set_session_email(request.session, email)
-    elif send_email == CREATE:
         # they've tried to reset with an invalid email, so send them an email
         # on how to create an account
         emailutils.send_invitation_after_password_reset(email)
@@ -833,18 +839,26 @@
             msgs = {
                 'email_to': email,
                 'email_from': settings.NOREPLY_FROM_ADDRESS,
+                'support_form_url': settings.SUPPORT_FORM_URL,
             }
+            reason = _(
+                "We've just emailed "
+                "%(email_to)s (from %(email_from)s) with "
+                "instructions on resetting your password.<br/><br/>"
+                "If the email address you provided has not been verified "
+                "we'll use your account's verified email address instead. "
+                "If you don't have a verified email address please "
+                "<a href='%(support_form_url)s'>contact support</a>."
+            ) % msgs
             return display_email_sent(
                 request,
                 email,
                 _("Forgotten your password?"),
-                _("We&rsquo;ve just emailed "
-                  "%(email_to)s (from %(email_from)s) with "
-                  "instructions on resetting your password.") % msgs,
+                reason,
                 _("Check that you&rsquo;ve actually "
                   "entered a subscribed email address."),
                 token=token,
-                rpconfig=rpconfig
+                rpconfig=rpconfig,
             )
         else:
             # track form errors
@@ -864,6 +878,7 @@
 
     atrequest = verify_token_string(authtoken, email_address)
     account = atrequest and atrequest.requester
+
     if (atrequest is None or
             atrequest.token_type != TokenType.PASSWORDRECOVERY or
             not account.can_reset_password):
@@ -894,14 +909,6 @@
                 msg = _("Your account was reactivated, and the preferred "
                         "email address was set to {email}.")
                 messages.info(request, msg.format(email=email_obj.email))
-            elif account.preferredemail is None:
-                # handle this case gracefully according to #524582
-                email_obj, created = EmailAddress.objects.get_or_create(
-                    account=account,
-                    email=atrequest.requester_email,
-                    defaults={'status': EmailStatus.PREFERRED})
-                if not created:
-                    account.preferredemail = email_obj
             email = account.preferredemail.email
             account.set_password(password)
             user = auth.authenticate(username=email, password=password)