Merge lp:~tartley/canonical-identity-provider/allow-new-emails into lp:canonical-identity-provider/release

[Jonathan Hartley]

Restore users ability to send password reset email to new addresses.

A branch was merged before Christmas to fix a security hole in the
password reset process. In that branch, out of an abundance of
caution, we also prevented password reset emails from being sent
to 'new' email addresses.
https://code.launchpad.net/~tartley/canonical-identity-provider/password-reset/+merge/376991

On reflection, the latter part was more cautious than required.
This MP restores the ability for the password reset email logic
to fall back to using an account's 'new' email address if no
preferred or validated email addresses exist.

[Jonathan Hartley (tartley)]

diff comments for reviewers

[Maximiliano Bertacchini (maxiberta)]

LGTM +1

review: Approve

[Daniel Manrique (roadmr)]

+1, LGTM the only thing I'm missing (but I may be missing it by not having read stuff not in the diff) is a test that preferredemail for an account with two NEW addresses returns the most recently-added one.

review: Approve

[Jonathan Hartley (tartley)]

> +1, LGTM the only thing I'm missing (but I may be missing it by not having
> read stuff not in the diff) is a test that preferredemail for an account with
> two NEW addresses returns the most recently-added one.

You are right. That functionality is existing, not new in this MP, and I didn't add a test for it. I'll see whether I can quickly add a line to existing tests to test it easily.

[Jonathan Hartley (tartley)]

The only change of this latest commit is adding the new block of tests at MP line 171, and tweaking comments that talked about "most recent address" to say "oldest address".

[Daniel Manrique (roadmr)]

Surprise surprise is surprising - Yesterday I showed a scenario where returning the newest address made sense, on the other hand it seems reasonable to preserve the old behavior for now, since tweaking that behavior should be out of scope for this restorative MP.

+1 again, just one comment below (though it would apply to other places in the code).

review: Approve

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2019-12-19 22:43:09 +0000
+++ src/api/v20/handlers.py	2020-01-08 22:49:02 +0000
@@ -254,8 +254,8 @@
     def create(self, request):
         data = request.data
         # This email, provided in the password reset request,
-        # may be from an attacker. Even if it case-insentively matches an
-        # email in our database, unicode case trickery mean it might actually
+        # may be from an attacker. Even if it case-insensitively matches an
+        # email in our database, Unicode case trickery mean it might actually
         # be a distinct email address, controlled by an attacker.
         # So it's important not to use it to generate tokens, send emails.
         provided_email = valid_email(data.get('email'))
@@ -318,19 +318,17 @@
         if matching_validated:
             account_email = matching_validated.first().email
         else:
-            # Since no validated email matched, presuambly provided_email
-            # matches one of this account's "new" emails.
-            # Set account_email to the account's preferred email.
-            # If that doesn't exist, this getter falls back to any of the
-            # account's validated emails.
-            preferred = account._get_preferredemail(use_new=False)
+            # provided_email doesn't match any of this account's validated
+            # email addresses. So get an account email address from our db.
+            # This getter falls back through preferred, validated, new.
+            preferred = account.preferredemail
             if preferred:
                 account_email = preferred.email
 
         if not account_email:
-            # The account has no preferred or validated email.
-            # For security, we do not fallback to using new email addresses,
-            # nor the provided_email from the password reset request.
+            # This should be impossible. We found this account using
+            # 'provided_email', but now we're finding the account has no
+            # useable email addresses.
             return errors.CAN_NOT_RESET_PASSWORD()
 
         tokens = AuthToken.objects.filter(

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2019-12-19 19:38:06 +0000
+++ src/api/v20/tests/test_handlers.py	2020-01-08 22:49:02 +0000
@@ -1851,6 +1851,17 @@
         # Then password reset uses the stored validated email
         self.assert_email_used(result, '[e-mail]')
 
+    def test_reset_password_given_new_email_uses_new(self):
+        # Given an account with one new email address
+        self.factory.make_account(
+            email="[e-mail]", email_validated=False)
+
+        # When a password reset request matches the new address
+        result = self.do_post({'email': "[e-mail]"}, status_code=201)
+
+        # Then the password reset uses the stored new email
+        self.assert_email_used(result, '[e-mail]')
+
     def test_reset_password_given_validated_email_uses_it(self):
         # Given an account with one preferred email address
         account = self.factory.make_account(email="[e-mail]")
@@ -1864,24 +1875,6 @@
         # Then the password reset uses the stored validated email
         self.assert_email_used(result, '[e-mail]')
 
-    def test_reset_password_with_no_validated_email_refuses(self):
-        # Given an account with one new email address
-        self.factory.make_account(
-            email="[e-mail]", email_validated=False)
-
-        # When a password reset request matches the new address
-        result = self.do_post({'email': "[e-mail]"}, status_code=403)
-
-        # Then password reset is refused
-        self.assertEqual(
-            {
-                'message': 'Can not reset password. '
-                           'Please contact login support',
-                'code': 'CAN_NOT_RESET_PASSWORD',
-                'extra': {}
-            },
-            result)
-
     def test_too_many_tokens(self):
         with self.settings(MAX_PASSWORD_RESET_TOKENS=0):
             content = self.do_post(data=self.data, status_code=403)

=== modified file 'src/identityprovider/models/account.py'
--- src/identityprovider/models/account.py	2019-12-19 16:30:42 +0000
+++ src/identityprovider/models/account.py	2020-01-08 22:49:02 +0000
@@ -318,15 +318,15 @@
         self.accountpassword.save()
         self.invalidate_oauth_tokens()
 
-    def _get_preferredemail(self, use_new=True):
+    def _get_preferredemail(self):
         """
         Returns current preferred email address.
 
         If there is already one, and it is verified, use it.
-        If not, promote the most recently validated email to preferred,
+        If not, promote the oldest validated email to preferred,
         and return that.
-        If there are none, then return a 'new' email.
-        Pass use_new=False to turn off this final fallback to new emails.
+        If there are none, then return the oldest 'new' email,
+        but don't promote that email to preferred.
         """
         email = getattr(self, '_preferredemail', None)
 
@@ -340,14 +340,17 @@
                 all_emails = sorted(
                     self.emailaddress_set.all(),
                     key=attrgetter('date_created'))
+
+                # Get the preferred email address.
                 emails = [
                     e for e in all_emails if e.status == EmailStatus.PREFERRED]
                 if emails:
                     email = emails[0]
 
+                # If no preferred email address found,
+                # then get the oldest validated address by date_created,
+                # and promote it to preferred.
                 if not email:
-                    # Try to determine a suitable address, and mark it
-                    # as preferred.
                     emails = [
                         e for e in all_emails
                         if e.status == EmailStatus.VALIDATED]
@@ -360,14 +363,15 @@
                             email, self
                         )
 
-                if not email and use_new:
-                    # we have no validated email, so use the first NEW email
-                    # but don't save it
+                # If no validated email found,
+                # then get the oldest new address. Don't promote it.
+                if not email:
                     emails = [
                         e for e in all_emails if e.status == EmailStatus.NEW]
                     if emails:
                         email = emails[0]
 
+                # Cache our result to short-circuit this method next time.
                 self._preferredemail = email
             except Exception:
                 # no error in preferred email should raise, as this breaks

=== modified file 'src/identityprovider/tests/test_models_account.py'
--- src/identityprovider/tests/test_models_account.py	2019-12-19 16:30:42 +0000
+++ src/identityprovider/tests/test_models_account.py	2020-01-08 22:49:02 +0000
@@ -443,10 +443,6 @@
         account = self.factory.make_account(email_validated=False)
         self.assertIsNotNone(account.preferredemail)
 
-    def test_get_preferred_without_falling_back_to_new(self):
-        account = self.factory.make_account(email_validated=False)
-        self.assertIsNone(account._get_preferredemail(use_new=False))
-
     def test_set_preferred_with_unverified_email(self):
         account = self.factory.make_account(email_validated=False)
         with self.assertRaises(ValidationError):
@@ -687,6 +683,50 @@
             consumer__user__username=account.openid_identifier)
         self.assertEqual(tokens.count(), 0)
 
+    def test_get_preferredemail_returns_preferred(self):
+        # Given an account with a preferred email
+        account = self.factory.make_account()
+        email = account.emailaddress_set.first()
+        self.assertEqual(EmailStatus.PREFERRED, email.status)  # sanity check
+        # When getting .preferredemail
+        # Then it returns the preferred email
+        self.assertEqual(email, account.preferredemail)
+
+    def test_get_preferredemail_returns_and_promotes_oldest_validated(self):
+        # Given an account with two *validated* emails
+        account = self.factory.make_account(email_validated=False)
+        email1 = self.factory.make_email_for_account(account, "[e-mail]")
+        email2 = self.factory.make_email_for_account(account, "[e-mail]")
+
+        # When getting .preferredemail
+        # Then it returns email1, the oldest by date_created
+        self.assertEqual(email1, account.preferredemail)
+        # and email1 has been promoted to preferred
+        email1.refresh_from_db()
+        self.assertEqual(EmailStatus.PREFERRED, email1.status)
+        # but email2 has not
+        email2.refresh_from_db()
+        self.assertEqual(EmailStatus.VALIDATED, email2.status)
+
+    def test_get_preferredemail_returns_oldest_new(self):
+        # Given an account with two *new* emails
+        account = self.factory.make_account(email_validated=False)
+        email1 = account.emailaddress_set.first()
+        self.assertEqual(EmailStatus.NEW, email1.status)  # sanity check
+        email2 = self.factory.make_email_for_account(
+            account, email="[e-mail]", status=EmailStatus.NEW)
+        self.assertEqual(EmailStatus.NEW, email2.status)  # sanity check
+
+        # When getting .preferredemail
+        # Then it returns email1, the oldest by date_created
+        self.assertEqual(email1, account.preferredemail)
+        # and email1 has *not* been promoted to preferred
+        email1.refresh_from_db()
+        self.assertEqual(EmailStatus.NEW, email1.status)
+        # and email2 has not either.
+        email2.refresh_from_db()
+        self.assertEqual(EmailStatus.NEW, email2.status)
+
     def test_reset_password_default_email(self):
         account = self.factory.make_account()
 

=== modified file 'src/webui/tests/test_views_ui.py'
--- src/webui/tests/test_views_ui.py	2019-12-19 20:48:07 +0000
+++ src/webui/tests/test_views_ui.py	2020-01-08 22:49:02 +0000
@@ -755,8 +755,7 @@
 
         self.client.post(reverse('forgot_password'), {'email': self.email})
 
-        # we do not send an email if there is no validated email
-        self.assertEqual(self.mock_send_email.call_count, 0)
+        self.assertEqual(self.mock_send_email.call_count, 1)
 
     def test_claim_consumed_token(self):
         self.client.post(reverse('forgot_password'), {'email': self.email})
@@ -1021,14 +1020,12 @@
 
         self.client.post(reverse('forgot_password'), {'email': self.email})
 
-        # we do not sent an email, there being no validated email address
-        self.assertEqual(self.mock_send_email.call_count, 0)
+        self.assertEqual(self.mock_send_email.call_count, 1)
 
     def test_reset_password_when_account_no_verified_emails(self):
         self.account.emailaddress_set.update(status=EmailStatus.NEW)
         self.client.post(reverse('forgot_password'), {'email': self.email})
-        # we do not send an email, there being no validated email address
-        self.assertEqual(self.mock_send_email.call_count, 0)
+        self.assertEqual(self.mock_send_email.call_count, 1)
 
     def test_reset_password_unverified_email_but_account_verified_email(self):
         verified = self.account.verified_emails()