Merge lp:~wgrant/launchpad/bug-1005330 into lp:launchpad

[William Grant]

Add and export PersonSet.getByOpenIDIdentifier to let applications map from an SSO account to the corresponding Launchpad one.

This branch adds and exports PersonSet.getByOpenIDIdentifier, an API method to look up the person associated with a given OpenID identifier, satisfying the needs of Summit and probably other SSO+Launchpad consumers, thereby alleviating many of the issues that make bug #881019 important.

Unlike other methods around the application that purport to deal with OpenID identifiers, this method actually does. The rest of our code thinks an identifier is the unique "4tLsDY8" part of "https://login.launchpad.net/+id/4tLsDY8", when in fact the full URL is the identifier. Since it's designed to be externally consumed, PersonSet.getByOpenIDIdentifier accepts a URL, checks that it's from one of our providers (login.launchpad.net or login.ubuntu.com), and extracts the unique suffix to look up in the DB.

I replaced the PersonSet webservice doctest with unit tests, and added new ones for this. I also added a new admin_logged_in context manager; it's less slow, less verbose, and less emaily than celebrity_logged_in('admin'). I'll be cleaning up other tests to use that in a later branch.

The + in the path was initially some cause for concern, as applications have a habit of reencoding specialish characters. But by RFC 2396 section 2.2 it is reserved and cannot be reencoded, and we know that SSO always sends it unencoded. So I've opted to be strict and only accept the canonical form sent by SSO. It shouldn't be a problem, but if it proves to be we can come up with a more lenient implementation.

[Benji York (benji)]

This branch looks good. The translation of doctests to unittests was well done (the prose was carried over well and resulted in nicely readable tests).

review: Approve (code)

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2012-01-30 04:17:27 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2012-05-28 10:50:39 +0000
@@ -7,8 +7,10 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.testing import (
+    admin_logged_in,
     launchpadlib_for,
     login,
+    logout,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
@@ -74,3 +76,90 @@
         self.assertEquals(
             rendered_comment,
             '<a href="/~test-person" class="sprite person">Test Person</a>')
+
+
+class PersonSetWebServiceTests(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(PersonSetWebServiceTests, self).setUp()
+        self.webservice = LaunchpadWebServiceCaller('test', None)
+        logout()
+
+    def assertReturnsPeople(self, expected_names, path):
+        self.assertEqual(
+            expected_names,
+            [person['name'] for person in
+             self.webservice.get(path).jsonBody()['entries']])
+
+    def test_default_content(self):
+        # /people lists the 50 people with the most karma, excluding
+        # those with no karma at all.
+        self.assertEqual(
+            4, len(self.webservice.get('/people').jsonBody()['entries']))
+
+    def test_find(self):
+        # It's possible to find people by name.
+        with admin_logged_in():
+            person_name = self.factory.makePerson().name
+        self.assertReturnsPeople(
+            [person_name], '/people?ws.op=find&text=%s' % person_name)
+
+    def test_findTeam(self):
+        # The search can be restricted to teams.
+        with admin_logged_in():
+            person_name = self.factory.makePerson().name
+            team_name = self.factory.makeTeam(
+                name='%s-team' % person_name).name
+        self.assertReturnsPeople(
+            [team_name], '/people?ws.op=findTeam&text=%s' % person_name)
+
+    def test_findPerson(self):
+        # The search can be restricted to people.
+        with admin_logged_in():
+            person_name = self.factory.makePerson().name
+            self.factory.makeTeam(name='%s-team' % person_name)
+        self.assertReturnsPeople(
+            [person_name], '/people?ws.op=findPerson&text=%s' % person_name)
+
+    def test_find_by_date(self):
+        # Creation date filtering is supported.
+        self.assertReturnsPeople(
+            [u'bac'],
+            '/people?ws.op=findPerson&text='
+            '&created_after=2008-06-27&created_before=2008-07-01')
+
+    def test_getByEmail(self):
+        # You can get a person by their email address.
+        with admin_logged_in():
+            person = self.factory.makePerson()
+            person_name = person.name
+            person_email = person.preferredemail.email
+        self.assertEqual(
+            person_name,
+            self.webservice.get(
+                '/people?ws.op=getByEmail&email=%s' % person_email
+                ).jsonBody()['name'])
+
+    def test_getByEmail_checks_format(self):
+        # A malformed email address is rejected.
+        e = self.assertRaises(
+            ValueError,
+            self.webservice.get(
+                '/people?ws.op=getByEmail&email=foo@').jsonBody)
+        self.assertEqual("email: Invalid email 'foo@'.", e[0])
+
+    def test_getByOpenIDIdentifier(self):
+        # You can get a person by their OpenID identifier URL.
+        with admin_logged_in():
+            person = self.factory.makePerson()
+            person_name = person.name
+            person_openid = person.account.openid_identifiers.one().identifier
+        self.assertEqual(
+            person_name,
+            self.webservice.get(
+                '/people?ws.op=getByOpenIDIdentifier&'
+                'identifier=http://openid.launchpad.dev/%%2Bid/%s'
+                % person_openid,
+                api_version='devel').jsonBody()['name'])

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-05-04 14:52:44 +0000
+++ lib/lp/registry/interfaces/person.py	2012-05-28 10:50:39 +0000
@@ -2152,6 +2152,18 @@
         on the displayname or other arguments.
         """
 
+    @operation_parameters(identifier=TextLine(required=True))
+    @operation_returns_entry(IPerson)
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getByOpenIDIdentifier(identifier):
+        """Get the person for a given OpenID identifier.
+
+        :param openid_identifier: full OpenID identifier URL for the user.
+        :return: the corresponding `IPerson` or None if the identifier is
+            unknown
+        """
+
     def getOrCreateByOpenIDIdentifier(openid_identifier, email,
                                       full_name, creation_rationale, comment):
         """Get or create a person for a given OpenID identifier.
@@ -2166,7 +2178,8 @@
         If there is no existing Launchpad person for the account, we
         create it.
 
-        :param openid_identifier: representing the authenticated user.
+        :param openid_identifier: OpenID identifier suffix for the user.
+            This is *not* the full URL, just the unique suffix portion.
         :param email_address: the email address of the user.
         :param full_name: the full name of the user.
         :param creation_rationale: When an account or person needs to

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-05-14 10:56:16 +0000
+++ lib/lp/registry/model/person.py	2012-05-28 10:50:39 +0000
@@ -304,6 +304,7 @@
 from lp.services.verification.model.logintoken import LoginToken
 from lp.services.webapp.dbpolicy import MasterDatabasePolicy
 from lp.services.webapp.interfaces import ILaunchBag
+from lp.services.webapp.vhosts import allvhosts
 from lp.services.worlddata.model.language import Language
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -3166,6 +3167,30 @@
             key=lambda obj: (obj.karma, obj.displayname, obj.id),
             reverse=True)
 
+    def getByOpenIDIdentifier(self, identifier):
+        """See `IPersonSet`."""
+        # We accept a full OpenID identifier URL from either the
+        # Launchpad- or Ubuntu-branded OpenID services. But we only
+        # store the unique suffix of the identifier, so we need to strip
+        # the rest of the URL.
+        # + is reserved, so is not allowed to be reencoded in transit, so
+        # should never appear as its percent-encoded equivalent.
+        identifier_suffix = None
+        for vhost in ('openid', 'ubuntu_openid'):
+            root = '%s+id/' % allvhosts.configs[vhost].rooturl
+            if identifier.startswith(root):
+                identifier_suffix = identifier.replace(root, '', 1)
+                break
+        if identifier_suffix is None:
+            return None
+
+        try:
+            account = getUtility(IAccountSet).getByOpenIDIdentifier(
+                identifier_suffix)
+        except LookupError:
+            return None
+        return IPerson(account)
+
     def getOrCreateByOpenIDIdentifier(
         self, openid_identifier, email_address, full_name,
         creation_rationale, comment):

=== removed file 'lib/lp/registry/stories/webservice/xx-people.txt'
--- lib/lp/registry/stories/webservice/xx-people.txt	2010-01-05 15:59:03 +0000
+++ lib/lp/registry/stories/webservice/xx-people.txt	1970-01-01 00:00:00 +0000
@@ -1,71 +0,0 @@
-= The set of people and teams =
-
-The set of people and teams in Launchpad is represented by the
-collection found at /people.  By default it lists the (up to) 50
-people with the most karma, not including people with no karma.
-
-    >>> people = webservice.get("/people").jsonBody()
-    >>> len(people['entries'])
-    4
-
-
-== Custom operations ==
-
-But it also offer operations to filter that list.  For example, the
-find operation will search for people and teams matching the given
-search text.
-
-    >>> search = webservice.get(
-    ...     "/people?ws.op=find&text=salgado").jsonBody()
-    >>> sorted(person['name'] for person in search['entries'])
-    [u'salgado']
-
-There are also operations to search only for people or only for teams.
-
-    >>> search = webservice.get(
-    ...     "/people?ws.op=findTeam&text=salgado").jsonBody()
-    >>> sorted(person['name'] for person in search['entries'])
-    []
-
-    >>> search = webservice.get(
-    ...     "/people?ws.op=findPerson&text=salgado").jsonBody()
-    >>> sorted(person['name'] for person in search['entries'])
-    [u'salgado']
-
-    >>> results = webservice.get(
-    ...     "/people?ws.op=findPerson&text="
-    ...     "&created_after=2008-06-27&created_before=2008-07-01").jsonBody()
-    >>> sorted(person['name'] for person in results['entries'])
-    [u'bac']
-
-It's possible to get a list of teams by virtue of which a person
-belongs to a particular team.
-
-    # XXX: salgado, 2008-08-01: Commented because method has been Unexported;
-    # it should be re-enabled after the operation is exported again.
-    # >>> response = webservice.named_get(
-    # ...     '/~salgado', 'findPathToTeam',
-    # ...     team=webservice.getAbsoluteUrl('/~mailing-list-experts'))
-
-    # >>> path = response.jsonBody()
-    # >>> path['total_size']
-    # 2
-    # >>> [person['name'] for person in path['entries']]
-    # [u'admins', u'mailing-list-experts']
-
-If you know the email address of a person/team, it's possible to
-retrieve it using that email.
-
-    >>> daf = webservice.get(
-    ...     "/people?ws.op=getByEmail&email=[e-mail]").jsonBody()
-    >>> daf['name']
-    u'daf'
-
-When looking up a person by email address, you have to provide a valid
-email address.
-
-    >>> print webservice.get(
-    ...     "/people?ws.op=getByEmail&email=daf@")
-    HTTP/1.1 400 Bad Request
-    ...
-    email: Invalid email 'daf@'.

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2012-03-02 23:32:28 +0000
+++ lib/lp/registry/tests/test_personset.py	2012-05-28 10:50:39 +0000
@@ -140,6 +140,40 @@
                 person.preferredemail
         self.assertThat(recorder, HasQueryCount(LessThan(1)))
 
+    def test_getByOpenIDIdentifier_returns_person(self):
+        # getByOpenIDIdentifier takes a full OpenID identifier and
+        # returns the corresponding person.
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            identifier = person.account.openid_identifiers.one().identifier
+        self.assertEqual(
+            person,
+            self.person_set.getByOpenIDIdentifier(
+                u'http://openid.launchpad.dev/+id/%s' % identifier))
+        self.assertEqual(
+            person,
+            self.person_set.getByOpenIDIdentifier(
+                u'http://ubuntu-openid.launchpad.dev/+id/%s' % identifier))
+
+    def test_getByOpenIDIdentifier_for_nonexistent_identifier_is_none(self):
+        # None is returned if there's no matching person.
+        self.assertIs(
+            None,
+            self.person_set.getByOpenIDIdentifier(
+                u'http://openid.launchpad.dev/+id/notanid'))
+
+    def test_getByOpenIDIdentifier_for_bad_domain_is_none(self):
+        # Even though the OpenIDIdentifier table doesn't store the
+        # domain, we verify it against our known OpenID faux-vhosts.
+        # If it doesn't match, we don't even try to check the identifier.
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            identifier = person.account.openid_identifiers.one().identifier
+        self.assertIs(
+            None,
+            self.person_set.getByOpenIDIdentifier(
+                u'http://not.launchpad.dev/+id/%s' % identifier))
+
 
 class TestPersonSetMergeMailingListSubscriptions(TestCaseWithFactory):
 

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2012-05-03 14:06:03 +0000
+++ lib/lp/testing/__init__.py	2012-05-28 10:50:39 +0000
@@ -10,6 +10,7 @@
 __all__ = [
     'AbstractYUITestCase',
     'ANONYMOUS',
+    'admin_logged_in',
     'anonymous_logged_in',
     'api_url',
     'BrowserTestCase',
@@ -157,6 +158,7 @@
 # Import the login helper functions here as it is a much better
 # place to import them from in tests.
 from lp.testing._login import (
+    admin_logged_in,
     anonymous_logged_in,
     celebrity_logged_in,
     login,
@@ -183,6 +185,7 @@
 
 # The following names have been imported for the purpose of being
 # exported. They are referred to here to silence lint warnings.
+admin_logged_in
 anonymous_logged_in
 api_url
 celebrity_logged_in

=== modified file 'lib/lp/testing/_login.py'
--- lib/lp/testing/_login.py	2012-05-03 20:27:12 +0000
+++ lib/lp/testing/_login.py	2012-05-28 10:50:39 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'admin_logged_in',
     'anonymous_logged_in',
     'celebrity_logged_in',
     'login',
@@ -129,6 +130,13 @@
     return login_as(celeb, participation=participation)
 
 
+def login_admin(ignored, participation=None):
+    """Log in as an admin."""
+    login(ANONYMOUS)
+    admin = getUtility(ILaunchpadCelebrities).admin.teamowner
+    return login_as(admin, participation=participation)
+
+
 def logout():
     """Tear down after login(...), ending the current interaction.
 
@@ -180,6 +188,12 @@
     return _with_login(login_celebrity, celebrity_name)
 
 
+@contextmanager
+def admin_logged_in():
+    # Use teamowner to avoid expensive and noisy team member additions.
+    return _with_login(login_admin, None)
+
+
 with_anonymous_login = decorate_with(person_logged_in, None)