launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/remove-bad-subscribers into lp:launchpad.

Requested reviews:
  Curtis Hovey (sinzui)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/remove-bad-subscribers/+merge/117771

Summary
=======
Currently, if a bug is transitioned to a private status, then all direct
subscribers are given access to the private bug. We really only want those who
have the appropriate APG or AAG, as well as those in some special roles to
have access.

This updates the code to ensure only those special roles are ensured access,
and that subscriptions are handled appropriately.

Preimp
======
Spoke with Purple Squad.

Implementation
==============
Missing subscribers has been updated to required_subscribers; this is because
they are the only people required after transition.

The order of events has been updated--the information type is changed and the
access grants are reconciled before moving forward, so that we can rely on the
loss of access as a means of determining who needs to be removed.

Instead of all subscribers without access being granted access, only those who
are in required subscribers are given access.

Because all subscribers are not now given access, if the unsubscribe job
feature flag is not set, we manually remove the subscribers. This can be
removed once we no longer have the flag on the creation of the job.

Tests
=====
bin/test -vvcm lp.bugs.model.tests.test_bug

QA
==
Switch a bug to private; make sure only the expected parties still have
access/are subscribed.

LoC
===
This is part of disclosure.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/tests/test_bug.py
  lib/lp/bugs/model/bug.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/remove-bad-subscribers/+merge/117771
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-31 22:13:45 +0000
+++ lib/lp/bugs/model/bug.py	2012-08-01 19:56:20 +0000
@@ -1748,70 +1748,79 @@
         if information_type in PRIVATE_INFORMATION_TYPES:
             self.who_made_private = who
             self.date_made_private = UTC_NOW
-            missing_subscribers = set([who, self.owner])
+            required_subscribers = set([who, self.owner])
         else:
             self.who_made_private = None
             self.date_made_private = None
-            missing_subscribers = set()
+            required_subscribers = set()
         # XXX: This should be a bulk update. RBC 20100827
         # bug=https://bugs.launchpad.net/storm/+bug/625071
         for attachment in self.attachments_unpopulated:
             attachment.libraryfile.restricted = (
                 information_type in PRIVATE_INFORMATION_TYPES)
 
-        # There are several people we need to ensure are subscribed.
-        # If the information type is userdata, we need to check for bug
-        # supervisors who aren't subscribed and should be. If there is no
-        # bug supervisor, we need to subscribe the maintainer.
+        self.information_type = information_type
+        self._reconcileAccess()
+
         pillars = self.affected_pillars
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        subscribers = self.getDirectSubscribers()
+
+        # We have to capture subscribers that must exist after transition. In
+        # the case of a transition to USERDATA, we want the bug supervisor or
+        # maintainer. For SECURITY types, we want the security contact or
+        # maintainer. In either case, if the driver is already subscribed,
+        # then the driver is also required.
+        # Ubuntu is special: we don't want to add required subscribers in that
+        # case.
         if information_type == InformationType.USERDATA:
-            ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
             for pillar in pillars:
-                # Ubuntu is special cased; no one else should be added in the
-                # USERDATA case.
+                if pillar.driver in subscribers:
+                    required_subscribers.add(pillar.driver)
                 if pillar != ubuntu:
                     if pillar.bug_supervisor is not None:
-                        missing_subscribers.add(pillar.bug_supervisor)
+                        required_subscribers.add(pillar.bug_supervisor)
                     else:
-                        missing_subscribers.add(pillar.owner)
+                        required_subscribers.add(pillar.owner)
 
-        # If the information type is security related, we need to ensure
-        # the security contacts are subscribed. If there is no security
-        # contact, we need to subscribe the maintainer.
         if information_type in SECURITY_INFORMATION_TYPES:
             for pillar in pillars:
+                if pillar.driver in subscribers:
+                    required_subscribers.add(pillar.driver)
                 if pillar.security_contact is not None:
-                    missing_subscribers.add(pillar.security_contact)
+                    required_subscribers.add(pillar.security_contact)
                 else:
-                    missing_subscribers.add(pillar.owner)
-
-        for s in missing_subscribers:
-            # Don't subscribe someone if they're already subscribed via a
-            # team.
-            already_subscribed_teams = self.getSubscribersForPerson(s)
-            if already_subscribed_teams.is_empty():
-                self.subscribe(s, who)
-
-        self.information_type = information_type
-        self._reconcileAccess()
-
-        # If the transition makes the bug private, then we need to ensure all
-        # subscribers can see the bug. For any who can't, we create an access
-        # artifact grant. Note that the previous value of information_type may
-        # also have been private but we still need to perform the access check
-        # since any access policy grants will not confer access with the new
-        # information type value.
+                    required_subscribers.add(pillar.owner)
+
+        # If we've made the bug private, we need to do some cleanup.
+        # Required subscribers must be given access.
+        # People without existing access who aren't required should be
+        # unsubscribed. Even if we're transitioning from one private type to
+        # another, we must do this check, as different policies are granted to
+        # different users/teams.
         if information_type in PRIVATE_INFORMATION_TYPES:
             # Grant the subscriber access if they can't see the bug.
-            subscribers = self.getDirectSubscribers()
             if subscribers:
                 service = getUtility(IService, 'sharing')
-                blind_subscribers = service.getPeopleWithoutAccess(
-                    self, subscribers)
-                if len(blind_subscribers):
+                subscribers_to_remove = set(service.getPeopleWithoutAccess(
+                    self, subscribers)).difference(required_subscribers)
+                if len(required_subscribers):
                     service.ensureAccessGrants(
-                        blind_subscribers, who, bugs=[self],
+                        required_subscribers, who, bugs=[self],
                         ignore_permissions=True)
+                # There is a job to do the unsubscribe, but it's behind a
+                # flag. If that flag is not set, do it manually.
+                if len(subscribers_to_remove) and not bool(
+                    getFeatureFlag('disclosure.unsubscribe_jobs.enabled')):
+                    for s in subscribers_to_remove:
+                        self.unsubscribe(s, who, ignore_permissions=True)
+
+        # Add the required subscribers, but not if they are all already
+        # subscribed via a team.
+        for s in required_subscribers:
+            already_subscribed_teams = self.getSubscribersForPerson(s)
+            if already_subscribed_teams.is_empty():
+                self.subscribe(s, who)
 
         self.updateHeat()
 

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2012-07-31 00:07:02 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2012-08-01 19:56:20 +0000
@@ -9,14 +9,12 @@
     )
 
 from pytz import UTC
-from storm.expr import Join
 from storm.store import Store
 from testtools.testcase import ExpectedException
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
-from lp.app.interfaces.services import IService
 from lp.bugs.adapters.bugchange import BugTitleChange
 from lp.bugs.enums import (
     BugNotificationLevel,
@@ -30,7 +28,6 @@
     BugNotification,
     BugSubscriptionInfo,
     )
-from lp.bugs.model.bugnotification import BugNotificationRecipient
 from lp.registry.enums import InformationType
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactSource,
@@ -586,10 +583,13 @@
     def test_setPrivate_does_not_subscribe_member_of_subscribed_team(self):
         # When setPrivate(True) is called on a bug, the person who is
         # marking the bug private will not be subscribed if they're
-        # already a member of a team which is a direct subscriber.
+        # already a member of a team which is a direct subscriber and
+        # maintains access after transition.
         bug = self.factory.makeBug()
         person = self.factory.makePerson(name='teamowner')
         team = self.factory.makeTeam(owner=person, name='team')
+        artifact = self.factory.makeAccessArtifact(bug)
+        self.factory.makeAccessArtifactGrant(artifact, team)
         with person_logged_in(person):
             bug.subscribe(team, person)
             bug.setPrivate(True, person)
@@ -654,13 +654,11 @@
             bug.transitionToInformationType(
                 InformationType.PRIVATESECURITY, who=who)
             subscribers = bug.getDirectSubscribers()
-            initial_subscribers.update(bug.getDirectSubscribers())
         expected_subscribers = set((
-            bugtask_a.owner,
             default_bugtask.pillar.driver,
             default_bugtask.pillar.security_contact,
-            bug_owner, who))
-        expected_subscribers.update(initial_subscribers)
+            bug_owner,
+            who))
         self.assertContentEqual(expected_subscribers, subscribers)
 
     def test_transition_to_USERDATA_information_type(self):
@@ -672,7 +670,7 @@
         # - and bug/pillar owners, drivers if they are already subscribed
 
         (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
-            self.createBugTasksAndSubscribers(private_security_related=True))
+                self.createBugTasksAndSubscribers())
         initial_subscribers = set((
             self.factory.makePerson(name='subscriber'), bug_owner,
             bugtask_a.pillar.security_contact, bugtask_a.pillar.driver))
@@ -686,8 +684,8 @@
         expected_subscribers = set((
             default_bugtask.pillar.bug_supervisor,
             default_bugtask.pillar.driver,
-            bug_owner, who))
-        expected_subscribers.update(initial_subscribers)
+            bug_owner,
+            who))
         self.assertContentEqual(expected_subscribers, subscribers)
 
     def test_transition_to_PUBLICSECURITY_information_type(self):
@@ -739,23 +737,6 @@
             subscribers_before_public,
             subscribers_after_public)
 
-    def test_transition_to_private_grants_subscribers_access(self):
-        # When a bug is made private, any direct subscribers should be granted
-        # access.
-        (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
-            self.createBugTasksAndSubscribers())
-        some_person = self.factory.makePerson()
-        with person_logged_in(bug_owner):
-            bug.subscribe(some_person, bug_owner)
-            subscribers = bug.getDirectSubscribers()
-            who = self.factory.makePerson(name='who')
-            bug.transitionToInformationType(
-                InformationType.USERDATA, who)
-
-        service = getUtility(IService, 'sharing')
-        peopleWithoutAccess = service.getPeopleWithoutAccess(bug, subscribers)
-        self.assertContentEqual([], peopleWithoutAccess)
-
     def test_setPillarOwnerSubscribedIfNoBugSupervisor(self):
         # The pillar owner is subscribed if the bug supervisor is not set and
         # the bug is marked as USERDATA.