Merge lp:~canonical-isd-hackers/canonical-identity-provider/limit-sql-queries into lp:canonical-identity-provider/release

[Łukasz Czyżykowski]

Limited number of SQL queries when user logs in.

Overview
========
This branch limits number of queries issued by the SSO when user is logging in.

Details
=======
There are couple ways in which the SQL queries are limited:
- using django's ability to query over foreign keys (which means that Django will issue a SQL JOIN),
- using select_related() to get linked models,
- not getting the object twice from the db (LoginForm and login view).

Testing
=======
Follow instructions in README and then '$ fab test' to verify all things are still working. Using $ fab run and logging in you can see exact queries which are issued (with summary saying 11 queries with 2 duplicates).

[Ricardo Kirkner (ricardokirkner)]

Excellent work!

Really like the changes around l. 303-308

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2011-06-29 19:47:22 +0000
+++ .bzrignore	2011-07-15 12:32:45 +0000
@@ -13,3 +13,7 @@
 *.xml
 tags
 .backup
+
+# other
+.pyflymakerc*
+.noseids

=== modified file 'identityprovider/auth.py'
--- identityprovider/auth.py	2011-05-30 14:23:26 +0000
+++ identityprovider/auth.py	2011-07-15 12:32:45 +0000
@@ -9,55 +9,57 @@
 from piston.authentication import OAuthAuthentication
 from piston.oauth import OAuthError as PistonOAuthError
 
-from identityprovider.models import Account, AccountPassword, EmailAddress
+from identityprovider.models import Account, AccountPassword
 from identityprovider.models.api import APIUser
-from identityprovider.models.const import EmailStatus
-from identityprovider.utils import validate_launchpad_password
+from identityprovider.models.const import EmailStatus, AccountStatus
+from identityprovider.utils import (
+    validate_launchpad_password, get_object_or_none)
 
 
 class LaunchpadBackend(object):
 
+    valid_email_statuses = (
+        EmailStatus.PREFERRED, EmailStatus.VALIDATED, EmailStatus.NEW)
+
     def authenticate(self, username=None, password=None,
                      encrypted_password=None):
         try:
-            email = EmailAddress.objects.get(email__iexact=username)
-        except EmailAddress.DoesNotExist:
-            return None
-
-        if email.status not in (EmailStatus.PREFERRED,
-                                EmailStatus.VALIDATED,
-                                EmailStatus.NEW):
-            return None
-        account = email.account
-
-        # only allow authentication if account is active
+            account = Account.objects.select_related('accountpassword').get(
+                         emailaddress__email__iexact=username,
+                         emailaddress__status__in=self.valid_email_statuses)
+        except Account.DoesNotExist:
+            return None
+
         if not account.is_active:
             return None
 
         try:
-            expected_encrypted = account.accountpassword.password
+            ap = account.accountpassword
+            if ap is None:
+                return None
         except AccountPassword.DoesNotExist:
-            # password lost somehow, user should reset it
-            return None
-
-        def ok():
-            account.last_login = datetime.now()
-            return account
-
-        if password is not None:
-            if validate_launchpad_password(password, expected_encrypted):
-                return ok()
-        elif encrypted_password is not None:
-            if encrypted_password == expected_encrypted:
-                return ok()
+            return None
+
+        try:
+            if password is not None:
+                if not validate_launchpad_password(password, ap.password):
+                    return None
+            elif encrypted_password is not None:
+                if encrypted_password != ap.password:
+                    return None
+            else:
+                return None
+        except (UnicodeDecodeError, UnicodeEncodeError):
+            # Password not ASCII, we must fail
+            return None
+
+        account.last_login = datetime.now()
+        return account
 
     def get_user(self, user_id):
         """ Returns a Launchpad Account object instead of Django's built-in
         User object. """
-        try:
-            return Account.objects.get(pk=user_id)
-        except Account.DoesNotExist:
-            return None
+        return get_object_or_none(Account.objects.select_related(), pk=user_id)
 
 
 def basic_authenticate(username, password):
@@ -93,6 +95,7 @@
         resp.status_code = 401
         return resp
 
+
 def oauth_authenticate(oauth_consumer, oauth_token, parameters):
     """Currently only checks that given consumer and token are in database"""
     try:

=== modified file 'identityprovider/forms.py'
--- identityprovider/forms.py	2011-07-11 20:27:02 +0000
+++ identityprovider/forms.py	2011-07-15 12:32:45 +0000
@@ -59,39 +59,26 @@
     def clean(self):
         if 'email' in self.cleaned_data and 'password' in self.cleaned_data:
             email = self.cleaned_data['email']
-            verified = False
-            try:
-                email_addr = EmailAddress.objects.get(email__iexact=email)
-                verified = email_addr.status in [EmailStatus.VALIDATED,
-                        EmailStatus.OLD, EmailStatus.PREFERRED]
-            except EmailAddress.DoesNotExist:
-                pass
-
-            if not verified:
-                raise forms.ValidationError(
-                    mark_safe(_("Password didn't match.")))
-
             password = self.cleaned_data['password']
-            try:
-                user = auth.authenticate(username=email, password=password)
-            except UnicodeEncodeError:
-                # Password not ASCII, we must fail
-                raise forms.ValidationError(mark_safe(
-                    _("Password didn't match.")))
-
-            account = Account.objects.get_by_email(email)
-            if not account.is_active:
-                raise forms.ValidationError(_("Your account has been "
-                    "deactivated"))
-
-            if user is not None and user.is_active:
-                    return self.cleaned_data
-            else:
-                raise forms.ValidationError(mark_safe(
-                    _("Password didn't match.")))
+
+            account = auth.authenticate(username=email, password=password)
+            if account is None:
+                account = Account.objects.get_by_email(email)
+                if account and not account.is_active:
+                    raise forms.ValidationError(_("Your account has been "
+                                                  "deactivated"))
+                else:
+                    raise forms.ValidationError(
+                        mark_safe(_("Password didn't match.")))
+
+            # Pass account to the view, as it's already authenticated and
+            # there's no good reason to do exactly the same call there.
+            self.cleaned_data['account'] = account
+            return self.cleaned_data
 
 
 class ForgotPasswordForm(GenericEmailForm):
+
     def clean_email(self):
         if 'email' in self.cleaned_data:
             email = self.cleaned_data['email']
@@ -109,12 +96,12 @@
     password = fields.CharField(error_messages=default_errors,
         widget=widgets.PasswordInput(attrs={
             'class': 'textType',
-            'size': '20'
+            'size': '20',
         }))
     passwordconfirm = fields.CharField(error_messages=default_errors,
         widget=widgets.PasswordInput(attrs={
             'class': 'textType',
-            'size': '20'
+            'size': '20',
         }))
 
     def clean_password(self):

=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2011-07-07 14:02:24 +0000
+++ identityprovider/models/account.py	2011-07-15 12:32:45 +0000
@@ -16,15 +16,16 @@
 from identityprovider.utils import (
     encrypt_launchpad_password,
     generate_openid_identifier,
+    get_object_or_none,
 )
 
-__all__ = (
+__all__ = [
     'Account',
     'AccountPassword',
     'DisplaynameField',
     'PasswordField',
-    'LPOpenIdIdentifier'
-)
+    'LPOpenIdIdentifier',
+]
 
 
 class AccountManager(models.Manager):
@@ -56,12 +57,8 @@
         return account
 
     def get_by_email(self, email):
-        from identityprovider.models import EmailAddress
-        try:
-            email = EmailAddress.objects.get(email__iexact=email)
-            return email.account
-        except EmailAddress.DoesNotExist:
-            return None
+        return get_object_or_none(Account.objects.select_related(),
+                                  emailaddress__email__iexact=email)
 
 
 class DisplaynameField(models.TextField):
@@ -174,28 +171,36 @@
                     self._person = persons[0]
         return getattr(self, '_person', None)
 
+    @property
+    def user(self):
+        if not hasattr(self, '_user'):
+            # When in read only mode we don't want to create users so we'll
+            # return what is or None if not there
+            if settings.READ_ONLY_MODE:
+                self._user = get_object_or_none(
+                    User, username=self.openid_identifier)
+            else:
+                if self.preferredemail:
+                    self._user, _ = User.objects.get_or_create(
+                        username=self.openid_identifier,
+                        defaults={'email': self.preferredemail})
+                else:
+                    return None
+
+        return self._user
+
     def _get_last_login(self):
-        try:
-            user = User.objects.get(username=self.openid_identifier)
-            return user.last_login
-        except User.DoesNotExist:
-            return None
+        if self.user is not None:
+            return self.user.last_login
 
     def _set_last_login(self, login_datetime):
+        # we can live without last_login
         if settings.READ_ONLY_MODE:
-            # we can live without last_login
             return
-        try:
-            user = User.objects.get(username=self.openid_identifier)
-        except User.DoesNotExist:
-            email = self.preferredemail
-            if email is None:
-                return
-            else:
-                user = User.objects.create_user(self.openid_identifier,
-                                                email.email)
-        user.last_login = login_datetime
-        user.save()
+
+        if self.user is not None:
+            self.user.last_login = login_datetime
+            self.user.save()
 
     last_login = property(_get_last_login, _set_last_login)
 

=== modified file 'identityprovider/tests/test_models_account.py'
--- identityprovider/tests/test_models_account.py	2011-07-07 14:02:24 +0000
+++ identityprovider/tests/test_models_account.py	2011-07-15 12:32:45 +0000
@@ -40,8 +40,7 @@
 
     def test_last_login_when_user_doesnt_exists(self):
         account = self.get_new_account()
-
-        self.assertTrue(account.last_login is None)
+        self.assertTrue(account.last_login is not None)
 
     def test_last_login_when_user_exists(self):
         account = self.get_existing_account()
@@ -75,12 +74,14 @@
 
     def test_set_last_login_when_no_preferredemail(self):
         account = self.get_new_account()
-        for email in account.emailaddress_set.all():
-            email.status = EmailStatus.NEW
-            email.save()
+        account.emailaddress_set.all().update(status=EmailStatus.NEW)
         account.last_login = datetime.now()
-        self.assertRaises(User.DoesNotExist, User.objects.get,
-            username=account.openid_identifier)
+        # Changed from assertRaises, as using it to test for database
+        # exceptions causes transaction to be aborted and all tests following
+        # it will fail. (It's only the case if the test case inherits
+        # django.test.TestCase)
+        self.assertEqual(
+            0, User.objects.filter(username=account.openid_identifier).count())
 
     def test_is_active(self):
         account = self.get_new_account()

=== modified file 'identityprovider/tests/test_views_ui.py'
--- identityprovider/tests/test_views_ui.py	2011-07-12 21:42:47 +0000
+++ identityprovider/tests/test_views_ui.py	2011-07-15 12:32:45 +0000
@@ -753,7 +753,7 @@
         self.assertTemplateUsed(r, 'account/suspended.html')
 
     def test_invalid_email_does_not_blow_up(self):
-        r = _post_new_account(self.client, email='what~@ever.com')
+        r = _post_new_account(self.client, email='what<~@ever.com')
         self.assertEquals(200, r.status_code)
         self.assertContains(r, 'Invalid email')
 

=== modified file 'identityprovider/utils.py'
--- identityprovider/utils.py	2011-06-29 21:29:09 +0000
+++ identityprovider/utils.py	2011-07-15 12:32:45 +0000
@@ -12,6 +12,7 @@
 from email.Header import Header
 from email.Utils import formataddr
 
+from django.core.exceptions import ObjectDoesNotExist
 from django.utils.translation import ugettext as _
 
 SALT_LENGTH = 20
@@ -176,6 +177,15 @@
     return data, headers
 
 
+def get_object_or_none(qs_or_model, **kwargs):
+    if hasattr(qs_or_model, 'objects'):
+        qs_or_model = qs_or_model.objects.all()
+    try:
+        return qs_or_model.get(**kwargs)
+    except ObjectDoesNotExist:
+        return None
+
+
 def is_django_13():
     import django
     return django.VERSION[:2] == (1, 3)

=== modified file 'identityprovider/views/ui.py'
--- identityprovider/views/ui.py	2011-07-12 21:42:47 +0000
+++ identityprovider/views/ui.py	2011-07-15 12:32:45 +0000
@@ -24,9 +24,9 @@
 from identityprovider.branding import current_brand
 from identityprovider.decorators import (check_readonly, dont_cache,
     guest_required, limitlogin, requires_cookies)
-from identityprovider.forms import (LoginForm, ForgotPasswordForm,
-    ResetPasswordForm, OldNewAccountForm, NewAccountForm, ConfirmNewAccountForm,
-    TokenForm)
+from identityprovider.forms import (
+    LoginForm, ForgotPasswordForm, ResetPasswordForm, OldNewAccountForm,
+    NewAccountForm, ConfirmNewAccountForm, TokenForm)
 from identityprovider.models import (Account, AccountPassword, AuthToken,
     AuthTokenFactory, EmailAddress, get_type_of_token, verify_token_string)
 from identityprovider.models.authtoken import InvalidEmailException
@@ -66,10 +66,8 @@
     if request.method == 'POST':
         form = LoginForm(request.POST)
         if form.is_valid():
-            username = form.cleaned_data['email']
-            password = form.cleaned_data['password']
-            user = auth.authenticate(username=username, password=password)
-            auth.login(request, user)
+            account = form.cleaned_data['account']
+            auth.login(request, account)
             next = request.POST.get('next')
             limitlogin().reset_count(request)
             if next and _is_safe_redirect_url(next):

=== modified file 'identityprovider/views/utils.py'
--- identityprovider/views/utils.py	2010-06-17 02:10:58 +0000
+++ identityprovider/views/utils.py	2011-07-15 12:32:45 +0000
@@ -12,6 +12,7 @@
     else:
         return "/"
 
+
 def set_session_token_info(session, token):
     """Places information about the token into the session in a
     uniform place.  Specifically, places the token's e-mail address at

=== modified file 'wsgi_test_server.py'
--- wsgi_test_server.py	2011-02-22 00:01:16 +0000
+++ wsgi_test_server.py	2011-07-15 12:32:45 +0000
@@ -17,7 +17,7 @@
 from django.conf import settings
 from django.core import mail
 from django.db import connection
-from django.test.utils import TestSMTPConnection
+from django.core.mail.backends import locmem
 
 from wsgiref.simple_server import (make_server, WSGIRequestHandler,
                                    ServerHandler)
@@ -34,7 +34,7 @@
 )
 settings.SSO_RESTRICT_RP = False
 # Set up email sending into a sandbox
-mail.SMTPConnection = TestSMTPConnection
+mail.SMTPConnection = locmem.EmailBackend
 mail.outbox = []
 
 server_port = 80  # We need to be run here for the LP tests to find us