Merge lp:~maxiberta/canonical-identity-provider/prevent-forgot-password-user-enumeration into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Redirect forgot password valid POST to email-sent view regardless of whether the account exists or not.

[Tom Wardill (twom)]

review: Approve

[Otto Co-Pilot (otto-copilot)]

Merging failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/216/

[lp:~maxiberta/canonical-identity-provider/prevent-forgot-password-user-enumeration]

1673. By Maximiliano Bertacchini on 2019-01-15

Merge trunk.

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2018-12-20 20:25:26 +0000
+++ src/api/v20/handlers.py	2019-01-15 17:33:10 +0000
@@ -15,7 +15,6 @@
 )
 from django.core.validators import validate_email
 from django.http import HttpResponse
-from django.utils.translation import ugettext_lazy as _
 from django_statsd.clients import statsd
 from gargoyle import gargoyle
 from piston.handler import AnonymousBaseHandler, BaseHandler
@@ -221,9 +220,9 @@
                 # but there was some with it invalidated
                 return errors.EMAIL_INVALIDATED()
             else:
-                msg = _("No account associated with {email}")
-                # there is no account, neither with this email invalidated
-                return errors.INVALID_DATA(email=[msg.format(email=email)])
+                response = rc.CREATED
+                response.content = dict(email=email)
+                return response
 
         if not account.can_reset_password:
             if account.status == AccountStatus.SUSPENDED:
 
=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2018-12-20 20:25:26 +0000
+++ src/api/v20/tests/test_handlers.py	2019-01-15 17:33:10 +0000
@@ -1600,12 +1600,13 @@
     def test_email_missing(self):
         self.assert_field_required(data={}, field='email')
 
-    def test_invalid_email(self):
+    def test_nonexistent_account_email(self):
         mock_send_invitation = self.patch(
             'api.v20.handlers.emailutils.send_invitation_after_password_reset')
+        email = '[e-mail]'
+        body = self.do_post({'email': email}, status_code=201)
 
-        extra = {'email': ['No account associated with [e-mail]']}
-        self.assert_bad_request(data={'email': '[e-mail]'}, extra=extra)
+        self.assertEqual(body['email'], email)
         mock_send_invitation.assert_called_once_with(
             self.sso_root_url, '[e-mail]')
 
 
=== modified file 'src/identityprovider/tests/openid_server/per_version/test_sso_workflow_reset_password.py'
--- src/identityprovider/tests/openid_server/per_version/test_sso_workflow_reset_password.py	2019-01-09 18:37:18 +0000
+++ src/identityprovider/tests/openid_server/per_version/test_sso_workflow_reset_password.py	2019-01-15 17:33:10 +0000
@@ -26,10 +26,7 @@
         data = dict(email='[e-mail]')
         response = self.client.post(link, data=data, follow=True)
 
-        self.assertContains(response, "Reset password")
-        self.assertFormError(
-            response, 'form', 'email',
-            'No account associated with [e-mail]')
+        self.assertContains(response, "Step 2 of 3: Check your email")
 
     def test_email_for_team(self):
         response = self.do_openid_dance()
@@ -40,9 +37,7 @@
         data = dict(email='[e-mail]')
         response = self.client.post(link, data=data, follow=True)
 
-        self.assertContains(response, "Reset password")
-        self.assertFormError(response, 'form', 'email',
-                             'No account associated with [e-mail]')
+        self.assertContains(response, "Step 2 of 3: Check your email")
 
     def test_valid_email(self):
         # Finally, lets try and recover the password for a [e-mail]:
 
=== modified file 'src/webui/tests/test_loginservice.py'
--- src/webui/tests/test_loginservice.py	2016-05-10 14:42:33 +0000
+++ src/webui/tests/test_loginservice.py	2019-01-15 17:33:10 +0000
@@ -46,23 +46,23 @@
 
 class NewAccountTest(SSOBaseTestCase):
 
+    fixtures = ["test"]
+
     def test_newaccount_existing(self):
         query = {
-            'email': '[e-mail]',
+            'displayname': 'Tester',
+            'email': '[e-mail]',
+            'password': 'Testing123',
+            'passwordconfirm': 'Testing123',
+            'accept_tos': True,
         }
         self.preseed_cookie_check()
-        response = self.client.post('/+forgot_password', query)
-        self.assertEqual(response.status_code, 200)
-        self.assertIn("Reset password", response.content)
-        self.assertIn("[e-mail]", response.content)
+        response = self.client.post('/+new_account', query)
+        self.assertContains(response, "[e-mail]")
         self.assertEqual(len(mail.outbox), 1)
-        self.assertEqual(
-            mail.outbox[0].subject,
-            u"%s: Password reset request" % settings.NOREPLY_FROM_NAME)
-        self.assertIn('[e-mail]', mail.outbox[0].body)
-        self.assertIn('+new_account', mail.outbox[0].body)
-        self.assertFormError(response, 'form', 'email',
-                             'No account associated with [e-mail]')
+        self.assertEqual(mail.outbox[0].subject,
+                         u"%s: Warning" % settings.NOREPLY_FROM_NAME)
+        mail.outbox = []
 
     def test_newaccount_no_tos_accept(self):
         self.preseed_cookie_check()
@@ -85,19 +85,20 @@
 
     def test_forgottenpass_nonexisting(self):
         query = {
-            'displayname': 'Tester',
-            'email': '[e-mail]',
-            'password': 'Testing123',
-            'passwordconfirm': 'Testing123',
-            'accept_tos': True,
+            'email': '[e-mail]',
         }
         self.preseed_cookie_check()
-        response = self.client.post('/+new_account', query)
-        self.assertContains(response, "[e-mail]")
+        response = self.client.post('/+forgot_password', query)
+        self.assertEqual(response.status_code, 200)
+        self.assertIn("Reset password", response.content)
+        self.assertIn("[e-mail]", response.content)
         self.assertEqual(len(mail.outbox), 1)
-        self.assertEqual(mail.outbox[0].subject,
-                         u"%s: Warning" % settings.NOREPLY_FROM_NAME)
-        mail.outbox = []
+        self.assertEqual(
+            mail.outbox[0].subject,
+            u"%s: Password reset request" % settings.NOREPLY_FROM_NAME)
+        self.assertIn('[e-mail]', mail.outbox[0].body)
+        self.assertIn('+new_account', mail.outbox[0].body)
+        self.assertTemplateUsed(response, 'registration/email_sent.html')
 
     def test_resetform_success(self):
         query = {
@@ -112,5 +113,6 @@
             mail.outbox[0].subject,
             u"%s: Forgotten Password" % settings.NOREPLY_FROM_NAME)
         mail.outbox = []
+        self.assertTemplateUsed(response, 'registration/email_sent.html')
 
         AuthToken.objects.all().delete()
 
=== modified file 'src/webui/tests/test_views_registration.py'
--- src/webui/tests/test_views_registration.py	2019-01-09 18:37:18 +0000
+++ src/webui/tests/test_views_registration.py	2019-01-15 17:33:10 +0000
@@ -490,11 +490,13 @@
         self.assertEqual(ctx['rpconfig'], None)
         self.assert_form_displayed(response)
         self.assert_stat_calls(['requested'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_get_with_email(self):
         response = self.get(data=dict(email='[e-mail]'))
         ctx = response.context_data
         self.assertEqual(ctx['form']['email'].value(), '[e-mail]')
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_with_initial_data(self):
         data = dict(email='[e-mail]', forgot_password='')
@@ -514,6 +516,7 @@
         response = self.post()
         self.assert_form_displayed(response, email=FIELD_REQUIRED)
         self.assert_stat_calls(['error.form'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_invalid_data(self):
         response = self.post(data=dict(email="111"))
@@ -521,6 +524,7 @@
         self.assert_form_displayed(response, email=INVALID_EMAIL)
         self.assert_form_displayed(response)
         self.assert_stat_calls(['error.form'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_invalid_data_with_token(self):
         token = '12345678' * 2
@@ -531,16 +535,19 @@
 
         self.assert_form_displayed(response)
         self.assert_stat_calls(['error.form'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_success(self):
         response = self.post(data=self.data)
 
         self.mock_api_request_password_reset.assert_called_once_with(
             email=self.data['email'], token=None)
-
         self.assertEqual(response.status_code, 200)
         self.assertEqual(
             self.client.session['token_email'], self.data['email'])
+        self.assertEqual(response.context_data['email_heading'],
+                         'Reset password')
+        self.assertTemplateUsed(response, 'registration/email_sent.html')
 
     def test_post_success_with_token(self):
         token = '12345678' * 2
@@ -551,6 +558,19 @@
         self.assertEqual(response.status_code, 200)
         self.assertEqual(
             self.client.session['token_email'], self.data['email'])
+        self.assertTemplateUsed(response, 'registration/email_sent.html')
+
+    def test_post_with_unknown_email(self):
+        response = self.post(data=self.data)
+
+        self.mock_api_request_password_reset.assert_called_once_with(
+            email=self.data['email'], token=None)
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(
+            self.client.session['token_email'], self.data['email'])
+        self.assertEqual(response.context_data['email_heading'],
+                         'Reset password')
+        self.assertTemplateUsed(response, 'registration/email_sent.html')
 
     def test_post_email_invalidated(self):
         exc = api_errors.EmailInvalidated(Mock())
@@ -558,6 +578,7 @@
         response = self.post(data=self.data)
         self.assertEqual(response.status_code, 200)
         self.assert_stat_calls(['error.email_invalidated'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_account_suspended(self):
         exc = api_errors.AccountSuspended(Mock())
@@ -565,6 +586,7 @@
         response = self.post(data=self.data)
         self.assertEqual(response.status_code, 200)
         self.assert_stat_calls(['error.account_suspended'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_account_deactivated(self):
         exc = api_errors.AccountDeactivated(Mock())
@@ -572,6 +594,7 @@
         response = self.post(data=self.data)
         self.assertEqual(response.status_code, 200)
         self.assert_stat_calls(['error.account_deactivated'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_can_not_reset_password(self):
         exc = api_errors.CanNotResetPassword(Mock())
@@ -579,6 +602,7 @@
         response = self.post(data=self.data)
         self.assertEqual(response.status_code, 200)
         self.assert_stat_calls(['error.can_not_reset_password'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_too_many_tokens(self):
         exc = api_errors.TooManyTokens(Mock())
@@ -586,6 +610,7 @@
         response = self.post(data=self.data)
         self.assertEqual(response.status_code, 200)
         self.assert_stat_calls(['error.too_many_tokens'])
+        self.assertTemplateUsed(response, 'registration/forgot_password.html')
 
     def test_post_too_many_requests(self):
         exc = api_errors.TooManyRequests(Mock())
@@ -595,18 +620,6 @@
         self.assert_form_displayed(response, __all__=ERROR_TOO_MANY_REQUESTS)
         self.assert_stat_calls(['error.too_many_requests'])
 
-    def test_reset_password(self):
-        response = self.post(data=self.data)
-
-        self.mock_api_request_password_reset.assert_called_once_with(
-            email=self.data['email'], token=None)
-
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(
-            self.client.session['token_email'], self.data['email'])
-        self.assertEqual(response.context_data['email_heading'],
-                         'Reset password')
-
     def test_method_not_allowed(self):
         response = self.put(data=self.data)
         self.assertEqual(response.status_code, 405)