launchpad-dev team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Hi all LP coders,
this is a discussion that I have already had on the lazr-js sprint and
that now came up again as Adi, Translations' top community contributor,
touched on it, too.

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.

One possible solution would be to create different "setStatus" methods,
like "setStatusOwner" and "setStatusAdmin" which statically restrict
parameter values. Access to these methods could then be controlled in
the normal fashion through the zcml. I was advised at the sprint that
this is not a good solution, though, but I never discussed it much. [2]

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)". The
consequence of this is that we'd have to create checkers in security.py
that do not relate directly to any zcml directive as they are used to
check parameter values. I had viewed security.py as our Python interface
to the zcml. Using these checkers for other purposes might be misusing
them but I am not sure. Also, we'd have to come up with a naming
convention for these classes (anything derived from AuthorizationBase).
These names have been pretty arbitrary so far, either denoting the
permission and the interface (i.e. "ViewAccount") or what they are
checking for (like "OnlyRosettaExpoertsAndAdmin") or a combination of
those. The reason for this was that they were never called from our code
but only through the authorization framework, so nobody really cared
about their names.

So, the questions are: Do we agree on consolidating security-related
code into a single location (file or package)? Should it be in
security.py? Should we use permission checkers (anything derived from
AuthorizationBase) in model code?

Any comments welcome, hoping for some easy consensus here... ;)

Henning


[1] Why do that in the model? Because the model gets directly exposed
through the web API without the use of a (explicit) view class.

[2] Using "check_permission" in its current from in model code is out of
the question as it implicitly checks request.user which model code does
not have. Do a "grep check_permission lib/lp/*/model/*.py" for kicks.