launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00305
[Merge] lp:~brian-murray/launchpad/bug-556489-distro-struct-sub into lp:launchpad/devel
Brian Murray has proposed merging lp:~brian-murray/launchpad/bug-556489-distro-struct-sub into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#556489 Remove direct subscribers from ubuntu
https://bugs.launchpad.net/bugs/556489
= Summary =
The Ubuntu distribution has a large number of subscribers whose accounts are
deactivated, https://bugs.edge.launchpad.net/ubuntu/+subscribe, probably due
to the volume of the bug mail they ended up receiving. This is problematic to
some degree as all the subscribers show up on every bug report about Ubuntu.
Subscribing to all of Ubuntu bugs can also cause an unpleasant user experience
due to the volume of email they end up receiving and it not being clear how to
stop all of it. Additionally, if someone really wants to subscribe to every
bug report about Ubuntu there is a mailing list
(https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs) for that.
== Proposed fix ==
To prevent people from over subscribing themselves a permissions check should
be added to distribution stuctural subscriptions to ensure that only admins or
bug supervisors, if one exists, can subscribe to the distribution.
== Pre-implementation notes ==
This was discussed with Deryck who reported bug 556489 regarding this issue
and we discussed the process for removing existing subscribers to Ubuntu.
== Implementation details ==
While writing a test for the special case of distribution structural
subscriptions, I discovered that test_structuralsubscriptiontarget.py really
used structural-subscription-target.txt which needed some rewriting as a unit
test.
lib/lp/registry/model/structuralsubscription.py:
* Added distribution check to addBugSubscription
lib/lp/registry/tests/test_structuralsubscriptiontarget.py:
* Refactored most of structural-subscription-target.txt as a unit test
lib/lp/registry/tests/structural-subscription-target.txt:
* Removed portions implemented in test_structuralsubscriptiontarget.py
== Tests ==
bin/test -vv -t test_structuralsubscriptiontarget -t structural-subscription-target.txt
--
https://code.launchpad.net/~brian-murray/launchpad/bug-556489-distro-struct-sub/+merge/31090
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~brian-murray/launchpad/bug-556489-distro-struct-sub into lp:launchpad/devel.
=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py 2010-04-19 09:39:29 +0000
+++ lib/lp/registry/model/structuralsubscription.py 2010-07-27 21:06:09 +0000
@@ -105,11 +105,12 @@
elif self.distroseries is not None:
return self.distroseries
else:
- raise AssertionError, 'StructuralSubscription has no target.'
+ raise AssertionError('StructuralSubscription has no target.')
class StructuralSubscriptionTargetMixin:
"""Mixin class for implementing `IStructuralSubscriptionTarget`."""
+
@property
def _target_args(self):
"""Target Arguments.
@@ -184,6 +185,16 @@
# subscription and immediately giving it a full
# bug notification level. It is useful so long as
# subscriptions are mainly used to implement bug contacts.
+
+ admins = getUtility(ILaunchpadCelebrities).admin
+
+ if IDistribution.providedBy(self) and (
+ subscriber != self.bug_supervisor
+ and self.bug_supervisor is not None
+ and not subscribed_by.inTeam(admins)):
+ raise UserCannotSubscribePerson(
+ 'Only the bug supervisor can subscribe to distribution bugs.')
+
sub = self.addSubscription(subscriber, subscribed_by)
sub.bug_notification_level = BugNotificationLevel.COMMENTS
return sub
@@ -238,7 +249,7 @@
for key, value in self._target_args.items():
if value is None:
target_clause_parts.append(
- "StructuralSubscription.%s IS NULL " % (key,))
+ "StructuralSubscription.%s IS NULL " % (key, ))
else:
target_clause_parts.append(
"StructuralSubscription.%s = %s " % (key, quote(value)))
=== modified file 'lib/lp/registry/tests/structural-subscription-target.txt'
--- lib/lp/registry/tests/structural-subscription-target.txt 2010-04-19 08:11:52 +0000
+++ lib/lp/registry/tests/structural-subscription-target.txt 2010-07-27 21:06:09 +0000
@@ -1,144 +1,15 @@
= IStructuralSubscriptionTarget =
-Structural subscriptions allow users to subscribe to notifications about
-new items and changes to existing items for a Launchpad structure such as
-Product, ProjectGroup, Distribution, Milestone and series.
-
- >>> from canonical.launchpad.interfaces import (
- ... IStructuralSubscriptionTarget)
- >>> from canonical.launchpad.webapp.testing import verifyObject
- >>> verifyObject(IStructuralSubscriptionTarget, target)
- True
-
-== Bug Subscriptions ==
-
-Bug subscriptions are structural subscriptions to notifications about
-bug activity.
-
- >>> target.bug_subscriptions
- []
-
-First, we try to create a new subscription.
+
+Let's subscribe ubuntu-team.
>>> from canonical.launchpad.interfaces import IPersonSet
>>> personset = getUtility(IPersonSet)
+ >>> ubuntu_team = personset.getByName("ubuntu-team")
>>> no_priv = personset.getByName("no-priv")
-
-Only authenticated users can create subscriptions.
-
- >>> target.addBugSubscription(no_priv, no_priv)
- Traceback (most recent call last):
- ...
- Unauthorized: ...
-
-Let's login then to add a subscription:
-
+ >>> foobar = personset.getByName("name16")
>>> from canonical.launchpad.ftests import login
>>> login("foo.bar@xxxxxxxxxxxxx")
-
- >>> target.addBugSubscription(no_priv, no_priv)
- <StructuralSubscription ...>
- >>> [sub.subscriber.name for sub in target.bug_subscriptions]
- [u'no-priv']
-
-Trying to add a subscription to a target when that person
-or team is already subscribed to that target will return
-the existing subscription.
-
- >>> target.addBugSubscription(no_priv, no_priv)
- <StructuralSubscription ...>
-
-People can only be subscribed by themselves, and only the team admins may
-subscribe a team.
-
-no-priv, who has no relationship to ubuntu-team, cannot subscribe it.
-
- >>> ubuntu_team = personset.getByName("ubuntu-team")
- >>> target.addBugSubscription(ubuntu_team, no_priv)
- Traceback (most recent call last):
- ...
- UserCannotSubscribePerson: no-priv does not have permission to subscribe ubuntu-team.
-
-But kamion, an admin of the team, can.
-
- >>> kamion = personset.getByName("kamion")
- >>> target.addBugSubscription(ubuntu_team, kamion)
- <StructuralSubscription ...>
-
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- [u'no-priv', u'ubuntu-team']
-
-foobar, a Launchpad administrator, can as well.
-
- >>> foobar = personset.getByName("name16")
- >>> target.addBugSubscription(ubuntu_team, foobar)
- <StructuralSubscription ...>
-
-A non-admin cannot subscribe a person other than themselves.
-
- >>> target.addBugSubscription(kamion, no_priv)
- Traceback (most recent call last):
- ...
- UserCannotSubscribePerson: no-priv does not have permission to subscribe kamion.
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- [u'no-priv', u'ubuntu-team']
-
-But again, an admin can.
-
- >>> target.addBugSubscription(kamion, foobar)
- <StructuralSubscription ...>
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- [u'kamion', u'no-priv', u'ubuntu-team']
-
-To remove a bug subscription, use
-IStructuralSubscriptionTarget.removeBugSubscription:
-
- >>> target.removeBugSubscription(no_priv, no_priv)
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- [u'kamion', u'ubuntu-team']
-
-The subscription rules apply to unsubscription as well.
-
-An unprivileged user cannot unsubscribe a team.
-
- >>> target.removeBugSubscription(ubuntu_team, no_priv)
- Traceback (most recent call last):
- ...
- UserCannotSubscribePerson: no-priv does not have permission to unsubscribe ubuntu-team.
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- [u'kamion', u'ubuntu-team']
-
-But a team admin can.
-
- >>> target.removeBugSubscription(ubuntu_team, kamion)
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- [u'kamion']
-
-An unprivileged user also cannot unsubscribe another user.
-
- >>> target.removeBugSubscription(kamion, no_priv)
- Traceback (most recent call last):
- ...
- UserCannotSubscribePerson: no-priv does not have permission to unsubscribe kamion.
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- [u'kamion']
-
-But the user themselves can.
-
- >>> target.removeBugSubscription(kamion, kamion)
- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
- []
-
-Trying to remove a subscription that doesn't exist on a target raises a
-DeleteSubscriptionError.
-
- >>> target.removeBugSubscription(foobar, foobar)
- Traceback (most recent call last):
- ...
- DeleteSubscriptionError: ...
-
-Let's subscribe ubuntu-team again.
-
>>> target.addBugSubscription(ubuntu_team, foobar)
<StructuralSubscription ...>
@@ -191,7 +62,7 @@
=== Structural subscriptions and indirect bug subscriptions ===
-
+
>>> bug = filebug(target, 'test bug one')
>>> indirect_subscribers = set(
... subscriber.name for subscriber in bug.getIndirectSubscribers())
=== modified file 'lib/lp/registry/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/registry/tests/test_structuralsubscriptiontarget.py 2009-12-05 18:37:28 +0000
+++ lib/lp/registry/tests/test_structuralsubscriptiontarget.py 2010-07-27 21:06:09 +0000
@@ -4,10 +4,13 @@
"""Test harness for running tests against IStructuralsubscriptionTarget
implementations.
"""
+__metaclass__ = type
import unittest
from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
+from zope.security.proxy import ProxyFactory, removeSecurityProxy
from canonical.launchpad.webapp.interfaces import ILaunchBag
from lp.bugs.interfaces.bug import CreateBugParams
@@ -19,6 +22,234 @@
from canonical.launchpad.testing.systemdocs import (
LayeredDocFileSuite, setUp, tearDown)
from canonical.testing import LaunchpadFunctionalLayer
+from canonical.launchpad.webapp.testing import verifyObject
+from lp.testing import (
+ ANONYMOUS, login, login_person, TestCaseWithFactory)
+from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.structuralsubscription import (
+ DeleteSubscriptionError, IStructuralSubscriptionTarget,
+ UserCannotSubscribePerson)
+from lp.registry.model.structuralsubscription import StructuralSubscription
+
+
+class StructuralSubscriptionTestBase:
+
+ def setUp(self):
+ super(StructuralSubscriptionTestBase, self).setUp()
+ self.ordinary_subscriber = self.factory.makePerson()
+ self.bug_supervisor_subscriber = self.factory.makePerson()
+ self.team_owner = self.factory.makePerson()
+ self.team = self.factory.makeTeam(owner=self.team_owner)
+
+ def test_target_implements_structural_subscription_target(self):
+ self.assertTrue(verifyObject(IStructuralSubscriptionTarget,
+ self.target))
+
+ def test_anonymous_cannot_subscribe_anyone(self):
+ # only authenticated users can create structural subscriptions
+ login(ANONYMOUS)
+ self.assertRaises(Unauthorized, getattr, self.target,
+ 'addBugSubscription')
+
+ def test_person_structural_subscription_by_other_person(self):
+ # a person can not subscribe someone else willy nilly
+ login_person(self.ordinary_subscriber)
+ self.assertRaises(UserCannotSubscribePerson,
+ self.target.addBugSubscription,
+ self.team_owner, self.ordinary_subscriber)
+
+ def test_team_structural_subscription_by_non_team_member(self):
+ # a person not related to a team cannot subscribe it
+ login_person(self.ordinary_subscriber)
+ self.assertRaises(UserCannotSubscribePerson,
+ self.target.addBugSubscription,
+ self.team, self.ordinary_subscriber)
+
+ def test_admin_can_subscribe_anyone(self):
+ # a launchpad admin can create a structural subscription for
+ # anyone
+ login('foo.bar@xxxxxxxxxxxxx')
+ foobar = getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx')
+ #with celebrity_logged_in('admin'):
+ self.assertIsInstance(
+ self.target.addBugSubscription(self.ordinary_subscriber, foobar),
+ StructuralSubscription)
+
+ def test_secondary_structural_subscription(self):
+ # creating a structural subscription a 2nd time returns the
+ # first structural subscription
+ login_person(self.bug_supervisor_subscriber)
+ subscription1 = self.target.addBugSubscription(
+ self.bug_supervisor_subscriber, self.bug_supervisor_subscriber)
+ subscription2 = self.target.addBugSubscription(
+ self.bug_supervisor_subscriber, self.bug_supervisor_subscriber)
+ self.assertIs(subscription1.id, subscription2.id)
+
+ def test_remove_structural_subscription(self):
+ # an unprivileged user cannot unsubscribe a team
+ login_person(self.ordinary_subscriber)
+ self.assertRaises(UserCannotSubscribePerson,
+ self.target.removeBugSubscription,
+ self.team, self.ordinary_subscriber)
+
+ def test_remove_nonexistant_structural_subscription(self):
+ # removing a nonexistant subscription raises a
+ # DeleteSubscriptionError
+ login_person(self.ordinary_subscriber)
+ self.assertRaises(DeleteSubscriptionError,
+ self.target.removeBugSubscription,
+ self.ordinary_subscriber, self.ordinary_subscriber)
+
+
+class UnrestrictedStructuralSubscription(StructuralSubscriptionTestBase):
+
+ def test_structural_subscription_by_ordinary_user(self):
+ # ordinary users can subscribe themselves
+ login_person(self.ordinary_subscriber)
+ self.assertIsInstance(
+ self.target.addBugSubscription(
+ self.ordinary_subscriber, self.ordinary_subscriber),
+ StructuralSubscription)
+
+ def test_remove_structural_subscription_by_ordinary_user(self):
+ # ordinary users can unsubscribe themselves
+ login_person(self.ordinary_subscriber)
+ self.assertIsInstance(
+ self.target.addBugSubscription(
+ self.ordinary_subscriber, self.ordinary_subscriber),
+ StructuralSubscription)
+ self.assertEqual(
+ self.target.removeBugSubscription(
+ self.ordinary_subscriber, self.ordinary_subscriber),
+ None)
+
+ def test_team_structural_subscription_by_team_owner(self):
+ # team owners can subscribe their team
+ login_person(self.team_owner)
+ self.assertIsInstance(
+ self.target.addBugSubscription(
+ self.team, self.team_owner),
+ StructuralSubscription)
+
+ def test_remove_team_structural_subscription_by_team_owner(self):
+ # team owners can unsubscribe their team
+ login_person(self.team_owner)
+ self.assertIsInstance(
+ self.target.addBugSubscription(
+ self.team, self.team_owner),
+ StructuralSubscription)
+ self.assertEqual(
+ self.target.removeBugSubscription(
+ self.team, self.team_owner),
+ None)
+
+
+class TestStructuralSubscriptionForDistro(StructuralSubscriptionTestBase,
+ TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestStructuralSubscriptionForDistro, self).setUp()
+ self.target = self.factory.makeDistribution()
+ naked_distro = removeSecurityProxy(self.target)
+ naked_distro.bug_supervisor = self.bug_supervisor_subscriber
+
+ def test_distribution_subscription_by_ordinary_user(self):
+ # ordinary users can not subscribe themselves to a distribution
+ login_person(self.ordinary_subscriber)
+ self.assertRaises(UserCannotSubscribePerson,
+ self.target.addBugSubscription,
+ self.ordinary_subscriber, self.ordinary_subscriber)
+
+ def test_team_distribution_structural_subscription_by_team_owner(self):
+ # team owners cannot subscribe their team to a distribution
+ login_person(self.team_owner)
+ self.assertRaises(UserCannotSubscribePerson,
+ self.target.addBugSubscription,
+ self.team, self.team_owner)
+
+ def test_distribution_subscription_by_bug_supervisor(self):
+ # bug supervisor can subscribe themselves
+ login_person(self.bug_supervisor_subscriber)
+ self.assertIsInstance(
+ self.target.addBugSubscription(
+ self.bug_supervisor_subscriber,
+ self.bug_supervisor_subscriber),
+ StructuralSubscription)
+
+ def test_distribution_subscription_by_bug_supervisor_team(self):
+ # team admins can subscribe team if team is bug supervisor
+ removeSecurityProxy(self.target).bug_supervisor = self.team
+ login_person(self.team_owner)
+ self.assertIsInstance(
+ self.target.addBugSubscription(self.team, self.team_owner),
+ StructuralSubscription)
+
+ def test_distribution_unsubscription_by_bug_supervisor_team(self):
+ # team admins can unsubscribe team if team is bug supervisor
+ removeSecurityProxy(self.target).bug_supervisor = self.team
+ login_person(self.team_owner)
+ self.assertIsInstance(
+ self.target.addBugSubscription(self.team, self.team_owner),
+ StructuralSubscription)
+ self.assertEqual(
+ self.target.removeBugSubscription(self.team, self.team_owner),
+ None)
+
+ def test_distribution_subscription_without_bug_supervisor(self):
+ # for a distribution without a bug supervisor anyone can
+ # subscribe
+ removeSecurityProxy(self.target).bug_supervisor = None
+ login_person(self.ordinary_subscriber)
+ self.assertIsInstance(
+ self.target.addBugSubscription(
+ self.ordinary_subscriber, self.ordinary_subscriber),
+ StructuralSubscription)
+
+
+class TestStructuralSubscriptionForProduct(
+ UnrestrictedStructuralSubscription, TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestStructuralSubscriptionForProduct, self).setUp()
+ self.target = self.factory.makeProduct()
+
+
+class TestStructuralSubscriptionForDistroSourcePackage(
+ UnrestrictedStructuralSubscription, TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestStructuralSubscriptionForDistroSourcePackage, self).setUp()
+ self.target = self.factory.makeDistributionSourcePackage()
+ self.target = ProxyFactory(self.target)
+
+
+class TestStructuralSubscriptionForMilestone(
+ UnrestrictedStructuralSubscription, TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestStructuralSubscriptionForMilestone, self).setUp()
+ self.target = self.factory.makeMilestone()
+ self.target = ProxyFactory(self.target)
+
+
+class TestStructuralSubscriptionForDistroSeries(
+ UnrestrictedStructuralSubscription, TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestStructuralSubscriptionForDistroSeries, self).setUp()
+ self.target = self.factory.makeDistroSeries()
+ self.target = ProxyFactory(self.target)
+
def distributionSourcePackageSetUp(test):
setUp(test)
@@ -27,27 +258,32 @@
test.globs['other_target'] = ubuntu.getSourcePackage('pmount')
test.globs['filebug'] = bugtarget_filebug
+
def productSetUp(test):
setUp(test)
test.globs['target'] = getUtility(IProductSet).getByName('firefox')
test.globs['filebug'] = bugtarget_filebug
+
def distributionSetUp(test):
setUp(test)
test.globs['target'] = getUtility(IDistributionSet).getByName('ubuntu')
test.globs['filebug'] = bugtarget_filebug
+
def milestone_filebug(milestone, summary, status=None):
bug = bugtarget_filebug(milestone.target, summary, status=status)
bug.bugtasks[0].milestone = milestone
return bug
+
def milestoneSetUp(test):
setUp(test)
firefox = getUtility(IProductSet).getByName('firefox')
test.globs['target'] = firefox.getMilestone('1.0')
test.globs['filebug'] = milestone_filebug
+
def distroseries_sourcepackage_filebug(distroseries, summary, status=None):
params = CreateBugParams(
getUtility(ILaunchBag).user, summary, comment=summary, status=status)
@@ -60,15 +296,18 @@
nomination.approve(distroseries.distribution.owner)
return bug
+
def distroSeriesSourcePackageSetUp(test):
setUp(test)
test.globs['target'] = (
getUtility(IDistributionSet).getByName('ubuntu').getSeries('hoary'))
test.globs['filebug'] = distroseries_sourcepackage_filebug
+
def test_suite():
"""Return the `IStructuralSubscriptionTarget` TestSuite."""
suite = unittest.TestSuite()
+ suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
setUpMethods = [
distributionSourcePackageSetUp,
Follow ups
