Code review comment for lp:~henninge/launchpad/bug-503454-security-py

Hi Henning,

This branch is a nice improvement. It's always good to shrink files.

I have a few comments below, but I also would like this change made,
which isn't actually part of your diff. In PersonRoles.__getattr__
please add info to the exception you raise so that it matches the error
python gives you for its default types such as:
    AttributeError: 'int' object has no attribute 'foo'

merge-conditional

-Edwin

>=== added file 'lib/canonical/launchpad/doc/personroles.txt'
>--- lib/canonical/launchpad/doc/personroles.txt 1970-01-01 00:00:00 +0000
>+++ lib/canonical/launchpad/doc/personroles.txt 2010-01-13 16:45:40 +0000
>@@ -0,0 +1,131 @@
>+= PersonRoles =
>+
>+To make the checking for certain roles that a person can have more convenient

This makes it a little easier to read:
    "To make it more convenient to check which roles a person has,"

>+the IPersonRoles adapter is provided. It's main use is to easily check if a
>+person is the member of a celebrity team or is a celebrity person. In
>+addition, methods for common checks are provided, too.
>+
>+The IPersonRoles interface is closely tight to the ILaunchpadCelebrities
>+interface. Any addition or removal of a person celebrity must be reflected in
>+adding or removing the corresponding property in IPersonRoles. Luckily the
>+celbrities.txt doctest includes a check for this and will give a useful
>+information of which attribute needs to be added or removed. Both interfaces

s/celbrities.txt/celebrities.txt/
s/give a useful/give useful/
s/information of/information on/

>+are found in the same file. There is no need to adapt the implementation
>+class PersonRoles, though (thanks to __getattr__).
>+
>+PersonRoles is most prominent in AuthenticationBase in security.py. The user
>+parameter to checkAuthenticated is a PersonRoles object (used to be a Person
>+object).

s/used to be/was/
or "this used to be" to differentiate it from "this is used to be".
Otherwise, it's not clear if "this" or "this is" was dropped until
you get to the middle of the sentence.

>+== The person object and the adapter ==
>+
>+PersonRoles is registered as an unnamed adapter for IPersonRoles.
>+
>+ >>> from canonical.launchpad.interfaces.launchpad import IPersonRoles
>+ >>> person = factory.makePerson()
>+ >>> print IPersonRoles(person)
>+ <canonical.launchpad.utilities.personroles.PersonRoles object at ...>
>+
>+The original Person object can be reached through the person attribute.
>+
>+ >>> roles = IPersonRoles(person)
>+ >>> print roles.person is person
>+ True
>+
>+
>+== Celebrity persons ==
>+
>+There are a number of celebrity persons defined in ILaunchpadCelebrities.
>+PersonRoles has a corresponding attribute of the same name prefixed with
>+"in_" to check if the person in question is a member of this celebrity or is
>+this celebrity. The following tests are identical.
>+
>+ >>> from canonical.launchpad.interfaces.launchpad import (
>+ ... ILaunchpadCelebrities)
>+ >>> rosetta_experts = getUtility(ILaunchpadCelebrities).rosetta_experts
>+ >>> print person.inTeam(rosetta_experts)
>+ False
>+
>+ >>> print roles.in_rosetta_experts
>+ False
>+
>+The test will succeed once we make person a member of the team. Need to be an
>+admin to do that.
>+
>+ >>> login("<email address hidden>")
>+ >>> rosetta_experts.addMember(person, rosetta_experts.teamowner)
>+ (True, ...Approved>)
>+ >>> print roles.in_rosetta_experts
>+ True
>+
>+To stay consistent, all attributes are prefixed with "in_" although the
>+attributes names of ILaunchpadCelebrities are not all lexically correct

s/attributes names/attribute names/

>+plurals nor are all the attributes teams. This makes for odd sounding

There needs to be a comma before "nor" since that clause has
a subject.

>+attribute names in IPersonRoles.
>+
>+ >>> print roles.in_admin
>+ False
>+ >>> print roles.in_janitor
>+ False
>+
>+ >>> janitor = getUtility(ILaunchpadCelebrities).janitor
>+ >>> janitor_roles = IPersonRoles(janitor)
>+ >>> print janitor_roles.in_janitor
>+ True
>+
>+
>+== inTeam ==
>=== modified file 'lib/canonical/launchpad/security.py'
>--- lib/canonical/launchpad/security.py 2010-01-09 03:31:20 +0000
>+++ lib/canonical/launchpad/security.py 2010-01-13 16:45:40 +0000
>@@ -153,7 +153,7 @@
> if person is None:
> return self.checkUnauthenticated()
> else:
>- return self.checkAuthenticated(person)
>+ return self.checkAuthenticated(IPersonRoles(person))
>
>
> class ViewEmailAddress(AuthorizationBase):
>@@ -2309,15 +2215,15 @@
> self.obj.person.hide_email_addresses):
> return True
>
>- user = IPerson(account, None)
>- if user is None:
>+ person = IPerson(account, None)
>+ if person is None:
> return False
>
>- celebrities = getUtility(ILaunchpadCelebrities)
>+ user = IPersonRoles(person)

EDG: Perhaps, this can be changed to:
    user = IPersonRoles(IPerson(account, None), None)
    if user is None:
        return False

This will prevent any confusion between the `person` object and the
`self.obj.person` object.

> return (self.obj.person is not None and user.inTeam(self.obj.person)
>- or user.inTeam(celebrities.commercial_admin)
>- or user.inTeam(celebrities.registry_experts)
>- or user.inTeam(celebrities.admin))
>+ or user.in_commercial_admin
>+ or user.in_registry_experts
>+ or user.in_admin)
>
>