launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/open-delegate-subscription-problems into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/open-delegate-subscription-problems/+merge/81090

Summary
=======
For privacy/security reasons, we don't want open or delegated teams to be able
to be subscribed to private bugs. Doing so allows essentially anyone to access
the bug, since there is no control over who joins the team.

This branch is the first to address that problem, tackling the actual
+addsubscriber view. A subsequent branch (or branches) may be needed to catch
javascript subscription mechanisms, and implement the same behavior for bug
assignment.

Preimp
======
Spoke with Curtis Hovey

Implementation
==============
The BugSubscriptionAddView has been given a validate hook that checks the
subscription policy of teams being subscribed if the bug in question is
private. If the subscription policy is OPEN or DELEGATED, and error is
returned.

Tests
=====
bin/test -vvct BugsubscriptionPrivacy

QA
==
Go to the +addsubscriber view for a private bug, and attempt to add a team
with open or delegated subscription policies. It should fail with an error
message explaining the rule.

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/~jcsackett/launchpad/open-delegate-subscription-problems/+merge/81090
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/open-delegate-subscription-problems 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-02 22:06:32 +0000
@@ -53,6 +53,7 @@
 from lp.bugs.model.structuralsubscription import (
     get_structural_subscriptions_for_bug,
     )
+from lp.registry.interfaces.person import TeamSubscriptionPolicy
 from lp.services.propertycache import cachedproperty
 
 
@@ -68,6 +69,22 @@
         super(BugSubscriptionAddView, self).setUpFields()
         self.form_fields['person'].for_input = True
 
+    def validate(self, data):
+        '''Validation hook.
+        Ensures that open and delegated teams aren't allowed to be subscribed
+        to private bugs.
+        '''
+        person = data['person']
+        if person.isTeam() and self.context.bug.private:
+            bad_types = (
+                TeamSubscriptionPolicy.OPEN,
+                TeamSubscriptionPolicy.DELEGATED
+                )
+            if person.subscriptionpolicy in bad_types:
+                error_msg = ("Open and delegated teams cannot be subscribed "
+                    "to private bugs.")
+                self.setFieldError('person', error_msg)
+
     @action('Subscribe user', name='add')
     def add_action(self, action, data):
         person = data['person']

=== 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-02 22:06:32 +0000
@@ -9,6 +9,7 @@
 from storm.store import Store
 from testtools.matchers import Equals
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 from zope.traversing.browser import absoluteURL
 
 from canonical.launchpad.ftests import LaunchpadFormHarness
@@ -17,11 +18,15 @@
 from lazr.restful.interfaces import IWebServiceClientRequest
 from lp.bugs.browser.bugsubscription import (
     BugPortletSubscribersWithDetails,
+    BugSubscriptionAddView,
     BugSubscriptionListView,
     BugSubscriptionSubscribeSelfView,
     )
 from lp.bugs.enum import BugNotificationLevel
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    TeamSubscriptionPolicy,
+    )
 from lp.testing import (
     person_logged_in,
     StormStatementRecorder,
@@ -36,6 +41,41 @@
 OFF = None
 
 
+class BugsubscriptionPrivacyTests(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(BugsubscriptionPrivacyTests, self).setUp()
+        self.user = self.factory.makePerson()
+        self.bug = self.factory.makeBug(owner=self.user)
+        removeSecurityProxy(self.bug).private = True
+
+    def _assert_subscription_fails(self, team):
+        with person_logged_in(self.user):
+            harness = LaunchpadFormHarness(
+                self.bug.default_bugtask, BugSubscriptionAddView)
+            form_data = {'field.person': team.name}
+            harness.submit('add', form_data)
+        subscription = removeSecurityProxy(self.bug).getSubscriptionForPerson(
+            team)
+        error_msg = harness.getFieldError('person')
+        expected_msg = (u'Open and delegated teams cannot be subscribed to '
+            'private bugs.')
+        self.assertEqual(expected_msg, error_msg)
+        self.assertIs(None, subscription)
+
+    def test_open_team_cannot_be_subscribed_to_private_bug(self):
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.OPEN)
+        self._assert_subscription_fails(team)
+
+    def test_delegated_team_cannot_be_subscribed_to_private_bug(self):
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.DELEGATED)
+        self._assert_subscription_fails(team)
+
+
 class BugSubscriptionAdvancedFeaturesTestCase(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer