Merge lp:~salgado/launchpad/create-account-for-unseen-openid-id into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

We'll soon break the auth tables out of the replication cluster, which
means we'll have one copy of them used by login.lp.net (and
login.ubuntu.com) and a separate one used by launchpad.net.

That means accounts created after the split won't be seen by Launchpad,
so we need to automatically create them when we see a new identity URL.

This is a temporary thing as soon afterwards we'll refactor the Account
table used by launchpad.net -- at that time we'll only need to register
the identity URL and maybe associate it with an existing profile.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/database/account.py
  lib/canonical/launchpad/webapp/login.py
  lib/canonical/launchpad/webapp/tests/test_login.py
  lib/canonical/launchpad/webapp/tests/no-anonymous-session-cookies.txt
  lib/canonical/launchpad/webapp/tests/cookie-authentication.txt
  lib/canonical/launchpad/webapp/tests/login.txt

This is a lie; Browser is defined in the test globals.

== Pyflakes Doctest notices ==

lib/canonical/launchpad/webapp/tests/login.txt
    11: undefined name 'Browser'

[Michael Nelson (michael.nelson)]

Hi Guilherme,

I've just got a few small questions below, and one suggestion for the name of your context manager.

Cheers,
-Michael

> === modified file 'lib/canonical/launchpad/webapp/login.py'
> --- lib/canonical/launchpad/webapp/login.py 2010-02-18 10:52:51 +0000
> +++ lib/canonical/launchpad/webapp/login.py 2010-02-25 11:53:51 +0000
> @@ -12,6 +12,7 @@
> from BeautifulSoup import UnicodeDammit
>
> from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
> +from openid.extensions import sreg
> from openid.fetchers import setDefaultFetcher, Urllib2Fetcher
>
> import transaction
> @@ -27,7 +28,6 @@
>
> from z3c.ptcompat import ViewPageTemplateFile
>
> -from canonical.cachedproperty import cachedproperty
> from canonical.config import config
> from canonical.launchpad import _
> from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
> @@ -181,6 +181,8 @@
> openid_vhost = config.launchpad.openid_provider_vhost
> self.openid_request = consumer.begin(
> allvhosts.configs[openid_vhost].rooturl)
> + self.openid_request.addExtension(
> + sreg.SRegRequest(optional=['email', 'fullname']))
>
> trust_root = self.request.getApplicationURL()
> assert not self.openid_request.shouldSendRedirect(), (
> @@ -258,8 +260,34 @@
>
> def render(self):
> if self.openid_response.status == SUCCESS:
> - account = getUtility(IAccountSet).getByOpenIDIdentifier(
> - self.openid_response.identity_url.split('/')[-1])
> + identifier = self.openid_response.identity_url.split('/')[-1]
> + account_set = getUtility(IAccountSet)
> + try:
> + account = account_set.getByOpenIDIdentifier(identifier)
> + except LookupError:
> + # 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
> + # user's email address in the response when asked to. Once we

s/email address/email address and full name ? Or is full name returned from
all OP's when requested?

> + # start using other OPs we won't be able to make this
> + # assumption here as they might not include what we want in
> + # the response.
> + sreg_response = sreg.SRegResponse.fromSuccessResponse(
> + self.openid_response)
> + assert sreg_response is not None, (
> + "OP didn't include an sreg extension in the response.")
> + email_address = sreg_response.get('email')
> + full_name = 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.")
> + account, email = account_set.createAccountAndEmail(
> + email_address,
> + PersonCreationRati...

review: Approve (code)

[Guilherme Salgado (salgado)]

On Thu, 2010-02-25 at 12:30 +0000, Michael Nelson wrote:
> Review: Approve code
> Hi Guilherme,
>
> I've just got a few small questions below, and one suggestion for the name of your context manager.
>
> Cheers,
> -Michael
>
> > === modified file 'lib/canonical/launchpad/webapp/login.py'
> > --- lib/canonical/launchpad/webapp/login.py 2010-02-18 10:52:51 +0000
> > +++ lib/canonical/launchpad/webapp/login.py 2010-02-25 11:53:51 +0000
> > @@ -12,6 +12,7 @@
> > from BeautifulSoup import UnicodeDammit
> >
> > from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
> > +from openid.extensions import sreg
> > from openid.fetchers import setDefaultFetcher, Urllib2Fetcher
> >
> > import transaction
> > @@ -27,7 +28,6 @@
> >
> > from z3c.ptcompat import ViewPageTemplateFile
> >
> > -from canonical.cachedproperty import cachedproperty
> > from canonical.config import config
> > from canonical.launchpad import _
> > from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
> > @@ -181,6 +181,8 @@
> > openid_vhost = config.launchpad.openid_provider_vhost
> > self.openid_request = consumer.begin(
> > allvhosts.configs[openid_vhost].rooturl)
> > + self.openid_request.addExtension(
> > + sreg.SRegRequest(optional=['email', 'fullname']))
> >
> > trust_root = self.request.getApplicationURL()
> > assert not self.openid_request.shouldSendRedirect(), (
> > @@ -258,8 +260,34 @@
> >
> > def render(self):
> > if self.openid_response.status == SUCCESS:
> > - account = getUtility(IAccountSet).getByOpenIDIdentifier(
> > - self.openid_response.identity_url.split('/')[-1])
> > + identifier = self.openid_response.identity_url.split('/')[-1]
> > + account_set = getUtility(IAccountSet)
> > + try:
> > + account = account_set.getByOpenIDIdentifier(identifier)
> > + except LookupError:
> > + # 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
> > + # user's email address in the response when asked to. Once we
>
> s/email address/email address and full name ? Or is full name returned from
> all OP's when requested?

No, by default they only include the identity URL in the response. I've
updated the comment.

>
> > === renamed file 'lib/canonical/launchpad/webapp/tests/test_new_login.py' => 'lib/canonical/launchpad/webapp/tests/test_login.py'
> > --- lib/canonical/launchpad/webapp/tests/test_new_login.py 2010-02-17 19:40:41 +0000
> > +++ lib/canonical/launchpad/webapp/tests/test_login.py 2010-02-25 11:53:51 +0000
[...]
> > @@ -42,25 +52,47 @@
> > self.login_called = True
> >
> >
> > +@contextmanager
> > +def stub_SRegResponse_fromSuccessResponse():
> > + def sregFromFakeSuccessResponse(cls, success_response, signed_only=True):
> > + return {'email': success_response.sreg_email,
> > + 'fullname': success_response.sreg_fullname}
> > +...

=== modified file 'lib/canonical/launchpad/database/account.py'
--- lib/canonical/launchpad/database/account.py	2009-08-14 12:50:43 +0000
+++ lib/canonical/launchpad/database/account.py	2010-02-25 13:13:19 +0000
@@ -221,11 +221,12 @@
     implements(IAccountSet)
 
     def new(self, rationale, displayname, password=None,
-            password_is_encrypted=False):
+            password_is_encrypted=False, openid_identifier=DEFAULT):
         """See `IAccountSet`."""
 
         account = Account(
-            displayname=displayname, creation_rationale=rationale)
+            displayname=displayname, creation_rationale=rationale,
+            openid_identifier=openid_identifier)
 
         # Create the password record.
         if password is not None:
@@ -243,13 +244,15 @@
         return account
 
     def createAccountAndEmail(self, email, rationale, displayname, password,
-                              password_is_encrypted=False):
+                              password_is_encrypted=False,
+                              openid_identifier=DEFAULT):
         """See `IAccountSet`."""
         # Convert the PersonCreationRationale to an AccountCreationRationale.
         account_rationale = getattr(AccountCreationRationale, rationale.name)
         account = self.new(
             account_rationale, displayname, password=password,
-            password_is_encrypted=password_is_encrypted)
+            password_is_encrypted=password_is_encrypted,
+            openid_identifier=openid_identifier)
         account.status = AccountStatus.ACTIVE
         email = getUtility(IEmailAddressSet).new(
             email, status=EmailAddressStatus.PREFERRED, account=account)

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-02-18 10:52:51 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-02-25 13:13:19 +0000
@@ -12,6 +12,7 @@
 from BeautifulSoup import UnicodeDammit
 
 from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
+from openid.extensions import sreg
 from openid.fetchers import setDefaultFetcher, Urllib2Fetcher
 
 import transaction
@@ -27,7 +28,6 @@
 
 from z3c.ptcompat import ViewPageTemplateFile
 
-from canonical.cachedproperty import cachedproperty
 from canonical.config import config
 from canonical.launchpad import _
 from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
@@ -181,6 +181,8 @@
         openid_vhost = config.launchpad.openid_provider_vhost
         self.openid_request = consumer.begin(
             allvhosts.configs[openid_vhost].rooturl)
+        self.openid_request.addExtension(
+            sreg.SRegRequest(optional=['email', 'fullname']))
 
         trust_root = self.request.getApplicationURL()
         assert not self.openid_request.shouldSendRedirect(), (
@@ -258,8 +260,34 @@
 
     def render(self):
         if self.openid_response.status == SUCCESS:
-            account = getUtility(IAccountSet).getByOpenIDIdentifier(
-                self.openid_response.identity_url.split('/')[-1])
+            identifier = self.openid_response.identity_url.split('/')[-1]
+            account_set = getUtility(IAccountSet)
+            try:
+                account = account_set.getByOpenIDIdentifier(identifier)
+            except LookupError:
+                # 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
+                # user's email address and full name in the response when
+                # asked to.  Once we start using other OPs we won't be able to
+                # make this assumption here as they might not include what we
+                # want in the response.
+                sreg_response = sreg.SRegResponse.fromSuccessResponse(
+                    self.openid_response)
+                assert sreg_response is not None, (
+                    "OP didn't include an sreg extension in the response.")
+                email_address = sreg_response.get('email')
+                full_name = 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.")
+                account, email = account_set.createAccountAndEmail(
+                    email_address,
+                    PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
+                    full_name,
+                    password=None,
+                    openid_identifier=identifier)
+
             if account.status == AccountStatus.SUSPENDED:
                 return self.suspended_account_template()
             if IPerson(account, None) is None:

=== modified file 'lib/canonical/launchpad/webapp/tests/cookie-authentication.txt'
--- lib/canonical/launchpad/webapp/tests/cookie-authentication.txt	2010-02-17 19:40:41 +0000
+++ lib/canonical/launchpad/webapp/tests/cookie-authentication.txt	2010-02-25 13:13:19 +0000
@@ -20,7 +20,7 @@
     <html>...<body onload="document.forms[0].submit();"...
     >>> browser.getControl('Continue').click()
 
-    >>> from canonical.launchpad.webapp.tests.test_new_login import (
+    >>> from canonical.launchpad.webapp.tests.test_login import (
     ...     fill_login_form_and_submit)
     >>> fill_login_form_and_submit(browser, '[e-mail]', 'test')
     >>> print extract_text(find_tag_by_id(browser.contents, 'logincontrol'))

=== modified file 'lib/canonical/launchpad/webapp/tests/login.txt'
--- lib/canonical/launchpad/webapp/tests/login.txt	2010-02-17 19:40:41 +0000
+++ lib/canonical/launchpad/webapp/tests/login.txt	2010-02-25 13:13:19 +0000
@@ -25,7 +25,7 @@
 
     >>> print browser.title
     Login
-    >>> from canonical.launchpad.webapp.tests.test_new_login import (
+    >>> from canonical.launchpad.webapp.tests.test_login import (
     ...     fill_login_form_and_submit)
     >>> fill_login_form_and_submit(browser, '[e-mail]', 'test')
 

=== modified file 'lib/canonical/launchpad/webapp/tests/no-anonymous-session-cookies.txt'
--- lib/canonical/launchpad/webapp/tests/no-anonymous-session-cookies.txt	2010-02-17 19:40:41 +0000
+++ lib/canonical/launchpad/webapp/tests/no-anonymous-session-cookies.txt	2010-02-25 13:13:19 +0000
@@ -25,7 +25,7 @@
     <html>...<body onload="document.forms[0].submit();"...
     >>> browser.getControl('Continue').click()
 
-    >>> from canonical.launchpad.webapp.tests.test_new_login import (
+    >>> from canonical.launchpad.webapp.tests.test_login import (
     ...     fill_login_form_and_submit)
     >>> fill_login_form_and_submit(browser, '[e-mail]', 'test')
     >>> print extract_text(find_tag_by_id(browser.contents, 'logincontrol'))

=== renamed file 'lib/canonical/launchpad/webapp/tests/test_new_login.py' => 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_new_login.py	2010-02-17 19:40:41 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-02-25 13:13:19 +0000
@@ -1,24 +1,31 @@
 # Copyright 2009 Canonical Ltd.  All rights reserved.
+from __future__ import with_statement
 
+# pylint: disable-msg=W0105
 """Test harness for running the new-login.txt tests."""
 
 __metaclass__ = type
 
 __all__ = []
 
+from contextlib import contextmanager
 import httplib
 import unittest
 
 import mechanize
 
 from openid.consumer.consumer import FAILURE, SUCCESS
-
-from canonical.launchpad.interfaces.account import AccountStatus
+from openid.extensions import sreg
+
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
 from canonical.launchpad.testing.pages import (
     extract_text, find_main_content, find_tag_by_id, find_tags_by_class)
 from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
 from canonical.launchpad.testing.browser import Browser, setUp, tearDown
-from canonical.launchpad.webapp.login import OpenIDCallbackView
+from canonical.launchpad.webapp.login import OpenIDCallbackView, OpenIDLogin
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import AppServerLayer, DatabaseFunctionalLayer
 
@@ -29,10 +36,13 @@
 
 class FakeOpenIDResponse:
 
-    def __init__(self, identity_url, status=SUCCESS, message=''):
+    def __init__(self, identity_url, status=SUCCESS, message='', email=None,
+                 full_name=None):
         self.message = message
         self.status = status
         self.identity_url = identity_url
+        self.sreg_email = email
+        self.sreg_fullname = full_name
 
 
 class StubbedOpenIDCallbackView(OpenIDCallbackView):
@@ -42,25 +52,47 @@
         self.login_called = True
 
 
+@contextmanager
+def SRegResponse_fromSuccessResponse_stubbed():
+    def sregFromFakeSuccessResponse(cls, success_response, signed_only=True):
+        return {'email': success_response.sreg_email,
+                'fullname': success_response.sreg_fullname}
+
+    orig_method = sreg.SRegResponse.fromSuccessResponse
+    # Use a stub SRegResponse.fromSuccessResponse that works with
+    # FakeOpenIDResponses instead of real ones.
+    sreg.SRegResponse.fromSuccessResponse = classmethod(
+        sregFromFakeSuccessResponse)
+
+    yield
+
+    sreg.SRegResponse.fromSuccessResponse = orig_method
+
+
 class TestOpenIDCallbackView(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
 
-    def _createView(self, account, response_status=SUCCESS, response_msg=''):
+    def _createViewWithResponse(
+            self, account, response_status=SUCCESS, response_msg=''):
+        openid_response = FakeOpenIDResponse(
+            ITestOpenIDPersistentIdentity(account).openid_identity_url,
+            status=response_status, message=response_msg)
+        return self._createView(openid_response)
+
+    def _createView(self, response):
         request = LaunchpadTestRequest(
             form={'starting_url': 'http://launchpad.dev/after-login'},
             environ={'PATH_INFO': '/'})
         view = StubbedOpenIDCallbackView(object(), request)
         view.initialize()
-        view.openid_response = FakeOpenIDResponse(
-            ITestOpenIDPersistentIdentity(account).openid_identity_url,
-            status=response_status, message=response_msg)
+        view.openid_response = response
         return view
 
     def test_full_fledged_account(self):
         # In the common case we just login and redirect to the URL specified
         # in the 'starting_url' query arg.
         person = self.factory.makePerson()
-        view = self._createView(person.account)
+        view = self._createViewWithResponse(person.account)
         view.render()
         self.assertTrue(view.login_called)
         response = view.request.response
@@ -73,7 +105,7 @@
         # create one.
         account = self.factory.makeAccount('Test account')
         self.assertIs(None, IPerson(account, None))
-        view = self._createView(account)
+        view = self._createViewWithResponse(account)
         view.render()
         self.assertIsNot(None, IPerson(account, None))
         self.assertTrue(view.login_called)
@@ -82,13 +114,38 @@
         self.assertEquals(view.request.form['starting_url'],
                           response.getHeader('Location'))
 
+    def test_unseen_identity(self):
+        # When we get a positive assertion about an identity URL we've never
+        # seen, we automatically register an account with that identity
+        # because someone who registered on login.lp.net or login.u.c should
+        # be able to login here without any further steps.
+        identifier = '4w7kmzU'
+        account_set = getUtility(IAccountSet)
+        self.assertRaises(
+            LookupError, account_set.getByOpenIDIdentifier, identifier)
+        openid_response = FakeOpenIDResponse(
+            'http://testopenid.dev/+id/%s' % identifier, status=SUCCESS,
+            email='[e-mail]', full_name='Foo User')
+        view = self._createView(openid_response)
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view.render()
+        self.assertTrue(view.login_called)
+        account = account_set.getByOpenIDIdentifier(identifier)
+        self.assertIsNot(None, account)
+        self.assertEquals(AccountStatus.ACTIVE, account.status)
+        self.assertEquals('[e-mail]',
+                          removeSecurityProxy(account.preferredemail).email)
+        person = IPerson(account, None)
+        self.assertIsNot(None, person)
+        self.assertEquals('Foo User', person.displayname)
+
     def test_deactivated_account(self):
         # The user has the account's password and is trying to login, so we'll
         # just re-activate their account.
         account = self.factory.makeAccount(
             'Test account', status=AccountStatus.DEACTIVATED)
         self.assertIs(None, IPerson(account, None))
-        view = self._createView(account)
+        view = self._createViewWithResponse(account)
         view.render()
         self.assertIsNot(None, IPerson(account, None))
         self.assertTrue(view.login_called)
@@ -102,7 +159,7 @@
         # login, but we must not allow that.
         account = self.factory.makeAccount(
             'Test account', status=AccountStatus.SUSPENDED)
-        view = self._createView(account)
+        view = self._createViewWithResponse(account)
         html = view.render()
         self.assertFalse(view.login_called)
         main_content = extract_text(find_main_content(html))
@@ -112,7 +169,7 @@
         # The OpenID provider responded with a negative assertion, so the
         # login error page is shown.
         account = self.factory.makeAccount('Test account')
-        view = self._createView(
+        view = self._createViewWithResponse(
             account, response_status=FAILURE,
             response_msg='Server denied check_authentication')
         html = view.render()
@@ -139,6 +196,14 @@
     handler_classes['_redirect'] = MyHTTPRedirectHandler
 
 
+def fill_login_form_and_submit(browser, email_address, password):
+    assert browser.getControl(name='field.email') is not None, (
+        "We don't seem to be looking at a login form.")
+    browser.getControl(name='field.email').value = email_address
+    browser.getControl(name='field.password').value = password
+    browser.getControl('Continue').click()
+
+
 class TestOpenIDReplayAttack(TestCaseWithFactory):
     layer = AppServerLayer
 
@@ -172,12 +237,51 @@
                           extract_text(error_msg))
 
 
-def fill_login_form_and_submit(browser, email_address, password):
-    assert browser.getControl(name='field.email') is not None, (
-        "We don't seem to be looking at a login form.")
-    browser.getControl(name='field.email').value = email_address
-    browser.getControl(name='field.password').value = password
-    browser.getControl('Continue').click()
+class FakeOpenIDRequest:
+    extensions = None
+
+    def addExtension(self, extension):
+        if self.extensions is None:
+            self.extensions = [extension]
+        else:
+            self.extensions.append(extension)
+
+    def shouldSendRedirect(self):
+        return False
+
+    def htmlMarkup(self, trust_root, return_to):
+        return None
+
+
+class FakeOpenIDConsumer:
+    def begin(self, url):
+        return FakeOpenIDRequest()
+
+
+class StubbedOpenIDLogin(OpenIDLogin):
+    def _getConsumer(self):
+        return FakeOpenIDConsumer()
+
+
+class TestOpenIDLogin(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def test_sreg_fields(self):
+        # We request the user's email address and Full Name (through the SREG
+        # extension) to the OpenID provider so that we can automatically
+        # register unseen OpenID identities.
+        request = LaunchpadTestRequest()
+        # This is a hack to make the request.getURL(1) call issued by the view
+        # not raise an IndexError.
+        request._app_names = ['foo']
+        view = StubbedOpenIDLogin(object(), request)
+        view()
+        extensions = view.openid_request.extensions
+        self.assertIsNot(None, extensions)
+        sreg_extension = extensions[0]
+        self.assertIsInstance(sreg_extension, sreg.SRegRequest)
+        self.assertEquals(['email', 'fullname'],
+                          sorted(sreg_extension.allRequestedFields()))
 
 
 def test_suite():