launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-porlet-subscribers-errors-884222 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #884222 in Launchpad itself: "Fix BugTask:+bug-portlet-subscribers-details Unauthorized errors"
  https://bugs.launchpad.net/launchpad/+bug/884222

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-porlet-subscribers-errors-884222/+merge/81101

== Implementation ==

The BugPortletSubscribersWithDetails view uses the bug subscriber_data property which invokes getDirectSubscribersWithDetails() and getIndirectDirectSubscribers().
The subscribers in the result are filtered. I've added a check to both the direct and indirect subscribers methods using check_permission('launchpad.View', person) so that private teams a user cannot see are not included. This stops the subsequent oops when trying to get the team name when rendering.

== Tests ==

Added a new test to BugPortletSubscribersWithDetailsTests:
  - test_data_unauthorised_private_team_excluded

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugsubscription.py
  lib/lp/bugs/browser/tests/test_bugsubscription_views.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/bug-porlet-subscribers-errors-884222/+merge/81101
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-porlet-subscribers-errors-884222 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2011-09-27 07:53:02 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2011-11-03 02:14:25 +0000
@@ -537,9 +537,13 @@
         details = list(bug.getDirectSubscribersWithDetails())
         for person, subscribed_by, subscription in details:
             can_edit = subscription.canBeUnsubscribedByUser(self.user)
-            if person == self.user or (person.private and not can_edit):
+            if (person == self.user
+                or (person.private
+                    and (not can_edit
+                        or not check_permission('launchpad.View', person)))):
                 # Skip the current user viewing the page,
-                # and private teams user is not a member of.
+                # and private teams user is not a member of or does not have
+                # permission to view.
                 continue
 
             subscriber = {
@@ -567,8 +571,11 @@
 
         others = list(bug.getIndirectSubscribers())
         for person in others:
-            if person == self.user:
-                # Skip the current user viewing the page.
+            if (person == self.user
+                or (person.private
+                    and not check_permission('launchpad.View', person))):
+                # Skip the current user viewing the page,
+                # and private teams user does not have permission to view.
                 continue
             subscriber = {
                 'name': person.name,

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-07-21 18:07:26 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-11-03 02:14:25 +0000
@@ -21,7 +21,10 @@
     BugSubscriptionSubscribeSelfView,
     )
 from lp.bugs.enum import BugNotificationLevel
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    PersonVisibility,
+    )
 from lp.testing import (
     person_logged_in,
     StormStatementRecorder,
@@ -457,8 +460,13 @@
 
         self.assertEqual(bug_content, bugtask_content)
 
-    def _makeBugWithNoSubscribers(self):
-        bug = self.factory.makeBug()
+    def _makeBugWithNoSubscribers(self, product_owner=None,
+                                  official_malone=None):
+        registrant = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=product_owner, registrant=registrant,
+            official_malone=official_malone)
+        bug = self.factory.makeBug(product=product)
         with person_logged_in(bug.owner):
             # Unsubscribe the bug reporter to ensure we have no subscribers.
             bug.unsubscribe(bug.owner, bug.owner)
@@ -716,3 +724,46 @@
             harness = LaunchpadFormHarness(
                 bug, BugPortletSubscribersWithDetails)
             self.assertEqual(dumps([]), harness.view.subscriber_data_js)
+
+    def test_data_unauthorised_private_team_excluded(self):
+        # Private teams a user cannot see are not included in the results.
+        pillar_owner = self.factory.makeTeam(
+            name='pillar-owner', displayname='Pillar Owner',
+            visibility=PersonVisibility.PRIVATE)
+        # We create a bug with a private indirect subscriber who will not be
+        # visible in the results.
+        bug = self._makeBugWithNoSubscribers(
+            product_owner=pillar_owner, official_malone=True)
+        user = self.factory.makePerson()
+        # subscriber is someone we will see in the results.
+        subscriber = self.factory.makePerson(
+            name='subscriber', displayname='Subscriber')
+        # We will not see the private team.
+        private_team = self.factory.makeTeam(
+            name='team', displayname='Team Name',
+            visibility=PersonVisibility.PRIVATE)
+
+        with person_logged_in(user):
+            bug.subscribe(private_team, user,
+                          level=BugNotificationLevel.LIFECYCLE)
+            sub = bug.subscribe(subscriber, user,
+                          level=BugNotificationLevel.LIFECYCLE)
+
+        with person_logged_in(user):
+            harness = LaunchpadFormHarness(
+                bug, BugPortletSubscribersWithDetails)
+            api_request = IWebServiceClientRequest(harness.request)
+            expected_result = {
+                'subscriber': {
+                    'name': 'subscriber',
+                    'display_name': 'Subscriber',
+                    'is_team': False,
+                    'can_edit': True,
+                    'web_link': canonical_url(subscriber),
+                    'self_link': absoluteURL(subscriber, api_request),
+                    'display_subscribed_by': sub.display_subscribed_by,
+                    },
+                'subscription_level': "Lifecycle",
+                }
+            self.assertEqual(dumps([expected_result]),
+                harness.view.subscriber_data_js)