launchpad-dev team mailing list archive

[Bjorn Tillenius <bjorn@xxxxxxxxxxxxx>]

On Wed, Dec 16, 2009 at 10:29:53AM +0100, Henning Eggers wrote:
> Please consider a method called "setStatus(new_status)" on a model
> class. This is a fairly common pattern as many objects in the model have
> some sort of status attribute and you want to control access to it [1].
> Using the zcml and permission checkers in security.py you can control
> who can access (i.e. call) "setStatus" on the class but you cannot
> control which parameters may be passed into "setStatus", i.e. which
> statuses the current user may set or not. This is a known limitation of
> the zope security model, I guess.

Well, kind of. The zope security machinery handles only attribute
getting and setting.


> The current practice in Launchpad code is to pass the requesting user
> into the method, i.e. "setStatus(new_status, user)" and let the model
> method do the checking. I found that this results in a lot of repetitive
> code between the model and security.py. The checks are similar, mostly
> testing "user.inTeam(...)" and require boiler plate code like
> "getUtility(ILaunchpadCelebrities)".  I tried to reduce some of that
> code duplication by introducing security_helpers.py which has simple
> methods like "is_admin_or_rosetta_expert(user)" which can both be used
> by the checkers in security.py and the model code for parameter value
> enforcement. The file security_helpers.py is currently located in
> lp/translation/utilities but I already have a branch here that moves it
> to lp/services for more general use. That location is still up to
> debate, though.
> 
> Adi suggested today that it would be less confusing if we had all the
> security related code in security.py and use the checkers from model
> code, i.e. calling something like
> "OnlyRosettaExpertsAndAdmins(self).checkAuthenticated(user)".

I think you're on a slipperly slope here. Basically what you want to do
is to replace things like (using pseudo code here):

    if user_is_admin() or user_is_rosetta_expert():
        do_things()

with:

    if user_is_admin_or_rosetta_expert():
        do_things()

I.e., you want to take an if condition and replace it with a function.
Instead of using Python's 'or' key word, you want to construct text
strings. Your example is simple, but what happens when you need to add
a condition? You have to add another function, which has the same name,
but with '_or_new_condition' appended to it. It quickly leads to names
like EditDistributionMirrorByOwnerOrDistroOwnerOrMirrorAdminsOrAdmins,
which is hard to read, and you can't wrap the line, if it gets too long.
You also have the issue that some checkers need a specific context
object, and will check the properties of that object.

Let's keep the canonical.launchpad.security separate, and let's not
depend on it. It's the security policy that is used by the web app
mainly. For example, in scripts, we don't use it, and shouldn't have to
import it really. I don't think it provides a nice API for the checks
that you want to do. Sometimes it makes sense to share code. What we
usally do is to put a method on the model class, which the security
policy can use. See PublicToAllOrPrivateToExplicitSubscribersForBugTask
for an example.



-- 
Björn Tillenius | https://launchpad.net/~bjornt