launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08242
[Merge] lp:~wgrant/launchpad/bug-1005330 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-1005330 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1005330 in Launchpad itself: "Applications can't easily map an SSO account to a Launchpad one"
https://bugs.launchpad.net/launchpad/+bug/1005330
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1005330/+merge/107605
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.
--
https://code.launchpad.net/~wgrant/launchpad/bug-1005330/+merge/107605
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1005330 into lp:launchpad.
=== 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=daf@xxxxxxxxxxxxx").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)
Follow ups
