Merge lp:~henninge/launchpad/bug-503454 into lp:launchpad

[Henning Eggers]

Added IPersonRoles to augment ILaunchpadCelebrities and get rid of permission_helpers.py.

[Henning Eggers (henninge)]

= Details =

See bug 503454. This is a branch that reliefs me of some techdebt that I incurred when I introduced the file permission_helpers.py. It was conclude on the mailing list that such specific functions (like is_admin_or_rosetta_expert) are not usefull. On the other hand, I still disliked the overhead needed to check if a person is an admin. So I proposed this adapter and discussed it with Curtis, wo agreed that it is a good idea. Curtis and I agreed on the name PersonRoles for the adapter because it resembles goes in the same direction as the roles concept that he had put forward earlier.

This branch introduces PersonRoles and removes permission_helpers.py, replacing all references to its functions with checks on attributes of PersonRoles. A follow-up branch will make AuthorizationBase use the adapter directly.

Care was taken to make sure that ILaunchpadCelebrities and IPersonRoles stay in sync, so that whenever a person celebrity is added or removed, the corresponding entry in IPersonRoles has to be added or removed, too.

== Implementation details ==

lib/canonical/launchpad/doc/celebrities.txt

 * Added a test for celebs.person_names.

lib/canonical/launchpad/interfaces/launchpad.py

 * Added the person_names attribute to ILaunchpadCelebrities. This is mainly
   intended to be used by the test that keeps ILauchpatCelebrities in sync
   with IPersonRoles while tests should not be importing from model code.

 * Added IPersonRoles. I placed this in this file intentionally instead of
   giving it its own because it needs to be kept synced with
   ILaunchpadCelbrities. The attributes are named exactly like the
   corresponding attributes in ILaunchpadCelebrities, prefixed with "in_".

lib/canonical/launchpad/security.py

 * Removed import for permission_helpers because that file is deleted.

 * Replaced uses of functions from permission_helpers with checks on the
   adapter attributes. The adapter still needs to be applied locally in each
   function.

lib/canonical/launchpad/tests/test_personroles.py

 * Added interface test for PersonRoles. This will fail if a person celbrity
   is removed from ILaunchpadCelebrity but not from IPersonRoles, or such
   an attribute has been renamed.

 * Added test that checks number of person celebrities in both interfaces.
   This will fail when a person celebtrity is added to ILaunchpadCelebrities
   but not to IPersonRoles.

 * Added coverage for the "in_" attributes for all currently known person
   celebrities, most of which are teams, some are individuals.

 * The rest of the test case is concerned with the methods as defined in
   IPersonRoles.

lib/canonical/launchpad/utilities/celebrities.py

 * Added implementation for person_names. PersonCelebrityDescritor was
   already collection them in a "set" attribute.

lib/canonical/launchpad/utilities/personroles.py

 * Implemented the "in_" attributes using __getattr__, thus automatically
   getting the corresponding attribute from the ILaunchpadCelebrities
   implementation by stripping the prefix off the name.

 * isDriver is implemented to also check for the "drivers" attribute, so that
   all possible drivers are checked.

lib/canonical/launc...

[Henning Eggers (henninge)]

I moved the test from test_personroles.py to celbrities.txt to be sure that somebody adding a celebrity is made aware of the need to update IPersonRoles, too.

[Abel Deuring (adeuring)]

Hi Henning,

overall, a very nice branch! r=me with the changes from
http://paste.ubuntu.com/353497/.

We discussed on IRC that it would be better not to use the list of
"SQL names" of the IPerson celebrities for the comparison of the
set person celebrities and the corresponding set of in_.* methods
in IPersonRoles.

Other that that, I have a few mostly formal nitpicks, see below.

r=me

Abel

>
> === modified file 'lib/canonical/launchpad/doc/celebrities.txt'
> --- lib/canonical/launchpad/doc/celebrities.txt 2009-11-18 07:19:14 +0000
> +++ lib/canonical/launchpad/doc/celebrities.txt 2010-01-08 10:36:25 +0000
> @@ -225,6 +225,49 @@
> True
>
>
> +== Person celebrities ==
> +
> +A list of all IPerson celebrities is provided.
> +
> + >>> for name in celebs.person_names:
> + ... print name
> + admins
> + bazaar-experts
> + bug-importer
> + bug-watch-updater
> + commercial-admins
> + hwdb-team
> + janitor
> + katie
> + launchpad
> + launchpad-beta-testers
> + launchpad-buildd-admins
> + mailing-list-experts
> + ppa-key-guard
> + registry
> + rosetta-admins
> + shipit-admins
> + techboard
> + ubuntu-branches
> + ubuntu-security
> + vcs-imports
> +
> +The number of Person celebrities is identical to the number of "in_"
> +attributes of the IPersonRoles interface. If they differ, IPersonRoles needs
> +to be synced to ILaunchpadCelebrities by adding/removing the appropriate
> +"in_" attribute(s).
> +
> + >>> from canonical.launchpad.interfaces.launchpad import IPersonRoles
> + >>> def number_of_in_attributes():
> + ... num = 0
> + ... for name in IPersonRoles.names():
> + ... if name.startswith("in_"):
> + ... num += 1
> + ... return num
> + >>> print number_of_in_attributes() == len(celebs.person_names)
> + True
> +
> +
> == Detecting if a person is a celebrity ==
>
> We can ask if a person has celebrity status.
>
> === modified file 'lib/canonical/launchpad/interfaces/launchpad.py'
> --- lib/canonical/launchpad/interfaces/launchpad.py 2009-12-03 04:50:40 +0000
> +++ lib/canonical/launchpad/interfaces/launchpad.py 2010-01-08 10:36:25 +0000
> @@ -57,6 +57,7 @@
> 'IPasswordChangeApp',
> 'IPasswordEncryptor',
> 'IPasswordResets',
> + 'IPersonRoles',
> 'IPrivateApplication',
> 'IPrivateMaloneApplication',
> 'IPrivacy',
> @@ -133,11 +134,110 @@
> ubuntu_techboard = Attribute("The Ubuntu technical board.")
> vcs_imports = Attribute("The 'vcs-imports' team.")
>
> + person_names = Attribute("A list of the names of all celebrity persons.")
> +
> def isCelebrityPerson(name):
> """Return true if there is an IPerson celebrity with the given name.
> """
>
>
> +class IPersonRoles(Interface):
> + """What celebrity teams a person is member of and similar helpers.
> +
> + Convenience methods that remove frequent calls to ILaunchpadCelebrities
> + and IPerson.inTeam from permission checkers. May also be used in model
> + or view code.
> +
> + All person celebrities in ILaunchpadCelbrities must have a matching
> + in_ attribute here and vice versa.
>...

review: Approve (code)

[Henning Eggers (henninge)]

> overall, a very nice branch! r=me with the changes from
> http://paste.ubuntu.com/353497/.
>
> We discussed on IRC that it would be better not to use the list of
> "SQL names" of the IPerson celebrities for the comparison of the
> set person celebrities and the corresponding set of in_.* methods
> in IPersonRoles.

Thanks, I did that as seen in the paste and some more. The test will now fail with a message that describes what needs to be done to sync the two Interfaces again. I like this solution a lot. ;)

>
> Other that that, I have a few mostly formal nitpicks, see below.

Cool, please see my comments below.

> > + def isDriver(obj):
> > + """Is this person on of the drivers of the object?"""
>
> s/on of/one of/

Done.

>
> > +
> > + def isOneOf(obj, attributes):
> > + """Is this person on of the roles in relation to the object?
>
> s/on of/one of/
>

Done.

> > +
> > + Check if the person is (inTeam) one of the given IPerson attributes
>
> s/(inTeam) one/(inTeam) of one/

Fixed. Thanks for you patience.

> > @@ -195,7 +194,8 @@
> > usedfor = None
> >
> > def checkAuthenticated(self, user):
> > - return is_admin_or_registry_expert(user)
> > + user = IPersonRoles(user)
>
> I find this sort of using one variable for somwhat different thingies
> (first for an IPerson, next for an IPersonRoles) a bit confusing. What
> do you think about "user_roles = IPersonRoles(user)"?
>

Yes, I know what you mean. I am not going to do that here, though, because in my next branch, the user parameter will already be IPersonRoles, thus eliminating this line completely again.

But I fixed it in translationimportqueue.py. Thanks for pointing it out.

> > === added file 'lib/canonical/launchpad/tests/test_personroles.py'
> > + def _test_in_team(self, attribute):
> > + roles_attribute = self.prefix+attribute
>
> Please insert spaces before and after the '+'.

Fixed that, thank you.

> > + def test_in_teams(self):
> > + # Test all celebrity teams are available.
> > + team_attributes = [
> > + 'admin',
> > + 'bazaar_experts',
> > + 'buildd_admin',
> > + 'commercial_admin',
> > + 'hwdb_team',
> > + 'launchpad_beta_testers',
> > + 'launchpad_developers',
> > + 'mailing_list_experts',
> > + 'registry_experts',
> > + 'rosetta_experts',
> > + 'shipit_admin',
> > + 'ubuntu_branches',
> > + 'ubuntu_security',
> > + 'ubuntu_techboard',
> > + 'vcs_imports',
> > + ]
>
> This list isn't complete ;) I think you can create the list of
> person celebrity attribute names here in the same way as in
> celebrities.txt in http://paste.ubuntu.com/353497/.
>

That is a great idea and it has the code much shorter. Thanks for that! ;)

> > + for attribute in team_attributes:
> > + self._test_in_team(attribute)
> > +
> > + def _test_in_person(self, attribute):
> > + roles_attribute = self.prefix+attribute
>
> Please insert spaces before and after the '+'.

Done.

Thank you for your review, my c...

[Henning Eggers (henninge)]

=== modified file 'lib/canonical/launchpad/doc/celebrities.txt'
--- lib/canonical/launchpad/doc/celebrities.txt 2010-01-08 10:34:28 +0000
+++ lib/canonical/launchpad/doc/celebrities.txt 2010-01-08 15:25:33 +0000
@@ -8,7 +8,8 @@
 to these well-known objects through attributes.

     >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
- >>> from canonical.launchpad.interfaces import ILanguageSet, IPersonSet
+ >>> from lp.services.worlddata.interfaces.language import ILanguageSet
+ >>> from lp.registry.interfaces.person import IPerson, IPersonSet
     >>> celebs = getUtility(ILaunchpadCelebrities)

     >>> from canonical.launchpad.webapp.testing import verifyObject
@@ -227,45 +228,32 @@

 == Person celebrities ==

-A list of all IPerson celebrities is provided.
-
- >>> for name in celebs.person_names:
- ... print name
- admins
- bazaar-experts
- bug-importer
- bug-watch-updater
- commercial-admins
- hwdb-team
- janitor
- katie
- launchpad
- launchpad-beta-testers
- launchpad-buildd-admins
- mailing-list-experts
- ppa-key-guard
- registry
- rosetta-admins
- shipit-admins
- techboard
- ubuntu-branches
- ubuntu-security
- vcs-imports
-
-The number of Person celebrities is identical to the number of "in_"
-attributes of the IPersonRoles interface. If they differ, IPersonRoles needs
+Each person celebrity has a corresponding "in_" attribute in IPersonRoles, to
+check if a person has that role. If the attributes differ, IPersonRoles needs
 to be synced to ILaunchpadCelebrities by adding/removing the appropriate
 "in_" attribute(s).

     >>> from canonical.launchpad.interfaces.launchpad import IPersonRoles
- >>> def number_of_in_attributes():
- ... num = 0
+ >>> def get_person_celebrity_names():
+ ... for name in ILaunchpadCelebrities.names():
+ ... if IPerson.providedBy(getattr(celebs, name)):
+ ... yield "in_" + name
+ >>> def get_person_roles_names():
     ... for name in IPersonRoles.names():
     ... if name.startswith("in_"):
- ... num += 1
- ... return num
- >>> print number_of_in_attributes() == len(celebs.person_names)
- True
+ ... yield name
+
+Treating the lists as sets and determining their difference gives us a clear
+picture of what is missing where.
+
+ >>> person_celebrity_names = set(get_person_celebrity_names())
+ >>> person_roles_names = set(get_person_roles_names())
+ >>> print "Please add to IPersonRoles: " + (
+ ... ", ".join(list(person_celebrity_names - person_roles_names)))
+ Please add to IPersonRoles:
+ >>> print "Please remove from IPersonRoles: " + (
+ ... ", ".join(list(person_roles_names - person_celebrity_names)))
+ Please remove from IPersonRoles:

 == Detecting if a person is a celebrity ==

=== modified file 'lib/canonical/launchpad/interfaces/launchpad.py'
--- lib/canonical/launchpad/interfaces/launchpad.py 2010-01-08 09:55:37 +0000
+++ lib/canonical/launchpad/interfaces/launchpad.py 2010-01-08 16:13:43 +0000
@@ -134,8 +134,6 @@
     ubuntu_techboard = Attribute("The Ubuntu t...

=== modified file 'lib/canonical/launchpad/doc/celebrities.txt'
--- lib/canonical/launchpad/doc/celebrities.txt	2009-11-18 07:19:14 +0000
+++ lib/canonical/launchpad/doc/celebrities.txt	2010-01-08 23:43:19 +0000
@@ -8,7 +8,8 @@
 to these well-known objects through attributes.
 
     >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
-    >>> from canonical.launchpad.interfaces import ILanguageSet, IPersonSet
+    >>> from lp.services.worlddata.interfaces.language import ILanguageSet
+    >>> from lp.registry.interfaces.person import IPerson, IPersonSet
     >>> celebs = getUtility(ILaunchpadCelebrities)
 
     >>> from canonical.launchpad.webapp.testing import verifyObject
@@ -225,6 +226,36 @@
     True
 
 
+== Person celebrities ==
+
+Each person celebrity has a corresponding "in_" attribute in IPersonRoles, to
+check if a person has that role. If the attributes differ, IPersonRoles needs
+to be synced to ILaunchpadCelebrities by adding/removing the appropriate
+"in_" attribute(s).
+
+    >>> from canonical.launchpad.interfaces.launchpad import IPersonRoles
+    >>> def get_person_celebrity_names():
+    ...     for name in ILaunchpadCelebrities.names():
+    ...         if IPerson.providedBy(getattr(celebs, name)):
+    ...             yield "in_" + name
+    >>> def get_person_roles_names():
+    ...     for name in IPersonRoles.names():
+    ...         if name.startswith("in_"):
+    ...             yield name
+
+Treating the lists as sets and determining their difference gives us a clear
+picture of what is missing where.
+
+    >>> person_celebrity_names = set(get_person_celebrity_names())
+    >>> person_roles_names = set(get_person_roles_names())
+    >>> print "Please add to IPersonRoles: " + (
+    ...       ", ".join(list(person_celebrity_names - person_roles_names)))
+    Please add to IPersonRoles:
+    >>> print "Please remove from IPersonRoles: " + (
+    ...       ", ".join(list(person_roles_names - person_celebrity_names)))
+    Please remove from IPersonRoles:
+
+
 == Detecting if a person is a celebrity ==
 
 We can ask if a person has celebrity status.

=== modified file 'lib/canonical/launchpad/interfaces/launchpad.py'
--- lib/canonical/launchpad/interfaces/launchpad.py	2009-12-03 04:50:40 +0000
+++ lib/canonical/launchpad/interfaces/launchpad.py	2010-01-08 23:43:19 +0000
@@ -57,6 +57,7 @@
     'IPasswordChangeApp',
     'IPasswordEncryptor',
     'IPasswordResets',
+    'IPersonRoles',
     'IPrivateApplication',
     'IPrivateMaloneApplication',
     'IPrivacy',
@@ -138,6 +139,103 @@
         """
 
 
+class IPersonRoles(Interface):
+    """What celebrity teams a person is member of and similar helpers.
+
+    Convenience methods that remove frequent calls to ILaunchpadCelebrities
+    and IPerson.inTeam from permission checkers. May also be used in model
+    or view code.
+
+    All person celebrities in ILaunchpadCelbrities must have a matching
+    in_ attribute here and vice versa.
+    """
+
+    person = Attribute("The IPerson object that these checks refer to.")
+
+    in_admin = Bool(
+        title=_("True if this person is a Launchpad admin."),
+        required=True, readonly=True)
+    in_bazaar_experts = Bool(
+        title=_("True if this person is a Bazaar expert."),
+        required=True, readonly=True)
+    in_bug_importer = Bool(
+        title=_("True if this person is a bug importer."),
+        required=True, readonly=True)
+    in_bug_watch_updater = Bool(
+        title=_("True if this person is a bug watch updater."),
+        required=True, readonly=True)
+    in_buildd_admin = Bool(
+        title=_("True if this person is a buildd admin."),
+        required=True, readonly=True)
+    in_commercial_admin = Bool(
+        title=_("True if this person is a commercial admin."),
+        required=True, readonly=True)
+    in_hwdb_team = Bool(
+        title=_("True if this person is on the hwdb team."),
+        required=True, readonly=True)
+    in_janitor = Bool(
+        title=_("True if this person is the janitor."),
+        required=True, readonly=True)
+    in_katie = Bool(
+        title=_("True if this person is Katie."),
+        required=True, readonly=True)
+    in_launchpad_beta_testers = Bool(
+        title=_("True if this person is a Launchpad beta tester."),
+        required=True, readonly=True)
+    in_launchpad_developers = Bool(
+        title=_("True if this person is a Launchpad developer."),
+        required=True, readonly=True)
+    in_mailing_list_experts = Bool(
+        title=_("True if this person is a mailing list expert."),
+        required=True, readonly=True)
+    in_ppa_key_guard = Bool(
+        title=_("True if this person is the ppa key guard."),
+        required=True, readonly=True)
+    in_registry_experts = Bool(
+        title=_("True if this person is a registry expert."),
+        required=True, readonly=True)
+    in_rosetta_experts = Bool(
+        title=_("True if this person is a rosetta expert."),
+        required=True, readonly=True)
+    in_shipit_admin = Bool(
+        title=_("True if this person is a ShipIt admin."),
+        required=True, readonly=True)
+    in_ubuntu_branches = Bool(
+        title=_("True if this person is on the Ubuntu branches team."),
+        required=True, readonly=True)
+    in_ubuntu_security = Bool(
+        title=_("True if this person is on the Ubuntu security team."),
+        required=True, readonly=True)
+    in_ubuntu_techboard = Bool(
+        title=_("True if this person is on the Ubuntu tech board."),
+        required=True, readonly=True)
+    in_vcs_imports = Bool(
+        title=_("True if this person is on the vcs-imports team."),
+        required=True, readonly=True)
+
+    def inTeam(team):
+        """Is this person a member or the owner of `team`?
+
+        Passed through to the same method in 'IPersonPublic'.
+        """
+
+    def isOwner(obj):
+        """Is this person the owner of the object?"""
+
+    def isDriver(obj):
+        """Is this person one of the drivers of the object?"""
+
+    def isOneOf(obj, attributes):
+        """Is this person one of the roles in relation to the object?
+
+        Check if the person is inTeam of one of the given IPerson attributes
+        of the object.
+
+        :param obj: The object to check the relation to.
+        :param attributes: A list of attribute names to check with inTeam.
+        """
+
+
 class ICrowd(Interface):
 
     def __contains__(person_or_team_or_anything):

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-01-07 06:27:46 +0000
+++ lib/canonical/launchpad/security.py	2010-01-08 23:43:19 +0000
@@ -53,12 +53,11 @@
 from canonical.launchpad.interfaces.hwdb import (
     IHWDBApplication, IHWDevice, IHWDeviceClass, IHWDriver, IHWDriverName,
     IHWDriverPackageName, IHWSubmission, IHWSubmissionDevice, IHWVendorID)
-from lp.services.permission_helpers import (
-    is_admin, is_admin_or_registry_expert, is_admin_or_rosetta_expert)
 from lp.services.worlddata.interfaces.language import ILanguage, ILanguageSet
 from lp.translations.interfaces.languagepack import ILanguagePack
 from canonical.launchpad.interfaces.launchpad import (
-    IBazaarApplication, IHasBug, IHasDrivers, ILaunchpadCelebrities)
+    IBazaarApplication, IHasBug, IHasDrivers, ILaunchpadCelebrities,
+    IPersonRoles)
 from lp.registry.interfaces.role import IHasOwner
 from lp.registry.interfaces.location import IPersonLocation
 from lp.registry.interfaces.mailinglist import IMailingListSet
@@ -195,7 +194,8 @@
     usedfor = None
 
     def checkAuthenticated(self, user):
-        return is_admin_or_registry_expert(user)
+        user = IPersonRoles(user)
+        return user.in_admin or user.in_registry_experts
 
 
 class ReviewProduct(ReviewByRegistryExpertsOrAdmins):
@@ -231,9 +231,10 @@
         if self.obj.active:
             return True
         else:
-            celebrities = getUtility(ILaunchpadCelebrities)
-            return (user.inTeam(celebrities.commercial_admin)
-                    or is_admin_or_registry_expert(user))
+            user = IPersonRoles(user)
+            return (user.in_commercial_admin or
+                    user.in_admin or
+                    user.in_registry_experts)
 
 
 class EditAccountBySelfOrAdmin(AuthorizationBase):
@@ -247,7 +248,8 @@
             EditAccountBySelfOrAdmin, self).checkAccountAuthenticated(account)
 
     def checkAuthenticated(self, user):
-        return is_admin(user)
+        user = IPersonRoles(user)
+        return user.in_admin
 
 
 class ViewAccount(EditAccountBySelfOrAdmin):
@@ -259,7 +261,8 @@
 
     def checkAuthenticated(self, user):
         """Extend permission to registry experts."""
-        return is_admin_or_registry_expert(user)
+        user = IPersonRoles(user)
+        return user.in_admin or user.in_registry_experts
 
 
 class ModerateAccountByRegistryExpert(AuthorizationBase):
@@ -267,7 +270,8 @@
     permission = 'launchpad.Moderate'
 
     def checkAuthenticated(self, user):
-        return is_admin_or_registry_expert(user)
+        user = IPersonRoles(user)
+        return user.in_admin or user.in_registry_experts
 
 
 class EditOAuthAccessToken(AuthorizationBase):
@@ -498,7 +502,8 @@
 
     def checkAuthenticated(self, user):
         """Allow Launchpad's admins and Rosetta experts edit all fields."""
-        return is_admin_or_rosetta_expert(user)
+        user = IPersonRoles(user)
+        return user.in_admin or user.in_rosetta_experts
 
 
 class AdminProductTranslations(AuthorizationBase):
@@ -511,8 +516,10 @@
         Any Launchpad/Launchpad Translations administrator or owners are
         able to change translation settings for a product.
         """
-        return (user.inTeam(self.obj.owner) or
-                is_admin_or_rosetta_expert(user))
+        user = IPersonRoles(user)
+        return (user.isOwner(self.obj) or
+                user.in_rosetta_experts or
+                user.in_admin)
 
 
 class AdminSeriesByVCSImports(AuthorizationBase):

=== added file 'lib/canonical/launchpad/tests/test_personroles.py'
--- lib/canonical/launchpad/tests/test_personroles.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/tests/test_personroles.py	2010-01-08 23:43:19 +0000
@@ -0,0 +1,151 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import unittest
+
+from zope.interface.verify import verifyObject
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.interfaces.launchpad import (
+    ILaunchpadCelebrities, IPersonRoles)
+from lp.registry.interfaces.person import IPerson
+
+from lp.testing import TestCaseWithFactory
+from canonical.testing import ZopelessDatabaseLayer
+
+
+class TestPersonRoles(TestCaseWithFactory):
+    """Test IPersonRoles adapter.
+     """
+
+    layer = ZopelessDatabaseLayer
+
+    prefix = 'in_'
+
+    def setUp(self):
+        super(TestPersonRoles, self).setUp()
+        self.person = self.factory.makePerson()
+        self.celebs = getUtility(ILaunchpadCelebrities)
+
+    def test_interface(self):
+        roles = IPersonRoles(self.person)
+        verifyObject(IPersonRoles, roles)
+
+    def test_person(self):
+        # The person is available through the person attribute.
+        roles = IPersonRoles(self.person)
+        self.assertIs(self.person, roles.person)
+
+    def _get_person_celebrities(self, is_team):
+        for name in ILaunchpadCelebrities.names():
+            attr = getattr(self.celebs, name)
+            if IPerson.providedBy(attr) and attr.isTeam() == is_team:
+                yield (name, attr)
+
+    def test_in_teams(self):
+        # Test all celebrity teams are available.
+        for name, team in self._get_person_celebrities(is_team=True):
+            roles_attribute = self.prefix + name
+            roles = IPersonRoles(self.person)
+            self.assertFalse(
+                getattr(roles, roles_attribute),
+                "%s should be False" % roles_attribute)
+
+            team.addMember(self.person, team.teamowner)
+            roles = IPersonRoles(self.person)
+            self.assertTrue(
+                getattr(roles, roles_attribute),
+                "%s should be True" % roles_attribute)
+            self.person.leave(team)
+
+    def test_is_person(self):
+        # All celebrity persons are available.
+        for name, celeb in self._get_person_celebrities(is_team=False):
+            roles_attribute = self.prefix + name
+            roles = IPersonRoles(celeb)
+            self.assertTrue(
+                getattr(roles, roles_attribute),
+                "%s should be True" % roles_attribute)
+
+    def test_in_AttributeError(self):
+        # Do not check for non-existent attributes, even if it has the
+        # right prefix.
+        roles = IPersonRoles(self.person)
+        fake_attr = self.factory.getUniqueString()
+        self.assertRaises(AttributeError, getattr, roles, fake_attr)
+        fake_attr = self.factory.getUniqueString(self.prefix)
+        self.assertRaises(AttributeError, getattr, roles, fake_attr)
+
+    def test_inTeam(self):
+        # The method person.inTeam is available as the inTeam attribute.
+        roles = IPersonRoles(self.person)
+        self.assertEquals(self.person.inTeam, roles.inTeam)
+
+    def test_inTeam_works(self):
+        # Make sure it actually works.
+        team = self.factory.makeTeam(self.person)
+        roles = IPersonRoles(self.person)
+        self.assertTrue(roles.inTeam(team))
+
+    def test_isOwner(self):
+        # The person can be the owner of something, e.g. a product.
+        product = self.factory.makeProduct(owner=self.person)
+        roles = IPersonRoles(self.person)
+        self.assertTrue(roles.isOwner(product))
+
+    def test_isDriver(self):
+        # The person can be the driver of something, e.g. a sprint.
+        sprint = self.factory.makeSprint()
+        sprint.driver = self.person
+        roles = IPersonRoles(self.person)
+        self.assertTrue(roles.isDriver(sprint))
+
+    def test_isDriver_multiple_drivers(self):
+        # The person can be one of multiple drivers of if a product and its
+        # series each has a driver.
+        productseries = self.factory.makeProductSeries()
+        productseries.product.driver = self.person
+        productseries.driver = self.factory.makePerson()
+        roles = IPersonRoles(self.person)
+        self.assertTrue(roles.isDriver(productseries))
+
+    def test_isOneOf(self):
+        # Objects may have multiple roles that a person can fulfill.
+        # Specifications are such a case.
+        spec = removeSecurityProxy(self.factory.makeSpecification())
+        spec.owner = self.factory.makePerson()
+        spec.drafter = self.factory.makePerson()
+        spec.assignee = self.factory.makePerson()
+        spec.approver = self.person
+
+        roles = IPersonRoles(self.person)
+        self.assertTrue(roles.isOneOf(
+            spec, ['owner', 'drafter', 'assignee', 'approver']))
+
+    def test_isOneOf_None(self):
+        # Objects may have multiple roles that a person can fulfill.
+        # Specifications are such a case. Some roles may be None.
+        spec = removeSecurityProxy(self.factory.makeSpecification())
+        spec.owner = self.factory.makePerson()
+        spec.drafter = None
+        spec.assignee = None
+        spec.approver = self.person
+
+        roles = IPersonRoles(self.person)
+        self.assertTrue(roles.isOneOf(
+            spec, ['owner', 'drafter', 'assignee', 'approver']))
+
+    def test_isOneOf_AttributeError(self):
+        # Do not try to check for none-existent attributes.
+        obj = self.factory.makeProduct()
+        fake_attr = self.factory.getUniqueString()
+        roles = IPersonRoles(self.person)
+        self.assertRaises(AttributeError, roles.isOneOf, obj, [fake_attr])
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)
+

=== added file 'lib/canonical/launchpad/utilities/personroles.py'
--- lib/canonical/launchpad/utilities/personroles.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/utilities/personroles.py	2010-01-08 23:43:19 +0000
@@ -0,0 +1,55 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Class that implements the IPersonRoles interface."""
+
+__metaclass__ = type
+__all__ = ['PersonRoles']
+
+from zope.interface import implements
+from zope.component import adapts, getUtility
+from canonical.launchpad.interfaces import (
+    ILaunchpadCelebrities, IPersonRoles)
+
+from lp.registry.interfaces.person import IPerson
+
+
+class PersonRoles:
+    implements(IPersonRoles)
+    adapts(IPerson)
+
+    def __init__(self, person):
+        self.person = person
+        self._celebrities = getUtility(ILaunchpadCelebrities)
+        self.inTeam = self.person.inTeam
+
+    def __getattr__(self, name):
+        """Handle all in_* attributes."""
+        prefix = 'in_'
+        if not name.startswith(prefix):
+            raise AttributeError
+        attribute = name[len(prefix):]
+        return self.person.inTeam(getattr(self._celebrities, attribute))
+
+    def isOwner(self, obj):
+        """See IPersonRoles."""
+        return self.person.inTeam(obj.owner)
+
+    def isDriver(self, obj):
+        """See IPersonRoles."""
+        drivers = getattr(obj, 'drivers', None)
+        if drivers is None:
+            return self.person.inTeam(obj.driver)
+        for driver in drivers:
+            if self.person.inTeam(driver):
+                return True
+        return False
+
+    def isOneOf(self, obj, attributes):
+        """See IPersonRoles."""
+        for attr in attributes:
+            role = getattr(obj, attr)
+            if self.person.inTeam(role):
+                return True
+        return False
+

=== modified file 'lib/canonical/launchpad/zcml/launchpad.zcml'
--- lib/canonical/launchpad/zcml/launchpad.zcml	2009-12-23 16:17:12 +0000
+++ lib/canonical/launchpad/zcml/launchpad.zcml	2010-01-08 23:43:19 +0000
@@ -356,6 +356,16 @@
 
 </facet>
 
+<class
+    class="canonical.launchpad.utilities.personroles.PersonRoles">
+    <allow
+        interface="canonical.launchpad.interfaces.IPersonRoles"/>
+</class>
+<adapter
+    for="lp.registry.interfaces.person.IPerson"
+    provides="canonical.launchpad.interfaces.IPersonRoles"
+    factory="canonical.launchpad.utilities.personroles.PersonRoles" />
+
 
 <adapter
   provides="canonical.lazr.interfaces.IObjectPrivacy"

=== removed file 'lib/lp/services/permission_helpers.py'
--- lib/lp/services/permission_helpers.py	2009-12-15 21:34:43 +0000
+++ lib/lp/services/permission_helpers.py	1970-01-01 00:00:00 +0000
@@ -1,46 +0,0 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Helpful functions to enforce permissions."""
-
-__metaclass__ = type
-
-__all__ = [
-    'is_admin',
-    'is_admin_or_registry_expert',
-    'is_admin_or_rosetta_expert',
-    'is_registry_expert',
-    'is_rosetta_expert',
-    ]
-
-from zope.component import getUtility
-
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
-
-
-def is_admin(user):
-    """Check if the user is a Launchpad admin."""
-    celebrities = getUtility(ILaunchpadCelebrities)
-    return user.inTeam(celebrities.admin)
-
-
-def is_rosetta_expert(user):
-    """Check if the user is a Rosetta expert."""
-    celebrities = getUtility(ILaunchpadCelebrities)
-    return user.inTeam(celebrities.rosetta_experts)
-
-
-def is_registry_expert(user):
-    """Check if the user is a registry expert."""
-    celebrities = getUtility(ILaunchpadCelebrities)
-    return user.inTeam(celebrities.registry_experts)
-
-
-def is_admin_or_rosetta_expert(user):
-    """Check if the user is a Launchpad admin or a Rosetta expert."""
-    return is_admin(user) or is_rosetta_expert(user)
-
-
-def is_admin_or_registry_expert(user):
-    """Check if the user is a Launchpad admin or a registry expert."""
-    return is_admin(user) or is_registry_expert(user)

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2009-12-24 11:34:12 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2010-01-08 23:43:19 +0000
@@ -32,7 +32,8 @@
 from canonical.database.constants import UTC_NOW, DEFAULT
 from canonical.database.enumcol import EnumCol
 from canonical.launchpad.helpers import shortlist
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.interfaces.launchpad import (
+    ILaunchpadCelebrities, IPersonRoles)
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.webapp.interfaces import NotFoundError
 from lp.registry.interfaces.distribution import IDistribution
@@ -43,7 +44,6 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.sourcepackage import ISourcePackage
-from lp.services.permission_helpers import is_admin_or_rosetta_expert
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.translations.interfaces.pofile import IPOFileSet
 from lp.translations.interfaces.potemplate import IPOTemplateSet
@@ -285,12 +285,13 @@
 
     def isUserUploaderOrOwner(self, user):
         """See `ITranslationImportQueueEntry`."""
-        if user.inTeam(self.importer):
+        roles = IPersonRoles(user)
+        if roles.inTeam(self.importer):
             return True
         if self.productseries is not None:
-            return user.inTeam(self.productseries.product.owner)
+            return roles.isOwner(self.productseries.product)
         if self.distroseries is not None:
-            return user.inTeam(self.distroseries.distribution.owner)
+            return roles.isOwner(self.distroseries.distribution)
         return False
 
     def canSetStatus(self, new_status, user):
@@ -298,7 +299,8 @@
         if user is None:
             # Anonymous user cannot do anything.
             return False
-        can_admin = (is_admin_or_rosetta_expert(user) or
+        roles = IPersonRoles(user)
+        can_admin = (roles.in_admin or roles.in_rosetta_experts or
                      self.isUbuntuAndIsUserTranslationGroupOwner(user))
         if new_status == RosettaImportStatus.APPROVED:
             # Only administrators are able to set the APPROVED status, and
@@ -309,11 +311,11 @@
             # Only rosetta experts are able to set the IMPORTED status, and
             # that's only possible if we know where to import it
             # (import_into not None).
-            return (is_admin_or_rosetta_expert(user) and
+            return ((roles.in_admin or roles.in_rosetta_experts) and
                     self.import_into is not None)
         if new_status == RosettaImportStatus.FAILED:
             # Only rosetta experts are able to set the FAILED status.
-            return is_admin_or_rosetta_expert(user)
+            return roles.in_admin or roles.in_rosetta_experts
         # All other statuses can bset set by all authorized persons.
         return self.isUserUploaderOrOwner(user) or can_admin