Merge lp:~stevenk/launchpad/structsub-private-bugs-redux into lp:launchpad

[Steve Kowalik]

Filter direct (in one case), indirect, structural and duplicate subscriptions on bugs by those who have an AAG or APG.

Filter indirect, structural and duplicate subscriptions by those who have an AAG or APG. Remove include_master_dupe_subscribers for being pointless and unused.

I have removed a whole swatch of tests for being completely pointless with this change.

[William Grant (wgrant)]

Looking very good, Steve. I have many comments, as always, including a couple of bugs.

152 -The upstream maintainer will be subscribed to security-related private
153 -bugs, because upstream has no security contact, in this case.

This branch is a prerequisite for the work that will make this test obsolete, but I believe it's still relevant. It's talking about an actual BugSubscription here.

170 === modified file 'lib/lp/bugs/doc/security-teams.txt'

Likewise with both changes here. This branch only enables implicit subscriptions -- we still need to test that explicit subscriptions work until we remove them.

201 - def getBugNotificationRecipients(duplicateof=None, old_bug=None,
202 - include_master_dupe_subscribers=False):
203 + def getBugNotificationRecipients(duplicateof=None, old_bug=None):

Nice cleanup of a dead codepath.

278 + def forbidden_recipients_filter(self, column=None):

This actually filters to the allowed recipients, not the forbidden ones. I also wonder if it would be better to return a single expression, which is the Or or True, letting callsites just include it inline without extend()ing. And is it really worth having column= be optional?

337 + all = Select(BugTask.assigneeID, BugTask.bug == self.bug)
338 + assignees = load_people(Person.id.is_in(all))

Why have you split the subquery out into a separate local? Might as well inline it, or at least not shadow a builtin. Also, did you consider inlining the subsequent filter into load_people's where argument?

368 + return subscribers.difference(
369 + self.direct_subscribers_at_all_levels,

Direct subscribers will usually have a grant, but not always (eg. after their project access has been revoked, but the sub removal job hasn't run yet). This needs to be filtered too.

392 + if bug.private:
393 + filters.extend([
394 + Or(*get_bug_privacy_filter_terms(
395 + StructuralSubscription.subscriberID)),
396 + BugTaskFlat.bug == bug])

I'm a little concerned about this. The function didn't previously return nothing for private bugs, so this may be the wrong level to filter at. Did you consider other locations?

424 - def test_structural_bug_supervisor_becomes_direct_on_private(self):
425 - # If a bug supervisor has a structural subscription to the bug, and
426 - # the bug is marked as private, the supervisor should get a direct
427 - # subscription. Otherwise they should be removed, per other tests.

Why is this test no longer valid? Yes, the behaviour should be removed later, but this branch doesn't do it.

448 - def test_information_type_does_not_leak(self):
449 - # Make sure that bug notifications for private bugs do not leak to
450 - # people with a subscription on the product.

This seems like a reasonable integration test to retain. You've added a test that sharing => notification in test_private_bug_with_structural_subscriber_with_access, but unless I've missed one this is the only one that tests !sharing => !notification.

797 - def test_private_bug_target_change_doesnt_add_everyone(self):

Again, I don't see that this test's value is gone.

review: Needs Fixing (code)

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-07-25 18:56:53 +0000
+++ database/schema/security.cfg	2012-07-31 01:59:19 +0000
@@ -1513,8 +1513,10 @@
 [bugnotification]
 groups=script
 public.accessartifact                   = SELECT, INSERT, DELETE
+public.accessartifactgrant              = SELECT
 public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
+public.accesspolicygrant                = SELECT
 public.account                          = SELECT
 public.answercontact                    = SELECT
 public.archive                          = SELECT

=== modified file 'lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt'
--- lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2012-07-27 01:15:04 +0000
+++ lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2012-07-31 01:59:19 +0000
@@ -387,17 +387,6 @@
     >>> filebug_view.added_bug.information_type
     <DBItem InformationType.USERDATA, (4) Private>
 
-Since the bug was marked private before it was filed, only the bug
-reporter has been subscribed to the bug and there should be no indirect
-subscribers.
-
-    >>> for subscriber in filebug_view.added_bug.getDirectSubscribers():
-    ...     print subscriber.displayname
-    No Privileges Person
-
-    >>> filebug_view.added_bug.getIndirectSubscribers()
-    []
-
 The user will be notified that the bug has been marked as private.
 
     >>> print filebug_view.request.response.notifications[2].message
@@ -594,11 +583,6 @@
     Mark Shuttleworth
     No Privileges Person
 
-Since the bug is private, there should be no indirect subscribers.
-
-    >>> filebug_view.added_bug.getIndirectSubscribers()
-    []
-
 The user will be notified that Mark Shuttleworth has been subscribed to
 this bug and that the bug has been marked as private.
 
@@ -718,8 +702,9 @@
     >>> product.setBugSupervisor(owner, owner)
     >>> supervisor_fields = set(filebug_view.field_names)
 
-Privileged users get most of the same fields as normal users, plus a few extra.
-The security_related checkbox is replaced by an information_type radio group.
+Privileged users get most of the same fields as normal users, plus a few
+extra.  The security_related checkbox is replaced by an information_type
+radio group.
 
     >>> normal_fields.remove('security_related')
     >>> owner_fields == supervisor_fields

=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt	2012-07-19 04:40:03 +0000
+++ lib/lp/bugs/doc/bug.txt	2012-07-31 01:59:19 +0000
@@ -169,7 +169,7 @@
 private. A bug cannot be made private by an anonymous user.
 
     >>> from lp.services.webapp.interfaces import ILaunchBag
-    
+
     >>> def current_user():
     ...     return getUtility(ILaunchBag).user
 
@@ -423,11 +423,6 @@
     >>> [subscriber.name for subscriber in private_bug.getDirectSubscribers()]
     [u'name16']
 
-Since it's private, there are no indirect subscribers.
-
-    >>> private_bug.getIndirectSubscribers()
-    []
-
 It's up to the submitter to subscribe the maintainer, if they so choose.
 
 This works similarly for distributions; in this case the
@@ -449,8 +444,6 @@
     >>> private_bug = bugset.get(added_bug.id)
     >>> [subscriber.name for subscriber in private_bug.getDirectSubscribers()]
     [u'name16']
-    >>> private_bug.getIndirectSubscribers()
-    []
 
 
 Prevent reporter from being subscribed to filed bugs
@@ -1084,10 +1077,10 @@
 The thunderbird project does not use Launchpad to track bugs.
 Incomplete, unattended bug reports cannot ever expire for this project.
 
-    >>> # create_old_bug creates an bug with a bugtask that is eligible for
-    >>> # expiration, so long as the pillar object has enabled bug expiration.
-    >>> # Every change to a bug or bugtask must be synced back to the
-    >>> # database to test can_expire.
+create_old_bug creates an bug with a bugtask that is eligible for expiration,
+so long as the pillar object has enabled bug expiration. Every change to a
+bug or bugtask must be synced back to the database to test can_expire.
+
     >>> from lp.bugs.tests.bug import create_old_bug
 
     >>> upstream_bugtask = create_old_bug('bug a', 1, thunderbird)

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt	2012-07-27 01:15:04 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt	2012-07-31 01:59:19 +0000
@@ -400,18 +400,6 @@
     Mark Shuttleworth
     Robert Collins
 
-A private bug never has indirect subscribers. Let's add an indirect subscriber
-to show that they still aren't included in the indirect subscriptions.
-
-    >>> linux_source_bug.bugtasks[0].transitionToAssignee(
-    ...     personset.getByName("martin-pitt"))
-
-    >>> linux_source_bug.getIndirectSubscribers()
-    []
-
-    >>> linux_source_bug.getSubscribersFromDuplicates()
-    ()
-
 Direct subscriptions always take precedence over indirect subscriptions.
 So, if we unmark the above bug as private, indirect_subscribers will include
 any subscribers not converted to direct subscribers.
@@ -425,14 +413,12 @@
     Robert Collins
 
     >>> print_displayname(linux_source_bug.getIndirectSubscribers())
-    Martin Pitt
     No Privileges Person
     Sample Person
     Scott James Remnant
     Ubuntu Team
 
     >>> print_displayname(linux_source_bug.getAlsoNotifiedSubscribers())
-    Martin Pitt
     No Privileges Person
     Sample Person
     Scott James Remnant
@@ -786,8 +772,11 @@
     ...     owner=foobar, information_type=InformationType.PRIVATESECURITY)
     >>> new_bug = firefox.createBug(params)
 
-    >>> getSubscribers(new_bug)
-    ['[e-mail]', '[e-mail]']
+    >>> print '\n'.join(getSubscribers(new_bug))
+    [e-mail]
+    [e-mail]
+    [e-mail]
+    [e-mail]
 
 Now let's create a bug on a specific package, which has no package bug
 contacts:

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2012-07-30 00:05:44 +0000
+++ lib/lp/bugs/interfaces/bug.py	2012-07-31 01:59:19 +0000
@@ -518,8 +518,7 @@
             `BugSubscriptionLevel.LIFECYCLE` if unspecified.
         """
 
-    def getBugNotificationRecipients(duplicateof=None, old_bug=None,
-                                     include_master_dupe_subscribers=False):
+    def getBugNotificationRecipients(duplicateof=None, old_bug=None):
         """Return a complete INotificationRecipientSet instance.
 
         The INotificationRecipientSet instance will contain details of
@@ -527,8 +526,6 @@
         includes email addresses and textual and header-ready
         rationales. See `BugNotificationRecipients` for
         details of this implementation.
-        If this bug is a dupe, set include_master_dupe_subscribers to
-        True to include the master bug's subscribers as recipients.
         """
 
     def canBeAQuestion():

=== modified file 'lib/lp/bugs/mail/bugnotificationrecipients.py'
--- lib/lp/bugs/mail/bugnotificationrecipients.py	2012-07-27 01:36:30 +0000
+++ lib/lp/bugs/mail/bugnotificationrecipients.py	2012-07-31 01:59:19 +0000
@@ -56,7 +56,7 @@
         duplicateof parameter above and the addDupeSubscriber method.
         Don't confuse them!
         """
-        NotificationRecipientSet.__init__(self)
+        super(BugNotificationRecipients, self).__init__()
         self.duplicateof = duplicateof
         self.subscription_filters = set()
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-30 00:05:44 +0000
+++ lib/lp/bugs/model/bug.py	2012-07-31 01:59:19 +0000
@@ -166,8 +166,6 @@
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
     IAccessArtifactSource,
-    IAccessPolicyGrantSource,
-    IAccessPolicySource,
     )
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
@@ -979,7 +977,8 @@
         """See `IBug`."""
         return self.getSubscriptionInfo().direct_subscriptions
 
-    def getDirectSubscribers(self, recipients=None, level=None):
+    def getDirectSubscribers(self, recipients=None, level=None,
+                             filter_visible=False):
         """See `IBug`.
 
         The recipients argument is private and not exposed in the
@@ -991,6 +990,13 @@
             level = BugNotificationLevel.LIFECYCLE
         direct_subscribers = (
             self.getSubscriptionInfo(level).direct_subscribers)
+        if filter_visible:
+            filtered_subscribers = IStore(Person).find(Person,
+                Person.id.is_in([s.id for s in direct_subscribers]),
+                self.getSubscriptionInfo().visible_recipients_filter(
+                    Person.id))
+            direct_subscribers = BugSubscriberSet(
+                direct_subscribers.intersection(filtered_subscribers))
         if recipients is not None:
             for subscriber in direct_subscribers:
                 recipients.addDirectSubscriber(subscriber)
@@ -1123,27 +1129,13 @@
         return get_also_notified_subscribers(self, recipients, level)
 
     def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
-                                     level=None,
-                                     include_master_dupe_subscribers=False):
+                                     level=None):
         """See `IBug`."""
         recipients = BugNotificationRecipients(duplicateof=duplicateof)
-        self.getDirectSubscribers(recipients, level=level)
-        if self.private:
-            assert self.getIndirectSubscribers() == [], (
-                "Indirect subscribers found on private bug. "
-                "A private bug should never have implicit subscribers!")
-        else:
-            self.getIndirectSubscribers(recipients, level=level)
-            if include_master_dupe_subscribers and self.duplicateof:
-                # This bug is a public duplicate of another bug, so include
-                # the dupe target's subscribers in the recipient list. Note
-                # that we only do this for duplicate bugs that are public;
-                # changes in private bugs are not broadcast to their dupe
-                # targets.
-                dupe_recipients = (
-                    self.duplicateof.getBugNotificationRecipients(
-                        duplicateof=self.duplicateof, level=level))
-                recipients.update(dupe_recipients)
+        self.getDirectSubscribers(
+            recipients, level=level, filter_visible=True)
+        self.getIndirectSubscribers(recipients, level=level)
+
         # XXX Tom Berger 2008-03-18:
         # We want to look up the recipients for `old_bug` too,
         # but for this to work, this code has to move out of the
@@ -2241,9 +2233,6 @@
     else:
         raise ValueError('First argument must be bug or bugtask')
 
-    if bug.private:
-        return []
-
     # Subscribers to exclude.
     exclude_subscribers = frozenset().union(
         info.direct_subscribers_at_all_levels, info.muted_subscribers)
@@ -2452,6 +2441,17 @@
         muted_people = Select(BugMute.person_id, BugMute.bug == self.bug)
         return load_people(Person.id.is_in(muted_people))
 
+    def visible_recipients_filter(self, column):
+        # Circular fail :(
+        from lp.bugs.model.bugtaskflat import BugTaskFlat
+        from lp.bugs.model.bugtasksearch import get_bug_privacy_filter_terms
+
+        if self.bug.private:
+            return [Or(*get_bug_privacy_filter_terms(column)),
+                BugTaskFlat.bug_id == self.bug.id]
+        else:
+            return True
+
     @cachedproperty
     @freeze(BugSubscriptionSet)
     def direct_subscriptions(self):
@@ -2496,21 +2496,20 @@
     def duplicate_subscriptions(self):
         """Subscriptions to duplicates of the bug.
 
-        Excludes muted subscriptions.
+        Excludes muted subscriptions, and subscribers who can not see the
+        master bug.
         """
-        if self.bug.private:
-            return ()
-        else:
-            return IStore(BugSubscription).find(
-                BugSubscription,
-                BugSubscription.bug_notification_level >= self.level,
-                BugSubscription.bug_id == Bug.id,
-                Bug.duplicateof == self.bug,
-                Not(In(
-                    BugSubscription.person_id,
-                    Select(
-                        BugMute.person_id, BugMute.bug_id == Bug.id,
-                        tables=[BugMute]))))
+        return IStore(BugSubscription).find(
+            BugSubscription,
+            BugSubscription.bug_notification_level >= self.level,
+            BugSubscription.bug_id == Bug.id,
+            Bug.duplicateof == self.bug,
+            Not(In(
+                BugSubscription.person_id,
+                Select(
+                    BugMute.person_id, BugMute.bug_id == Bug.id,
+                    tables=[BugMute]))),
+            self.visible_recipients_filter(BugSubscription.person_id))
 
     @property
     def duplicate_subscribers(self):
@@ -2571,22 +2570,26 @@
         *Does not* exclude muted subscribers.
         """
         if self.bugtask is None:
-            assignees = Select(BugTask.assigneeID, BugTask.bug == self.bug)
-            return load_people(Person.id.is_in(assignees))
-        else:
-            return load_people(Person.id == self.bugtask.assigneeID)
+            assignees = load_people(
+                Person.id.is_in(Select(BugTask.assigneeID,
+                    BugTask.bug == self.bug)))
+        else:
+            assignees = load_people(Person.id == self.bugtask.assigneeID)
+        if self.bug.private:
+            return IStore(Person).find(Person,
+                Person.id.is_in([a.id for a in assignees]),
+                self.visible_recipients_filter(Person.id))
+        else:
+            return assignees
 
     @cachedproperty
     def also_notified_subscribers(self):
         """All subscribers except direct, dupe, and muted subscribers."""
-        if self.bug.private:
-            return BugSubscriberSet()
-        else:
-            subscribers = BugSubscriberSet().union(
-                self.structural_subscribers, self.all_assignees)
-            return subscribers.difference(
-                self.direct_subscribers_at_all_levels,
-                self.muted_subscribers)
+        subscribers = BugSubscriberSet().union(
+            self.structural_subscribers, self.all_assignees)
+        return subscribers.difference(
+            self.direct_subscribers_at_all_levels,
+            self.muted_subscribers)
 
     @cachedproperty
     def indirect_subscribers(self):

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2012-06-14 05:18:22 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2012-07-31 01:59:19 +0000
@@ -702,6 +702,9 @@
     :param direct_subscribers: a collection of Person objects who are
                                directly subscribed to the bug.
     """
+    # Circular. :-(
+    from lp.bugs.model.bugtaskflat import BugTaskFlat
+    from lp.bugs.model.bugtasksearch import get_bug_privacy_filter_terms
     # We get the ids because we need to use group by in order to
     # look at the filters' tags in aggregate.  Once we have the ids,
     # we can get the full set of what we need in subsuming or
@@ -738,6 +741,11 @@
             Not(In(StructuralSubscription.subscriberID,
                    Select(BugSubscription.person_id,
                           BugSubscription.bug == bug))))
+    if bug.private:
+        filters.extend([
+            Or(*get_bug_privacy_filter_terms(
+                StructuralSubscription.subscriberID)),
+            BugTaskFlat.bug == bug])
     candidates = list(_get_structural_subscriptions(
         StructuralSubscription.id, query_arguments, *filters))
     if not candidates:

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2012-07-26 05:18:32 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2012-07-31 01:59:19 +0000
@@ -891,25 +891,6 @@
             bug.setPrivate(True, bug.owner)
         self.assertEqual(InformationType.USERDATA, bug.information_type)
 
-    def test_information_type_does_not_leak(self):
-        # Make sure that bug notifications for private bugs do not leak to
-        # people with a subscription on the product.
-        product = self.factory.makeProduct()
-        with person_logged_in(product.owner):
-            product.addSubscription(product.owner, product.owner)
-        reporter = self.factory.makePerson()
-        bug = self.factory.makeBug(
-            information_type=InformationType.USERDATA, product=product,
-            owner=reporter)
-        recipients = Store.of(bug).using(
-            BugNotificationRecipient,
-            Join(BugNotification, BugNotification.bugID == bug.id)).find(
-            BugNotificationRecipient,
-            BugNotificationRecipient.bug_notificationID ==
-                BugNotification.id)
-        self.assertEqual(
-            [reporter], [recipient.person for recipient in recipients])
-
     def test__reconcileAccess_handles_all_targets(self):
         # _reconcileAccess gets the pillar from any task
         # type.

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-07-27 01:15:04 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-07-31 01:59:19 +0000
@@ -273,18 +273,6 @@
         self.assertContentEqual(
             [duplicate_bug_subscription], found_subscriptions)
 
-    def test_duplicate_for_private_bug(self):
-        # The set of subscribers from duplicate bugs is always empty when the
-        # master bug is private.
-        duplicate_bug = self.factory.makeBug(product=self.target)
-        with person_logged_in(duplicate_bug.owner):
-            duplicate_bug.markAsDuplicate(self.bug)
-        with person_logged_in(self.bug.owner):
-            self.bug.setPrivate(True, self.bug.owner)
-        found_subscriptions = self.getInfo().duplicate_subscriptions
-        self.assertContentEqual([], found_subscriptions)
-        self.assertContentEqual([], found_subscriptions.subscribers)
-
     def test_duplicate_only(self):
         # The set of duplicate subscriptions where the subscriber has no other
         # subscriptions.
@@ -430,33 +418,6 @@
         found_subscribers = self.getInfo().also_notified_subscribers
         self.assertContentEqual([], found_subscribers)
 
-    def test_also_notified_subscribers_for_private_bug(self):
-        # The set of also notified subscribers is always empty when the master
-        # bug is private.
-        assignee = self.factory.makePerson()
-        bugtask = self.bug.default_bugtask
-        with person_logged_in(bugtask.pillar.bug_supervisor):
-            bugtask.transitionToAssignee(assignee)
-        with person_logged_in(self.bug.owner):
-            self.bug.setPrivate(True, self.bug.owner)
-        found_subscribers = self.getInfo().also_notified_subscribers
-        self.assertContentEqual([], found_subscribers)
-
-    def test_indirect_subscribers(self):
-        # The set of indirect subscribers is the union of also notified
-        # subscribers and subscribers to duplicates.
-        assignee = self.factory.makePerson()
-        bugtask = self.bug.default_bugtask
-        with person_logged_in(bugtask.pillar.bug_supervisor):
-            bugtask.transitionToAssignee(assignee)
-        duplicate_bug = self.factory.makeBug(product=self.target)
-        with person_logged_in(duplicate_bug.owner):
-            duplicate_bug.markAsDuplicate(self.bug)
-        found_subscribers = self.getInfo().indirect_subscribers
-        self.assertContentEqual(
-            [assignee, duplicate_bug.owner],
-            found_subscribers)
-
 
 class TestBugSubscriptionInfoPermissions(TestCaseWithFactory):
 
@@ -584,7 +545,7 @@
         self.make_duplicate_bug()
         with person_logged_in(self.bug.owner):
             self.bug.setPrivate(True, self.bug.owner)
-        with self.exactly_x_queries(0):
+        with self.exactly_x_queries(1):
             self.info.duplicate_subscriptions
         with self.exactly_x_queries(0):
             self.info.duplicate_subscriptions.subscribers

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2012-05-08 17:04:43 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2012-07-31 01:59:19 +0000
@@ -313,8 +313,7 @@
         # Construct the real notification with recipients.
         bug = notification.bug
         recipients = bug.getBugNotificationRecipients(
-            level=BugNotificationLevel.LIFECYCLE,
-            include_master_dupe_subscribers=False)
+            level=BugNotificationLevel.LIFECYCLE)
         message = notification.message
         is_comment = notification.is_comment
         activity = notification.activity

=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2012-05-08 01:06:31 +0000
+++ lib/lp/bugs/subscribers/bug.py	2012-07-31 01:59:19 +0000
@@ -156,12 +156,11 @@
             level=BugNotificationLevel.METADATA)
         recipients.update(old_bugtask_recipients)
     for change in changes:
+        bug = bug_delta.bug
         if isinstance(change, BugDuplicateChange):
-            no_dupe_master_recipients = (
-                bug_delta.bug.getBugNotificationRecipients(
-                    old_bug=bug_delta.bug_before_modification,
-                    level=change.change_level,
-                    include_master_dupe_subscribers=False))
+            no_dupe_master_recipients = bug.getBugNotificationRecipients(
+                old_bug=bug_delta.bug_before_modification,
+                level=change.change_level)
             bug_delta.bug.addChange(
                 change, recipients=no_dupe_master_recipients)
         elif (isinstance(change, BugTaskAssigneeChange) and
@@ -173,11 +172,9 @@
             bug_delta.bug.addChange(change, recipients=recipients)
         else:
             if change.change_level == BugNotificationLevel.LIFECYCLE:
-                change_recipients = (
-                    bug_delta.bug.getBugNotificationRecipients(
-                        old_bug=bug_delta.bug_before_modification,
-                        level=change.change_level,
-                        include_master_dupe_subscribers=False))
+                change_recipients = bug.getBugNotificationRecipients(
+                    old_bug=bug_delta.bug_before_modification,
+                    level=change.change_level)
                 recipients.update(change_recipients)
             # Additionally, if we are re-targetting a bugtask for a private
             # bug, we need to ensure the new bug supervisor and maintainer are

=== modified file 'lib/lp/bugs/tests/test_bug_notification_recipients.py'
--- lib/lp/bugs/tests/test_bug_notification_recipients.py	2012-07-24 05:43:46 +0000
+++ lib/lp/bugs/tests/test_bug_notification_recipients.py	2012-07-31 01:59:19 +0000
@@ -5,7 +5,13 @@
 
 __metaclass__ = type
 
+from zope.component import getUtility
+
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessPolicySource,
+    )
 from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
@@ -59,6 +65,7 @@
             bug.getBugNotificationRecipients())
 
     def test_private_bug(self):
+        # Only the owner is notified about a private bug.
         owner = self.factory.makePerson()
         bug = self.factory.makeBug(
             owner=owner, information_type=InformationType.USERDATA)
@@ -67,6 +74,7 @@
                 [owner], bug.getBugNotificationRecipients())
 
     def test_private_bug_with_subscriber(self):
+        # Subscribing a user to a bug grants access, so they will be notified.
         owner = self.factory.makePerson()
         subscriber = self.factory.makePerson()
         bug = self.factory.makeBug(
@@ -76,7 +84,23 @@
             self.assertContentEqual(
                 [owner, subscriber], bug.getBugNotificationRecipients())
 
+    def test_private_bug_with_subscriber_without_access(self):
+        # A subscriber without access to a private bug isn't notified.
+        owner = self.factory.makePerson()
+        subscriber = self.factory.makePerson()
+        bug = self.factory.makeBug(
+            owner=owner, information_type=InformationType.USERDATA)
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
+        with person_logged_in(owner):
+            bug.subscribe(subscriber, owner)
+            getUtility(IAccessArtifactGrantSource).revokeByArtifact(
+                [artifact], [subscriber])
+            self.assertContentEqual(
+                [owner], bug.getBugNotificationRecipients())
+
     def test_private_bug_with_structural_subscriber(self):
+        # A structural subscriber without access does not get notified about
+        # a private bug.
         owner = self.factory.makePerson()
         subscriber = self.factory.makePerson()
         product = self.factory.makeProduct()
@@ -89,7 +113,26 @@
             self.assertContentEqual(
                 [owner], bug.getBugNotificationRecipients())
 
+    def test_private_bug_with_structural_subscriber_with_access(self):
+        # When a structural subscriber has access to a private bug, they are
+        # notified.
+        owner = self.factory.makePerson()
+        subscriber = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        with person_logged_in(subscriber):
+            product.addBugSubscription(subscriber, subscriber)
+        policy = getUtility(IAccessPolicySource).find(
+            [(product, InformationType.USERDATA)]).one()
+        self.factory.makeAccessPolicyGrant(policy=policy, grantee=subscriber)
+        bug = self.factory.makeBug(
+            product=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        with person_logged_in(owner):
+            self.assertContentEqual(
+                [owner, subscriber], bug.getBugNotificationRecipients())
+
     def test_private_bug_assignee(self):
+        # Assigning a user to a private bug does not give them visibility.
         owner = self.factory.makePerson()
         assignee = self.factory.makePerson()
         bug = self.factory.makeBug(
@@ -99,7 +142,22 @@
             self.assertContentEqual(
                 [owner], bug.getBugNotificationRecipients())
 
+    def test_private_bug_assignee_with_access(self):
+        # An assignee with access will get notified.
+        owner = self.factory.makePerson()
+        assignee = self.factory.makePerson()
+        bug = self.factory.makeBug(
+            owner=owner, information_type=InformationType.USERDATA)
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
+        self.factory.makeAccessArtifactGrant(
+            artifact=artifact, grantee=assignee)
+        with person_logged_in(owner):
+            bug.default_bugtask.transitionToAssignee(assignee)
+            self.assertContentEqual(
+                [owner, assignee], bug.getBugNotificationRecipients())
+
     def test_private_bug_with_duplicate_subscriber(self):
+        # A subscriber to a duplicate of a private bug will not be notified.
         owner = self.factory.makePerson()
         subscriber = self.factory.makePerson()
         bug = self.factory.makeBug(
@@ -110,3 +168,20 @@
             dupe.markAsDuplicate(bug)
             self.assertContentEqual(
                 [owner], bug.getBugNotificationRecipients())
+
+    def test_private_bug_with_duplicate_subscriber_with_access(self):
+        # A subscriber to a duplicate of a private bug will be notified, if
+        # they have access.
+        owner = self.factory.makePerson()
+        subscriber = self.factory.makePerson()
+        bug = self.factory.makeBug(
+            owner=owner, information_type=InformationType.USERDATA)
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
+        self.factory.makeAccessArtifactGrant(
+            artifact=artifact, grantee=subscriber)
+        dupe = self.factory.makeBug(owner=owner)
+        with person_logged_in(owner):
+            dupe.subscribe(subscriber, owner)
+            dupe.markAsDuplicate(bug)
+            self.assertContentEqual(
+                [owner, subscriber], bug.getBugNotificationRecipients())

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2012-07-19 04:40:03 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2012-07-31 01:59:19 +0000
@@ -31,7 +31,6 @@
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     api_url,
-    celebrity_logged_in,
     launchpadlib_for,
     login_person,
     person_logged_in,
@@ -1046,83 +1045,6 @@
             expected_activity=expected_activity,
             expected_notification=expected_notification)
 
-    def _test_retarget_private_security_bug_to_product(self,
-                                                       bug, maintainer,
-                                                       bug_supervisor=None):
-        # When a private security related bug has a bugtask retargetted to a
-        # different product, a notification is sent to the new bug supervisor
-        # and maintainer. If they are the same person, only one notification
-        # is sent. They only get notifications if they can see the bug.
-
-        # Create the private bug.
-        bug_task = bug.bugtasks[0]
-        new_target = self.factory.makeProduct(
-            owner=maintainer, bug_supervisor=bug_supervisor)
-        self.saveOldChanges(bug)
-
-        bug_task_before_modification = Snapshot(
-            bug_task, providing=providedBy(bug_task))
-        bug_task.transitionToTarget(new_target, self.user)
-        notify(ObjectModifiedEvent(
-            bug_task, bug_task_before_modification,
-            ['target', 'product'], user=self.user))
-
-        expected_activity = {
-            'person': self.user,
-            'whatchanged': 'affects',
-            'oldvalue': bug_task_before_modification.bugtargetname,
-            'newvalue': bug_task.bugtargetname,
-            }
-
-        expected_recipients = [self.user]
-        expected_reasons = ['Subscriber']
-        if bug.userCanView(maintainer):
-            expected_recipients.append(maintainer)
-            expected_reasons.append('Maintainer')
-        if (bug_supervisor and not bug_supervisor.inTeam(maintainer)
-                and bug.userCanView(bug_supervisor)):
-            expected_recipients.append(bug_supervisor)
-            expected_reasons.append('Bug Supervisor')
-        expected_notification = {
-            'text': u"** Project changed: %s => %s" % (
-                bug_task_before_modification.bugtargetname,
-                bug_task.bugtargetname),
-            'person': self.user,
-            'recipients': expected_recipients,
-            'recipient_reasons': expected_reasons
-            }
-        self.assertRecordedChange(
-            expected_activity=expected_activity,
-            expected_notification=expected_notification, bug=bug)
-
-    def test_retarget_private_security_bug_to_product(self):
-        # A series of tests for re-targetting a private bug task.
-        bug = self.factory.makeBug(
-            product=self.product, owner=self.user,
-            information_type=InformationType.USERDATA)
-        maintainer = self.factory.makePerson()
-        bug_supervisor = self.factory.makePerson()
-
-        # Test with no bug supervisor
-        self._test_retarget_private_security_bug_to_product(bug, maintainer)
-        # Test with bug supervisor = maintainer.
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, maintainer)
-        # Test with different bug supervisor
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, bug_supervisor)
-
-        # Now make the bug visible to the bug supervisor and re-test.
-        with celebrity_logged_in('admin'):
-            bug.default_bugtask.transitionToAssignee(bug_supervisor)
-
-        # Test with bug supervisor = maintainer.
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, maintainer)
-        # Test with different bug supervisor
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, bug_supervisor)
-
     def test_target_bugtask_to_sourcepackage(self):
         # When a bugtask's target is changed, BugActivity and
         # BugNotification get updated.

=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py	2012-06-19 02:14:21 +0000
+++ lib/lp/registry/model/accesspolicy.py	2012-07-31 01:59:19 +0000
@@ -334,7 +334,7 @@
 
     @classmethod
     def revokeByArtifact(cls, artifacts, grantees=None):
-        """See `IAccessPolicyGrantSource`."""
+        """See `IAccessArtifactGrantSource`."""
         cls.findByArtifact(artifacts, grantees).remove()
 
 

=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2012-07-19 04:40:03 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-07-31 01:59:19 +0000
@@ -19,7 +19,6 @@
 from lp.registry.enums import InformationType
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
-    IAccessArtifactSource,
     IAccessPolicySource,
     )
 from lp.registry.interfaces.person import TeamSubscriptionPolicy
@@ -182,9 +181,9 @@
         job, job_type = create_job(distro, bug, grantee, owner)
         # Subscribing grantee has created an artifact grant so we need to
         # revoke that to test the job.
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
-            getUtility(IAccessArtifactSource).find(
-                [bug]), [grantee])
+            [artifact], [grantee])
         transaction.commit()
 
         out, err, exit_code = run_script(
@@ -295,9 +294,9 @@
             CodeReviewNotificationLevel.NOEMAIL, owner)
         # Subscribing policy_team_grantee has created an artifact grant so we
         # need to revoke that to test the job.
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
-            getUtility(IAccessArtifactSource).find(
-                [bug]), [policy_team_grantee])
+            [artifact], [policy_team_grantee])
 
         # policy grantees are subscribed because the job has not been run yet.
         subscribers = removeSecurityProxy(bug).getDirectSubscribers()
@@ -367,9 +366,9 @@
         bug.subscribe(artifact_indirect_grantee, owner)
         # Subscribing policy_team_grantee has created an artifact grant so we
         # need to revoke that to test the job.
+        artifact = self.factory.makeAccessArtifact(concrete=branch)
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
-            getUtility(IAccessArtifactSource).find(
-                [branch]), [policy_team_grantee])
+            [artifact], [policy_team_grantee])
 
         # policy grantees are subscribed because the job has not been run yet.
         #subscribers = removeSecurityProxy(branch).subscribers
@@ -432,10 +431,9 @@
             bug.subscribe(grantee, owner)
         # Subscribing grantee to bug creates an access grant so we need to
         # revoke that for our test.
-        accessartifact_source = getUtility(IAccessArtifactSource)
-        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
-        accessartifact_grant_source.revokeByArtifact(
-            accessartifact_source.find([bug]), [grantee])
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
+        getUtility(IAccessArtifactGrantSource).revokeByArtifact(
+            [artifact], [grantee])
 
         return bug, owner