launchpad-dev team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Am 16.12.2009 11:27, Bjorn Tillenius schrieb:
> 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.

Ah no, that is not the main intention. I am happy with using the python
or operator. I came up with is_admin_or_rosetta_expert because there
already was "OnlyRosettaExpertsAndAdmins" in security.py and I needed to
do the same check in model code. My hunch is that naming the checker in
security.py like this was a bad idea in the first place.

My first goal was to avoid repetitive constructs of

    celeb = getUtiltiy(ILaunchpadCelebrities)
    user.inTeam(celeb.admin)

or longish things like

    user.inTeam(getUtiltiy(ILaunchpadCelebrities).admin)

.
My second goal was to consolidate checks into one place but I agree that
 inTeam checks are only part of the story and that checks may be too
diverse and individual to put them into some kind of "library".

I have since had the idea that it would be nice add those checks
directly to the IPerson, preferably by the way of annotation. So the
above would look like this:

    check_user = UserChecker(user)
    if check_user.is_admin or check_user.is_rosetta_expert:
        do_something()

UserChecker could provide attributes like this for all celebrity teams
and the wrapping could happen in AuthorizationBase so that
checkAuthorized already gets the annotated user.

Stuart just tells me about the crowd concept that might help here, too.

> 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.

That is what I felt, too. Glad we agree here.

> 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.

Yes, I have done that myself before. This question was about common
patterns but thanks for the reminder.