launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/devel-bug-638920-private-branch into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #638920 Private translations export branches break productseries translation page
  https://bugs.launchpad.net/bugs/638920


= Bug 638920 =

The translations page for a product series shows links the import and export Bazaar branches, if there are any. If either of these branches is private and the user no permissions to view them, the page fails with "unauthorized". The main page for a product series also displays a link to the series branch (which is the import branch in case of translations) but if it is private, it pretends that no branch has been set at all. Yes, that's flat out lying ;-). But since that page gets away with, the translations page can (in fact must) do the same.


== Proposed fix ==

Change the has_imports_enabled and has_exports_enabled flags on the view class to check for "View" permission on the branch. If the user lacks permission, the should return "False" just like when no imports or exports have been enabled.


== Implementation details ==

The actual fix is a few lines in lp/translations/browser/productseries.py that add a check_permission call to each of the two properties. It also removes a redundant check from uses_bzr_sync which is now the simple "or" combination of the other two flags.

The rest of the branch consists of tests for the two flags which were missing before and a pagetest that reproduced the bug nicely.


== Tests ==

I learned a little more about test layers because the new test would not work on the ZopelessLayer but needs the FunctionalLayer instead to be able to check permissions. Notice how I removed the security proxy from the productseries to be able to set it up (assign a branch, set import mode) for the test but the view itself is created using the secured copy of the productseries.
LaunchpadZopelessLayer which includes the LibrarianLayer and MemcachedLayer is not needed, though, so I changed that for the other tests.

bin/test -vvcm lp.translations \
    -t TestProductSeriesViewBzrUsage \
    -t xx-productseries-translations.txt


== Demo and QA ==

See my little screen cast that I produced for this just for the fun of it. ;-)

http://people.canonical.com/~henninge/screencasts/private_branch_link.ogv

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/productseries.py
  lib/lp/translations/browser/tests/test_productserieslanguage_views.py
  lib/lp/translations/stories/productseries/xx-productseries-translations.txt
  lib/lp/translations/templates/productseries-translations.pt

-- 
https://code.launchpad.net/~henninge/launchpad/devel-bug-638920-private-branch/+merge/39540
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-bug-638920-private-branch into lp:launchpad/devel.

=== modified file 'lib/lp/translations/browser/productseries.py'
--- lib/lp/translations/browser/productseries.py	2010-09-27 21:46:10 +0000
+++ lib/lp/translations/browser/productseries.py	2010-10-28 15:23:45 +0000
@@ -142,6 +142,7 @@
     def has_imports_enabled(self):
         """Is imports enabled for the series?"""
         return (self.context.branch is not None and
+                check_permission("launchpad.View", self.context.branch) and
                 self.context.translations_autoimport_mode !=
                 TranslationsBranchImportMode.NO_IMPORT)
 
@@ -346,11 +347,12 @@
             self.context.getCurrentTranslationTemplates().count() > 1)
 
         self.has_exports_enabled = (
-            self.context.translations_branch is not None)
+            self.context.translations_branch is not None and
+            check_permission(
+                "launchpad.View", self.context.translations_branch))
 
         self.uses_bzr_sync = (
-            (self.context.branch is not None and self.has_imports_enabled) or
-            self.has_exports_enabled)
+            self.has_imports_enabled or self.has_exports_enabled)
 
     @property
     def productserieslanguages(self):

=== modified file 'lib/lp/translations/browser/tests/test_productserieslanguage_views.py'
--- lib/lp/translations/browser/tests/test_productserieslanguage_views.py	2010-10-27 11:44:17 +0000
+++ lib/lp/translations/browser/tests/test_productserieslanguage_views.py	2010-10-28 15:23:45 +0000
@@ -3,20 +3,29 @@
 
 __metaclass__ = type
 
+from zope.security.proxy import removeSecurityProxy
+
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import LaunchpadZopelessLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.testing import (
     login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.translations.browser.productseries import ProductSeriesView
 from lp.translations.browser.serieslanguage import ProductSeriesLanguageView
+from lp.translations.interfaces.translations import (
+    TranslationsBranchImportMode,
+    )
 
 
 class TestProductSeriesView(TestCaseWithFactory):
     """Test ProductSeries view in translations facet."""
 
-    layer = LaunchpadZopelessLayer
+    layer = ZopelessDatabaseLayer
 
     def setUp(self):
         # Create a productseries that uses translations.
@@ -147,10 +156,98 @@
         self.assertEquals([], self._getProductserieslanguages(view))
 
 
+class TestProductSeriesViewBzrUsage(TestCaseWithFactory):
+    """Test ProductSeries view in translations facet."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        # Create a productseries that uses translations.
+        # Strip off the security proxy to allow customization.
+        super(TestProductSeriesViewBzrUsage, self).setUp()
+        self.secured_productseries = self.factory.makeProductSeries()
+        self.productseries = removeSecurityProxy(self.secured_productseries)
+
+    def _createView(self):
+        # The view operates on the secured product series!
+        view = ProductSeriesView(
+            self.secured_productseries, LaunchpadTestRequest())
+        view.initialize()
+        return view
+
+    def test_has_imports_enabled_no_branch(self):
+        view = self._createView()
+        self.assertFalse(view.has_imports_enabled)
+
+    def test_has_exports_enabled_no_branch(self):
+        view = self._createView()
+        self.assertFalse(view.has_exports_enabled)
+
+    def test_has_imports_enabled_with_branch_imports_disabled(self):
+        self.productseries.branch = self.factory.makeBranch()
+        self.productseries.translations_autoimport_mode = (
+                TranslationsBranchImportMode.NO_IMPORT)
+        view = self._createView()
+        self.assertFalse(view.has_imports_enabled)
+
+    def test_has_imports_enabled_with_branch_template_imports_enabled(self):
+        self.productseries.branch = self.factory.makeBranch()
+        self.productseries.translations_autoimport_mode = (
+            TranslationsBranchImportMode.IMPORT_TEMPLATES)
+        view = self._createView()
+        self.assertTrue(view.has_imports_enabled)
+
+    def test_has_imports_enabled_with_branch_trans_imports_enabled(self):
+        self.productseries.branch = self.factory.makeBranch()
+        self.productseries.translations_autoimport_mode = (
+            TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
+        view = self._createView()
+        self.assertTrue(view.has_imports_enabled)
+
+    def test_has_imports_enabled_private_branch_non_privileged(self):
+        # Private branches are hidden from non-privileged users. The view
+        # pretends that it is not used for imports.
+        self.productseries.branch = self.factory.makeBranch(private=True)
+        self.productseries.translations_autoimport_mode = (
+            TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
+        view = self._createView()
+        self.assertFalse(view.has_imports_enabled)
+
+    def test_has_imports_enabled_private_branch_privileged(self):
+        # Private branches are visible for privileged users.
+        self.productseries.branch = self.factory.makeBranch(private=True)
+        self.productseries.translations_autoimport_mode = (
+            TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
+        with person_logged_in(self.productseries.branch.owner):
+            view = self._createView()
+            self.assertTrue(view.has_imports_enabled)
+
+    def test_has_exports_enabled_with_branch(self):
+        self.productseries.translations_branch = self.factory.makeBranch()
+        view = self._createView()
+        self.assertTrue(view.has_exports_enabled)
+
+    def test_has_exports_enabled_private_branch_non_privileged(self):
+        # Private branches are hidden from non-privileged users. The view
+        # pretends that it is not used for exports.
+        self.productseries.translations_branch = self.factory.makeBranch(
+            private=True)
+        view = self._createView()
+        self.assertFalse(view.has_exports_enabled)
+
+    def test_has_exports_enabled_private_branch_privileged(self):
+        # Private branches are visible for privileged users.
+        self.productseries.translations_branch = self.factory.makeBranch(
+            private=True)
+        with person_logged_in(self.productseries.translations_branch.owner):
+            view = self._createView()
+            self.assertTrue(view.has_exports_enabled)
+
+
 class TestProductSeriesLanguageView(TestCaseWithFactory):
     """Test ProductSeriesLanguage view."""
 
-    layer = LaunchpadZopelessLayer
+    layer = ZopelessDatabaseLayer
 
     def setUp(self):
         # Create a productseries that uses translations.

=== modified file 'lib/lp/translations/stories/productseries/xx-productseries-translations.txt'
--- lib/lp/translations/stories/productseries/xx-productseries-translations.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/translations/stories/productseries/xx-productseries-translations.txt	2010-10-28 15:23:45 +0000
@@ -164,7 +164,8 @@
     >>> print extract_text(
     ...     find_tag_by_id(
     ...         owner_browser.contents, 'translations-explanation'))
-    Launchpad allows communities to translate projects using imports or a branch.
+    Launchpad allows communities to translate projects using
+    imports or a branch.
     Getting started with translating your project in Launchpad
     Configure translations
 
@@ -186,7 +187,8 @@
     >>> print extract_text(
     ...     find_tag_by_id(
     ...         admin_browser.contents, 'translations-explanation'))
-    Launchpad allows communities to translate projects using imports or a branch.
+    Launchpad allows communities to translate projects using
+    imports or a branch.
     Getting started with translating your project in Launchpad
     Configure translations
 
@@ -229,14 +231,23 @@
 Branch synchronization options
 ------------------------------
 
-If automatic import and export are set, but import branch is unset, we only
-indicate that export is happening.
+When no imports or exports have been set up, the page indicates that
+
+    >>> browser.open(frobnicator_trunk_url)
+    >>> sync_settings = first_tag_by_class(
+    ...     browser.contents, 'automatic-synchronization')
+    >>> print extract_text(sync_settings)
+    Automatic synchronization
+    This project is currently not using any synchronization
+    with bazaar branches.
+
+If a translation branch is set we indicate that exports are happening.
+Imports are not mentioned until a series branch has been set.
 
     >>> login('foo.bar@xxxxxxxxxxxxx')
     >>> from lp.translations.interfaces.translations import (
     ...     TranslationsBranchImportMode)
     >>> branch = factory.makeBranch(product=frobnicator)
-    >>> branch_name = branch.name
     >>> frobnicator_trunk.branch = None
     >>> frobnicator_trunk.translations_autoimport_mode = (
     ...     TranslationsBranchImportMode.IMPORT_TEMPLATES)
@@ -244,10 +255,42 @@
     >>> logout()
 
     >>> browser.open(frobnicator_trunk_url)
-    >>> print extract_text(browser.contents)
-    Translations...
+    >>> sync_settings = first_tag_by_class(
+    ...     browser.contents, 'automatic-synchronization')
+    >>> print extract_text(sync_settings)
+    Automatic synchronization
     Translations are exported daily to branch
-    ...
+    lp://dev/~person-name.../frobnicator/branch....
+
+If the branch is private, though the page pretends to non-privileged users
+that no synchronization has been set up.
+
+    >>> login('foo.bar@xxxxxxxxxxxxx')
+    >>> private_branch = factory.makeBranch(product=frobnicator, private=True)
+    >>> frobnicator_trunk.translations_branch = private_branch
+    >>> logout()
+
+    >>> browser.open(frobnicator_trunk_url)
+    >>> sync_settings = first_tag_by_class(
+    ...     browser.contents, 'automatic-synchronization')
+    >>> print extract_text(sync_settings)
+    Automatic synchronization
+    This project is currently not using any synchronization
+    with bazaar branches.
+
+Imports are indicated in likewise manner once a series branch has been set.
+
+    >>> login('foo.bar@xxxxxxxxxxxxx')
+    >>> frobnicator_trunk.branch = branch
+    >>> logout()
+
+    >>> browser.open(frobnicator_trunk_url)
+    >>> sync_settings = first_tag_by_class(
+    ...     browser.contents, 'automatic-synchronization')
+    >>> print extract_text(sync_settings)
+    Automatic synchronization
+    Translations are imported with every update from branch
+    lp://dev/frobnicator.
 
 
 Translation focus

=== modified file 'lib/lp/translations/templates/productseries-translations.pt'
--- lib/lp/translations/templates/productseries-translations.pt	2010-09-24 15:30:10 +0000
+++ lib/lp/translations/templates/productseries-translations.pt	2010-10-28 15:23:45 +0000
@@ -98,7 +98,7 @@
       </div>
       <div class="yui-g">
         <div class="yui-u first">
-          <div class="portlet">
+          <div class="portlet automatic-synchronization">
             <h3>Automatic synchronization</h3>
             <tal:uses-bzr-sync condition="view/uses_bzr_sync">
               <tal:imports condition="view/has_imports_enabled">