Merge lp:~brian-murray/launchpad/bug-556489-distro-struct-sub into lp:launchpad

[Brian Murray]

If a distribution has a bug supervisor set, only allow the bug supervisor or members of that team to subscribe to all of the distribution's bug reports.

= 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

[Robert Collins (lifeless)]

I'm not clear why this makes sense.

 - users can subscribe to the bugs list [same amount of mail]
 - there are uses to being notified of every bug, same as things like
gmane are subscribed to lists
 - this makes lp less flexible not more.

I suspect you're doing a bandaid to the real issues:
 - subscriber lists take up a lot of space and are rarely important
 - we don't prune deactivated accounts effectively
 - the implications of structural subscription are not clear to users

Could you please enlarge on why the approach you've put forward makes
sense long term? If we do need a bandaid, I'm positive doing that, but
I get the impression you don't consider this a bandaid, and given the
downsides: more interrupts from valid subscribers, more code to handle
special-only-in-scale problems, more ui complexity that is hard to
explain, I'd really prefer to see a path to a long term unified fix!

-Rob

[Brian Murray (brian-murray)]

On Wed, Jul 28, 2010 at 05:13:42AM -0000, Robert Collins wrote:
> I'm not clear why this makes sense.
>
> - users can subscribe to the bugs list [same amount of mail]

However, it is harder to subscribe to the mailing list than structurally
subscribe to Ubuntu.

> - there are uses to being notified of every bug, same as things like
> gmane are subscribed to lists
> - this makes lp less flexible not more.

Yes, but this only applies to distribution's using Launchpad that have a
bug supervisor. Additionally, it just so happens that Ubuntu has a team
set as the bug supervisor which has the email address set to a mailing
list. Other distributions do not, or should not, have to have a bug
supervisor set and in the event that they do that could be a team
without a mailing list set for the contact address.

> I suspect you're doing a bandaid to the real issues:
> - subscriber lists take up a lot of space and are rarely important
> - we don't prune deactivated accounts effectively
> - the implications of structural subscription are not clear to users
>
> Could you please enlarge on why the approach you've put forward makes
> sense long term? If we do need a bandaid, I'm positive doing that, but
> I get the impression you don't consider this a bandaid, and given the
> downsides: more interrupts from valid subscribers, more code to handle
> special-only-in-scale problems, more ui complexity that is hard to
> explain, I'd really prefer to see a path to a long term unified fix!

I do agree that is a band-aid in that we are preventing people from
doing something they don't really understand the consequences of.
Looking at my personal email archive I have some emails from people who
mistakenly subscribed to all bug reports about Ubuntu, did not know how
to unsubscribe and were receiving hundreds of emails a day. While
emails provide an unsubscribe link for individual bug reports there is
no link for how to remove a structural subscription.

I'd guess that we will get less requests from people wanting to
subscribe to all bug reports about Ubuntu than we will about people
wanting to be unsubscribed. Perhaps we should track this somehow?

--
Brian Murray

[Brian Murray (brian-murray)]

A structural subscription unsubscribe link is covered in bug 484319.

[Abel Deuring (adeuring)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 28.07.2010 19:41, Brian Murray wrote:
> I do agree that is a band-aid in that we are preventing people from
> doing something they don't really understand the consequences of.
> Looking at my personal email archive I have some emails from people who
> mistakenly subscribed to all bug reports about Ubuntu, did not know how
> to unsubscribe and were receiving hundreds of emails a day. While
> emails provide an unsubscribe link for individual bug reports there is
> no link for how to remove a structural subscription.

Another example: <email address hidden> receives more or less regulary
complaints from people who feel "drowning" by the amount of mails they
get from bug subscriptions.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iD8DBQFMUIF/ekBPhm8NrtARAtQqAKCQ2872DAsH0hByPaK8dzTlLk/jiwCeOjwE
LI/vZ8xrFAShGAD7DE6MBjY=
=50CY
-----END PGP SIGNATURE-----

[Deryck Hodge (deryck)]

While I understand Rob's concerns, I think in this case we do want to be opinionated and keep people from getting themselves into trouble. "less flexible" is a feature not a bug in this case. :-)

Yes, we should fix the surrounding issues, some of which will be fixed by our current work to improve subscriptions. But there is rarely a case where subscribing to all of a distribution's bugs is what people actually want to do. For the times where there is -- e.g., someone wants to archive all bug activity for a distribution -- there is an easy work around by subscribing to the mailing list or join the bug supervisor team. Perhaps this adds some UI complexity to communicate how one would go about this, but I think this risk is acceptable given the alternatives and the few number of people who desire this.

I'll also take a look at the code now to review this for Brian, to get him unblocked on this work.

Cheers,
deryck

[Deryck Hodge (deryck)]

Hi, Brian.

I've had a chance to review the code itself now. Thanks for the great job here you did refactoring the tests! Well done!

I do, however, see a couple problems that need to be addressed before this is ready to land.

First, you do a great job of avoiding sample data in the test except for your admin test, which uses Foo Bar. Your commented out attempt at using a with block and logging in the admin celebrity is the right way to go. You can reference the currently logged in user inside that block. Look in lp.testing.tests.test_login and look at test_with_celebrity_logged_in and getLoggedInPerson for ideas of how to do this. If you need more help working this out, ping me on IRC.

Second, this work ensures that addBugSubscription won't succeed, but the user will still have a subscribe to bug mail link, which will then OOPS if that click-path is followed and the user tries to subscribe. The code that checks that the user can subscribe should be refactored into a helper method which can be called from the model and the view so that the link is never presented in the first place. See lp.bugs.browser.bugtask.BugTaskSearchListingMenu.subscribe for where to do this check in the view.

Again, feel free to ping me if you need to talk more about these changes.

Cheers,
deryck

review: Needs Fixing (code)

[Deryck Hodge (deryck)]

The code itself looks fine now. But I led you wrong on where to check to display the link, I think. This didn't work for me testing locally. Did it work for you?

I didn't realize there is a mix at lp.registry.browser.structuralsubscription.StructuralSubscriptionMenuMixin where the link gets setup now. Sorry. The check is good, I believe, but needs to be done inside the mixin.

[Robert Collins (lifeless)]

So, to make sure I understand:
 - a distro without a bug supervisor does not permit structural subscription?

We are expecting many more distros though - derived distros are being worked on.

I have no problem with being opinionated; I'd -much- rather we warned
than stopped people - simply because if we prohibit something we
*will* get bug reports that its prohibited (the same as we get users
who were not warned about the firehose - who were *invited* into doing
something silly).

-Rob

[Brian Murray (brian-murray)]

On Tue, Aug 03, 2010 at 10:11:04PM -0000, Robert Collins wrote:
> So, to make sure I understand:
> - a distro without a bug supervisor does not permit structural subscription?

No, this is backwards.

A distro without a bug supervisor allows structural subscription.

A distro with a bug supervisor only allows the bug supervisor, or
members of that team, to structurally subscribe.

[Deryck Hodge (deryck)]

On Tue, Aug 3, 2010 at 5:11 PM, Robert Collins
<email address hidden> wrote:
> So, to make sure I understand:
>  - a distro without a bug supervisor does not permit structural subscription?
>
> We are expecting many more distros though - derived distros are being worked on.
>
> I have no problem with being opinionated; I'd -much- rather we warned
> than stopped people - simply because if we prohibit something we
> *will* get bug reports that its prohibited (the same as we get users
> who were not warned about the firehose - who were *invited* into doing
> something silly).
>

Right, but we get bug reports regardless of what we do. :-) The
threat of bug reports makes me neither fear nor regret the change. ;)

The real question is "should we allow users to subscribe to all of a
distributions bugs?" I think for distributions with a large number of
users and bugs this makes no sense. Brian's choice to limit this to
distros that have defined a bug supervisor seems about the best check
we have for "is this a large distro?" Not perfect, sure, but not bad
either.

The cases of someone wanting to subscribe to every bug email for
everything in a distro are few -- people like Brian and the Debian PTS
-- and all of these cases have an option to get at the notifications
(subscribe to mailing list or join bug supervisor team), and in the
common case, we prevent people from getting themselves into trouble
and giving Launchpad a bad reputation for spamming people. Also,
we're not changing anyone's current subscription, just preventing them
in the future. We're only going to clean up deactivated accounts once
this work lands.

People don't read warnings. There is already a warning.

Finally, I really don't understand why this change is a big deal. In
the world of Launchpad, this is a very minor area of potential
complexity and confusion, and it brings great benefit in terms of
sparing less-experienced users getting spammed by the bug tracker.
It's well known and well documented that, despite our best efforts,
people end up subscribing to all of Ubuntu's bugs. Why not prevent
that and be done with it? :-)

Cheers,
deryck

--
Deryck Hodge
https://launchpad.net/~deryck
http://www.devurandom.org/

[Robert Collins (lifeless)]

>> Finally, I really don't understand why this change is a big deal.

Its not a big deal :- I'm still getting a handle on a couple of things.

One of them is how to be clear what hat I'm when I say something; in
this conversation its a bit unclear - sorry. I wasn't trying to swing
the TA bat at this particular mp - rather I trying to really
understand the tradeoffs we're making, and why - and expressing my
personal opinions freely as I learn.

The other thing *is* architectural, which added to the confusion i
generated with the first thing : I'm still refining my understanding /
story around what bits of our UI are for machines, and what aren't :
the email interfaces are kindof-both and as such there are tensions
around making it really excellent. (Contrast with the web UI - humans
only, and the rest UI - machines only).

-Rob

[Brian Murray (brian-murray)]

It should be ready for re-review now. I tested it locally by setting a bug supervisor (guadamen) for Ubuntu and then confirming that the link did not appear for someone (sample person) who is not a member of that bug supervisor team.

[Deryck Hodge (deryck)]

Looks good. Thanks for all your work getting this ready.

Cheers,
deryck

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2010-08-07 14:54:40 +0000
+++ lib/lp/bugs/browser/bug.py	2010-08-17 14:18:48 +0000
@@ -251,7 +251,7 @@
         return Link('+addbranch', text, icon='add')
 
     def linktocve(self):
-        """Return the 'Link tp CVE' Link."""
+        """Return the 'Link to CVE' Link."""
         text = structured(
             'Link to '
             '<abbr title="Common Vulnerabilities and Exposures Index">'

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-08-03 08:49:19 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-08-17 14:18:48 +0000
@@ -2320,7 +2320,9 @@
             '+securitycontact', 'Change security contact', icon='edit')
 
     def subscribe(self):
-        return Link('+subscribe', 'Subscribe to bug mail', icon='edit')
+        user = getUtility(ILaunchBag).user
+        if self.context.userCanAlterBugSubscription(user):
+            return Link('+subscribe', 'Subscribe to bug mail', icon='edit')
 
     def nominations(self):
         return Link('+nominations', 'Review nominations', icon='bug')

=== modified file 'lib/lp/bugs/templates/bugtarget-portlet-bugfilters.pt'
--- lib/lp/bugs/templates/bugtarget-portlet-bugfilters.pt	2009-12-07 15:00:59 +0000
+++ lib/lp/bugs/templates/bugtarget-portlet-bugfilters.pt	2010-08-17 14:18:48 +0000
@@ -11,7 +11,7 @@
            tal:content="structure context/@@+bugtarget-portlet-bugfilters-info" />
     <tbody tal:define="menu context/menu:bugs">
       <tr tal:define="subscribe_link menu/subscribe|nothing"
-          tal:condition="subscribe_link">
+          tal:condition="python: subscribe_link and subscribe_link.enabled">
         <td class="bugs-count" style="padding-top: 1em">
           <a tal:attributes="href subscribe_link/url">
             <img tal:attributes="src subscribe_link/icon_url" />

=== modified file 'lib/lp/registry/browser/structuralsubscription.py'
--- lib/lp/registry/browser/structuralsubscription.py	2010-04-16 15:06:55 +0000
+++ lib/lp/registry/browser/structuralsubscription.py	2010-08-17 14:18:48 +0000
@@ -313,4 +313,8 @@
         else:
             text = 'Subscribe to bug mail'
             icon = 'add'
-        return Link('+subscribe', text, icon=icon, enabled=enabled)
+        if enabled == False or (
+            not sst.userCanAlterBugSubscription(self.user, self.user)):
+            return Link('+subscribe', text, icon=icon, enabled=False)
+        else:
+            return Link('+subscribe', text, icon=icon, enabled=enabled)

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2010-08-06 14:49:35 +0000
+++ lib/lp/registry/configure.zcml	2010-08-17 14:18:48 +0000
@@ -150,7 +150,8 @@
                 getSubscriptions
                 parent_subscription_target
                 target_type_display
-                userHasBugSubscriptions"/>
+                userHasBugSubscriptions
+                userCanAlterBugSubscription"/>
         <require
             permission="launchpad.AnyPerson"
             attributes="
@@ -294,7 +295,8 @@
                 getBugNotificationsRecipients
                 parent_subscription_target
                 target_type_display
-                userHasBugSubscriptions"/>
+                userHasBugSubscriptions
+                userCanAlterBugSubscription"/>
         <require
             permission="launchpad.AnyPerson"
             attributes="
@@ -417,7 +419,8 @@
                 official_bug_tags
                 findRelatedArchives
                 findRelatedArchivePublications
-                userHasBugSubscriptions"/>
+                userHasBugSubscriptions
+                userCanAlterBugSubscription"/>
         <require
             permission="launchpad.AnyPerson"
             attributes="
@@ -924,7 +927,8 @@
                 getBugNotificationsRecipients
                 parent_subscription_target
                 target_type_display
-                userHasBugSubscriptions"/>
+                userHasBugSubscriptions
+                userCanAlterBugSubscription"/>
         <require
             permission="launchpad.AnyPerson"
             attributes="
@@ -1195,7 +1199,8 @@
                 getBugNotificationsRecipients
                 parent_subscription_target
                 target_type_display
-                userHasBugSubscriptions"/>
+                userHasBugSubscriptions
+                userCanAlterBugSubscription"/>
         <require
             permission="launchpad.AnyPerson"
             attributes="
@@ -1344,7 +1349,8 @@
                 getSubscriptions
                 parent_subscription_target
                 target_type_display
-                userHasBugSubscriptions"/>
+                userHasBugSubscriptions
+                userCanAlterBugSubscription"/>
         <require
             permission="launchpad.AnyPerson"
             attributes="
@@ -1467,7 +1473,8 @@
                 getBugNotificationsRecipients
                 parent_subscription_target
                 target_type_display
-                userHasBugSubscriptions"/>
+                userHasBugSubscriptions
+                userCanAlterBugSubscription"/>
         <require
             permission="launchpad.AnyPerson"
             attributes="

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2010-08-12 19:35:11 +0000
+++ lib/lp/registry/interfaces/distribution.py	2010-08-17 14:18:48 +0000
@@ -183,7 +183,7 @@
                 "The person or team responsible for decisions about features "
                 "and bugs that will be targeted for any series in this "
                 "distribution. Note that you can also specify a driver "
-                "on each series who's permissions will be limited to that "
+                "on each series whose permissions will be limited to that "
                 "specific series."),
             required=False, vocabulary='ValidPersonOrTeam'))
     drivers = Attribute(

=== modified file 'lib/lp/registry/interfaces/structuralsubscription.py'
--- lib/lp/registry/interfaces/structuralsubscription.py	2010-07-28 20:44:25 +0000
+++ lib/lp/registry/interfaces/structuralsubscription.py	2010-08-17 14:18:48 +0000
@@ -184,6 +184,9 @@
         :return: The new subscription.
         """
 
+    def userCanAlterBugSubscription(subscriber, subscribed_by):
+        """Check if a user can change a bug subscription for a person."""
+
     @operation_parameters(
         subscriber=Reference(
             schema=IPerson,

=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py	2010-07-29 20:16:23 +0000
+++ lib/lp/registry/model/structuralsubscription.py	2010-08-17 14:18:48 +0000
@@ -179,12 +179,35 @@
                 subscribed_by=subscribed_by,
                 **self._target_args)
 
+    def userCanAlterBugSubscription(self, subscriber, subscribed_by):
+        """See `IStructuralSubscriptionTarget`."""
+
+        admins = getUtility(ILaunchpadCelebrities).admin
+        # If the object to be structurally subscribed to for bug
+        # notifications is a distribution and that distribution has a
+        # bug supervisor then only the bug supervisor or a member of
+        # that team or, of course, admins, can subscribe someone to it.
+        if IDistribution.providedBy(self) and self.bug_supervisor is not None:
+            if subscriber is None or subscribed_by is None:
+                return False
+            elif (subscriber != self.bug_supervisor
+                and not subscriber.inTeam(self.bug_supervisor)
+                and not subscribed_by.inTeam(admins)):
+                return False
+        return True
+
     def addBugSubscription(self, subscriber, subscribed_by):
         """See `IStructuralSubscriptionTarget`."""
         # This is a helper method for creating a structural
         # subscription and immediately giving it a full
         # bug notification level. It is useful so long as
         # subscriptions are mainly used to implement bug contacts.
+
+        if not self.userCanAlterBugSubscription(subscriber, subscribed_by):
+            raise UserCannotSubscribePerson(
+                '%s does not have permission to subscribe %s' % (
+                    subscribed_by.name, subscriber.name))
+
         sub = self.addSubscription(subscriber, subscribed_by)
         sub.bug_notification_level = BugNotificationLevel.COMMENTS
         return sub
@@ -239,7 +262,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-08-17 14:18:48 +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("[e-mail]")
-
-    >>> 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-08-17 14:18:48 +0000
@@ -4,12 +4,14 @@
 """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
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.product import IProductSet
@@ -18,7 +20,234 @@
     bugtarget_filebug)
 from canonical.launchpad.testing.systemdocs import (
     LayeredDocFileSuite, setUp, tearDown)
+from canonical.launchpad.webapp.interfaces import ILaunchBag
+from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import LaunchpadFunctionalLayer
+from lp.testing import (
+    ANONYMOUS, login, login_person, TestCaseWithFactory)
+from lp.testing._login import login_celebrity
+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
+        admin = login_celebrity('admin')
+        self.assertIsInstance(
+            self.target.addBugSubscription(self.ordinary_subscriber, admin),
+            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 +256,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 +294,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,