launchpad-dev team mailing list archive

[Adi Roiban <adi@xxxxxxxxx>]

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