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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
