Merge lp:~sinzui/launchpad/private-traversal-4 into lp:launchpad

[Curtis Hovey]

[r=stevenk,wallyworld][bug=605130,898669,903737] Allows subscribers to traverse to archives owned by private teams.

Allows subscribers to traverse to archives owned by private teams.

    Launchpad bug: https://bugs.launchpad.net/bugs/903737
    Pre-implementation: flacoste

The branch that fixed the security checker rules for private teams with
subscribable artefacts was rolled back because QA found traversal errors.
The test suite should not have passed. This issue was reported 12 months
ago, but the fix that was landed did not include a test for this case.

Module lp.registry.browser.person, line 447, in traverse_archive
return traverse_named_ppa(self.context.name, ppa_name)
Module lp.soyuz.browser.archive, line 196, in traverse_named_ppa
archive = person.getPPAByName(ppa_name)
Unauthorized: (<Person at ... canonical-isd (Canonical ISD)>,
               'getPPAByName', 'launchpad.View')<br />

--------------------------------------------------------------------

RULES

    * Write a test that reproduces the error seen on qastaging:
    * Update the interfaces or permissions as needed to make the
      test pass.
    * Consider previewing the branch on a server with production
      data to ensure subscribers can access private team archives.

QA

    * As an Lp engineer, visit
      https://qastaging.launchpad.net/~canonical-isd/+archive/enterprise-apps
    * Verify the page renders.
    * Verify you can expand the packages to learn the details.

LINT

    lib/canonical/launchpad/interfaces/_schema_circular_imports.py
    lib/lp/registry/interfaces/person.py
    lib/lp/soyuz/tests/test_archive_subscriptions.py

TEST

    ./bin/test -vvc lp.soyuz.tests.test_archive_subscriptions

IMPLEMENTATION

I tried the test_traverse helper, but discovered it fails because it does
not work with LaunchpadTestRequest, (the request object did not support
.features which is required Lp's base template).
I Wrote a browser test that uses traversal and supports proper view rendering.
I then moved getPPAByName() to IPersonLimitedView.
    lib/canonical/launchpad/interfaces/_schema_circular_imports.py
    lib/lp/registry/interfaces/person.py
    lib/lp/soyuz/tests/test_archive_subscriptions.py

[Curtis Hovey (sinzui)]

Hi Steve.

Can we put this branch on dogfood or staging so that we can see how this behaves with production data?

[Steve Kowalik (stevenk)]

Broadly, this looks good to me. The only comment I have is, have you thought about using with person_logged_in()?

review: Approve (code)

[Curtis Hovey (sinzui)]

I can certainly switch to person_logged_in()

[Ian Booth (wallyworld)]

Looks good. Thanks for fixing.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2011-12-13 13:53:12 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2011-12-14 03:28:31 +0000
@@ -135,7 +135,8 @@
     )
 from lp.registry.interfaces.person import (
     IPerson,
-    IPersonPublic,
+    IPersonLimitedView,
+    IPersonViewRestricted,
     ITeam,
     )
 from lp.registry.interfaces.pillar import (
@@ -310,10 +311,10 @@
 
 IPreviewDiff['branch_merge_proposal'].schema = IBranchMergeProposal
 
-patch_reference_property(IPersonPublic, 'archive', IArchive)
-patch_collection_property(IPersonPublic, 'ppas', IArchive)
-patch_entry_return_type(IPersonPublic, 'getPPAByName', IArchive)
-patch_entry_return_type(IPersonPublic, 'createPPA', IArchive)
+patch_reference_property(IPersonViewRestricted, 'archive', IArchive)
+patch_collection_property(IPersonViewRestricted, 'ppas', IArchive)
+patch_entry_return_type(IPersonLimitedView, 'getPPAByName', IArchive)
+patch_entry_return_type(IPersonViewRestricted, 'createPPA', IArchive)
 
 IHasBuildRecords['getBuildRecords'].queryTaggedValue(
     LAZR_WEBSERVICE_EXPORTED)[

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-12-13 13:53:12 +0000
+++ lib/canonical/launchpad/security.py	2011-12-14 03:28:31 +0000
@@ -63,6 +63,7 @@
     IBranch,
     user_has_special_branch_access,
     )
+from lp.code.interfaces.branchcollection import IBranchCollection
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.branchmergequeue import IBranchMergeQueue
 from lp.code.interfaces.codeimport import ICodeImport
@@ -800,6 +801,35 @@
             if (invitee.is_team and
                 invitee in user.person.getAdministratedTeams()):
                 return True
+        return False
+
+
+class PublicOrPrivateTeamsExistence(AuthorizationBase):
+    """Restrict knowing about private teams' existence.
+
+    Knowing the existence of a private team allow traversing to its URL and
+    displaying basic information like name, displayname.
+    """
+    permission = 'launchpad.LimitedView'
+    usedfor = IPersonLimitedView
+
+    def checkUnauthenticated(self):
+        """Unauthenticated users can only view public teams."""
+        if self.obj.visibility == PersonVisibility.PUBLIC:
+            return True
+        return False
+
+    def checkAuthenticated(self, user):
+        """By default, we simply perform a View permission check.
+
+        We also grant limited viewability to users who can see PPAs and
+        branches owned by the team. In other scenarios, the context in which
+        the permission is required is responsible for pre-caching the
+        launchpad.LimitedView permission on each team which requires it.
+        """
+        if self.forwardCheckAuthenticated(
+            user, self.obj, 'launchpad.View'):
+            return True
 
         if (self.obj.is_team
             and self.obj.visibility == PersonVisibility.PRIVATE):
@@ -813,33 +843,14 @@
                 ppa.id for ppa in self.obj.ppas if ppa.private)
             if len(subscriber_archive_ids.intersection(team_ppa_ids)) > 0:
                 return True
-        return False
-
-
-class PublicOrPrivateTeamsExistence(AuthorizationBase):
-    """Restrict knowing about private teams' existence.
-
-    Knowing the existence of a private team allow traversing to its URL and
-    displaying basic information like name, displayname.
-    """
-    permission = 'launchpad.LimitedView'
-    usedfor = IPersonLimitedView
-
-    def checkUnauthenticated(self):
-        """Unauthenticated users can only view public teams."""
-        if self.obj.visibility == PersonVisibility.PUBLIC:
-            return True
-        return False
-
-    def checkAuthenticated(self, user):
-        """By default, we simply perform a View permission check.
-
-        The context in which the permission is required is
-        responsible for pre-caching the launchpad.LimitedView permission on
-        each team which requires it.
-        """
-        return self.forwardCheckAuthenticated(
-            user, self.obj, 'launchpad.View')
+
+            # Grant visibility to people with subscriptions to branches owned
+            # by the private team.
+            owned_branches = getUtility(IBranchCollection).ownedBy(self.obj)
+            if owned_branches.visibleByUser(user.person).count() > 0:
+                return True
+
+        return False
 
 
 class EditPollByTeamOwnerOrTeamAdminsOrAdmins(

=== modified file 'lib/lp/app/browser/launchpad.py'
--- lib/lp/app/browser/launchpad.py	2011-12-13 13:53:12 +0000
+++ lib/lp/app/browser/launchpad.py	2011-12-14 03:28:31 +0000
@@ -724,8 +724,8 @@
                 # Check to see if this is a team, and if so, whether the
                 # logged in user is allowed to view the team, by virtue of
                 # team membership or Launchpad administration.
-                if (person.is_team
-                    and not check_permission('launchpad.View', person)):
+                if (person.is_team and
+                    not check_permission('launchpad.LimitedView', person)):
                     raise NotFound(self.context, name)
                 # Only admins are permitted to see suspended users.
                 if person.account_status == AccountStatus.SUSPENDED:

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2011-12-13 19:38:29 +0000
+++ lib/lp/app/browser/tales.py	2011-12-14 03:28:31 +0000
@@ -1266,6 +1266,14 @@
                 self.hidden)
         return super(TeamFormatterAPI, self).link(view_name, rootsite)
 
+    def icon(self, view_name):
+        team = self._context
+        if not check_permission('launchpad.LimitedView', team):
+            css_class = ObjectImageDisplayAPI(team).sprite_css()
+            return '<span class="' + css_class + '"></span>'
+        else:
+            return super(TeamFormatterAPI, self).icon(view_name)
+
     def displayname(self, view_name, rootsite=None):
         """See `PersonFormatterAPI`."""
         person = self._context

=== modified file 'lib/lp/app/tests/test_tales.py'
--- lib/lp/app/tests/test_tales.py	2011-12-13 13:53:12 +0000
+++ lib/lp/app/tests/test_tales.py	2011-12-14 03:28:31 +0000
@@ -149,18 +149,20 @@
         self.assertEqual(expected, result)
 
 
-class TestTalesFormatterAPI(TestCaseWithFactory):
-    """ Test permissions required to access TalesFormatterAPI methods.
+class TestTeamFormatterAPI(TestCaseWithFactory):
+    """ Test permissions required to access TeamFormatterAPI methods.
 
     A user must have launchpad.LimitedView permission to use
-    TestTalesFormatterAPI with private teams.
+    TeamFormatterAPI with private teams.
     """
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
-        super(TestTalesFormatterAPI, self).setUp()
+        super(TestTeamFormatterAPI, self).setUp()
+        icon = self.factory.makeLibraryFileAlias(
+            filename='smurf.png', content_type='image/png')
         self.team = self.factory.makeTeam(
-            name='team', displayname='a team',
+            name='team', displayname='a team', icon=icon,
             visibility=PersonVisibility.PRIVATE)
 
     def _make_formatter(self, cache_permission=False):
@@ -175,10 +177,11 @@
                 request, 'launchpad.LimitedView', [self.team])
         return formatter, request, any_person
 
-    def _tales_value(self, attr, request):
+    def _tales_value(self, attr, request, path='fmt'):
         # Evaluate the given formatted attribute value on team.
-        return test_tales(
-            "team/fmt:%s" % attr, team=self.team, request=request)
+        result = test_tales(
+            "team/%s:%s" % (path, attr), team=self.team, request=request)
+        return result
 
     def _test_can_view_attribute_no_login(self, attr, hidden=None):
         # Test attribute access with no login.
@@ -228,6 +231,10 @@
     def test_can_view_url(self):
         self._test_can_view_attribute('url')
 
+    def test_can_view_icon(self):
+        self._test_can_view_attribute(
+            'icon', '<span class="sprite team"></span>')
+
 
 class TestObjectFormatterAPI(TestCaseWithFactory):
     """Tests for ObjectFormatterAPI"""

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-12-13 13:53:12 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-12-14 03:28:31 +0000
@@ -629,7 +629,7 @@
         direct_subscriber = self.factory.makeTeam(
             name='team', displayname='Team Name', owner=teamowner,
             visibility=PersonVisibility.PRIVATE)
-        with person_logged_in(direct_subscriber.teamowner):
+        with person_logged_in(teamowner):
             bug.subscribe(direct_subscriber, direct_subscriber.teamowner,
                           level=BugNotificationLevel.LIFECYCLE)
 

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2011-12-13 13:53:12 +0000
+++ lib/lp/code/browser/branch.py	2011-12-14 03:28:31 +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-12-13 13:53:12 +0000
+++ lib/lp/code/browser/branchsubscription.py	2011-12-14 03:28:31 +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-13 13:53:12 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2011-12-14 03:28:31 +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."""
 

=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2011-12-13 13:53:12 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2011-12-14 03:28:31 +0000
@@ -613,13 +613,14 @@
     layer = DatabaseFunctionalLayer
 
     def _make_branch_for_private_team(self):
+        owner = self.factory.makePerson()
         private_team = self.factory.makeTeam(
-            name='shh', displayname='Shh',
+            name='shh', displayname='Shh', owner=owner,
             visibility=PersonVisibility.PRIVATE)
         member = self.factory.makePerson(
             email='[e-mail]', password='test')
-        with person_logged_in(private_team.teamowner):
-            private_team.addMember(member, private_team.teamowner)
+        with person_logged_in(owner):
+            private_team.addMember(member, owner)
         branch = self.factory.makeProductBranch(owner=private_team)
         return private_team, member, branch
 

=== modified file 'lib/lp/code/stories/branches/xx-subscribing-branches.txt'
--- lib/lp/code/stories/branches/xx-subscribing-branches.txt	2011-12-13 13:53:12 +0000
+++ lib/lp/code/stories/branches/xx-subscribing-branches.txt	2011-12-14 03:28:31 +0000
@@ -254,15 +254,19 @@
     Mark Shuttleworth
 
 
-Private team exclusion
-======================
-
-If a private team is subscribed to the branch, it is excluded from the list of
-subscribers if the user is not an admin or a member of the team.
-
-    >>> from lp.testing import ANONYMOUS, login, logout
+Private team's in public subscriptions
+======================================
+
+If a private team is subscribed to a publicthe branch, it is visible
+to everyone.
+
+    >>> from lp.testing import login, logout
+    >>> from lp.registry.interfaces.person import PersonVisibility
+    >>> from lp.code.enums import (
+    ...     BranchSubscriptionNotificationLevel,
+    ...     CodeReviewNotificationLevel)
+
     >>> login('[e-mail]')
-    >>> from lp.registry.interfaces.person import PersonVisibility
     >>> private_team = factory.makeTeam(
     ...     name='shh', displayname='Shh',
     ...     visibility=PersonVisibility.PRIVATE)
@@ -271,26 +275,16 @@
     >>> ignored = private_team.addMember(member, private_team.teamowner)
     >>> owner = factory.makePerson(name='branch-owner')
     >>> branch = factory.makeAnyBranch(owner=owner)
-    >>> from lp.code.enums import (
-    ...     BranchSubscriptionNotificationLevel,
-    ...     CodeReviewNotificationLevel)
     >>> ignored = branch.subscribe(
     ...     private_team, BranchSubscriptionNotificationLevel.NOEMAIL, None,
     ...     CodeReviewNotificationLevel.NOEMAIL, private_team.teamowner)
     >>> url = canonical_url(branch)
     >>> logout()
 
-No-priv is not a member of the private team, so the private team is not shown
-as a subscriber.
+No-priv is not a member of the private team, but he can see the team's
+display name in the subscriber list.
 
     >>> browser.open(url)
     >>> print_subscribers(browser.contents)
     Branch-owner
-
-A team member looking at the branch will see the private team subscribed.
-
-    >>> private_browser = setupBrowser(auth='Basic [e-mail]:test')
-    >>> private_browser.open(url)
-    >>> print_subscribers(private_browser.contents)
-    Branch-owner
     Shh

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2011-12-13 13:53:12 +0000
+++ lib/lp/registry/configure.zcml	2011-12-14 03:28:31 +0000
@@ -912,8 +912,6 @@
             <allow
                 interface="lp.registry.interfaces.person.IPersonPublic"/>
             <allow
-                interface="lp.registry.interfaces.person.ITeamPublic"/>
-            <allow
                 interface="lp.registry.interfaces.person.IHasStanding"/>
             <allow
                 interface="lp.registry.interfaces.person.IPersonCommAdminWriteRestricted"/>
@@ -924,6 +922,9 @@
                 permission="launchpad.View"
                 interface="lp.registry.interfaces.person.IPersonViewRestricted"/>
             <require
+                permission="launchpad.View"
+                interface="lp.registry.interfaces.person.ITeamPublic"/>
+            <require
                 permission="launchpad.Edit"
                 interface="lp.registry.interfaces.location.ISetLocation"/>
             <require
@@ -931,9 +932,10 @@
                 interface="lp.registry.interfaces.person.IPersonEditRestricted"/>
             <require
                 permission="launchpad.Edit"
-                set_schema="lp.registry.interfaces.person.IPersonPublic
+                set_schema="lp.registry.interfaces.person.IPersonViewRestricted
+                            lp.registry.interfaces.person.IPersonPublic
                             lp.registry.interfaces.person.ITeamPublic"
-                set_attributes="displayname"/>
+                set_attributes="displayname icon logo"/>
             <require
                 permission="launchpad.Moderate"
                 set_attributes="name"/>

=== modified file 'lib/lp/registry/doc/private-team-visibility.txt'
--- lib/lp/registry/doc/private-team-visibility.txt	2011-12-13 13:53:12 +0000
+++ lib/lp/registry/doc/private-team-visibility.txt	2011-12-14 03:28:31 +0000
@@ -107,3 +107,64 @@
     ...
     Unauthorized: (<Person at ... priv-team (Priv Team)>,
         'name', 'launchpad.LimitedView')
+
+A person with visibility to any of the branches owned by the private team will
+be granted limited view permission on the team.
+
+For private branches, a user needs to be subscribed to the branch for the
+branch (and hence team) to be visible.
+
+    >>> login_person(priv_owner)
+    >>> private_team_branch = factory.makeBranch(
+    ...     owner=priv_team, private=True)
+    >>> login_person(pub_member)
+    >>> check_permission('launchpad.LimitedView', priv_team)
+    False
+
+    >>> login_person(priv_owner)
+    >>> sub = factory.makeBranchSubscription(
+    ...     branch=private_team_branch, person=pub_member,
+    ...     subscribed_by=priv_owner)
+
+    >>> login_person(pub_member)
+    >>> check_permission('launchpad.LimitedView', priv_team)
+    True
+
+Subscribers to the teams private PPA have limited view permission.
+
+    >>> login_person(priv_owner)
+    >>> archive = factory.makeArchive(private=True, owner=priv_team)
+    >>> archive_subscriber = factory.makePerson()
+    >>> sub = archive.newSubscription(
+    ...     archive_subscriber, registrant=archive.owner)
+
+    >>> login_person(archive_subscriber)
+    >>> check_permission('launchpad.LimitedView', priv_team)
+    True
+
+Users with LimitedView can know identifying information like name,
+displayname, and unique_name, but cannot know other information like
+teamowner.
+
+    >>> print priv_team.name
+    priv-team
+
+    >>> print priv_team.displayname
+    Priv Team
+
+    >>> print priv_team.unique_displayname
+    Priv Team (priv-team)
+
+    >>> print priv_team.icon
+    None
+
+    >>> print priv_team.allmembers
+    Traceback (most recent call last):
+    ...
+    Unauthorized: (<Person at ... priv-team (Priv Team)>,
+        'allmembers', 'launchpad.View')
+
+Anonymous users do not have permission.
+    >>> login(ANONYMOUS)
+    >>> check_permission('launchpad.LimitedView', priv_team)
+    False

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-12-13 13:53:12 +0000
+++ lib/lp/registry/interfaces/person.py	2011-12-14 03:28:31 +0000
@@ -15,7 +15,7 @@
     'IObjectReassignment',
     'IPerson',
     'IPersonClaim',
-    'IPersonPublic',  # Required for a monkey patch in interfaces/archive.py
+    'IPersonPublic',
     'IPersonSet',
     'IPersonSettings',
     'ISoftwareCenterAgentAPI',
@@ -657,26 +657,61 @@
         required=False, default=False)
 
 
-class IPersonPublic(IHasBranches, IHasSpecifications,
-                    IHasMergeProposals, IHasLogo, IHasMugshot, IHasIcon,
-                    IHasLocation, IHasRequestedReviews, IObjectWithLocation,
-                    IPrivacy, IHasBugs, IHasRecipes, IHasTranslationImports,
-                    IPersonSettings, IQuestionsPerson):
-    """Public attributes for a Person."""
+class IPersonPublic(IPrivacy):
+    """Public attributes for a Person.
+
+    Very few attributes on a person can be public because private teams
+    are also persons. The public attributes are generally information
+    needed by the system to determine if the principal in the current
+    interaction can work with the object.
+    """
 
     id = Int(title=_('ID'), required=True, readonly=True)
-    account = Object(schema=IAccount)
-    accountID = Int(title=_('Account ID'), required=True, readonly=True)
-    password = PasswordField(
-        title=_('Password'), required=True, readonly=False)
-    karma = exported(
-        Int(title=_('Karma'), readonly=True,
-            description=_('The cached total karma for this person.')))
-    homepage_content = exported(
-        Text(title=_("Homepage Content"), required=False,
-            description=_(
-                "The content of your profile page. Use plain text, "
-                "paragraphs are preserved and URLs are linked in pages.")))
+    # This is redefined from IPrivacy.private because the attribute is
+    # read-only. It is a summary of the team's visibility.
+    private = exported(Bool(
+            title=_("This team is private"),
+            readonly=True, required=False,
+            description=_("Private teams are visible only to "
+                          "their members.")))
+    is_valid_person = Bool(
+        title=_("This is an active user and not a team."), readonly=True)
+    is_valid_person_or_team = exported(
+        Bool(title=_("This is an active user or a team."), readonly=True),
+        exported_as='is_valid')
+    is_merge_pending = exported(Bool(
+        title=_("Is this person due to be merged with another?"),
+        required=False, default=False))
+    is_team = exported(
+        Bool(title=_('Is this object a team?'), readonly=True))
+    account_status = Choice(
+        title=_("The status of this person's account"), required=False,
+        readonly=True, vocabulary=AccountStatus)
+    account_status_comment = Text(
+        title=_("Why are you deactivating your account?"), required=False,
+        readonly=True)
+
+
+class IPersonLimitedView(IHasIcon, IHasLogo):
+    """IPerson attributes that require launchpad.LimitedView permission."""
+
+    name = exported(
+        PersonNameField(
+            title=_('Name'), required=True, readonly=False,
+            constraint=name_validator,
+            description=_(
+                "A short unique name, beginning with a lower-case "
+                "letter or number, and containing only letters, "
+                "numbers, dots, hyphens, or plus signs.")))
+    displayname = exported(
+        StrippedTextLine(
+            title=_('Display Name'), required=True, readonly=False,
+            description=_(
+                "Your name as you would like it displayed throughout "
+                "Launchpad. Most people use their full name here.")),
+        exported_as='display_name')
+    unique_displayname = TextLine(
+        title=_('Return a string of the form $displayname ($name).'))
     # NB at this stage we do not allow individual people to have their own
     # icon, only teams get that. People can however have a logo and mugshot
     # The icon is only used for teams; that's why we use /@@/team as the
@@ -690,7 +725,6 @@
             "displayed whenever the team name is listed - for example "
             "in listings of bugs or on a person's membership table."))
     iconID = Int(title=_('Icon ID'), required=True, readonly=True)
-
     logo = exported(
         LogoImageUpload(
             title=_("Logo"), required=False,
@@ -701,6 +735,44 @@
                 "is a logo, a small picture or a personal mascot. It should "
                 "be no bigger than 50kb in size.")))
     logoID = Int(title=_('Logo ID'), required=True, readonly=True)
+    # title is required for the Launchpad Page Layout main template
+    title = Attribute('Person Page Title')
+    is_probationary = exported(
+        Bool(title=_("Is this a probationary user?"), readonly=True))
+
+    @operation_parameters(
+        name=TextLine(required=True, constraint=name_validator))
+    @operation_returns_entry(Interface)  # Really IArchive.
+    @export_read_operation()
+    @operation_for_version("beta")
+    def getPPAByName(name):
+        """Return a PPA with the given name if it exists.
+
+        :param name: A string with the exact name of the ppa being looked up.
+        :raises: `NoSuchPPA` if a suitable PPA could not be found.
+
+        :return: a PPA `IArchive` record corresponding to the name.
+        """
+
+
+class IPersonViewRestricted(IHasBranches, IHasSpecifications,
+                    IHasMergeProposals, IHasMugshot,
+                    IHasLocation, IHasRequestedReviews, IObjectWithLocation,
+                    IHasBugs, IHasRecipes, IHasTranslationImports,
+                    IPersonSettings, IQuestionsPerson):
+    """IPerson attributes that require launchpad.View permission."""
+    account = Object(schema=IAccount)
+    accountID = Int(title=_('Account ID'), required=True, readonly=True)
+    password = PasswordField(
+        title=_('Password'), required=True, readonly=False)
+    karma = exported(
+        Int(title=_('Karma'), readonly=True,
+            description=_('The cached total karma for this person.')))
+    homepage_content = exported(
+        Text(title=_("Homepage Content"), required=False,
+            description=_(
+                "The content of your profile page. Use plain text, "
+                "paragraphs are preserved and URLs are linked in pages.")))
 
     mugshot = exported(MugshotImageUpload(
         title=_("Mugshot"), required=False,
@@ -755,26 +827,9 @@
                 readonly=False, required=False,
                 value_type=Reference(schema=ISSHKey)))
 
-    account_status = Choice(
-        title=_("The status of this person's account"), required=False,
-        readonly=True, vocabulary=AccountStatus)
-
-    account_status_comment = Text(
-        title=_("Why are you deactivating your account?"), required=False,
-        readonly=True)
-
     # Properties of the Person object.
     karma_category_caches = Attribute(
         'The caches of karma scores, by karma category.')
-    is_team = exported(
-        Bool(title=_('Is this object a team?'), readonly=True))
-    is_valid_person = Bool(
-        title=_("This is an active user and not a team."), readonly=True)
-    is_valid_person_or_team = exported(
-        Bool(title=_("This is an active user or a team."), readonly=True),
-        exported_as='is_valid')
-    is_probationary = exported(
-        Bool(title=_("Is this a probationary user?"), readonly=True))
     is_ubuntu_coc_signer = exported(
     Bool(title=_("Signed Ubuntu Code of Conduct"),
             readonly=True))
@@ -879,7 +934,6 @@
         exported_as='team_owner')
     teamownerID = Int(title=_("The Team Owner's ID or None"), required=False,
                       readonly=True)
-
     preferredemail = exported(
         Reference(title=_("Preferred email address"),
                description=_("The preferred email address for this person. "
@@ -916,9 +970,6 @@
             "this is set to None, then this Person has not been merged "
             "into another and is still valid"))
 
-    # title is required for the Launchpad Page Layout main template
-    title = Attribute('Person Page Title')
-
     archive = exported(
         Reference(
             title=_("Default PPA"),
@@ -987,18 +1038,6 @@
             readonly=True, required=False,
             value_type=Reference(schema=Interface)))  # HWSubmission
 
-    # This is redefined from IPrivacy.private because the attribute is
-    # read-only. It is a summary of the team's visibility.
-    private = exported(Bool(
-            title=_("This team is private"),
-            readonly=True, required=False,
-            description=_("Private teams are visible only to "
-                          "their members.")))
-
-    is_merge_pending = exported(Bool(
-        title=_("Is this person due to be merged with another?"),
-        required=False, default=False))
-
     administrated_teams = Attribute(
         u"the teams that this person/team is an administrator of.")
 
@@ -1414,20 +1453,6 @@
         """
 
     @operation_parameters(
-        name=TextLine(required=True, constraint=name_validator))
-    @operation_returns_entry(Interface)  # Really IArchive.
-    @export_read_operation()
-    @operation_for_version("beta")
-    def getPPAByName(name):
-        """Return a PPA with the given name if it exists.
-
-        :param name: A string with the exact name of the ppa being looked up.
-        :raises: `NoSuchPPA` if a suitable PPA could not be found.
-
-        :return: a PPA `IArchive` record corresponding to the name.
-        """
-
-    @operation_parameters(
         name=TextLine(required=True, constraint=name_validator),
         displayname=TextLine(required=False),
         description=TextLine(required=False),
@@ -1463,32 +1488,6 @@
         :return: a boolean.
         """
 
-
-class IPersonLimitedView(Interface):
-    """IPerson attributes that require launchpad.LimitedView permission."""
-
-    name = exported(
-        PersonNameField(
-            title=_('Name'), required=True, readonly=False,
-            constraint=name_validator,
-            description=_(
-                "A short unique name, beginning with a lower-case "
-                "letter or number, and containing only letters, "
-                "numbers, dots, hyphens, or plus signs.")))
-    displayname = exported(
-        StrippedTextLine(
-            title=_('Display Name'), required=True, readonly=False,
-            description=_(
-                "Your name as you would like it displayed throughout "
-                "Launchpad. Most people use their full name here.")),
-        exported_as='display_name')
-    unique_displayname = TextLine(
-        title=_('Return a string of the form $displayname ($name).'))
-
-
-class IPersonViewRestricted(Interface):
-    """IPerson attributes that require launchpad.View permission."""
-
     active_member_count = Attribute(
         "The number of real people who are members of this team.")
     # activemembers.value_type.schema will be set to IPerson once
@@ -2565,18 +2564,19 @@
     ]:
     IPersonViewRestricted[name].value_type.schema = IPerson
 
-IPersonPublic['sub_teams'].value_type.schema = ITeam
-IPersonPublic['super_teams'].value_type.schema = ITeam
+IPersonViewRestricted['sub_teams'].value_type.schema = ITeam
+IPersonViewRestricted['super_teams'].value_type.schema = ITeam
 # XXX: salgado, 2008-08-01: Uncomment these when teams_*participated_in are
 # exported again.
-# IPersonPublic['teams_participated_in'].value_type.schema = ITeam
-# IPersonPublic['teams_indirectly_participated_in'].value_type.schema = ITeam
+# IPersonViewRestricted['teams_participated_in'].value_type.schema = ITeam
+# IPersonViewRestricted[
+#   'teams_indirectly_participated_in'].value_type.schema = ITeam
 
 # Fix schema of operation parameters. We need zope.deferredimport!
 params_to_fix = [
     # XXX: salgado, 2008-08-01: Uncomment these when they are exported again.
-    # (IPersonPublic['findPathToTeam'], 'team'),
-    # (IPersonPublic['inTeam'], 'team'),
+    # (IPersonViewRestricted['findPathToTeam'], 'team'),
+    # (IPersonViewRestricted['inTeam'], 'team'),
     (IPersonEditRestricted['join'], 'team'),
     (IPersonEditRestricted['leave'], 'team'),
     (IPersonEditRestricted['addMember'], 'person'),

=== modified file 'lib/lp/registry/interfaces/role.py'
--- lib/lp/registry/interfaces/role.py	2011-12-13 13:53:12 +0000
+++ lib/lp/registry/interfaces/role.py	2011-12-14 03:28:31 +0000
@@ -119,7 +119,8 @@
     def inTeam(team):
         """Is this person a member or the owner of `team`?
 
-        Passed through to the same method in 'IPersonPublic'.
+        Passed through to the *unproxied* same method in
+        `IPersonViewRestricted`.
         """
 
     def isOwner(obj):

=== modified file 'lib/lp/soyuz/tests/test_archive_subscriptions.py'
--- lib/lp/soyuz/tests/test_archive_subscriptions.py	2011-12-13 18:05:18 +0000
+++ lib/lp/soyuz/tests/test_archive_subscriptions.py	2011-12-14 03:28:31 +0000
@@ -5,14 +5,21 @@
 
 from zope.security.interfaces import Unauthorized
 
+from canonical.launchpad.testing.pages import (
+    find_tag_by_id,
+    setupBrowserForUser
+    )
+from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.registry.interfaces.person import PersonVisibility
 from lp.testing import (
     celebrity_logged_in,
     login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.mail_helpers import pop_notifications
+from lp.testing import BrowserTestCase
 from lp.testing.views import create_initialized_view
 
 
@@ -111,3 +118,35 @@
             self.subscriber, registrant=self.archive.owner)
 
         self.assertEqual(0, len(pop_notifications()))
+
+
+class PrivateArtifactsViewTestCase(BrowserTestCase):
+    """ Tests that private team archives can be viewed."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        """Create a test archive."""
+        super(PrivateArtifactsViewTestCase, self).setUp()
+        self.owner = self.factory.makePerson()
+        self.private_team = self.factory.makeTeam(
+            visibility=PersonVisibility.PRIVATE,
+            name="subscribertest", owner=self.owner)
+        with person_logged_in(self.owner):
+            self.archive = self.factory.makeArchive(
+                private=True, owner=self.private_team)
+        self.subscriber = self.factory.makePerson()
+
+    def test_traverse_view_private_team_archive_subscriber(self):
+        # A subscriber can traverse and view the archive.
+        with person_logged_in(self.owner):
+            self.archive.newSubscription(
+                self.subscriber, registrant=self.archive.owner)
+        with person_logged_in(self.subscriber):
+            url = canonical_url(self.archive)
+        browser = setupBrowserForUser(self.subscriber)
+        browser.open(url)
+        content = find_tag_by_id(browser.contents, 'document')
+        self.assertIsNotNone(find_tag_by_id(content, 'ppa-install'))
+        self.assertIsNotNone(
+            find_tag_by_id(content, 'portlet-latest-updates'))