Merge lp:~jamestait/canonical-identity-provider/lp1648775-email-reminder-of-paper-exhaustion into lp:canonical-identity-provider/release

[James Tait]

Send an email reminder to renew paper codes that are close to exhaustion, as long as another list exists that does not need renewal (otherwise renewal is forced).

In the case where a user has multiple lists of paper codes and at least one is
close to exhaustion and at least one is not, send an email reminder to renew
the lists that are close to exhaustion. In the case where all lists are close
to exhaustion (where all can also mean all of a list of length 1), renewal is
forced at login.

[Daniel Manrique (roadmr)]

A couple of questions/nitpicks below.

review: Needs Information

[Daniel Manrique (roadmr)]

looks mostly good to me now, just let me know if you want to remove the "assert" and handle absence of preferredemail some other way (since you said you didn't want it to blow up).

[James Tait (jamestait)]

IRC conversation log:

2017-05-08T18:47:23+0100 <@roadmr> jayteeuk: so about the e-mail reminder thing...
2017-05-08T18:48:36+0100 < jayteeuk> roadmr, yeah. What do you think about the AuthToken?
2017-05-08T18:48:40+0100 <@roadmr> jayteeuk: authtokens are tricky because the token by design is lost once the e-mail is sent, so testing them is a bit messy. Would just directing the user to his devices page (instead of an individual link per almost-expired device) obviate the need for an extra token?
2017-05-08T18:51:24+0100 <@roadmr> jayteeuk: example, https://login.ubuntu.com/device-list is *your* device list, so there's nothing secret to tokenize there
2017-05-08T18:51:59+0100 <@roadmr> jayteeuk: if we clearly flag the about-to-expire devices there (may already be done in the other branch?) then we shouldn't need any trickery to protect the link
2017-05-08T18:52:10+0100 < jayteeuk> roadmr, I'm thinking about the case where the user has logged in, taken a device over the reminder threshold, logged out and then seen the email.
2017-05-08T18:52:47+0100 < jayteeuk> roadmr, right - because device-list is just the list for whichever user happens to then login.
2017-05-08T18:53:21+0100 <@roadmr> jayteeuk: I see.. so how about the "safety factor" idea? give them warning when there are n codes left (n > 1)
2017-05-08T18:53:45+0100 < jayteeuk> But are they able to login if their codes are exhausted? The e-mail should only have been sent if they have another set of paper codes they can use, so they ought to have at least one other OTP device.
2017-05-08T18:54:19+0100 < jayteeuk> roadmr, the exhaustion limit is configurable.
2017-05-08T18:54:23+0100 <@roadmr> jayteeuk: ah awesome :)
2017-05-08T18:54:45+0100 <@roadmr> jayteeuk: well if we know by design they have extra codes to use, then just the link should suffice, I guess
2017-05-08T18:54:53+0100 < jayteeuk> So for tests it's 1, but I think it's 3 in production.
2017-05-08T18:58:11+0100 < jayteeuk> roadmr, it's a tricky little area, because we don't want to make it possible to hijack the account, but nor do we want to lock them out of the account in the case where they happen to have two printed lists associated with the account because they lost the first and just created a new one.
2017-05-08T18:59:05+0100 <@roadmr> hm, interesting scenario
2017-05-08T18:59:30+0100 < jayteeuk> Maybe I'm over-thinking it (which is not unusual).
2017-05-08T19:01:07+0100 <@roadmr> hehe :) that's how you catch corner cases
2017-05-08T19:15:49+0100 < jayteeuk> roadmr, so WDYT? Should we go with the AuthToken approach to ensure we keep the account secure without risking locking the user out?
2017-05-08T19:18:11+0100 <@roadmr> jayteeuk: when would you send an authtoken'd link? if I had one of my paper devices close to exhaustion?
2017-05-08T19:18:30+0100 <@roadmr> 1- I use a paper code and go under the threshold (potentially meaning I have no more codes to use)
2017-05-08T19:18:44+0100 <@roadmr> 2- I get a link with which I can go into the account and... what?
2017-05-08T19:19:53+0100 < jayteeuk> roadmr: If the account only has one paper device and that device drops below the threshold (i.e. the user just used it), we force ...

[Daniel Manrique (roadmr)]

+1, FWIW most users *should* have a preferred email, but I think it's good to just warn instead of blowing up if someone doesn't. If we care about users having a preferred email, we could enforce that some other way. But "that is another story and shall be told another time".

review: Approve

=== modified file 'src/identityprovider/emailutils.py'
--- src/identityprovider/emailutils.py	2017-02-26 13:08:57 +0000
+++ src/identityprovider/emailutils.py	2017-05-09 18:04:11 +0000
@@ -222,6 +222,26 @@
     return token, invalidate_email_token
 
 
+def send_printed_codes_renewal_email(root_url, account, devices):
+    email = account.preferredemail
+    if email is None:
+        logger.warning(
+            'Unable to find a suitable email address to send OTP reminder '
+            'for account %s', account)
+        return
+    email = email.email
+    device_urls = []
+    for device in devices:
+        device_urls.append(
+            urljoin(root_url,
+                    reverse('device-print', kwargs=dict(device_id=device.id))))
+    context = dict(display_name=account.displayname, device_urls=device_urls)
+    send_fancy_email(
+        _("Printed list of backup codes is nearly used up"),
+        'email/printed-codes-nearly-exhausted-warning.txt',
+        context=context, email=email)
+
+
 def send_validation_email_request(root_url, account, email,
                                   redirection_url=None, oid_token=None):
     """

=== added file 'src/identityprovider/templates/email/printed-codes-nearly-exhausted-warning.txt'
--- src/identityprovider/templates/email/printed-codes-nearly-exhausted-warning.txt	1970-01-01 00:00:00 +0000
+++ src/identityprovider/templates/email/printed-codes-nearly-exhausted-warning.txt	2017-05-09 18:04:11 +0000
@@ -0,0 +1,19 @@
+{% load i18n %}{% load static_url %}{% blocktrans %}Hello {{ display_name }}!{% endblocktrans %}
+
+{% if device_urls|length == 1 %}
+{% blocktrans %}Your printed list of backup codes is nearly used up. Please
+print a new list by visiting the following page:{% endblocktrans %}
+{% else %}
+{% blocktrans %}Some of your printed lists of backup codes are nearly used up.
+Please print new lists by visiting the following pages:{% endblocktrans %}
+{% endif %}
+
+{% for device_url in device_urls %}
+ - {{ device_url }}
+{% endfor %}
+
+{% blocktrans %}If you have any questions, please contact us at:{% endblocktrans %}
+
+{{ "support_form"|static_url }}
+
+{% include "email/thank_you.txt" %}

=== modified file 'src/identityprovider/tests/test_emailutils.py'
--- src/identityprovider/tests/test_emailutils.py	2017-02-26 13:08:57 +0000
+++ src/identityprovider/tests/test_emailutils.py	2017-05-09 18:04:11 +0000
@@ -6,6 +6,7 @@
 import unittest
 from urlparse import urlparse, urlunparse
 
+from django.conf import settings
 from django.core import mail
 from django.core.urlresolvers import reverse
 from django.template.loader import render_to_string
@@ -27,6 +28,7 @@
     send_notification_to_invalidated_email_address,
     send_password_reset_email,
     send_preferred_changed_notification,
+    send_printed_codes_renewal_email,
     send_templated_email,
     send_twofactor_failure_warning,
     send_validation_email_request,
@@ -648,6 +650,75 @@
         self.test_send_action_applied_notice(action='delete',
                                              result='deleted')
 
+    def test_send_printed_codes_renewal_email_with_one_exhausted(self):
+        counter = (settings.TWOFACTOR_PAPER_CODES -
+                   settings.TWOFACTOR_PAPER_CODES_WARN_RENEWAL + 1)
+        device = self.factory.make_device(
+            account=self.account, device_type='paper', counter=counter)
+        send_printed_codes_renewal_email(
+            self.sso_root_url, self.account, [device])
+        url = self.request.build_absolute_uri(
+            reverse('device-print', kwargs=dict(device_id=device.id)))
+        context = dict(display_name=self.account.displayname,
+                       device_urls=[url])
+        self.assert_email_properly_formatted(
+            'Printed list of backup codes is nearly used up',
+            'email/printed-codes-nearly-exhausted-warning.txt',
+            context=context)
+        email = mail.outbox[0]
+        self.assertIn(
+            'Your printed list of backup codes is nearly used up', email.body)
+        self.assertIn(
+            'print a new list by visiting the following page',
+            email.body)
+        self.assertIn(url, email.body)
+
+    def test_send_printed_codes_renewal_email_with_many_exhausted(self):
+        counter = (settings.TWOFACTOR_PAPER_CODES -
+                   settings.TWOFACTOR_PAPER_CODES_WARN_RENEWAL + 1)
+        device1 = self.factory.make_device(
+            account=self.account, device_type='paper', counter=counter)
+        device2 = self.factory.make_device(
+            account=self.account, device_type='paper', counter=counter)
+        send_printed_codes_renewal_email(
+            self.sso_root_url, self.account, [device1, device2])
+        url1 = self.request.build_absolute_uri(
+            reverse('device-print', kwargs=dict(device_id=device1.id)))
+        url2 = self.request.build_absolute_uri(
+            reverse('device-print', kwargs=dict(device_id=device2.id)))
+        context = dict(display_name=self.account.displayname,
+                       device_urls=[url1, url2])
+        self.assert_email_properly_formatted(
+            'Printed list of backup codes is nearly used up',
+            'email/printed-codes-nearly-exhausted-warning.txt',
+            context=context)
+        email = mail.outbox[0]
+        self.assertIn(
+            'Some of your printed lists of backup codes are nearly used up',
+            email.body)
+        self.assertIn(
+            'print new lists by visiting the following pages',
+            email.body)
+        for url in (url1, url2):
+            self.assertIn(url, email.body)
+
+    def test_send_printed_codes_renewal_email_with_one_exhausted_no_addr(self):
+        self.account.emails().delete()
+        assert self.account.emails().count() == 0
+        # Force re-evaluation of account.preferredemail
+        self.account._preferredemail = None
+        counter = (settings.TWOFACTOR_PAPER_CODES -
+                   settings.TWOFACTOR_PAPER_CODES_WARN_RENEWAL + 1)
+        device = self.factory.make_device(
+            account=self.account, device_type='paper', counter=counter)
+        with patch('identityprovider.emailutils.logger') as mock_logger:
+            send_printed_codes_renewal_email(
+                self.sso_root_url, self.account, [device])
+        self.assertEqual(len(mail.outbox), 0)
+        mock_logger.warning.assert_called_once_with(
+            'Unable to find a suitable email address to send OTP reminder '
+            'for account %s', self.account)
+
 
 class SendEmailToPreferredAddressTestCase(SendEmailTestCase):
 

=== modified file 'src/webui/tests/test_views_account.py'
--- src/webui/tests/test_views_account.py	2017-04-19 15:36:34 +0000
+++ src/webui/tests/test_views_account.py	2017-05-09 18:04:11 +0000
@@ -587,7 +587,7 @@
     user_wants_twofactor = False
     user_wants_warn = True
     devices_count = 0
-    paper_device_exhausted = 0
+    paper_device_exhausted = False
 
     def setUp(self):
         super(AccountEditTestCase, self).setUp()
@@ -635,7 +635,7 @@
 
     def test_codes_nearly_exhausted_warning(self):
         response = self.client.get(self.url)
-        devices = self.account.devices.all()
+        devices = list(self.account.paper_devices_needing_renewal)
 
         if (self.twofactor_enabled and self.devices_count > 0 and
                 self.paper_device_exhausted):

=== modified file 'src/webui/tests/test_views_ui.py'
--- src/webui/tests/test_views_ui.py	2017-04-24 17:17:02 +0000
+++ src/webui/tests/test_views_ui.py	2017-05-09 18:04:11 +0000
@@ -2110,6 +2110,62 @@
         self.assertTrue(ui.twofactor.authenticate(account, otp))
 
 
+class PrintedCodesRenewalEmailTestCase(BaseTwoFactorLoginTestCase):
+    """We have two paper devices, one of which needs renewal - send email."""
+
+    def test_email_reminder_when_one_of_many_needs_renewal(self):
+        counter = (settings.TWOFACTOR_PAPER_CODES -
+                   settings.TWOFACTOR_PAPER_CODES_WARN_RENEWAL + 1)
+        nearly_exhausted = self.factory.make_device(
+            account=self.account, counter=counter, device_type='paper')
+        self.factory.make_device(account=self.account, device_type='paper')
+        self.patch('webui.views.ui.twofactor.authenticate', return_value=True)
+        self.do_login(next=reverse('account-edit'))
+        self.do_twofactor(oath_token='123456')
+        self.assertEqual(len(mail.outbox), 1)
+        email = mail.outbox[0]
+        self.assertEqual(email.to, [self.email])
+        self.assertIn(
+            'Printed list of backup codes is nearly used up', email.subject)
+        self.assertIn(
+            'Your printed list of backup codes is nearly used up.', email.body)
+        url = reverse(
+            'device-print', kwargs=dict(device_id=nearly_exhausted.id))
+        self.assertIn(url, email.body)
+
+    def test_email_reminder_when_many_of_many_need_renewal(self):
+        counter = (settings.TWOFACTOR_PAPER_CODES -
+                   settings.TWOFACTOR_PAPER_CODES_WARN_RENEWAL + 1)
+        nearly_exhausted1 = self.factory.make_device(
+            account=self.account, counter=counter, device_type='paper')
+        nearly_exhausted2 = self.factory.make_device(
+            account=self.account, counter=counter, device_type='paper')
+        self.factory.make_device(account=self.account, device_type='paper')
+        self.patch('webui.views.ui.twofactor.authenticate', return_value=True)
+        self.do_login(next=reverse('account-edit'))
+        self.do_twofactor(oath_token='123456')
+        self.assertEqual(len(mail.outbox), 1)
+        email = mail.outbox[0]
+        self.assertEqual(email.to, [self.email])
+        self.assertIn(
+            'Printed list of backup codes is nearly used up', email.subject)
+        self.assertIn(
+            'Some of your printed lists of backup codes are nearly used up',
+            email.body)
+        for device in (nearly_exhausted1, nearly_exhausted2):
+            url = reverse(
+                'device-print', kwargs=dict(device_id=device.id))
+            self.assertIn(url, email.body)
+
+    def test_no_email_reminder_unless_one_of_many_needs_renewal(self):
+        self.factory.make_device(account=self.account, device_type='paper')
+        self.factory.make_device(account=self.account, device_type='paper')
+        self.patch('webui.views.ui.twofactor.authenticate', return_value=True)
+        self.do_login(next=reverse('account-edit'))
+        self.do_twofactor(oath_token='123456')
+        self.assertEqual(len(mail.outbox), 0)
+
+
 class ComboViewTestCase(SSOBaseTestCase):
 
     def test_combine_files(self):

=== modified file 'src/webui/views/ui.py'
--- src/webui/views/ui.py	2017-04-24 17:17:02 +0000
+++ src/webui/views/ui.py	2017-05-09 18:04:11 +0000
@@ -34,6 +34,7 @@
 from gargoyle import gargoyle
 
 from identityprovider import emailutils
+from identityprovider.emailutils import send_printed_codes_renewal_email
 from identityprovider.forms import (
     LoginForm,
     NewAccountForm,
@@ -348,14 +349,20 @@
 
         next_url = request.POST.get('next')
         paper_devices = request.user.devices.filter(device_type='paper')
+        paper_devices_needing_renewal = list(
+            request.user.paper_devices_needing_renewal)
         # We only force renewal if all paper devices are at or near exhaustion
         needs_renewal = (paper_devices and len(paper_devices) ==
-                         len(list(request.user.paper_devices_needing_renewal)))
+                         len(paper_devices_needing_renewal))
         if token and needs_renewal:
             url = reverse('device-list', kwargs=dict(token=token))
             if next_url:
                 url += "?next=" + urlquote(next_url)
             return HttpResponseRedirect(url)
+        elif paper_devices_needing_renewal:
+            root_url = request.build_absolute_uri('/')
+            send_printed_codes_renewal_email(
+                root_url, request.user, paper_devices_needing_renewal)
         if next_url and is_safe_redirect_url(next_url):
             return HttpResponseRedirect(next_url)
         elif token: