Merge lp:~canonical-isd-hackers/canonical-identity-provider/session-refactor-email-sent into lp:canonical-identity-provider/release

[Simon Davy]

Refactor to not use session/redirect for displaying email sent page. Instead, we just render the page directly

Refactor to not use session/redirect for displaying email sent page. Instead, we just render the page directly

[Ricardo Kirkner (ricardokirkner)]

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?

review: Needs Fixing

[Simon Davy (bloodearnest)]

> 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 :)

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

=== modified file 'identityprovider/templates/launchpad/email/invitation.txt'
--- identityprovider/templates/launchpad/email/invitation.txt	2011-12-15 16:29:12 +0000
+++ identityprovider/templates/launchpad/email/invitation.txt	2011-12-16 12:35:28 +0000
@@ -1,11 +1,9 @@
 {% load i18n %}
 {% blocktrans %}Hello
 
-You recently requested a password reset for the email account {{ email }} for
-the Launchpad Login Service
+You recently requested a password reset for the email account {{ email }} for the Launchpad Login Service
 
-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 }}
+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 }}
 
 Thanks
 

=== modified file 'identityprovider/templates/ubuntu/email/invitation.txt'
--- identityprovider/templates/ubuntu/email/invitation.txt	2011-12-15 16:29:12 +0000
+++ identityprovider/templates/ubuntu/email/invitation.txt	2011-12-16 12:35:28 +0000
@@ -1,11 +1,9 @@
 {% load i18n %}
 {% blocktrans %}Hello
 
-You recently requested a password reset for the email account {{ email }} for
-the Ubuntu Single Sign On service.
+You recently requested a password reset for the email account {{ email }} for the Ubuntu Single Sign On service.
 
-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 }}
+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 }}
 
 Thanks
 

=== modified file 'identityprovider/tests/test_loginservice.py'
--- identityprovider/tests/test_loginservice.py	2011-12-15 16:29:12 +0000
+++ identityprovider/tests/test_loginservice.py	2011-12-16 12:35:28 +0000
@@ -33,11 +33,11 @@
             'email': '[e-mail]',
         }
         response = self.client.post('/+forgot_password', query)
-        self.assertEquals(response.status_code, 302)
-        self.assertEquals(response['location'],
-                          'http://testserver/+email-sent')
-        self.assertEquals(len(mail.outbox), 1)
-        self.assertEquals(mail.outbox[0].subject,
+        self.assertEqual(response.status_code, 200)
+        self.assertIn("Forgotten your password?", response.content)
+        self.assertIn("[e-mail]", response.content)
+        self.assertEqual(len(mail.outbox), 1)
+        self.assertEqual(mail.outbox[0].subject,
             u"%s: Password reset request" % current_brand.name)
         self.assertTrue('[e-mail]' in mail.outbox[0].body)
         self.assertTrue('+new_account' in mail.outbox[0].body)
@@ -55,11 +55,11 @@
             'passwordconfirm': 'Testing123'
         }
         response = self.client.post('/+new_account', query)
-        self.assertEquals(response.status_code, 302)
-        self.assertEquals(response['location'],
-                          'http://testserver/+email-sent')
-        self.assertEquals(len(mail.outbox), 1)
-        self.assertEquals(mail.outbox[0].subject,
+        self.assertEqual(response.status_code, 200)
+        self.assertIn("Registration mail sent", response.content)
+        self.assertIn("[e-mail]", response.content)
+        self.assertEqual(len(mail.outbox), 1)
+        self.assertEqual(mail.outbox[0].subject,
             u"%s: Warning" % current_brand.name)
         mail.outbox = []
 
@@ -68,11 +68,11 @@
             'email': '[e-mail]',
         }
         response = self.client.post('/+forgot_password', query)
-        self.assertEquals(response.status_code, 302)
-        self.assertEquals(response['location'],
-                          'http://testserver/+email-sent')
-        self.assertEquals(len(mail.outbox), 1)
-        self.assertEquals(mail.outbox[0].subject,
+        self.assertEqual(response.status_code, 200)
+        self.assertIn("Forgotten your password?", response.content)
+        self.assertIn("[e-mail]", response.content)
+        self.assertEqual(len(mail.outbox), 1)
+        self.assertEqual(mail.outbox[0].subject,
             u"%s: Forgotten Password" % current_brand.name)
         mail.outbox = []
 

=== modified file 'identityprovider/tests/test_views_account.py'
--- identityprovider/tests/test_views_account.py	2011-09-09 14:31:09 +0000
+++ identityprovider/tests/test_views_account.py	2011-12-16 12:35:28 +0000
@@ -85,7 +85,7 @@
             account=account,
             status=EmailStatus.NEW)
         r = self.client.get('/+verify-email', {'id': email.id})
-        self.assertEqual(r.status_code, 302)
+        self.assertEqual(r.status_code, 200)
         self.assertEqual(len(mail.outbox), 1)
         # Erasing emails. Necessary to make other tests pass.
         mail.outbox = []
@@ -97,13 +97,13 @@
     def test_new_email_post(self):
         r = self.client.post('/+new-email',
                              {'newemail': "[e-mail]"})
-        self.assertRedirects(r, '/+email-sent')
+        self.assertEqual(r.status_code, 200)
         mail.outbox = []
 
     def test_new_email_post_with_token(self):
         r = self.client.post('/thisissuperrando/+new-email',
                             {'newemail': "[e-mail]"})
-        self.assertEquals(len(mail.outbox), 1)
+        self.assertEqual(len(mail.outbox), 1)
         mail.outbox = []
 
 
@@ -189,7 +189,7 @@
         self.assertRedirects(r, '/+deactivated')
 
         # test token is preserved
-        self.assertEquals(self.client.session.get(token),
+        self.assertEqual(self.client.session.get(token),
                           'raw_orequest content')
 
 
@@ -237,4 +237,4 @@
         r = self.client.post('/+applications', {'token_id': token.token})
 
         self.assertRedirects(r, '/+applications')
-        self.assertEquals(Token.objects.filter(token=token.token).count(), 1)
+        self.assertEqual(Token.objects.filter(token=token.token).count(), 1)

=== modified file 'identityprovider/tests/test_views_ui.py'
--- identityprovider/tests/test_views_ui.py	2011-11-29 18:06:51 +0000
+++ identityprovider/tests/test_views_ui.py	2011-12-16 12:35:28 +0000
@@ -83,7 +83,7 @@
 
         r = _post_new_account(self.client, email='[e-mail]')
 
-        self.assertRedirects(r, '/+email-sent')
+        self.assertEqual(r.status_code, 200)
         self.assertEqual(len(mail.outbox), 1)
 
 
@@ -202,7 +202,7 @@
         session.save()
 
         self.client.get('/%s/+logout' % token)
-        self.assertEquals(self.client.session.get(token),
+        self.assertEqual(self.client.session.get(token),
                           'raw_orequest content')
 
     def create_token(self, token_type, email=None, redirection_url=None,
@@ -253,13 +253,13 @@
 
     def test_claim_unexisting_token(self):
         r = self.client.get('/token/%s/' % 0, {'email': 'fake%40example.com'})
-        self.assertEquals(r.status_code, 404)
+        self.assertEqual(r.status_code, 404)
 
     def test_claim_token_for_unexisting_token_type(self):
         token = self.create_token(9999, '[e-mail]')
         r = self.client.get('/token/%s/' % token.token,
                             {'email': 'fake%40example.com'})
-        self.assertEquals(r.status_code, 404)
+        self.assertEqual(r.status_code, 404)
 
     def test_claim_consumed_token(self):
         self.client.post('/+forgot_password',
@@ -306,11 +306,11 @@
 
     def test_token_form_email_when_it_is_turned_off(self):
         r = self.get_debug_token_response(False)
-        self.assertEquals(r.status_code, 404)
+        self.assertEqual(r.status_code, 404)
 
     def test_bad_token(self):
         r = self.client.get('/+bad-token')
-        self.assertEquals(r.status_code, 200)
+        self.assertEqual(r.status_code, 200)
 
     def test_non_field_errors_is_not_in_html(self):
         r = self.client.post('/+enter_token', {
@@ -322,7 +322,7 @@
 
     def test_logout_to_confirm(self):
         r = self.client.get('/+logout-to-confirm')
-        self.assertEquals(r.status_code, 200)
+        self.assertEqual(r.status_code, 200)
 
     def test_confirm_account_while_logged_in(self):
         token = self.create_token(at.LoginTokenType.NEWPERSONLESSACCOUNT,
@@ -343,7 +343,7 @@
         # get a valid session
         token1 = 'a' * 16
         r = _post_new_account(self.client, token=token1)
-        self.assertRedirects(r, '/%s/+email-sent' % token1)
+        self.assertEqual(r.status_code, 200)
 
         # claim token
         r = self.client.post(self._token_url('+newaccount'),
@@ -355,7 +355,7 @@
         # get a valid session
         token1 = create_token(16)
         r = _post_new_account(self.client, token=token1)
-        self.assertRedirects(r, '/%s/+email-sent' % token1)
+        self.assertEqual(r.status_code, 200)
 
         # claim token
         r = self.client.post(self._token_url('+newaccount'),
@@ -366,7 +366,7 @@
     def test_confirm_account_redirect_to_decide_with_rpconfig(self):
         token1 = create_token(16)
         r = _post_new_account(self.client, token=token1)
-        self.assertRedirects(r, '/%s/+email-sent' % token1)
+        self.assertEqual(r.status_code, 200)
 
         request = {'openid.mode': 'checkid_setup',
                    'openid.trust_root': 'http://localhost/',
@@ -664,7 +664,7 @@
             account.save()
 
             r = _post_new_account(self.client, email='[e-mail]')
-            self.assertRedirects(r, '/+email-sent')
+            self.assertEqual(r.status_code, 200)
 
             if status == AccountStatus.ACTIVE:
                 mails_sent = 1
@@ -736,7 +736,7 @@
         email.delete()
         # use verification token
         r = self.client.get("/token/%s/+newemail" % token.token)
-        self.assertEquals(r.status_code, 404)
+        self.assertEqual(r.status_code, 404)
 
     def test_confirm_email_get(self):
         self.authenticate()
@@ -761,10 +761,10 @@
 
         token_id = token.pk
 
-        self.assertEquals(r.status_code, 404)
-        self.assertEquals(0, at.AuthToken.objects.filter(pk=token_id).count())
+        self.assertEqual(r.status_code, 404)
+        self.assertEqual(0, at.AuthToken.objects.filter(pk=token_id).count())
         email = EmailAddress.objects.get(email='[e-mail]')
-        self.assertEquals(EmailStatus.NEW, email.status)
+        self.assertEqual(EmailStatus.NEW, email.status)
 
     def test_config_email_when_not_logged_in_redirects_to_login(self):
         account = Account.objects.get_by_email('[e-mail]')
@@ -803,12 +803,12 @@
 
     def test_invalid_email_does_not_blow_up(self):
         r = _post_new_account(self.client, email='what<~@ever.com')
-        self.assertEquals(200, r.status_code)
+        self.assertEqual(200, r.status_code)
         self.assertContains(r, 'Invalid email')
 
     def test_valid_email_redirects(self):
         r = _post_new_account(self.client, email='[e-mail]')
-        self.assertRedirects(r, '/+email-sent')
+        self.assertEqual(r.status_code, 200)
 
 
 @skipOnSqlite
@@ -826,7 +826,7 @@
         try:
             # test normal workflow
             r = _post_new_account(self.client, email='[e-mail]')
-            self.assertRedirects(r, '/+email-sent')
+            self.assertEqual(r.status_code, 200)
         finally:
             # restore model integrity
             email.lp_person_id = lp_person_id
@@ -866,10 +866,7 @@
             response = _post_new_account(
                 self.client,
                 email=email)
-            # should redirect to '/+email-sent'
-            self.assertEqual(response.status_code, 302)
-            redirect = response['location']
-            self.assertTrue(redirect.endswith('/+email-sent'))
+            self.assertEqual(response.status_code, 200)
 
     def test_new_account_captcha_whitelist_with_uuid(self):
         email = '[e-mail]'
@@ -882,10 +879,7 @@
             response = _post_new_account(
                 self.client,
                 email=email)
-            # should redirect to '/+email-sent'
-            self.assertEqual(response.status_code, 302)
-            redirect = response['location']
-            self.assertTrue(redirect.endswith('/+email-sent'))
+            self.assertEqual(response.status_code, 200)
 
     def test_new_account_captcha_whitelist_fail(self):
         email = '[e-mail]'
@@ -976,7 +970,7 @@
 
     def test_cookie_is_sessionless(self):
         self.client.get('/+login')
-        self.assertEquals(self._count_sessions(), 0)
+        self.assertEqual(self._count_sessions(), 0)
 
 
 class LoginFlowStatsTestCase(SSOBaseTestCase):

=== modified file 'identityprovider/urls.py'
--- identityprovider/urls.py	2011-12-15 16:29:12 +0000
+++ identityprovider/urls.py	2011-12-16 12:35:28 +0000
@@ -59,7 +59,6 @@
 
     (r'^\+bad-token', 'bad_token'),
     (r'^\+logout-to-confirm', 'logout_to_confirm'),
-    (r'^%(optional_token)s\+email-sent$' % repls, 'email_sent'),
     (r'^debug-token/(?P<email>.*\@.*\..*)$', 'token_from_email'),
     (r'^\+deactivated$', 'deactivated'),
     (r'^\+suspended$', 'suspended'),

=== modified file 'identityprovider/views/account.py'
--- identityprovider/views/account.py	2011-11-21 19:57:56 +0000
+++ identityprovider/views/account.py	2011-12-16 12:35:28 +0000
@@ -23,7 +23,7 @@
 from identityprovider.signals import account_email_added
 
 from identityprovider.views.server import xrds
-
+from identityprovider.views.ui import display_email_sent
 
 @vary_on_headers('Accept')
 def index(request, token=None):
@@ -124,7 +124,7 @@
     return HttpResponseRedirect('/+deactivated')
 
 
-def _send_verification_email(account, session, email, tokenid=None):
+def _send_verification_email(request, email, tokenid=None):
     # TODO: This makes use of `tokenid` if it's passed in, but the
     # call to `send_validation_email_request` always generates a new
     # token.  Is this right?
@@ -134,22 +134,22 @@
         status=EmailStatus.NEW).delete()
 
     # Ensure that this account has such email address; return value is not used
-    account.emailaddress_set.get_or_create(
-        email=email, lp_person=account.person, status=EmailStatus.NEW)
+    request.user.emailaddress_set.get_or_create(
+        email=email, lp_person=request.user.person, status=EmailStatus.NEW)
 
     redirection_url = redirection_url_for_token(tokenid)
 
-    token = send_validation_email_request(account, email, redirection_url)
-    set_session_token_info(session, token)
+    token = send_validation_email_request(request.user, email, redirection_url)
+    set_session_token_info(request.session, token)
 
-    session['email_feedback'] = settings.FEEDBACK_TO_ADDRESS
-    session['email_heading'] = _("Validate your email address")
-    session['email_reason'] = _("We&rsquo;ve just "
-        "emailed %(email_to)s (from %(email_from)s) "
-        "with instructions on validating your email address.") % {
+    return display_email_sent(request,
+        _("Validate your email address"),
+        _("We&rsquo;ve just "
+           "emailed %(email_to)s (from %(email_from)s) "
+           "with instructions on validating your email address.") % {
             'email_to': email,
-            'email_from': settings.NOREPLY_FROM_ADDRESS,
-    }
+            'email_from': settings.NOREPLY_FROM_ADDRESS}
+    )
 
     return HttpResponseRedirect('./+email-sent')
 
@@ -167,8 +167,7 @@
             account_email_added.send(
                 openid_identifier=request.user.openid_identifier,
                 sender=None)
-            return _send_verification_email(request.user, request.session,
-                                            email, token)
+            return _send_verification_email(request, email, token)
 
     context = RequestContext(request, {'form': form})
     return render_to_response('account/new_email.html', context)
@@ -180,8 +179,7 @@
     emailid = request.GET.get('id', 0)
     email = get_object_or_404(request.user.emailaddress_set, pk=emailid,
                               status=EmailStatus.NEW)
-    return _send_verification_email(request.user, request.session, email.email,
-                                    token)
+    return _send_verification_email(request, email.email, token)
 
 
 @login_required

=== modified file 'identityprovider/views/ui.py'
--- identityprovider/views/ui.py	2011-12-15 16:29:12 +0000
+++ identityprovider/views/ui.py	2011-12-16 12:35:28 +0000
@@ -455,22 +455,15 @@
                 atoken.sendNewUserEmail(old=old)
                 set_session_token_info(request.session, atoken)
 
-            request.session['email_feedback'] = settings.FEEDBACK_TO_ADDRESS
-            request.session['email_heading'] = _("Registration mail sent")
-            request.session['email_reason'] = _("We&rsquo;ve just emailed "
+            return display_email_sent(request,
+                _("Registration mail sent"),
+                _("We&rsquo;ve just emailed "
                 "%(email_to)s (from %(email_from)s) to confirm "
                 "your address.") % {
                     'email_to': email,
-                    'email_from': settings.NOREPLY_FROM_ADDRESS,
-            }
-            request.session['email_notreceived_extra'] = None
-            if token is not None:
-                # Redirected from an OpenID request, must get back after
-                # confirmation
-                redirection_url = '/%s/+email-sent' % token
-            else:
-                redirection_url = '/+email-sent'
-            return HttpResponseRedirect(redirection_url)
+                    'email_from': settings.NOREPLY_FROM_ADDRESS },
+                token = token
+                )
         else:
             polite_form_errors(form._errors)
             # track number of form errors
@@ -619,17 +612,14 @@
         return HttpResponseNotFound()
 
 
-def email_sent(request, token=None):
-    request.token = token
+def display_email_sent(request, heading, reason, extra=None, token=None):
     context = RequestContext(request, {
-        'email_feedback': request.session.get('email_feedback'),
-        'email_heading': request.session.get('email_heading'),
-        'email_reason': request.session.get('email_reason'),
-        'email_notreceived_extra': request.session.get(
-            'email_notreceived_extra'),
+        'email_feedback': settings.FEEDBACK_TO_ADDRESS,
+        'email_heading': heading,
+        'email_reason': reason,
+        'email_notreceived_extra': extra,
         'token': token,
     })
-
     return render_to_response('registration/email_sent.html', context)
 
 
@@ -708,7 +698,8 @@
         # on how to create an account
         brand = unicode(current_brand.name)
         subject = u"%s: %s" % (brand, u"Password reset request")
-        context = { 'email': email, 'signup': reverse(new_account) }
+        url = urljoin(settings.SSO_ROOT_URL, reverse(new_account))
+        context = { 'email': email, 'signup': url }
         tmpl = '%s/email/invitation.txt' % current_brand.template_dir
         body = render_to_string(tmpl, context)
         send_mail(subject, body, settings.NOREPLY_FROM_ADDRESS, [email])
@@ -749,24 +740,18 @@
 
             #regardless of result, we always display the same information back
             # to the user
-            # TODO: remove this from session and display via direct render?
-            request.session['email_feedback'] = settings.FEEDBACK_TO_ADDRESS
-            request.session['email_heading'] = _("Forgotten your password?")
-            request.session['email_reason'] = _("We&rsquo;ve just emailed "
-                "%(email_to)s (from %(email_from)s) with "
-                "instructions on resetting your password.") % {
+            return display_email_sent(request,
+                _("Forgotten your password?"),
+                _("We&rsquo;ve just emailed "
+                  "%(email_to)s (from %(email_from)s) with "
+                  "instructions on resetting your password.") % {
                     'email_to': email,
                     'email_from': settings.NOREPLY_FROM_ADDRESS,
-            }
-            request.session['email_notreceived_extra'] = _("Check that "
-                "you&rsquo;ve actually entered a subscribed email address.")
-            if token is not None:
-                # Redirected from an OpenID request, must get back after
-                # confirmation
-                redirection_url = '/%s/+email-sent' % token
-            else:
-                redirection_url = '/+email-sent'
-            return HttpResponseRedirect(redirection_url)
+                    },
+                _("Check that you&rsquo;ve actually "
+                   "entered a subscribed email address."),
+                token=token
+                )
         else:
             # track form errors
             stats.increment('flows.forgot_password', key='error.form',