launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/view-private-branch-artifacts-605130 into lp:launchpad with lp:~wallyworld/launchpad/private-owned-entity-traversal as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #605130 in Launchpad itself: "subscribed team cannot view  branch owned by (a different) private team"
  https://bugs.launchpad.net/launchpad/+bug/605130

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/view-private-branch-artifacts-605130/+merge/84060

== Implementation ==

Use the same technique as for bugs - precache the launchpad.LimitedView permission for the branch owner and subscribers.

The previous version of this branch also solved the traversal issue which prevented branches owned by private teams from being traversed if the user could not see the private team. This work has been moved to an upstream branch.

== Test ==

Add new test case TestBranchViewPrivateArtifacts:
- test_view_branch_with_private_owner
- test_view_private_branch_with_private_owner
- test_anonymous_view_branch_with_private_owner
- test_view_branch_with_private_subscriber
- test_anonymous_view_branch_with_private_subscriber

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/branchsubscription.py
  lib/lp/code/browser/tests/test_branch.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/view-private-branch-artifacts-605130/+merge/84060
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/view-private-branch-artifacts-605130 into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2011-09-28 11:30:52 +0000
+++ lib/lp/code/browser/branch.py	2011-12-01 11:37:34 +0000
@@ -96,7 +96,10 @@
     stepthrough,
     stepto,
     )
-from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.authorization import (
+    check_permission,
+    precache_permission_for_objects,
+    )
 from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
 from canonical.launchpad.webapp.menu import structured
 from lp.app.browser.launchpad import Hierarchy
@@ -442,6 +445,11 @@
     def initialize(self):
         self.branch = self.context
         self.notices = []
+        # Cache permission so private team owner can be rendered.
+        authorised_people = [self.branch.owner]
+        if self.user is not None:
+            precache_permission_for_objects(
+                self.request, "launchpad.LimitedView", authorised_people)
         # Replace our context with a decorated branch, if it is not already
         # decorated.
         if not isinstance(self.context, DecoratedBranch):
@@ -532,7 +540,15 @@
     @cachedproperty
     def landing_targets(self):
         """Return a filtered list of landing targets."""
-        return latest_proposals_for_each_branch(self.context.landing_targets)
+        proposals = latest_proposals_for_each_branch(
+            self.context.landing_targets)
+        if self.user is not None:
+            reviewers = [
+            proposal.reviewer for proposal in proposals
+            if proposal.reviewer]
+            precache_permission_for_objects(
+                self.request, "launchpad.LimitedView", reviewers)
+        return proposals
 
     @property
     def latest_landing_candidates(self):

=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py	2011-09-19 06:57:55 +0000
+++ lib/lp/code/browser/branchsubscription.py	2011-12-01 11:37:34 +0000
@@ -19,7 +19,10 @@
     canonical_url,
     LaunchpadView,
     )
-from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.authorization import (
+    check_permission,
+    precache_permission_for_objects,
+    )
 from canonical.launchpad.webapp.interfaces import IPrimaryContext
 from canonical.launchpad.webapp.menu import structured
 from lp.app.browser.launchpadform import (
@@ -48,9 +51,18 @@
 
     def subscriptions(self):
         """Return a decorated list of branch subscriptions."""
+
+        # Cache permissions so private subscribers can be rendered.
+        if self.user is not None:
+            subscribers = [
+                subscription.person
+                for subscription in self.context.subscriptions]
+            precache_permission_for_objects(
+                self.request, "launchpad.LimitedView", subscribers)
+
         visible_subscriptions = [
             subscription for subscription in self.context.subscriptions
-            if check_permission('launchpad.View', subscription.person)]
+            if check_permission('launchpad.LimitedView', subscription.person)]
         return sorted(
             visible_subscriptions,
             key=lambda subscription: subscription.person.displayname)

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2011-10-21 02:47:28 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2011-12-01 11:37:34 +0000
@@ -5,12 +5,14 @@
 
 __metaclass__ = type
 
+from BeautifulSoup import BeautifulSoup
 from datetime import (
     datetime,
     )
 from textwrap import dedent
 
 import pytz
+from zope.publisher.interfaces import NotFound
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
@@ -26,7 +28,7 @@
     extract_text,
     find_tag_by_id,
     setupBrowser,
-    )
+    setupBrowserForUser)
 from lp.app.interfaces.headings import IRootContext
 from lp.bugs.interfaces.bugtask import (
     BugTaskStatus,
@@ -50,6 +52,7 @@
     BranchVisibilityRule,
     )
 from lp.code.interfaces.branchtarget import IBranchTarget
+from lp.registry.interfaces.person import PersonVisibility
 from lp.testing import (
     BrowserTestCase,
     login,
@@ -543,6 +546,102 @@
         self.assertEqual(linked_bug_urls[1], links[4]['href'])
 
 
+class TestBranchViewPrivateArtifacts(BrowserTestCase):
+    """ Tests that branches with private team artifacts can be viewed.
+
+    A Branch may be associated with a private team as follows:
+    - the owner is a private team
+    - a subscriber is a private team
+
+    A logged in user who is not authorised to see the private team(s) still
+    needs to be able to view the branch. The private team will be rendered in
+    the normal way, displaying the team name and Launchpad URL.
+    """
+
+    layer = DatabaseFunctionalLayer
+
+    def _getBrowser(self, user=None):
+        if user is None:
+            browser = setupBrowser()
+            logout()
+            return browser
+        else:
+            login_person(user)
+            return setupBrowserForUser(user=user)
+
+    def test_view_branch_with_private_owner(self):
+        # A branch with a private owner is rendered.
+        private_owner = self.factory.makeTeam(
+            displayname="PrivateTeam", visibility=PersonVisibility.PRIVATE)
+        branch = self.factory.makeAnyBranch(owner=private_owner)
+        # Ensure the branch owner is rendered.
+        url = canonical_url(branch, rootsite='code')
+        user = self.factory.makePerson()
+        browser = self._getBrowser(user)
+        browser.open(url)
+        soup = BeautifulSoup(browser.contents)
+        self.assertIsNotNone(soup.find('a', text="PrivateTeam"))
+
+    def test_view_private_branch_with_private_owner(self):
+        # A private branch with a private owner is rendered.
+        private_owner = self.factory.makeTeam(
+            displayname="PrivateTeam", visibility=PersonVisibility.PRIVATE)
+        branch = self.factory.makeAnyBranch(owner=private_owner)
+        # Ensure the branch owner is rendered.
+        url = canonical_url(branch, rootsite='code')
+        user = self.factory.makePerson()
+        # Subscribe the user so they can see the branch.
+        with person_logged_in(private_owner):
+            self.factory.makeBranchSubscription(branch, user, private_owner)
+        browser = self._getBrowser(user)
+        browser.open(url)
+        soup = BeautifulSoup(browser.contents)
+        self.assertIsNotNone(soup.find('a', text="PrivateTeam"))
+
+    def test_anonymous_view_branch_with_private_owner(self):
+        # A branch with a private owner is not rendered for anon users.
+        private_owner = self.factory.makeTeam(
+            visibility=PersonVisibility.PRIVATE)
+        branch = self.factory.makeAnyBranch(owner=private_owner)
+        # Viewing the branch results in an error.
+        url = canonical_url(branch, rootsite='code')
+        browser = self._getBrowser()
+        self.assertRaises(NotFound, browser.open, url)
+
+    def test_view_branch_with_private_subscriber(self):
+        # A branch with a private subscriber is rendered.
+        private_subscriber = self.factory.makeTeam(
+            name="privateteam", visibility=PersonVisibility.PRIVATE)
+        branch = self.factory.makeAnyBranch()
+        with person_logged_in(branch.owner):
+            self.factory.makeBranchSubscription(
+                branch, private_subscriber, branch.owner)
+        # Ensure the branch subscriber is rendered.
+        url = canonical_url(branch, rootsite='code')
+        user = self.factory.makePerson()
+        browser = self._getBrowser(user)
+        browser.open(url)
+        soup = BeautifulSoup(browser.contents)
+        self.assertIsNotNone(
+            soup.find('div', attrs={'id': 'subscriber-privateteam'}))
+
+    def test_anonymous_view_branch_with_private_subscriber(self):
+        # A branch with a private subscriber is not rendered for anon users.
+        private_subscriber = self.factory.makeTeam(
+            name="privateteam", visibility=PersonVisibility.PRIVATE)
+        branch = self.factory.makeAnyBranch()
+        with person_logged_in(branch.owner):
+            self.factory.makeBranchSubscription(
+                branch, private_subscriber, branch.owner)
+        # Viewing the branch results in an error.
+        url = canonical_url(branch, rootsite='code')
+        browser = self._getBrowser()
+        browser.open(url)
+        soup = BeautifulSoup(browser.contents)
+        self.assertIsNone(
+            soup.find('div', attrs={'id': 'subscriber-privateteam'}))
+
+
 class TestBranchAddView(TestCaseWithFactory):
     """Test the BranchAddView view."""