launchpad-dev team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

You may be used to calling check_permission to perform permission checks in
view code. Check_permission does not take a "user" parameter for whom to check
the permission because Zope figures that our from the current interaction,
i.e. the current request. That is fine for view code.

When writing security adapters (the classes in security.py), using
check_permission is the same as forwarding the request to another security
adapter which is looked up by name (permission) and source object. A security
check for a logged-in user is performed by checkAuthenticated(user). If
checkAuthenticated calls check_permission, the user parameter does not get
passed on but instead check_permission will figure it out for itself, as
described above.

This is bad for several reasons:
 - It is bad interface design if a method claims to perform some operation on
   a given parameter but then goes and throws that parameter away only to try
   and figure it out by itself.
 - Obviously, there is repetitive code involved which is a waste of CPU
   cycles.
 - While it may be quite unlikely, check_permission might still get it wrong
   and check the permission for a different user than the one being passed
   into checkAuthenticated. The code is not stopping it from doing that. This
   could get really ugly.

Revision 12867 of devel removes all uses of check_permission from
lib/canonical/launchapd/security.py (other security.py instances where not
affected) and introduces a 'forwardCheckAuthenticated' method on Authorization
base to replace them.

    def forwardCheckAuthenticated(self, user,
                                  obj=None, permission=None):
        """Forward request to another security adapter.

        Find a matching adapter and call checkAuthenticated on it. Intended
        to be used in checkAuthenticated.

        :param user: The IRolesPerson object that was passed in.
        :param obj: The object to check the permission for. If None, use
            the same object as this adapter.
        :param permission: The permission to check. If None, use the same
            permission as this adapter.
        :return: True or False.
        """

Usage example:

class EditDistroSeriesDifference(AuthorizationBase):
    """Anyone with lp.View on the distribution can edit a DSD."""
    permission = 'launchpad.Edit'
    usedfor = IDistroSeriesDifferenceEdit

    def checkAuthenticated(self, user):
        return self.forwardCheckAuthenticated(
            user, self.obj.derived_series.distribution, 'launchpad.View')

Oftentimes the permission remains the same so that the last parameter can be
omitted but it is also possible to do it the other way round, keep the object
but change the permission.

Please make use of forwardCheckAuthenticated and do not re-introduce
check_permission into security.py. Thank you. ;-)

Henning