Merge lp:~maxiberta/canonical-identity-provider/prevent-forgot-password-user-enumeration into lp:canonical-identity-provider/release
- prevent-forgot-password-user-enumeration
- Merge into trunk
Proposed by
Maximiliano Bertacchini
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Maximiliano Bertacchini | ||||
Approved revision: | no longer in the source branch. | ||||
Merge reported by: | Otto Co-Pilot | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~maxiberta/canonical-identity-provider/prevent-forgot-password-user-enumeration | ||||
Merge into: | lp:canonical-identity-provider/release | ||||
Diff against target: |
284 lines (+59/-49) 5 files modified
src/api/v20/handlers.py (+3/-4) src/api/v20/tests/test_handlers.py (+4/-3) src/identityprovider/tests/openid_server/per_version/test_sso_workflow_reset_password.py (+2/-7) src/webui/tests/test_loginservice.py (+24/-22) src/webui/tests/test_views_registration.py (+26/-13) |
||||
To merge this branch: | bzr merge lp:~maxiberta/canonical-identity-provider/prevent-forgot-password-user-enumeration | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tom Wardill (community) | Approve | ||
Review via email: mp+361166@code.launchpad.net |
Commit message
Redirect forgot password valid POST to email-sent view regardless of whether the account exists or not.
Description of the change
To post a comment you must log in.
Revision history for this message
Maximiliano Bertacchini (maxiberta) : | # |
Revision history for this message
Tom Wardill (twom) : | # |
review:
Approve
Revision history for this message
Otto Co-Pilot (otto-copilot) wrote : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'src/api/v20/handlers.py' | |||
2 | --- src/api/v20/handlers.py 2018-12-20 20:25:26 +0000 | |||
3 | +++ src/api/v20/handlers.py 2019-01-15 17:33:10 +0000 | |||
4 | @@ -15,7 +15,6 @@ | |||
5 | 15 | ) | 15 | ) |
6 | 16 | from django.core.validators import validate_email | 16 | from django.core.validators import validate_email |
7 | 17 | from django.http import HttpResponse | 17 | from django.http import HttpResponse |
8 | 18 | from django.utils.translation import ugettext_lazy as _ | ||
9 | 19 | from django_statsd.clients import statsd | 18 | from django_statsd.clients import statsd |
10 | 20 | from gargoyle import gargoyle | 19 | from gargoyle import gargoyle |
11 | 21 | from piston.handler import AnonymousBaseHandler, BaseHandler | 20 | from piston.handler import AnonymousBaseHandler, BaseHandler |
12 | @@ -221,9 +220,9 @@ | |||
13 | 221 | # but there was some with it invalidated | 220 | # but there was some with it invalidated |
14 | 222 | return errors.EMAIL_INVALIDATED() | 221 | return errors.EMAIL_INVALIDATED() |
15 | 223 | else: | 222 | else: |
19 | 224 | msg = _("No account associated with {email}") | 223 | response = rc.CREATED |
20 | 225 | # there is no account, neither with this email invalidated | 224 | response.content = dict(email=email) |
21 | 226 | return errors.INVALID_DATA(email=[msg.format(email=email)]) | 225 | return response |
22 | 227 | 226 | ||
23 | 228 | if not account.can_reset_password: | 227 | if not account.can_reset_password: |
24 | 229 | if account.status == AccountStatus.SUSPENDED: | 228 | if account.status == AccountStatus.SUSPENDED: |
25 | 230 | 229 | ||
26 | === modified file 'src/api/v20/tests/test_handlers.py' | |||
27 | --- src/api/v20/tests/test_handlers.py 2018-12-20 20:25:26 +0000 | |||
28 | +++ src/api/v20/tests/test_handlers.py 2019-01-15 17:33:10 +0000 | |||
29 | @@ -1600,12 +1600,13 @@ | |||
30 | 1600 | def test_email_missing(self): | 1600 | def test_email_missing(self): |
31 | 1601 | self.assert_field_required(data={}, field='email') | 1601 | self.assert_field_required(data={}, field='email') |
32 | 1602 | 1602 | ||
34 | 1603 | def test_invalid_email(self): | 1603 | def test_nonexistent_account_email(self): |
35 | 1604 | mock_send_invitation = self.patch( | 1604 | mock_send_invitation = self.patch( |
36 | 1605 | 'api.v20.handlers.emailutils.send_invitation_after_password_reset') | 1605 | 'api.v20.handlers.emailutils.send_invitation_after_password_reset') |
37 | 1606 | email = 'invalid@foo.com' | ||
38 | 1607 | body = self.do_post({'email': email}, status_code=201) | ||
39 | 1606 | 1608 | ||
42 | 1607 | extra = {'email': ['No account associated with invalid@foo.com']} | 1609 | self.assertEqual(body['email'], email) |
41 | 1608 | self.assert_bad_request(data={'email': 'invalid@foo.com'}, extra=extra) | ||
43 | 1609 | mock_send_invitation.assert_called_once_with( | 1610 | mock_send_invitation.assert_called_once_with( |
44 | 1610 | self.sso_root_url, 'invalid@foo.com') | 1611 | self.sso_root_url, 'invalid@foo.com') |
45 | 1611 | 1612 | ||
46 | 1612 | 1613 | ||
47 | === modified file 'src/identityprovider/tests/openid_server/per_version/test_sso_workflow_reset_password.py' | |||
48 | --- src/identityprovider/tests/openid_server/per_version/test_sso_workflow_reset_password.py 2019-01-09 18:37:18 +0000 | |||
49 | +++ src/identityprovider/tests/openid_server/per_version/test_sso_workflow_reset_password.py 2019-01-15 17:33:10 +0000 | |||
50 | @@ -26,10 +26,7 @@ | |||
51 | 26 | data = dict(email='no-account@example.com') | 26 | data = dict(email='no-account@example.com') |
52 | 27 | response = self.client.post(link, data=data, follow=True) | 27 | response = self.client.post(link, data=data, follow=True) |
53 | 28 | 28 | ||
58 | 29 | self.assertContains(response, "Reset password") | 29 | self.assertContains(response, "Step 2 of 3: Check your email") |
55 | 30 | self.assertFormError( | ||
56 | 31 | response, 'form', 'email', | ||
57 | 32 | 'No account associated with no-account@example.com') | ||
59 | 33 | 30 | ||
60 | 34 | def test_email_for_team(self): | 31 | def test_email_for_team(self): |
61 | 35 | response = self.do_openid_dance() | 32 | response = self.do_openid_dance() |
62 | @@ -40,9 +37,7 @@ | |||
63 | 40 | data = dict(email='support@ubuntu.com') | 37 | data = dict(email='support@ubuntu.com') |
64 | 41 | response = self.client.post(link, data=data, follow=True) | 38 | response = self.client.post(link, data=data, follow=True) |
65 | 42 | 39 | ||
69 | 43 | self.assertContains(response, "Reset password") | 40 | self.assertContains(response, "Step 2 of 3: Check your email") |
67 | 44 | self.assertFormError(response, 'form', 'email', | ||
68 | 45 | 'No account associated with support@ubuntu.com') | ||
70 | 46 | 41 | ||
71 | 47 | def test_valid_email(self): | 42 | def test_valid_email(self): |
72 | 48 | # Finally, lets try and recover the password for a test@canonical.com: | 43 | # Finally, lets try and recover the password for a test@canonical.com: |
73 | 49 | 44 | ||
74 | === modified file 'src/webui/tests/test_loginservice.py' | |||
75 | --- src/webui/tests/test_loginservice.py 2016-05-10 14:42:33 +0000 | |||
76 | +++ src/webui/tests/test_loginservice.py 2019-01-15 17:33:10 +0000 | |||
77 | @@ -46,23 +46,23 @@ | |||
78 | 46 | 46 | ||
79 | 47 | class NewAccountTest(SSOBaseTestCase): | 47 | class NewAccountTest(SSOBaseTestCase): |
80 | 48 | 48 | ||
81 | 49 | fixtures = ["test"] | ||
82 | 50 | |||
83 | 49 | def test_newaccount_existing(self): | 51 | def test_newaccount_existing(self): |
84 | 50 | query = { | 52 | query = { |
86 | 51 | 'email': 'nobody@debian.org', | 53 | 'displayname': 'Tester', |
87 | 54 | 'email': 'test@canonical.com', | ||
88 | 55 | 'password': 'Testing123', | ||
89 | 56 | 'passwordconfirm': 'Testing123', | ||
90 | 57 | 'accept_tos': True, | ||
91 | 52 | } | 58 | } |
92 | 53 | self.preseed_cookie_check() | 59 | self.preseed_cookie_check() |
97 | 54 | response = self.client.post('/+forgot_password', query) | 60 | response = self.client.post('/+new_account', query) |
98 | 55 | self.assertEqual(response.status_code, 200) | 61 | self.assertContains(response, "test@canonical.com") |
95 | 56 | self.assertIn("Reset password", response.content) | ||
96 | 57 | self.assertIn("nobody@debian.org", response.content) | ||
99 | 58 | self.assertEqual(len(mail.outbox), 1) | 62 | self.assertEqual(len(mail.outbox), 1) |
107 | 59 | self.assertEqual( | 63 | self.assertEqual(mail.outbox[0].subject, |
108 | 60 | mail.outbox[0].subject, | 64 | u"%s: Warning" % settings.NOREPLY_FROM_NAME) |
109 | 61 | u"%s: Password reset request" % settings.NOREPLY_FROM_NAME) | 65 | mail.outbox = [] |
103 | 62 | self.assertIn('nobody@debian.org', mail.outbox[0].body) | ||
104 | 63 | self.assertIn('+new_account', mail.outbox[0].body) | ||
105 | 64 | self.assertFormError(response, 'form', 'email', | ||
106 | 65 | 'No account associated with nobody@debian.org') | ||
110 | 66 | 66 | ||
111 | 67 | def test_newaccount_no_tos_accept(self): | 67 | def test_newaccount_no_tos_accept(self): |
112 | 68 | self.preseed_cookie_check() | 68 | self.preseed_cookie_check() |
113 | @@ -85,19 +85,20 @@ | |||
114 | 85 | 85 | ||
115 | 86 | def test_forgottenpass_nonexisting(self): | 86 | def test_forgottenpass_nonexisting(self): |
116 | 87 | query = { | 87 | query = { |
122 | 88 | 'displayname': 'Tester', | 88 | 'email': 'nobody@debian.org', |
118 | 89 | 'email': 'test@canonical.com', | ||
119 | 90 | 'password': 'Testing123', | ||
120 | 91 | 'passwordconfirm': 'Testing123', | ||
121 | 92 | 'accept_tos': True, | ||
123 | 93 | } | 89 | } |
124 | 94 | self.preseed_cookie_check() | 90 | self.preseed_cookie_check() |
127 | 95 | response = self.client.post('/+new_account', query) | 91 | response = self.client.post('/+forgot_password', query) |
128 | 96 | self.assertContains(response, "test@canonical.com") | 92 | self.assertEqual(response.status_code, 200) |
129 | 93 | self.assertIn("Reset password", response.content) | ||
130 | 94 | self.assertIn("nobody@debian.org", response.content) | ||
131 | 97 | self.assertEqual(len(mail.outbox), 1) | 95 | self.assertEqual(len(mail.outbox), 1) |
135 | 98 | self.assertEqual(mail.outbox[0].subject, | 96 | self.assertEqual( |
136 | 99 | u"%s: Warning" % settings.NOREPLY_FROM_NAME) | 97 | mail.outbox[0].subject, |
137 | 100 | mail.outbox = [] | 98 | u"%s: Password reset request" % settings.NOREPLY_FROM_NAME) |
138 | 99 | self.assertIn('nobody@debian.org', mail.outbox[0].body) | ||
139 | 100 | self.assertIn('+new_account', mail.outbox[0].body) | ||
140 | 101 | self.assertTemplateUsed(response, 'registration/email_sent.html') | ||
141 | 101 | 102 | ||
142 | 102 | def test_resetform_success(self): | 103 | def test_resetform_success(self): |
143 | 103 | query = { | 104 | query = { |
144 | @@ -112,5 +113,6 @@ | |||
145 | 112 | mail.outbox[0].subject, | 113 | mail.outbox[0].subject, |
146 | 113 | u"%s: Forgotten Password" % settings.NOREPLY_FROM_NAME) | 114 | u"%s: Forgotten Password" % settings.NOREPLY_FROM_NAME) |
147 | 114 | mail.outbox = [] | 115 | mail.outbox = [] |
148 | 116 | self.assertTemplateUsed(response, 'registration/email_sent.html') | ||
149 | 115 | 117 | ||
150 | 116 | AuthToken.objects.all().delete() | 118 | AuthToken.objects.all().delete() |
151 | 117 | 119 | ||
152 | === modified file 'src/webui/tests/test_views_registration.py' | |||
153 | --- src/webui/tests/test_views_registration.py 2019-01-09 18:37:18 +0000 | |||
154 | +++ src/webui/tests/test_views_registration.py 2019-01-15 17:33:10 +0000 | |||
155 | @@ -490,11 +490,13 @@ | |||
156 | 490 | self.assertEqual(ctx['rpconfig'], None) | 490 | self.assertEqual(ctx['rpconfig'], None) |
157 | 491 | self.assert_form_displayed(response) | 491 | self.assert_form_displayed(response) |
158 | 492 | self.assert_stat_calls(['requested']) | 492 | self.assert_stat_calls(['requested']) |
159 | 493 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
160 | 493 | 494 | ||
161 | 494 | def test_get_with_email(self): | 495 | def test_get_with_email(self): |
162 | 495 | response = self.get(data=dict(email='test@test.com')) | 496 | response = self.get(data=dict(email='test@test.com')) |
163 | 496 | ctx = response.context_data | 497 | ctx = response.context_data |
164 | 497 | self.assertEqual(ctx['form']['email'].value(), 'test@test.com') | 498 | self.assertEqual(ctx['form']['email'].value(), 'test@test.com') |
165 | 499 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
166 | 498 | 500 | ||
167 | 499 | def test_post_with_initial_data(self): | 501 | def test_post_with_initial_data(self): |
168 | 500 | data = dict(email='test@test.com', forgot_password='') | 502 | data = dict(email='test@test.com', forgot_password='') |
169 | @@ -514,6 +516,7 @@ | |||
170 | 514 | response = self.post() | 516 | response = self.post() |
171 | 515 | self.assert_form_displayed(response, email=FIELD_REQUIRED) | 517 | self.assert_form_displayed(response, email=FIELD_REQUIRED) |
172 | 516 | self.assert_stat_calls(['error.form']) | 518 | self.assert_stat_calls(['error.form']) |
173 | 519 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
174 | 517 | 520 | ||
175 | 518 | def test_post_invalid_data(self): | 521 | def test_post_invalid_data(self): |
176 | 519 | response = self.post(data=dict(email="111")) | 522 | response = self.post(data=dict(email="111")) |
177 | @@ -521,6 +524,7 @@ | |||
178 | 521 | self.assert_form_displayed(response, email=INVALID_EMAIL) | 524 | self.assert_form_displayed(response, email=INVALID_EMAIL) |
179 | 522 | self.assert_form_displayed(response) | 525 | self.assert_form_displayed(response) |
180 | 523 | self.assert_stat_calls(['error.form']) | 526 | self.assert_stat_calls(['error.form']) |
181 | 527 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
182 | 524 | 528 | ||
183 | 525 | def test_post_invalid_data_with_token(self): | 529 | def test_post_invalid_data_with_token(self): |
184 | 526 | token = '12345678' * 2 | 530 | token = '12345678' * 2 |
185 | @@ -531,16 +535,19 @@ | |||
186 | 531 | 535 | ||
187 | 532 | self.assert_form_displayed(response) | 536 | self.assert_form_displayed(response) |
188 | 533 | self.assert_stat_calls(['error.form']) | 537 | self.assert_stat_calls(['error.form']) |
189 | 538 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
190 | 534 | 539 | ||
191 | 535 | def test_post_success(self): | 540 | def test_post_success(self): |
192 | 536 | response = self.post(data=self.data) | 541 | response = self.post(data=self.data) |
193 | 537 | 542 | ||
194 | 538 | self.mock_api_request_password_reset.assert_called_once_with( | 543 | self.mock_api_request_password_reset.assert_called_once_with( |
195 | 539 | email=self.data['email'], token=None) | 544 | email=self.data['email'], token=None) |
196 | 540 | |||
197 | 541 | self.assertEqual(response.status_code, 200) | 545 | self.assertEqual(response.status_code, 200) |
198 | 542 | self.assertEqual( | 546 | self.assertEqual( |
199 | 543 | self.client.session['token_email'], self.data['email']) | 547 | self.client.session['token_email'], self.data['email']) |
200 | 548 | self.assertEqual(response.context_data['email_heading'], | ||
201 | 549 | 'Reset password') | ||
202 | 550 | self.assertTemplateUsed(response, 'registration/email_sent.html') | ||
203 | 544 | 551 | ||
204 | 545 | def test_post_success_with_token(self): | 552 | def test_post_success_with_token(self): |
205 | 546 | token = '12345678' * 2 | 553 | token = '12345678' * 2 |
206 | @@ -551,6 +558,19 @@ | |||
207 | 551 | self.assertEqual(response.status_code, 200) | 558 | self.assertEqual(response.status_code, 200) |
208 | 552 | self.assertEqual( | 559 | self.assertEqual( |
209 | 553 | self.client.session['token_email'], self.data['email']) | 560 | self.client.session['token_email'], self.data['email']) |
210 | 561 | self.assertTemplateUsed(response, 'registration/email_sent.html') | ||
211 | 562 | |||
212 | 563 | def test_post_with_unknown_email(self): | ||
213 | 564 | response = self.post(data=self.data) | ||
214 | 565 | |||
215 | 566 | self.mock_api_request_password_reset.assert_called_once_with( | ||
216 | 567 | email=self.data['email'], token=None) | ||
217 | 568 | self.assertEqual(response.status_code, 200) | ||
218 | 569 | self.assertEqual( | ||
219 | 570 | self.client.session['token_email'], self.data['email']) | ||
220 | 571 | self.assertEqual(response.context_data['email_heading'], | ||
221 | 572 | 'Reset password') | ||
222 | 573 | self.assertTemplateUsed(response, 'registration/email_sent.html') | ||
223 | 554 | 574 | ||
224 | 555 | def test_post_email_invalidated(self): | 575 | def test_post_email_invalidated(self): |
225 | 556 | exc = api_errors.EmailInvalidated(Mock()) | 576 | exc = api_errors.EmailInvalidated(Mock()) |
226 | @@ -558,6 +578,7 @@ | |||
227 | 558 | response = self.post(data=self.data) | 578 | response = self.post(data=self.data) |
228 | 559 | self.assertEqual(response.status_code, 200) | 579 | self.assertEqual(response.status_code, 200) |
229 | 560 | self.assert_stat_calls(['error.email_invalidated']) | 580 | self.assert_stat_calls(['error.email_invalidated']) |
230 | 581 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
231 | 561 | 582 | ||
232 | 562 | def test_post_account_suspended(self): | 583 | def test_post_account_suspended(self): |
233 | 563 | exc = api_errors.AccountSuspended(Mock()) | 584 | exc = api_errors.AccountSuspended(Mock()) |
234 | @@ -565,6 +586,7 @@ | |||
235 | 565 | response = self.post(data=self.data) | 586 | response = self.post(data=self.data) |
236 | 566 | self.assertEqual(response.status_code, 200) | 587 | self.assertEqual(response.status_code, 200) |
237 | 567 | self.assert_stat_calls(['error.account_suspended']) | 588 | self.assert_stat_calls(['error.account_suspended']) |
238 | 589 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
239 | 568 | 590 | ||
240 | 569 | def test_post_account_deactivated(self): | 591 | def test_post_account_deactivated(self): |
241 | 570 | exc = api_errors.AccountDeactivated(Mock()) | 592 | exc = api_errors.AccountDeactivated(Mock()) |
242 | @@ -572,6 +594,7 @@ | |||
243 | 572 | response = self.post(data=self.data) | 594 | response = self.post(data=self.data) |
244 | 573 | self.assertEqual(response.status_code, 200) | 595 | self.assertEqual(response.status_code, 200) |
245 | 574 | self.assert_stat_calls(['error.account_deactivated']) | 596 | self.assert_stat_calls(['error.account_deactivated']) |
246 | 597 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
247 | 575 | 598 | ||
248 | 576 | def test_post_can_not_reset_password(self): | 599 | def test_post_can_not_reset_password(self): |
249 | 577 | exc = api_errors.CanNotResetPassword(Mock()) | 600 | exc = api_errors.CanNotResetPassword(Mock()) |
250 | @@ -579,6 +602,7 @@ | |||
251 | 579 | response = self.post(data=self.data) | 602 | response = self.post(data=self.data) |
252 | 580 | self.assertEqual(response.status_code, 200) | 603 | self.assertEqual(response.status_code, 200) |
253 | 581 | self.assert_stat_calls(['error.can_not_reset_password']) | 604 | self.assert_stat_calls(['error.can_not_reset_password']) |
254 | 605 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
255 | 582 | 606 | ||
256 | 583 | def test_post_too_many_tokens(self): | 607 | def test_post_too_many_tokens(self): |
257 | 584 | exc = api_errors.TooManyTokens(Mock()) | 608 | exc = api_errors.TooManyTokens(Mock()) |
258 | @@ -586,6 +610,7 @@ | |||
259 | 586 | response = self.post(data=self.data) | 610 | response = self.post(data=self.data) |
260 | 587 | self.assertEqual(response.status_code, 200) | 611 | self.assertEqual(response.status_code, 200) |
261 | 588 | self.assert_stat_calls(['error.too_many_tokens']) | 612 | self.assert_stat_calls(['error.too_many_tokens']) |
262 | 613 | self.assertTemplateUsed(response, 'registration/forgot_password.html') | ||
263 | 589 | 614 | ||
264 | 590 | def test_post_too_many_requests(self): | 615 | def test_post_too_many_requests(self): |
265 | 591 | exc = api_errors.TooManyRequests(Mock()) | 616 | exc = api_errors.TooManyRequests(Mock()) |
266 | @@ -595,18 +620,6 @@ | |||
267 | 595 | self.assert_form_displayed(response, __all__=ERROR_TOO_MANY_REQUESTS) | 620 | self.assert_form_displayed(response, __all__=ERROR_TOO_MANY_REQUESTS) |
268 | 596 | self.assert_stat_calls(['error.too_many_requests']) | 621 | self.assert_stat_calls(['error.too_many_requests']) |
269 | 597 | 622 | ||
270 | 598 | def test_reset_password(self): | ||
271 | 599 | response = self.post(data=self.data) | ||
272 | 600 | |||
273 | 601 | self.mock_api_request_password_reset.assert_called_once_with( | ||
274 | 602 | email=self.data['email'], token=None) | ||
275 | 603 | |||
276 | 604 | self.assertEqual(response.status_code, 200) | ||
277 | 605 | self.assertEqual( | ||
278 | 606 | self.client.session['token_email'], self.data['email']) | ||
279 | 607 | self.assertEqual(response.context_data['email_heading'], | ||
280 | 608 | 'Reset password') | ||
281 | 609 | |||
282 | 610 | def test_method_not_allowed(self): | 623 | def test_method_not_allowed(self): |
283 | 611 | response = self.put(data=self.data) | 624 | response = self.put(data=self.data) |
284 | 612 | self.assertEqual(response.status_code, 405) | 625 | self.assertEqual(response.status_code, 405) |
Merging failed /jenkins. ols.canonical. com/online- services/ job/canonical- identity- provider/ 216/
https:/