Merge lp:~james-w/canonical-identity-provider/email-timeout into lp:canonical-identity-provider/release

[James Westby]

Treat email as case-insensitive when checking for verified emails.

The db indexes are for UPPER/LOWER on email, so not using __iexact
means that no index is used. In most other places email is taken
to be case-insensitive.

Hi,

As explained in the commit message use __iexact on an email lookup.

https://oops.canonical.com/oops/?oopsid=OOPS-ac9482fbb99b4e580c1434073ad0aa93 for an example of
the problem this fixes.

Thanks,

James

[James Westby (james-w)]

Updated with tests.

I feel a bit uneasy about this change though. It seems like the inconsistent
treatment of email addresses means that there could be bugs hiding, including
possibly security holes.

Here's what I've found:

management.commands.add_to_team: case-sensitive lookup: mostly fine, just have to lookup with the correct case
management.commands.create_test_team: case-sensitive lookup: would create a new email address with the settings.SSO_TEST_ACCOUNT_EMAIL case.
management.commands.cleanup: case-insensitive deletion
forms: case-insensitive lookup, both for previously invalidated, and duplicate check. However, duplicate check only activates if status=NEW. I don't see where the email is actually created based on this though.
login: case-insensitive lookup against invalidated,
models.emailaddress: create_from_phone_id: no case insensitive check before creation.
views.ui: _finish_account_creation: get_by_email, then case-sensitive lookup. Would just cause a crash if mismatch.
          confirm-email: case-insensitive lookup
          reset_password: case-insensitive lookup
views.account: _send_verification_email: case-insensitive delete, followed by case-sensitive lookup
               invalidate_email: case-sensitive lookup
api.v10.handlers: RegistrationHandler: case-insensitive lookup
                  validate_email: case-insensitive lookup for validation
api.v20.handlers: EmailsHandler: case-sensitive lookup/deletion

There's nothing obviously terrible here, but there is clearly some confusion. I wonder if it
is worth clearing this up? Or whether the differences are warranted?

Thanks,

James

[Ricardo Kirkner (ricardokirkner)]

LGTM. Thanks for the tests.

review: Approve

[Ricardo Kirkner (ricardokirkner)]

> Updated with tests.
>
> I feel a bit uneasy about this change though. It seems like the inconsistent
> treatment of email addresses means that there could be bugs hiding, including
> possibly security holes.
>
> Here's what I've found:
>
> management.commands.add_to_team: case-sensitive lookup: mostly fine, just have
> to lookup with the correct case
> management.commands.create_test_team: case-sensitive lookup: would create a
> new email address with the settings.SSO_TEST_ACCOUNT_EMAIL case.
> management.commands.cleanup: case-insensitive deletion
> forms: case-insensitive lookup, both for previously invalidated, and duplicate
> check. However, duplicate check only activates if status=NEW. I don't see
> where the email is actually created based on this though.
> login: case-insensitive lookup against invalidated,
> models.emailaddress: create_from_phone_id: no case insensitive check before
> creation.
> views.ui: _finish_account_creation: get_by_email, then case-sensitive lookup.
> Would just cause a crash if mismatch.
> confirm-email: case-insensitive lookup
> reset_password: case-insensitive lookup
> views.account: _send_verification_email: case-insensitive delete, followed by
> case-sensitive lookup
> invalidate_email: case-sensitive lookup
> api.v10.handlers: RegistrationHandler: case-insensitive lookup
> validate_email: case-insensitive lookup for validation
> api.v20.handlers: EmailsHandler: case-sensitive lookup/deletion
>
>
> There's nothing obviously terrible here, but there is clearly some confusion.
> I wonder if it
> is worth clearing this up? Or whether the differences are warranted?
>

It's probably worth to work on these issues at some point, at the very least make sure we always use the case insensitive approach.

> Thanks,
>
> James

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~james-w/canonical-identity-provider/email-timeout into lp:canonical-identity-provider failed. Below is the output from the failed tests.

Branching the download cache in dir /mnt/tarmac/cache/canonical-identity-provider/isd-download-cache
[localhost] local: which virtualenv
[localhost] local: /usr/bin/python /usr/bin/virtualenv --distribute --clear --system-site-packages .env
Not deleting .env/bin
New python executable in .env/bin/python
Installing distribute.............................................................................................................................................................................................done.
Installing pip...............done.
[localhost] local: dpkg -l libpq-dev 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l libxml2-dev 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l libxslt1-dev 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l memcached 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l postgresql-plpython-9.1 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l python-dev 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l python-m2crypto 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l swig 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l config-manager 2> /dev/null | grep '^ii' | wc -l
[localhost] local: dpkg -l python-egenix-mx-base-dev 2> /dev/null | grep '^ii' | wc -l
[localhost] local: /usr/lib/config-manager/cm.py update /tmp/tmp5CcF99
[localhost] local: . /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/.env/bin/activate && make install PACKAGES="-r /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/requirements.txt"
pip install --find-links=file:///mnt/tarmac/cache/canonical-identity-provider/isd-download-cache --no-index pip==dev
Ignoring indexes: http://pypi.python.org/simple/
Downloading/unpacking pip==dev
  Running setup.py egg_info for package pip

    warning: no files found matching '*.html' under directory 'docs'
    warning: no previously-included files matching '*.txt' found under directory 'docs/_build'
    no previously-included directories found matching 'docs/_build/_sources'
Installing collected packages: pip
  Found existing installation: pip 1.1
    Uninstalling pip:
      Successfully uninstalled pip
  Running setup.py install for pip

    warning: no files found matching '*.html' under directory 'docs'
    warning: no previously-included files matching '*.txt' found under directory 'docs/_build'
    no previously-included directories found matching 'docs/_build/_sources'
    Installing pip script to /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/.env/bin
    Installing pip-2.7 script to /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/.env/bin
Successfully installed pip
Cleaning up...
pip install --find-links=. --no-index -r /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/requirements.txt
Ignoring indexes: http://pypi.python.org/simple/
Downloading/unpacking bzr+http://bazaar.launchpad.net/~canonical-isd-hackers/u1-test-utils/trunk@81 (from -r /mnt/tarmac/cache/canonical-identity-prov...

=== modified file 'src/identityprovider/emailutils.py'
--- src/identityprovider/emailutils.py	2013-08-12 14:33:47 +0000
+++ src/identityprovider/emailutils.py	2013-08-23 13:39:28 +0000
@@ -95,7 +95,7 @@
 def _should_add_invalidation_link(email):
     account = Account.objects.get_by_email(email)
     if gargoyle.is_active('ALLOW_UNVERIFIED', account):
-        emails = EmailAddress.objects.filter(email=email)
+        emails = EmailAddress.objects.filter(email__iexact=email)
         result = (emails.count() == 0 or
                   emails.filter(status=EmailStatus.NEW).count() > 0)
         return result

=== modified file 'src/identityprovider/tests/test_emailutils.py'
--- src/identityprovider/tests/test_emailutils.py	2013-08-12 16:32:49 +0000
+++ src/identityprovider/tests/test_emailutils.py	2013-08-23 13:39:28 +0000
@@ -24,6 +24,7 @@
     send_templated_email,
     send_twofactor_failure_warning,
     send_validation_email_request,
+    _should_add_invalidation_link,
 )
 from identityprovider.models import Account, AuthToken, EmailAddress
 from identityprovider.models.const import EmailStatus, TokenType
@@ -700,3 +701,44 @@
     def test_recipients(self):
         email = self.send_email()
         self.assertEqual(self.recipients, email.recipients())
+
+
+class ShouldAddInvalidationLinkTests(SSOBaseTestCase):
+
+    @switches(ALLOW_UNVERIFIED=False)
+    def test_disallow_unverified_means_false(self):
+        email = self.factory.make_email_address()
+        self.assertEqual(False, _should_add_invalidation_link(email))
+
+    @switches(ALLOW_UNVERIFIED=True)
+    def test_an_email_means_false(self):
+        email = self.factory.make_email_address()
+        account = self.factory.make_account()
+        self.factory.make_email_for_account(account, email=email,
+                                            status=EmailStatus.VALIDATED)
+        self.assertEqual(False,
+                         _should_add_invalidation_link(email))
+
+    @switches(ALLOW_UNVERIFIED=True)
+    def test_an_email_means_false_different_case(self):
+        email = self.factory.make_email_address()
+        account = self.factory.make_account()
+        self.factory.make_email_for_account(account, email=email,
+                                            status=EmailStatus.VALIDATED)
+        self.assertEqual(False,
+                         _should_add_invalidation_link(email.upper()))
+
+    @switches(ALLOW_UNVERIFIED=True)
+    def test_no_emails_means_true(self):
+        email = self.factory.make_email_address()
+        self.assertEqual(True,
+                         _should_add_invalidation_link(email))
+
+    @switches(ALLOW_UNVERIFIED=True)
+    def test_new_emails_means_true(self):
+        email = self.factory.make_email_address()
+        account = self.factory.make_account()
+        self.factory.make_email_for_account(account, email=email,
+                                            status=EmailStatus.NEW)
+        self.assertEqual(True,
+                         _should_add_invalidation_link(email))