Code review comment for lp:~adiroiban/launchpad/bug-509252-take-2

Hi Adi,

Thanks for all the cleanups that you've done in this branch.

After reading the recent email discussion at:

https://lists.launchpad.net/launchpad-dev/msg02442.html

and particularly BjornT's message:

https://lists.launchpad.net/launchpad-dev/msg02504.html

I don't think putting the security check into a browser helper is the way to go.

As outlined in the above email:
"The current solution is to have this code in model code, and have the
security adapter ask the model. No code duplication really."

this ensures that any security can be applied not only in the view, but also via other modes of access (API), even if they are not yet enabled.

As it turns out in your code, it seems most of IDistroSeries.checkTranslationsViewable() is not actually security code, but simply appropriate error generation. The first few lines however are security related.

So I wonder if we can do something like this:

http://pastebin.ubuntu.com/379120/

That is, simply add a launchpad.View check for IDistroSeriesLanguage, which itself ensures that launchpad.View is granted if the user is an admin. That way, as in the example in the diff, the current double security checks:

         if not check_permission(
            'launchpad.TranslationsAdmin', distroserieslang):
            self.context.checkTranslationsViewable()

could be replaced simply by:

         if not check_permission(
            'launchpad.View', distroserieslang):
            translation_unavailable_error()

that way you'd have all the security in the right place, separate from the error generation (ie. translation_unavailable_error() would be your proposed view helper, check_distroseries_translations_viewable, just without the security checks?

I'm not 100%, but as far as I can see, the above should do what you're after while at the same time keeping all the security code in the one place. But I'll check with Henninge to be sure (hence marking this as needs info).

IRC log of conversation starts at:

http://irclogs.ubuntu.com/2010/02/18/%23launchpad-reviews.html#t11:18