Merge lp:~wallyworld/launchpad/private-owned-entity-traversal into lp:launchpad

[Ian Booth]

Users with access to private team's PPAs or branches have limited view permission on the team.

Users with access to private team's PPAs or branches have limited view permission on the team. This allows traversal to the branch or PPPA and also the team's name and URL to be known.

== Implementation ==

1. Change the traversal rules so that the "launchpad.LimitedView" permission is required to traverse private teams.
2. Enhance the private team security adaptor to check that the users with access to the team's PPAs or branches are authorised.

This change removes the hack put in place for bug 597783 which allowed subscribers to the private team's PPA to also see info they shouldn't have like team members etc.

== Tests ==

Re-run the TestArchiveSubscriptions.test_subscriber_can_access_private_team_ppa() test.
Add to the private-team-visibility.txt doc test.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/app/browser/launchpad.py
  lib/lp/registry/doc/private-team-visibility.txt

[Curtis Hovey (sinzui)]

I changed the archive bug you are fixing because the older bug was about not having access to the archive. The newer bug is about having access to non-traversed objects. I think your implementation is a good foundation that we can extend.

We might be missing a test. I like your additions to the doctest that we use to quickly say yes or no to a private team relationship. I see Julian's lib/lp/soyuz/tests/test_archive_subscriptions.py test that verifies access to team.name, which was the the *first* attr the code tried to access then oopsed. I do not see a test that verifies either the archive page or the branch page will render in this scenario. Look at
    https://launchpad.net/~launchpad/+archive/ppa
and
    https://code.launchpad.net/~launchpad/launchpad-help-moin-theme/moin_launchpad

In both pages I see the team *icon* with the team displayname that is a link made using the name. I do not certain limitedview permits me to see icon. Maybe we should also grant icon to limited view because we often use it with name and displayname. Does the tales formatter need to be aware that it cannot use an icon if the user has only LimiteView?

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2011-11-22 13:36:28 +0000
> +++ lib/canonical/launchpad/security.py 2011-12-01 15:21:25 +0000
...
> +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

checkAuthenticated returns false by default and I was puzzled. I think this guard
needs a comment to ensure dimwitted people like myself know that the forward
check covers public teams and users, which defaults to True.

        if self.forwardCheckAuthenticated(user, self.obj, 'launchpad.View'):
            # The user has full visibility of the user, the public team,
            # or has view permission of the private team.
            return True

> if (self.obj.is_team
> and self.obj.visibility == PersonVisibility.PRIVATE):
>
> + # Grant visibility to people with subscriptions to branches owned
> + # by the private team.
> + owned_branches = ge...

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)]

We want IPersonPublic and ITeamPublic to require launchpad.View to ensure information is not revealed when the user has launchpad.LimitedView

review: Needs Fixing (code)

[Curtis Hovey (sinzui)]

Hi Ian.

I have a working version of you branch that I am trying to land. Buildbot has been in a bad state. I landed to cleanup branches to address brittle/faulty tests that your attempt to refactor IPersonPublic revealed. My branch updates some authentication code to know it could be working with a secured person, and updates a dozen tests that could not login because it could not access the private team. I will land your other branch too if I can get my branch landed before your Monday.

[Curtis Hovey (sinzui)]

The branch was merged with my interface and test fixes.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-11-22 13:36:28 +0000
+++ lib/canonical/launchpad/security.py	2011-12-02 22:51:25 +0000
@@ -67,6 +67,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-11-30 14:41:22 +0000
+++ lib/lp/app/browser/launchpad.py	2011-12-02 22:51:25 +0000
@@ -738,8 +738,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/tests/test_tales.py'
--- lib/lp/app/tests/test_tales.py	2011-11-16 00:09:49 +0000
+++ lib/lp/app/tests/test_tales.py	2011-12-02 22:51:25 +0000
@@ -155,12 +155,16 @@
     A user must have launchpad.LimitedView permission to use
     TestTalesFormatterAPI with private teams.
     """
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         super(TestTalesFormatterAPI, self).setUp()
+        icon = self.factory.makeLibraryFileAlias(
+            filename='smurf.png', content_type='image/png')
+        logo = self.factory.makeLibraryFileAlias(
+            filename='papa.png', content_type='image/png')
         self.team = self.factory.makeTeam(
-            name='team', displayname='a team',
+            name='team', displayname='a team', icon=icon, logo=logo,
             visibility=PersonVisibility.PRIVATE)
 
     def _make_formatter(self, cache_permission=False):
@@ -175,10 +179,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 +233,26 @@
     def test_can_view_url(self):
         self._test_can_view_attribute('url')
 
+    def test_can_view_icon(self):
+        # Any user can view private team icons so there's no need to set up
+        # launchpad.LimitedView permissions to test that.
+        formatter, request, ignore = self._make_formatter()
+        value = self._tales_value('icon', request)
+        self.assertEqual(
+            '<img src="%s" width="14" height="14" />'
+            % self.team.icon.http_url,
+            value)
+
+    def test_can_view_logo(self):
+        # Any user can view private team logos so there's no need to set up
+        # launchpad.LimitedView permissions to test that.
+        formatter, request, ignore = self._make_formatter()
+        value = self._tales_value('logo', request, 'image')
+        self.assertEqual(
+            '<img alt="" width="64" height="64" src="%s" />'
+            % self.team.logo.http_url,
+            value)
+
 
 class TestObjectFormatterAPI(TestCaseWithFactory):
     """Tests for ObjectFormatterAPI"""

=== modified file 'lib/lp/registry/doc/private-team-visibility.txt'
--- lib/lp/registry/doc/private-team-visibility.txt	2011-11-16 00:09:49 +0000
+++ lib/lp/registry/doc/private-team-visibility.txt	2011-12-02 22:51:25 +0000
@@ -107,3 +107,41 @@
     ...
     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.
+
+    >>> 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.
+
+    >>> archive = factory.makeArchive(private=True, owner=priv_team)
+    >>> archive_subscriber = factory.makePerson()
+    >>> login_person(priv_owner)
+    >>> sub = archive.newSubscription(
+    ...     archive_subscriber, registrant=archive.owner)
+
+    >>> login_person(archive_subscriber)
+    >>> check_permission('launchpad.LimitedView', priv_team)
+    True
+
+Anonymous users do not have permission.
+    >>> login(ANONYMOUS)
+    >>> check_permission('launchpad.LimitedView', priv_team)
+    False

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-11-29 16:48:56 +0000
+++ lib/lp/testing/factory.py	2011-12-02 22:51:25 +0000
@@ -772,7 +772,7 @@
             address, person, email_status, account)
 
     def makeTeam(self, owner=None, displayname=None, email=None, name=None,
-                 description=None,
+                 description=None, icon=None, logo=None,
                  subscription_policy=TeamSubscriptionPolicy.OPEN,
                  visibility=None, members=None):
         """Create and return a new, arbitrary Team.
@@ -783,9 +783,11 @@
         :param displayname: The team's display name.  If not given we'll use
             the auto-generated name.
         :param description: Team team's description.
-        :type string:
+        :type description string:
         :param email: The email address to use as the team's contact address.
         :type email: string
+        :param icon: The team's icon.
+        :param logo: The team's logo.
         :param subscription_policy: The subscription policy of the team.
         :type subscription_policy: `TeamSubscriptionPolicy`
         :param visibility: The team's visibility. If it's None, the default
@@ -810,15 +812,19 @@
         team = getUtility(IPersonSet).newTeam(
             owner, name, displayname, teamdescription=description,
             subscriptionpolicy=subscription_policy)
+        naked_team = removeSecurityProxy(team)
         if visibility is not None:
             # Visibility is normally restricted to launchpad.Commercial, so
             # removing the security proxy as we don't care here.
-            removeSecurityProxy(team).visibility = visibility
+            naked_team.visibility = visibility
         if email is not None:
             team.setContactAddress(
                 getUtility(IEmailAddressSet).new(email, team))
+        if icon is not None:
+            naked_team.icon = icon
+        if logo is not None:
+            naked_team.logo = logo
         if members is not None:
-            naked_team = removeSecurityProxy(team)
             for member in members:
                 naked_team.addMember(member, owner)
         return team