Merge lp:~wallyworld/launchpad/view-private-branch-artifacts-605130 into lp:launchpad

[Ian Booth]

[r=sinzui] Grant LimitedView to private team branch subscribers.

Resubmitting to change the prerequisite branch (the prerequisite branch failed qa and needed to be fixed so a new mp has been created for that). Once the new prerequisite branch is approved, this mp will be the one that is landed so both branches are landed together.

== 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

[Curtis Hovey (sinzui)]

This implementation looks good. I think these can be linted and commented, then submitted for a real review. I think this implementation can be applied to the team p3a subscriber case -- that would prove that this implementation is robust.

[Curtis Hovey (sinzui)]

I think this implementation is good, as are the tests. I suspect a test is missing. As with my review of lp:~wallyworld/launchpad/private-owned-entity-traversal I am not certain the page will render if the private team has an icon. Thus sort of test is easy to add here.

Maybe we want just one test of how tales handles the rendering of a private team with a logo, and and icon when the user has launchpad.LimitedView.

review: Needs Information (code)

[Curtis Hovey (sinzui)]

We spoke about this and agreed we need only one test is needed to verify our tales code handles limitedview of a private team. We expect this to fail because the link formatter wants access to the icon (which is absent from the existing tests of private teams being rendered) and the IPrimaryContext tales adapter to render the logo. We want allow users with limitedview to see name, displayname, icon, and logo.

review: Approve (code)

[Curtis Hovey (sinzui)]

I moved this back to needs review because we need to check the interaction with IPersonPublic.

[Curtis Hovey (sinzui)]

This branch is okay to land when the pre-req branch has dealt with the IPersonPublic ITeamPublic permission issue.

[Curtis Hovey (sinzui)]

I am landing this via ec2 land so that It could be in build bot when you wake.

review: Approve (code)

=== 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-12 05:44:12 +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):

=== 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-12 05:44:12 +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-12-08 21:15:07 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2011-12-12 05:44:12 +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,6 +28,7 @@
     extract_text,
     find_tag_by_id,
     setupBrowser,
+    setupBrowserForUser
     )
 from lp.app.interfaces.headings import IRootContext
 from lp.bugs.interfaces.bugtask import (
@@ -50,6 +53,7 @@
     BranchVisibilityRule,
     )
 from lp.code.interfaces.branchtarget import IBranchTarget
+from lp.registry.interfaces.person import PersonVisibility
 from lp.testing import (
     BrowserTestCase,
     login,
@@ -541,6 +545,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."""