Hi, Since I am new to Python, Zope and LP code, feel free to ignore my comment if you consider them pure nonsenses.
În data de Lu, 21-12-2009 la 12:26 +0100, Henning Eggers a scris: > Am 20.12.2009 02:51, Jonathan Lange schrieb: > > The other thing is that the function's external API is pre-supposing > > certain policy decisions. It's much better to name the function after > > what you actually _want_ rather than the mechanism for figuring it > > out. > > > > e.g. can_translate() rather than user_is_admin_or_rosetta_expert() > > > > An example from the package branches extravaganza: rather than > > checking Branch.product is None, we now check > > Branch.supportsMergeProposals() (or something like that). > > I like that very much and have done that myself before. I think I can > take these rules from the discussion: > > 2. Permission checks specific to a model class should be placed into > that class as "canDoSomething(user)" methods and called from the > checkers in security.py on self.obj. I think that a lot of the checks > fall into this category and the checking code could be moved into the model. My questions were raised after seeing this method: TranslationImportQueueEntry.isUbuntuAndIsUserTranslationGroupOwner(user) I prefer "canDoSomething() or canDoSomethingElse()" to isUbuntuAndIsUserTranslationGroupOwner. > 1. Do not use the checker classes from security.py anywhere else, i.e. > do not call "SomeThing.checkAuthenticated(user)" from model code. > > 3. The functions I put into permission_helpers.py are either too > specific or too trivial. I see that now and will do away with it. I > guess what I am mostly concerned about is the ILaunchpadCelebrities > noise. I would prefer to be able to simply check a "user.is_admin" property. Since we already have AdminDistributionTranslations or AdminImportQueueEntry in security.py and basically they implements canAdminDistributionTranslations or canAdminImportQueueEntry why not reuse them or create a wrapper around them? Also since the current model is not user aware we can end up with something like the current implementation for checkTranslationsViewable() from registry.model.Distroseries.checkTranslationsViewable() and translations.browser.DistroSeriesView.checkTranslationsViewable() I like security.py + configure.zcml because they can provide an overview for the current security policy and I don't need to browse all models in search for a specific security policy. Also security.py classes are mapped to model classed proving a security interface for them. Maybe security.py interface is not enough for LP needs, but rather than putting security code into the model, why not create a dedicated module to handle those needs? Some of those classes could be mapped directly to related classed from security.py. >From my point of view the model should only have the role of a model, not model + security. But maybe there are some good reason for augmenting the model with security knowledge. Cheers, -- Adi Roiban _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
