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

= Bug 506454 =

This is a tech-dept relief that applies some refactoring to security.py. It may seem like a lot of work for the transition but afterwards will make it more straight forward to write security checkers.

This branch introduces and API change for "checkAuthenticated" which used to receive an "IPerson" object as its "user" parameter which has now been changed to be an "IPersonRoles" object. The old object can still be reached via "user.person".

This branch also contains a change to IPersonRoles and its implementation because at least one test wanted to check the "driver" attribute of an object although the object also has a "drivers" attribute (IHasDrivers). Although this may actually be a bug (why should the driver of a product have less permissions than the driver of a productseries?) I provided an explicit isOneOfDrivers method. This also makes isDriver symmetrical to isOwner.

Some tests needed to be adapted because they used "checkAuthenticated" on an object that implements "IAuthorization". "checkAuthenticated" has been replaced by "checkAccountAuthenticated" in "IAuthorization".

The diff is overly long because of the many very similar changes in security.py.

== Implementation details ==

lib/canonical/launchpad/doc/hasowner-authorization.txt

 * Replaced checkAuthenticated with checkAccountAuthenticated.
 * Added some print statements.

lib/canonical/launchpad/doc/personroles.txt

 * New doc test for PersonRoles so people have no fear of using it in security.py. ;-)

lib/canonical/launchpad/interfaces/launchpad.py

 * Added isOneOfDrivers method to IPersonRoles interface.

lib/canonical/launchpad/security.py

 * The main change in this branch: AuthorizationBase.checkAccountAuthenticated applies the IPersonRoles adapter to the IPerson object which it derives from the account. So unless checkAccountAuthenticated gets overwritten, checkAuthenticated reveives an IPersonRoles object instead of an IPerson object.
 * Replaced all calls to user.inTeam(celebrity.some_celeb) with user.in_some_celeb.
 * Replaced owner and driver checks with the respective methods from IPersonRoles.
 * Where ever the user object is still needed as an IPerson object, it was replaced with user.person.
 * A few checkers were optimized in the course of this refactoring.

lib/canonical/launchpad/utilities/personroles.py

 * Implementation of isOneOfDrivers which is mostly the previous implementation of isDriver but now checks for the IHasDrivers interface instead of the actual attribute. (Bye-bye duck-typing, hello contract-typing. :)

lib/lp/code/doc/branch-visibility.txt

 * Mechanical replacement of checkAuthenticated with checkAccountAuthenticated.

== Tests ==

As almost all of Launchpad is affected by security.py, only the full test suite gives that feeling of security.
Test PersonRoles itself like this:

bin/test -vvct personroles -t celebrities

== QA ==

Launchpad still working as it used to? Good.

= 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/security.py
  lib/canonical/launchpad/doc/hasowner-authorization.txt
  lib/canonical/launchpad/doc/personroles.txt
  lib/canonical/launchpad/interfaces/launchpad.py
  lib/canonical/launchpad/tests/test_personroles.py
  lib/canonical/launchpad/utilities/personroles.py
  lib/lp/code/doc/branch-visibility.txt

== Pyflakes notices ==

lib/canonical/launchpad/interfaces/launchpad.py
    24: 'UnexpectedFormData' imported but unused
    24: 'UnsafeFormGetSubmissionError' imported but unused
    24: 'NotFoundError' imported but unused
    24: 'IBasicLaunchpadRequest' imported but unused
    24: 'IOpenLaunchBag' imported but unused
    24: 'ILaunchpadRoot' imported but unused
    24: 'ILaunchBag' imported but unused