Merge lp:~salgado/launchpad/bug-568102 into lp:launchpad

[Guilherme Salgado]

This branch contains a fix for bug 568102.

The actual fix is trivial and can be seen on r10785, together with its
test.

However, to make the trivial fix possible I had to refactor a few things
on the view. This refactoring is on r10784.

I think it will be easier to review them separately.

[Gary Poster (gary)]

Very nicely done. Good advice to look at the two revisions separately, thank you.

My only suggestion is to add a comment within processPositiveAssertion for the code that creates an account when there was a positive OpenId assertion but no Account--something that just says what the code path is for. On this order:

# We got an OpenID response containing a positive assertion, but we don't have an account for the identifier. We'll create one.

Thank you!

Gary

review: Approve

[Guilherme Salgado (salgado)]

Went with a slightly modified version of that comment:

         # We got an OpenID response containing a positive
         # assertion, but we don't have an account for the
         # identifier or for the email address. We'll create one.

=== modified file 'lib/canonical/launchpad/testing/browser.py'
--- lib/canonical/launchpad/testing/browser.py	2010-03-19 19:57:30 +0000
+++ lib/canonical/launchpad/testing/browser.py	2010-04-27 19:15:40 +0000
@@ -28,7 +28,7 @@
 import weakref
 import transaction
 
-from zope.testbrowser.browser import Browser as _Browser
+from zope.testbrowser.browser import Browser as _Browser, fix_exception_name
 
 from canonical.launchpad.testing.pages import (
     extract_text, find_main_content, find_tag_by_id, get_feedback_messages)

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-04-06 20:40:14 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-04-27 19:15:40 +0000
@@ -262,7 +262,7 @@
     def sreg_response(self):
         return sreg.SRegResponse.fromSuccessResponse(self.openid_response)
 
-    def _createAccount(self, openid_identifier):
+    def _getEmailAddressAndFullName(self):
         # Here we assume the OP sent us the user's email address and
         # full name in the response. Note we can only do that because
         # we used a fixed OP (login.launchpad.net) that includes the
@@ -275,95 +275,119 @@
         email_address = self.sreg_response.get('email')
         full_name = self.sreg_response.get('fullname')
         assert email_address is not None and full_name is not None, (
-            "No email address or full name found in sreg response; "
-            "can't create a new account for this identity URL.")
-        email = getUtility(IEmailAddressSet).getByEmail(email_address)
-        if email is not None:
-            account = email.account
-            assert account is not None, (
-                "This email address should have an associated account.")
-            removeSecurityProxy(account).openid_identifier = openid_identifier
-            if account.status == AccountStatus.DEACTIVATED:
+            "No email address or full name found in sreg response")
+        return email_address, full_name
+
+    def _createAccount(self, openid_identifier, email_address, full_name):
+        account, email = getUtility(IAccountSet).createAccountAndEmail(
+            email_address, PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
+            full_name, password=None, openid_identifier=openid_identifier)
+        return account
+
+    def processPositiveAssertion(self):
+        """Process an OpenID response containing a positive assertion.
+
+        We'll get the account with the given OpenID identifier (creating one 
+        if it doesn't already exist) and act according to the account's state:
+          - If the account is suspended, we stop and render an error page;
+          - If the account is deactivated, we reactivate it and proceed;
+          - If the account is active, we just proceed.
+
+        After that we ensure there's an IPerson associated with the account
+        and login using that account.
+
+        We also update the 'last_write' key in the session if we've done any
+        DB writes, to ensure subsequent requests use the master DB and see
+        the changes we just did.
+        """
+        identifier = self.openid_response.identity_url.split('/')[-1]
+        should_update_last_write = False
+        email_set = getUtility(IEmailAddressSet)
+        # Force the use of the master database to make sure a lagged slave
+        # doesn't fool us into creating a Person/Account when one already
+        # exists.
+        with MasterDatabasePolicy():
+            try:
+                account = getUtility(IAccountSet).getByOpenIDIdentifier(
+                    identifier)
+            except LookupError:
+                # The two lines below are duplicated a few more lines down,
+                # but to avoid this duplication we'd have to refactor most of
+                # our tests to provide an SREG response, which would be rather
+                # painful.
+                email_address, full_name = self._getEmailAddressAndFullName()
+                email = email_set.getByEmail(email_address)
+                if email is None:
+                    # We got an OpenID response containing a positive
+                    # assertion, but we don't have an account for the
+                    # identifier or for the email address.  We'll create one.
+                    account = self._createAccount(
+                        identifier, email_address, full_name)
+                else:
+                    account = email.account
+                    assert account is not None, (
+                        "This email address should have an associated "
+                        "account.")
+                    removeSecurityProxy(account).openid_identifier = (
+                        identifier)
+                should_update_last_write = True
+
+            if account.status == AccountStatus.SUSPENDED:
+                return self.suspended_account_template()
+            elif account.status in [AccountStatus.DEACTIVATED,
+                                    AccountStatus.NOACCOUNT]:
                 comment = 'Reactivated by the user'
-                password = None # Needed just to please reactivate() below.
+                password = '' # Needed just to please reactivate() below.
+                email_address, dummy = self._getEmailAddressAndFullName()
+                email = email_set.getByEmail(email_address)
+                if email is None:
+                    email = email_set.new(email_address, account=account)
                 removeSecurityProxy(account).reactivate(
                     comment, password, removeSecurityProxy(email))
-        else:
-            account, email = getUtility(IAccountSet).createAccountAndEmail(
-                email_address,
-                PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
-                full_name,
-                password=None,
-                openid_identifier=openid_identifier)
-        return account
+            else:
+                # Account is active, so nothing to do.
+                pass
+            if IPerson(account, None) is None:
+                removeSecurityProxy(account).createPerson(
+                    PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
+                should_update_last_write = True
+            self.login(account)
+
+        if should_update_last_write:
+            # This is a GET request but we changed the database, so update
+            # session_data['last_write'] to make sure further requests use
+            # the master DB and thus see the changes we've just made.
+            session_data = ISession(self.request)['lp.dbpolicy']
+            session_data['last_write'] = datetime.utcnow()
+        self._redirect()
+        # No need to return anything as we redirect above.
+        return None
 
     def render(self):
         if self.openid_response.status == SUCCESS:
-            identifier = self.openid_response.identity_url.split('/')[-1]
-            should_update_last_write = False
-            # Force the use of the master database to make sure a lagged slave
-            # doesn't fool us into creating a Person/Account when one already
-            # exists.
-            with MasterDatabasePolicy():
-                try:
-                    account = getUtility(IAccountSet).getByOpenIDIdentifier(
-                        identifier)
-                except LookupError:
-                    account = self._createAccount(identifier)
-                    should_update_last_write = True
-
-                if account.status == AccountStatus.SUSPENDED:
-                    return self.suspended_account_template()
-                elif account.status == AccountStatus.DEACTIVATED:
-                    comment = 'Reactivated by the user'
-                    password = '' # Needed just to please reactivate() below.
-                    sreg_email = self.sreg_response.get('email')
-                    email_address = getUtility(IEmailAddressSet).getByEmail(
-                        sreg_email)
-                    if email_address is None:
-                        email_address = getUtility(IEmailAddressSet).new(
-                            sreg_email, account=account)
-                    removeSecurityProxy(account).reactivate(
-                        comment, password, removeSecurityProxy(email_address))
-                else:
-                    # Account is active, so nothing to do.
-                    pass
-                if IPerson(account, None) is None:
-                    removeSecurityProxy(account).createPerson(
-                        PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
-                    should_update_last_write = True
-                self.login(account)
-
-            if should_update_last_write:
-                # This is a GET request but we changed the database, so update
-                # session_data['last_write'] to make sure further requests use
-                # the master DB and thus see the changes we've just made.
-                session_data = ISession(self.request)['lp.dbpolicy']
-                session_data['last_write'] = datetime.utcnow()
+            return self.processPositiveAssertion()
+
+        if self.account is not None:
+            # The authentication failed (or was canceled), but the user is
+            # already logged in, so we just add a notification message and
+            # redirect.
+            self.request.response.addNoticeNotification(
+                _(u'Your authentication failed but you were already '
+                   'logged into Launchpad.'))
             self._redirect()
             # No need to return anything as we redirect above.
-            retval = None
+            return None
         else:
-            if self.account is not None:
-                # The authentication failed (or was canceled), but the user is
-                # already logged in, so we just add a notification message and
-                # redirect.
-                self.request.response.addNoticeNotification(
-                    _(u'Your authentication failed but you were already '
-                       'logged into Launchpad.'))
-                self._redirect()
-                # No need to return anything as we redirect above.
-                retval = None
-            else:
-                retval = OpenIDLoginErrorView(
-                    self.context, self.request, self.openid_response)()
+            return OpenIDLoginErrorView(
+                self.context, self.request, self.openid_response)()
 
-        # The consumer.complete() call above will create entries in
+    def __call__(self):
+        retval = super(OpenIDCallbackView, self).__call__()
+        # The consumer.complete() call in initialize() will create entries in
         # OpenIDConsumerNonce to prevent replay attacks, but since this will
         # be a GET request, the transaction would be rolled back, so we need
         # an explicit commit here.
         transaction.commit()
-
         return retval
 
     def _redirect(self):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-04-06 20:40:14 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-04-27 19:15:40 +0000
@@ -259,6 +259,28 @@
         # stuff. 
         self.assertLastWriteIsSet(view.request)
 
+    def test_never_used_account(self):
+        # The account was created by one of our scripts but was never
+        # activated, so we just activate it.
+        email = '[e-mail]'
+        account = self.factory.makeAccount(
+            'Test account', email=email, status=AccountStatus.NOACCOUNT)
+        self.assertIs(None, IPerson(account, None))
+        openid_identifier = removeSecurityProxy(account).openid_identifier
+        openid_response = FakeOpenIDResponse(
+            'http://testopenid.dev/+id/%s' % openid_identifier,
+            status=SUCCESS, email=email, full_name=account.displayname)
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view, html = self._createAndRenderView(openid_response)
+        self.assertIsNot(None, IPerson(account, None))
+        self.assertTrue(view.login_called)
+        self.assertEquals(AccountStatus.ACTIVE, account.status)
+        self.assertEquals(email, account.preferredemail.email)
+        # We also update the last_write flag in the session, to make sure
+        # further requests use the master DB and thus see the newly created
+        # stuff. 
+        self.assertLastWriteIsSet(view.request)
+
     def test_suspended_account(self):
         # There's a chance that our OpenID Provider lets a suspended account
         # login, but we must not allow that.