Merge lp:~canonical-isd-hackers/canonical-identity-provider/session-refactor-email-sent into lp:canonical-identity-provider/release
- session-refactor-email-sent
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Ricardo Kirkner |
Approved revision: | no longer in the source branch. |
Merged at revision: | 244 |
Proposed branch: | lp:~canonical-isd-hackers/canonical-identity-provider/session-refactor-email-sent |
Merge into: | lp:canonical-identity-provider/release |
Diff against target: |
500 lines (+83/-111) 8 files modified
identityprovider/templates/launchpad/email/invitation.txt (+2/-4) identityprovider/templates/ubuntu/email/invitation.txt (+2/-4) identityprovider/tests/test_loginservice.py (+15/-15) identityprovider/tests/test_views_account.py (+5/-5) identityprovider/tests/test_views_ui.py (+21/-27) identityprovider/urls.py (+0/-1) identityprovider/views/account.py (+15/-17) identityprovider/views/ui.py (+23/-38) |
To merge this branch: | bzr merge lp:~canonical-isd-hackers/canonical-identity-provider/session-refactor-email-sent |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ricardo Kirkner (community) | Approve | ||
Review via email: mp+85930@code.launchpad.net |
Commit message
Refactor to not use session/redirect for displaying email sent page. Instead, we just render the page directly
Description of the change
Refactor to not use session/redirect for displaying email sent page. Instead, we just render the page directly
Simon Davy (bloodearnest) wrote : | # |
> I like avoiding the use of session for this (+1)
>
> - tiny stylistic issue: we prefer singular assertion methods (assertEqual)
> - l. 249: always use RequestContext, otherwise context processors won't be
> applied (you won't get a style page for example)
Fixed
> One last question: given that there were certain precautions not to break the
> openid dance in this view, are we certain not redirecting will keep those
> scenarios working?
I'm not sure, as I'm not clear on the exact steps of the dance :) I just left anything relating to tokens alone, or reproduced it in the new code. Some day you'll have to teach me the dance :)
Preview Diff
1 | === modified file 'identityprovider/templates/launchpad/email/invitation.txt' | |||
2 | --- identityprovider/templates/launchpad/email/invitation.txt 2011-12-15 16:29:12 +0000 | |||
3 | +++ identityprovider/templates/launchpad/email/invitation.txt 2011-12-16 12:35:28 +0000 | |||
4 | @@ -1,11 +1,9 @@ | |||
5 | 1 | {% load i18n %} | 1 | {% load i18n %} |
6 | 2 | {% blocktrans %}Hello | 2 | {% blocktrans %}Hello |
7 | 3 | 3 | ||
10 | 4 | You recently requested a password reset for the email account {{ email }} for | 4 | You recently requested a password reset for the email account {{ email }} for the Launchpad Login Service |
9 | 5 | the Launchpad Login Service | ||
11 | 6 | 5 | ||
14 | 7 | This email is not associated with any current accounts. You are very welcome | 6 | This email is not associated with any current accounts. You are very welcome to create a Launchpad account - to do so, please go to {{ signup }} |
13 | 8 | to create a Launchpad account - to do so, please go to {{ signup }} | ||
15 | 9 | 7 | ||
16 | 10 | Thanks | 8 | Thanks |
17 | 11 | 9 | ||
18 | 12 | 10 | ||
19 | === modified file 'identityprovider/templates/ubuntu/email/invitation.txt' | |||
20 | --- identityprovider/templates/ubuntu/email/invitation.txt 2011-12-15 16:29:12 +0000 | |||
21 | +++ identityprovider/templates/ubuntu/email/invitation.txt 2011-12-16 12:35:28 +0000 | |||
22 | @@ -1,11 +1,9 @@ | |||
23 | 1 | {% load i18n %} | 1 | {% load i18n %} |
24 | 2 | {% blocktrans %}Hello | 2 | {% blocktrans %}Hello |
25 | 3 | 3 | ||
28 | 4 | You recently requested a password reset for the email account {{ email }} for | 4 | You recently requested a password reset for the email account {{ email }} for the Ubuntu Single Sign On service. |
27 | 5 | the Ubuntu Single Sign On service. | ||
29 | 6 | 5 | ||
32 | 7 | This email is not associated with any current accounts. You are very welcome | 6 | This email is not associated with any current accounts. You are very welcome to create an Ubuntu SSO account - to do so, please go to {{ signup }} |
31 | 8 | to create an Ubuntu SSO account - to do so, please go to {{ signup }} | ||
33 | 9 | 7 | ||
34 | 10 | Thanks | 8 | Thanks |
35 | 11 | 9 | ||
36 | 12 | 10 | ||
37 | === modified file 'identityprovider/tests/test_loginservice.py' | |||
38 | --- identityprovider/tests/test_loginservice.py 2011-12-15 16:29:12 +0000 | |||
39 | +++ identityprovider/tests/test_loginservice.py 2011-12-16 12:35:28 +0000 | |||
40 | @@ -33,11 +33,11 @@ | |||
41 | 33 | 'email': 'nobody@debian.org', | 33 | 'email': 'nobody@debian.org', |
42 | 34 | } | 34 | } |
43 | 35 | response = self.client.post('/+forgot_password', query) | 35 | response = self.client.post('/+forgot_password', query) |
49 | 36 | self.assertEquals(response.status_code, 302) | 36 | self.assertEqual(response.status_code, 200) |
50 | 37 | self.assertEquals(response['location'], | 37 | self.assertIn("Forgotten your password?", response.content) |
51 | 38 | 'http://testserver/+email-sent') | 38 | self.assertIn("nobody@debian.org", response.content) |
52 | 39 | self.assertEquals(len(mail.outbox), 1) | 39 | self.assertEqual(len(mail.outbox), 1) |
53 | 40 | self.assertEquals(mail.outbox[0].subject, | 40 | self.assertEqual(mail.outbox[0].subject, |
54 | 41 | u"%s: Password reset request" % current_brand.name) | 41 | u"%s: Password reset request" % current_brand.name) |
55 | 42 | self.assertTrue('nobody@debian.org' in mail.outbox[0].body) | 42 | self.assertTrue('nobody@debian.org' in mail.outbox[0].body) |
56 | 43 | self.assertTrue('+new_account' in mail.outbox[0].body) | 43 | self.assertTrue('+new_account' in mail.outbox[0].body) |
57 | @@ -55,11 +55,11 @@ | |||
58 | 55 | 'passwordconfirm': 'Testing123' | 55 | 'passwordconfirm': 'Testing123' |
59 | 56 | } | 56 | } |
60 | 57 | response = self.client.post('/+new_account', query) | 57 | response = self.client.post('/+new_account', query) |
66 | 58 | self.assertEquals(response.status_code, 302) | 58 | self.assertEqual(response.status_code, 200) |
67 | 59 | self.assertEquals(response['location'], | 59 | self.assertIn("Registration mail sent", response.content) |
68 | 60 | 'http://testserver/+email-sent') | 60 | self.assertIn("test@canonical.com", response.content) |
69 | 61 | self.assertEquals(len(mail.outbox), 1) | 61 | self.assertEqual(len(mail.outbox), 1) |
70 | 62 | self.assertEquals(mail.outbox[0].subject, | 62 | self.assertEqual(mail.outbox[0].subject, |
71 | 63 | u"%s: Warning" % current_brand.name) | 63 | u"%s: Warning" % current_brand.name) |
72 | 64 | mail.outbox = [] | 64 | mail.outbox = [] |
73 | 65 | 65 | ||
74 | @@ -68,11 +68,11 @@ | |||
75 | 68 | 'email': 'test@canonical.com', | 68 | 'email': 'test@canonical.com', |
76 | 69 | } | 69 | } |
77 | 70 | response = self.client.post('/+forgot_password', query) | 70 | response = self.client.post('/+forgot_password', query) |
83 | 71 | self.assertEquals(response.status_code, 302) | 71 | self.assertEqual(response.status_code, 200) |
84 | 72 | self.assertEquals(response['location'], | 72 | self.assertIn("Forgotten your password?", response.content) |
85 | 73 | 'http://testserver/+email-sent') | 73 | self.assertIn("test@canonical.com", response.content) |
86 | 74 | self.assertEquals(len(mail.outbox), 1) | 74 | self.assertEqual(len(mail.outbox), 1) |
87 | 75 | self.assertEquals(mail.outbox[0].subject, | 75 | self.assertEqual(mail.outbox[0].subject, |
88 | 76 | u"%s: Forgotten Password" % current_brand.name) | 76 | u"%s: Forgotten Password" % current_brand.name) |
89 | 77 | mail.outbox = [] | 77 | mail.outbox = [] |
90 | 78 | 78 | ||
91 | 79 | 79 | ||
92 | === modified file 'identityprovider/tests/test_views_account.py' | |||
93 | --- identityprovider/tests/test_views_account.py 2011-09-09 14:31:09 +0000 | |||
94 | +++ identityprovider/tests/test_views_account.py 2011-12-16 12:35:28 +0000 | |||
95 | @@ -85,7 +85,7 @@ | |||
96 | 85 | account=account, | 85 | account=account, |
97 | 86 | status=EmailStatus.NEW) | 86 | status=EmailStatus.NEW) |
98 | 87 | r = self.client.get('/+verify-email', {'id': email.id}) | 87 | r = self.client.get('/+verify-email', {'id': email.id}) |
100 | 88 | self.assertEqual(r.status_code, 302) | 88 | self.assertEqual(r.status_code, 200) |
101 | 89 | self.assertEqual(len(mail.outbox), 1) | 89 | self.assertEqual(len(mail.outbox), 1) |
102 | 90 | # Erasing emails. Necessary to make other tests pass. | 90 | # Erasing emails. Necessary to make other tests pass. |
103 | 91 | mail.outbox = [] | 91 | mail.outbox = [] |
104 | @@ -97,13 +97,13 @@ | |||
105 | 97 | def test_new_email_post(self): | 97 | def test_new_email_post(self): |
106 | 98 | r = self.client.post('/+new-email', | 98 | r = self.client.post('/+new-email', |
107 | 99 | {'newemail': "very-new-email@example.com"}) | 99 | {'newemail': "very-new-email@example.com"}) |
109 | 100 | self.assertRedirects(r, '/+email-sent') | 100 | self.assertEqual(r.status_code, 200) |
110 | 101 | mail.outbox = [] | 101 | mail.outbox = [] |
111 | 102 | 102 | ||
112 | 103 | def test_new_email_post_with_token(self): | 103 | def test_new_email_post_with_token(self): |
113 | 104 | r = self.client.post('/thisissuperrando/+new-email', | 104 | r = self.client.post('/thisissuperrando/+new-email', |
114 | 105 | {'newemail': "very-new-email@example.com"}) | 105 | {'newemail': "very-new-email@example.com"}) |
116 | 106 | self.assertEquals(len(mail.outbox), 1) | 106 | self.assertEqual(len(mail.outbox), 1) |
117 | 107 | mail.outbox = [] | 107 | mail.outbox = [] |
118 | 108 | 108 | ||
119 | 109 | 109 | ||
120 | @@ -189,7 +189,7 @@ | |||
121 | 189 | self.assertRedirects(r, '/+deactivated') | 189 | self.assertRedirects(r, '/+deactivated') |
122 | 190 | 190 | ||
123 | 191 | # test token is preserved | 191 | # test token is preserved |
125 | 192 | self.assertEquals(self.client.session.get(token), | 192 | self.assertEqual(self.client.session.get(token), |
126 | 193 | 'raw_orequest content') | 193 | 'raw_orequest content') |
127 | 194 | 194 | ||
128 | 195 | 195 | ||
129 | @@ -237,4 +237,4 @@ | |||
130 | 237 | r = self.client.post('/+applications', {'token_id': token.token}) | 237 | r = self.client.post('/+applications', {'token_id': token.token}) |
131 | 238 | 238 | ||
132 | 239 | self.assertRedirects(r, '/+applications') | 239 | self.assertRedirects(r, '/+applications') |
134 | 240 | self.assertEquals(Token.objects.filter(token=token.token).count(), 1) | 240 | self.assertEqual(Token.objects.filter(token=token.token).count(), 1) |
135 | 241 | 241 | ||
136 | === modified file 'identityprovider/tests/test_views_ui.py' | |||
137 | --- identityprovider/tests/test_views_ui.py 2011-11-29 18:06:51 +0000 | |||
138 | +++ identityprovider/tests/test_views_ui.py 2011-12-16 12:35:28 +0000 | |||
139 | @@ -83,7 +83,7 @@ | |||
140 | 83 | 83 | ||
141 | 84 | r = _post_new_account(self.client, email='mark@example.com') | 84 | r = _post_new_account(self.client, email='mark@example.com') |
142 | 85 | 85 | ||
144 | 86 | self.assertRedirects(r, '/+email-sent') | 86 | self.assertEqual(r.status_code, 200) |
145 | 87 | self.assertEqual(len(mail.outbox), 1) | 87 | self.assertEqual(len(mail.outbox), 1) |
146 | 88 | 88 | ||
147 | 89 | 89 | ||
148 | @@ -202,7 +202,7 @@ | |||
149 | 202 | session.save() | 202 | session.save() |
150 | 203 | 203 | ||
151 | 204 | self.client.get('/%s/+logout' % token) | 204 | self.client.get('/%s/+logout' % token) |
153 | 205 | self.assertEquals(self.client.session.get(token), | 205 | self.assertEqual(self.client.session.get(token), |
154 | 206 | 'raw_orequest content') | 206 | 'raw_orequest content') |
155 | 207 | 207 | ||
156 | 208 | def create_token(self, token_type, email=None, redirection_url=None, | 208 | def create_token(self, token_type, email=None, redirection_url=None, |
157 | @@ -253,13 +253,13 @@ | |||
158 | 253 | 253 | ||
159 | 254 | def test_claim_unexisting_token(self): | 254 | def test_claim_unexisting_token(self): |
160 | 255 | r = self.client.get('/token/%s/' % 0, {'email': 'fake%40example.com'}) | 255 | r = self.client.get('/token/%s/' % 0, {'email': 'fake%40example.com'}) |
162 | 256 | self.assertEquals(r.status_code, 404) | 256 | self.assertEqual(r.status_code, 404) |
163 | 257 | 257 | ||
164 | 258 | def test_claim_token_for_unexisting_token_type(self): | 258 | def test_claim_token_for_unexisting_token_type(self): |
165 | 259 | token = self.create_token(9999, 'fake@example.com') | 259 | token = self.create_token(9999, 'fake@example.com') |
166 | 260 | r = self.client.get('/token/%s/' % token.token, | 260 | r = self.client.get('/token/%s/' % token.token, |
167 | 261 | {'email': 'fake%40example.com'}) | 261 | {'email': 'fake%40example.com'}) |
169 | 262 | self.assertEquals(r.status_code, 404) | 262 | self.assertEqual(r.status_code, 404) |
170 | 263 | 263 | ||
171 | 264 | def test_claim_consumed_token(self): | 264 | def test_claim_consumed_token(self): |
172 | 265 | self.client.post('/+forgot_password', | 265 | self.client.post('/+forgot_password', |
173 | @@ -306,11 +306,11 @@ | |||
174 | 306 | 306 | ||
175 | 307 | def test_token_form_email_when_it_is_turned_off(self): | 307 | def test_token_form_email_when_it_is_turned_off(self): |
176 | 308 | r = self.get_debug_token_response(False) | 308 | r = self.get_debug_token_response(False) |
178 | 309 | self.assertEquals(r.status_code, 404) | 309 | self.assertEqual(r.status_code, 404) |
179 | 310 | 310 | ||
180 | 311 | def test_bad_token(self): | 311 | def test_bad_token(self): |
181 | 312 | r = self.client.get('/+bad-token') | 312 | r = self.client.get('/+bad-token') |
183 | 313 | self.assertEquals(r.status_code, 200) | 313 | self.assertEqual(r.status_code, 200) |
184 | 314 | 314 | ||
185 | 315 | def test_non_field_errors_is_not_in_html(self): | 315 | def test_non_field_errors_is_not_in_html(self): |
186 | 316 | r = self.client.post('/+enter_token', { | 316 | r = self.client.post('/+enter_token', { |
187 | @@ -322,7 +322,7 @@ | |||
188 | 322 | 322 | ||
189 | 323 | def test_logout_to_confirm(self): | 323 | def test_logout_to_confirm(self): |
190 | 324 | r = self.client.get('/+logout-to-confirm') | 324 | r = self.client.get('/+logout-to-confirm') |
192 | 325 | self.assertEquals(r.status_code, 200) | 325 | self.assertEqual(r.status_code, 200) |
193 | 326 | 326 | ||
194 | 327 | def test_confirm_account_while_logged_in(self): | 327 | def test_confirm_account_while_logged_in(self): |
195 | 328 | token = self.create_token(at.LoginTokenType.NEWPERSONLESSACCOUNT, | 328 | token = self.create_token(at.LoginTokenType.NEWPERSONLESSACCOUNT, |
196 | @@ -343,7 +343,7 @@ | |||
197 | 343 | # get a valid session | 343 | # get a valid session |
198 | 344 | token1 = 'a' * 16 | 344 | token1 = 'a' * 16 |
199 | 345 | r = _post_new_account(self.client, token=token1) | 345 | r = _post_new_account(self.client, token=token1) |
201 | 346 | self.assertRedirects(r, '/%s/+email-sent' % token1) | 346 | self.assertEqual(r.status_code, 200) |
202 | 347 | 347 | ||
203 | 348 | # claim token | 348 | # claim token |
204 | 349 | r = self.client.post(self._token_url('+newaccount'), | 349 | r = self.client.post(self._token_url('+newaccount'), |
205 | @@ -355,7 +355,7 @@ | |||
206 | 355 | # get a valid session | 355 | # get a valid session |
207 | 356 | token1 = create_token(16) | 356 | token1 = create_token(16) |
208 | 357 | r = _post_new_account(self.client, token=token1) | 357 | r = _post_new_account(self.client, token=token1) |
210 | 358 | self.assertRedirects(r, '/%s/+email-sent' % token1) | 358 | self.assertEqual(r.status_code, 200) |
211 | 359 | 359 | ||
212 | 360 | # claim token | 360 | # claim token |
213 | 361 | r = self.client.post(self._token_url('+newaccount'), | 361 | r = self.client.post(self._token_url('+newaccount'), |
214 | @@ -366,7 +366,7 @@ | |||
215 | 366 | def test_confirm_account_redirect_to_decide_with_rpconfig(self): | 366 | def test_confirm_account_redirect_to_decide_with_rpconfig(self): |
216 | 367 | token1 = create_token(16) | 367 | token1 = create_token(16) |
217 | 368 | r = _post_new_account(self.client, token=token1) | 368 | r = _post_new_account(self.client, token=token1) |
219 | 369 | self.assertRedirects(r, '/%s/+email-sent' % token1) | 369 | self.assertEqual(r.status_code, 200) |
220 | 370 | 370 | ||
221 | 371 | request = {'openid.mode': 'checkid_setup', | 371 | request = {'openid.mode': 'checkid_setup', |
222 | 372 | 'openid.trust_root': 'http://localhost/', | 372 | 'openid.trust_root': 'http://localhost/', |
223 | @@ -664,7 +664,7 @@ | |||
224 | 664 | account.save() | 664 | account.save() |
225 | 665 | 665 | ||
226 | 666 | r = _post_new_account(self.client, email='mark@example.com') | 666 | r = _post_new_account(self.client, email='mark@example.com') |
228 | 667 | self.assertRedirects(r, '/+email-sent') | 667 | self.assertEqual(r.status_code, 200) |
229 | 668 | 668 | ||
230 | 669 | if status == AccountStatus.ACTIVE: | 669 | if status == AccountStatus.ACTIVE: |
231 | 670 | mails_sent = 1 | 670 | mails_sent = 1 |
232 | @@ -736,7 +736,7 @@ | |||
233 | 736 | email.delete() | 736 | email.delete() |
234 | 737 | # use verification token | 737 | # use verification token |
235 | 738 | r = self.client.get("/token/%s/+newemail" % token.token) | 738 | r = self.client.get("/token/%s/+newemail" % token.token) |
237 | 739 | self.assertEquals(r.status_code, 404) | 739 | self.assertEqual(r.status_code, 404) |
238 | 740 | 740 | ||
239 | 741 | def test_confirm_email_get(self): | 741 | def test_confirm_email_get(self): |
240 | 742 | self.authenticate() | 742 | self.authenticate() |
241 | @@ -761,10 +761,10 @@ | |||
242 | 761 | 761 | ||
243 | 762 | token_id = token.pk | 762 | token_id = token.pk |
244 | 763 | 763 | ||
247 | 764 | self.assertEquals(r.status_code, 404) | 764 | self.assertEqual(r.status_code, 404) |
248 | 765 | self.assertEquals(0, at.AuthToken.objects.filter(pk=token_id).count()) | 765 | self.assertEqual(0, at.AuthToken.objects.filter(pk=token_id).count()) |
249 | 766 | email = EmailAddress.objects.get(email='newemail2@example.com') | 766 | email = EmailAddress.objects.get(email='newemail2@example.com') |
251 | 767 | self.assertEquals(EmailStatus.NEW, email.status) | 767 | self.assertEqual(EmailStatus.NEW, email.status) |
252 | 768 | 768 | ||
253 | 769 | def test_config_email_when_not_logged_in_redirects_to_login(self): | 769 | def test_config_email_when_not_logged_in_redirects_to_login(self): |
254 | 770 | account = Account.objects.get_by_email('mark@example.com') | 770 | account = Account.objects.get_by_email('mark@example.com') |
255 | @@ -803,12 +803,12 @@ | |||
256 | 803 | 803 | ||
257 | 804 | def test_invalid_email_does_not_blow_up(self): | 804 | def test_invalid_email_does_not_blow_up(self): |
258 | 805 | r = _post_new_account(self.client, email='what<~@ever.com') | 805 | r = _post_new_account(self.client, email='what<~@ever.com') |
260 | 806 | self.assertEquals(200, r.status_code) | 806 | self.assertEqual(200, r.status_code) |
261 | 807 | self.assertContains(r, 'Invalid email') | 807 | self.assertContains(r, 'Invalid email') |
262 | 808 | 808 | ||
263 | 809 | def test_valid_email_redirects(self): | 809 | def test_valid_email_redirects(self): |
264 | 810 | r = _post_new_account(self.client, email='what@ever.com') | 810 | r = _post_new_account(self.client, email='what@ever.com') |
266 | 811 | self.assertRedirects(r, '/+email-sent') | 811 | self.assertEqual(r.status_code, 200) |
267 | 812 | 812 | ||
268 | 813 | 813 | ||
269 | 814 | @skipOnSqlite | 814 | @skipOnSqlite |
270 | @@ -826,7 +826,7 @@ | |||
271 | 826 | try: | 826 | try: |
272 | 827 | # test normal workflow | 827 | # test normal workflow |
273 | 828 | r = _post_new_account(self.client, email='mark@example.com') | 828 | r = _post_new_account(self.client, email='mark@example.com') |
275 | 829 | self.assertRedirects(r, '/+email-sent') | 829 | self.assertEqual(r.status_code, 200) |
276 | 830 | finally: | 830 | finally: |
277 | 831 | # restore model integrity | 831 | # restore model integrity |
278 | 832 | email.lp_person_id = lp_person_id | 832 | email.lp_person_id = lp_person_id |
279 | @@ -866,10 +866,7 @@ | |||
280 | 866 | response = _post_new_account( | 866 | response = _post_new_account( |
281 | 867 | self.client, | 867 | self.client, |
282 | 868 | email=email) | 868 | email=email) |
287 | 869 | # should redirect to '/+email-sent' | 869 | self.assertEqual(response.status_code, 200) |
284 | 870 | self.assertEqual(response.status_code, 302) | ||
285 | 871 | redirect = response['location'] | ||
286 | 872 | self.assertTrue(redirect.endswith('/+email-sent')) | ||
288 | 873 | 870 | ||
289 | 874 | def test_new_account_captcha_whitelist_with_uuid(self): | 871 | def test_new_account_captcha_whitelist_with_uuid(self): |
290 | 875 | email = 'canonicaltest+something@gmail.com' | 872 | email = 'canonicaltest+something@gmail.com' |
291 | @@ -882,10 +879,7 @@ | |||
292 | 882 | response = _post_new_account( | 879 | response = _post_new_account( |
293 | 883 | self.client, | 880 | self.client, |
294 | 884 | email=email) | 881 | email=email) |
299 | 885 | # should redirect to '/+email-sent' | 882 | self.assertEqual(response.status_code, 200) |
296 | 886 | self.assertEqual(response.status_code, 302) | ||
297 | 887 | redirect = response['location'] | ||
298 | 888 | self.assertTrue(redirect.endswith('/+email-sent')) | ||
300 | 889 | 883 | ||
301 | 890 | def test_new_account_captcha_whitelist_fail(self): | 884 | def test_new_account_captcha_whitelist_fail(self): |
302 | 891 | email = 'notcanonicaltest@gmail.com' | 885 | email = 'notcanonicaltest@gmail.com' |
303 | @@ -976,7 +970,7 @@ | |||
304 | 976 | 970 | ||
305 | 977 | def test_cookie_is_sessionless(self): | 971 | def test_cookie_is_sessionless(self): |
306 | 978 | self.client.get('/+login') | 972 | self.client.get('/+login') |
308 | 979 | self.assertEquals(self._count_sessions(), 0) | 973 | self.assertEqual(self._count_sessions(), 0) |
309 | 980 | 974 | ||
310 | 981 | 975 | ||
311 | 982 | class LoginFlowStatsTestCase(SSOBaseTestCase): | 976 | class LoginFlowStatsTestCase(SSOBaseTestCase): |
312 | 983 | 977 | ||
313 | === modified file 'identityprovider/urls.py' | |||
314 | --- identityprovider/urls.py 2011-12-15 16:29:12 +0000 | |||
315 | +++ identityprovider/urls.py 2011-12-16 12:35:28 +0000 | |||
316 | @@ -59,7 +59,6 @@ | |||
317 | 59 | 59 | ||
318 | 60 | (r'^\+bad-token', 'bad_token'), | 60 | (r'^\+bad-token', 'bad_token'), |
319 | 61 | (r'^\+logout-to-confirm', 'logout_to_confirm'), | 61 | (r'^\+logout-to-confirm', 'logout_to_confirm'), |
320 | 62 | (r'^%(optional_token)s\+email-sent$' % repls, 'email_sent'), | ||
321 | 63 | (r'^debug-token/(?P<email>.*\@.*\..*)$', 'token_from_email'), | 62 | (r'^debug-token/(?P<email>.*\@.*\..*)$', 'token_from_email'), |
322 | 64 | (r'^\+deactivated$', 'deactivated'), | 63 | (r'^\+deactivated$', 'deactivated'), |
323 | 65 | (r'^\+suspended$', 'suspended'), | 64 | (r'^\+suspended$', 'suspended'), |
324 | 66 | 65 | ||
325 | === modified file 'identityprovider/views/account.py' | |||
326 | --- identityprovider/views/account.py 2011-11-21 19:57:56 +0000 | |||
327 | +++ identityprovider/views/account.py 2011-12-16 12:35:28 +0000 | |||
328 | @@ -23,7 +23,7 @@ | |||
329 | 23 | from identityprovider.signals import account_email_added | 23 | from identityprovider.signals import account_email_added |
330 | 24 | 24 | ||
331 | 25 | from identityprovider.views.server import xrds | 25 | from identityprovider.views.server import xrds |
333 | 26 | 26 | from identityprovider.views.ui import display_email_sent | |
334 | 27 | 27 | ||
335 | 28 | @vary_on_headers('Accept') | 28 | @vary_on_headers('Accept') |
336 | 29 | def index(request, token=None): | 29 | def index(request, token=None): |
337 | @@ -124,7 +124,7 @@ | |||
338 | 124 | return HttpResponseRedirect('/+deactivated') | 124 | return HttpResponseRedirect('/+deactivated') |
339 | 125 | 125 | ||
340 | 126 | 126 | ||
342 | 127 | def _send_verification_email(account, session, email, tokenid=None): | 127 | def _send_verification_email(request, email, tokenid=None): |
343 | 128 | # TODO: This makes use of `tokenid` if it's passed in, but the | 128 | # TODO: This makes use of `tokenid` if it's passed in, but the |
344 | 129 | # call to `send_validation_email_request` always generates a new | 129 | # call to `send_validation_email_request` always generates a new |
345 | 130 | # token. Is this right? | 130 | # token. Is this right? |
346 | @@ -134,22 +134,22 @@ | |||
347 | 134 | status=EmailStatus.NEW).delete() | 134 | status=EmailStatus.NEW).delete() |
348 | 135 | 135 | ||
349 | 136 | # Ensure that this account has such email address; return value is not used | 136 | # Ensure that this account has such email address; return value is not used |
352 | 137 | account.emailaddress_set.get_or_create( | 137 | request.user.emailaddress_set.get_or_create( |
353 | 138 | email=email, lp_person=account.person, status=EmailStatus.NEW) | 138 | email=email, lp_person=request.user.person, status=EmailStatus.NEW) |
354 | 139 | 139 | ||
355 | 140 | redirection_url = redirection_url_for_token(tokenid) | 140 | redirection_url = redirection_url_for_token(tokenid) |
356 | 141 | 141 | ||
359 | 142 | token = send_validation_email_request(account, email, redirection_url) | 142 | token = send_validation_email_request(request.user, email, redirection_url) |
360 | 143 | set_session_token_info(session, token) | 143 | set_session_token_info(request.session, token) |
361 | 144 | 144 | ||
367 | 145 | session['email_feedback'] = settings.FEEDBACK_TO_ADDRESS | 145 | return display_email_sent(request, |
368 | 146 | session['email_heading'] = _("Validate your email address") | 146 | _("Validate your email address"), |
369 | 147 | session['email_reason'] = _("We’ve just " | 147 | _("We’ve just " |
370 | 148 | "emailed %(email_to)s (from %(email_from)s) " | 148 | "emailed %(email_to)s (from %(email_from)s) " |
371 | 149 | "with instructions on validating your email address.") % { | 149 | "with instructions on validating your email address.") % { |
372 | 150 | 'email_to': email, | 150 | 'email_to': email, |
375 | 151 | 'email_from': settings.NOREPLY_FROM_ADDRESS, | 151 | 'email_from': settings.NOREPLY_FROM_ADDRESS} |
376 | 152 | } | 152 | ) |
377 | 153 | 153 | ||
378 | 154 | return HttpResponseRedirect('./+email-sent') | 154 | return HttpResponseRedirect('./+email-sent') |
379 | 155 | 155 | ||
380 | @@ -167,8 +167,7 @@ | |||
381 | 167 | account_email_added.send( | 167 | account_email_added.send( |
382 | 168 | openid_identifier=request.user.openid_identifier, | 168 | openid_identifier=request.user.openid_identifier, |
383 | 169 | sender=None) | 169 | sender=None) |
386 | 170 | return _send_verification_email(request.user, request.session, | 170 | return _send_verification_email(request, email, token) |
385 | 171 | email, token) | ||
387 | 172 | 171 | ||
388 | 173 | context = RequestContext(request, {'form': form}) | 172 | context = RequestContext(request, {'form': form}) |
389 | 174 | return render_to_response('account/new_email.html', context) | 173 | return render_to_response('account/new_email.html', context) |
390 | @@ -180,8 +179,7 @@ | |||
391 | 180 | emailid = request.GET.get('id', 0) | 179 | emailid = request.GET.get('id', 0) |
392 | 181 | email = get_object_or_404(request.user.emailaddress_set, pk=emailid, | 180 | email = get_object_or_404(request.user.emailaddress_set, pk=emailid, |
393 | 182 | status=EmailStatus.NEW) | 181 | status=EmailStatus.NEW) |
396 | 183 | return _send_verification_email(request.user, request.session, email.email, | 182 | return _send_verification_email(request, email.email, token) |
395 | 184 | token) | ||
397 | 185 | 183 | ||
398 | 186 | 184 | ||
399 | 187 | @login_required | 185 | @login_required |
400 | 188 | 186 | ||
401 | === modified file 'identityprovider/views/ui.py' | |||
402 | --- identityprovider/views/ui.py 2011-12-15 16:29:12 +0000 | |||
403 | +++ identityprovider/views/ui.py 2011-12-16 12:35:28 +0000 | |||
404 | @@ -455,22 +455,15 @@ | |||
405 | 455 | atoken.sendNewUserEmail(old=old) | 455 | atoken.sendNewUserEmail(old=old) |
406 | 456 | set_session_token_info(request.session, atoken) | 456 | set_session_token_info(request.session, atoken) |
407 | 457 | 457 | ||
411 | 458 | request.session['email_feedback'] = settings.FEEDBACK_TO_ADDRESS | 458 | return display_email_sent(request, |
412 | 459 | request.session['email_heading'] = _("Registration mail sent") | 459 | _("Registration mail sent"), |
413 | 460 | request.session['email_reason'] = _("We’ve just emailed " | 460 | _("We’ve just emailed " |
414 | 461 | "%(email_to)s (from %(email_from)s) to confirm " | 461 | "%(email_to)s (from %(email_from)s) to confirm " |
415 | 462 | "your address.") % { | 462 | "your address.") % { |
416 | 463 | 'email_to': email, | 463 | 'email_to': email, |
427 | 464 | 'email_from': settings.NOREPLY_FROM_ADDRESS, | 464 | 'email_from': settings.NOREPLY_FROM_ADDRESS }, |
428 | 465 | } | 465 | token = token |
429 | 466 | request.session['email_notreceived_extra'] = None | 466 | ) |
420 | 467 | if token is not None: | ||
421 | 468 | # Redirected from an OpenID request, must get back after | ||
422 | 469 | # confirmation | ||
423 | 470 | redirection_url = '/%s/+email-sent' % token | ||
424 | 471 | else: | ||
425 | 472 | redirection_url = '/+email-sent' | ||
426 | 473 | return HttpResponseRedirect(redirection_url) | ||
430 | 474 | else: | 467 | else: |
431 | 475 | polite_form_errors(form._errors) | 468 | polite_form_errors(form._errors) |
432 | 476 | # track number of form errors | 469 | # track number of form errors |
433 | @@ -619,17 +612,14 @@ | |||
434 | 619 | return HttpResponseNotFound() | 612 | return HttpResponseNotFound() |
435 | 620 | 613 | ||
436 | 621 | 614 | ||
439 | 622 | def email_sent(request, token=None): | 615 | def display_email_sent(request, heading, reason, extra=None, token=None): |
438 | 623 | request.token = token | ||
440 | 624 | context = RequestContext(request, { | 616 | context = RequestContext(request, { |
446 | 625 | 'email_feedback': request.session.get('email_feedback'), | 617 | 'email_feedback': settings.FEEDBACK_TO_ADDRESS, |
447 | 626 | 'email_heading': request.session.get('email_heading'), | 618 | 'email_heading': heading, |
448 | 627 | 'email_reason': request.session.get('email_reason'), | 619 | 'email_reason': reason, |
449 | 628 | 'email_notreceived_extra': request.session.get( | 620 | 'email_notreceived_extra': extra, |
445 | 629 | 'email_notreceived_extra'), | ||
450 | 630 | 'token': token, | 621 | 'token': token, |
451 | 631 | }) | 622 | }) |
452 | 632 | |||
453 | 633 | return render_to_response('registration/email_sent.html', context) | 623 | return render_to_response('registration/email_sent.html', context) |
454 | 634 | 624 | ||
455 | 635 | 625 | ||
456 | @@ -708,7 +698,8 @@ | |||
457 | 708 | # on how to create an account | 698 | # on how to create an account |
458 | 709 | brand = unicode(current_brand.name) | 699 | brand = unicode(current_brand.name) |
459 | 710 | subject = u"%s: %s" % (brand, u"Password reset request") | 700 | subject = u"%s: %s" % (brand, u"Password reset request") |
461 | 711 | context = { 'email': email, 'signup': reverse(new_account) } | 701 | url = urljoin(settings.SSO_ROOT_URL, reverse(new_account)) |
462 | 702 | context = { 'email': email, 'signup': url } | ||
463 | 712 | tmpl = '%s/email/invitation.txt' % current_brand.template_dir | 703 | tmpl = '%s/email/invitation.txt' % current_brand.template_dir |
464 | 713 | body = render_to_string(tmpl, context) | 704 | body = render_to_string(tmpl, context) |
465 | 714 | send_mail(subject, body, settings.NOREPLY_FROM_ADDRESS, [email]) | 705 | send_mail(subject, body, settings.NOREPLY_FROM_ADDRESS, [email]) |
466 | @@ -749,24 +740,18 @@ | |||
467 | 749 | 740 | ||
468 | 750 | #regardless of result, we always display the same information back | 741 | #regardless of result, we always display the same information back |
469 | 751 | # to the user | 742 | # to the user |
476 | 752 | # TODO: remove this from session and display via direct render? | 743 | return display_email_sent(request, |
477 | 753 | request.session['email_feedback'] = settings.FEEDBACK_TO_ADDRESS | 744 | _("Forgotten your password?"), |
478 | 754 | request.session['email_heading'] = _("Forgotten your password?") | 745 | _("We’ve just emailed " |
479 | 755 | request.session['email_reason'] = _("We’ve just emailed " | 746 | "%(email_to)s (from %(email_from)s) with " |
480 | 756 | "%(email_to)s (from %(email_from)s) with " | 747 | "instructions on resetting your password.") % { |
475 | 757 | "instructions on resetting your password.") % { | ||
481 | 758 | 'email_to': email, | 748 | 'email_to': email, |
482 | 759 | 'email_from': settings.NOREPLY_FROM_ADDRESS, | 749 | 'email_from': settings.NOREPLY_FROM_ADDRESS, |
493 | 760 | } | 750 | }, |
494 | 761 | request.session['email_notreceived_extra'] = _("Check that " | 751 | _("Check that you’ve actually " |
495 | 762 | "you’ve actually entered a subscribed email address.") | 752 | "entered a subscribed email address."), |
496 | 763 | if token is not None: | 753 | token=token |
497 | 764 | # Redirected from an OpenID request, must get back after | 754 | ) |
488 | 765 | # confirmation | ||
489 | 766 | redirection_url = '/%s/+email-sent' % token | ||
490 | 767 | else: | ||
491 | 768 | redirection_url = '/+email-sent' | ||
492 | 769 | return HttpResponseRedirect(redirection_url) | ||
498 | 770 | else: | 755 | else: |
499 | 771 | # track form errors | 756 | # track form errors |
500 | 772 | stats.increment('flows.forgot_password', key='error.form', | 757 | stats.increment('flows.forgot_password', key='error.form', |
I like avoiding the use of session for this (+1)
- tiny stylistic issue: we prefer singular assertion methods (assertEqual)
- l. 249: always use RequestContext, otherwise context processors won't be applied (you won't get a style page for example)
One last question: given that there were certain precautions not to break the openid dance in this view, are we certain not redirecting will keep those scenarios working?