launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/RemoveGranteeSubscriptionsJob-admins-1009283 into lp:launchpad with lp:~wallyworld/launchpad/sharing-cleanup as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1009283 in Launchpad itself: "RemoveGranteeSubscriptionsJob._unsubscribe_pillar_artifacts breaks for admins"
  https://bugs.launchpad.net/launchpad/+bug/1009283

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/RemoveGranteeSubscriptionsJob-admins-1009283/+merge/109279

== Implementation ==

If a RemoveGranteeSubscriptionsJob is asked to remove subscriptions for users who can see the artifacts, it does not remove those subscriptions. It does this by calling userCanView() and visibleToUser() and skipping to the next one if required.

== Tests ==

Add new tests to test_sharingjob

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/sharingjob.py
  lib/lp/registry/tests/test_sharingjo
-- 
https://code.launchpad.net/~wallyworld/launchpad/RemoveGranteeSubscriptionsJob-admins-1009283/+merge/109279
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/RemoveGranteeSubscriptionsJob-admins-1009283 into lp:launchpad.

=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py	2012-05-24 01:43:42 +0000
+++ lib/lp/registry/model/sharingjob.py	2012-06-08 05:53:21 +0000
@@ -331,19 +331,27 @@
         logger = logging.getLogger()
         logger.info(self.getOperationDescription())
 
-        # Unsubscribe grantee from the specified bugs.
+        # Unsubscribe grantee from the specified bugs if they can't see the
+        # bug.
         if self.bug_ids:
             bugs = getUtility(IBugSet).getByNumbers(self.bug_ids)
-            for bug in bugs:
+            inaccessible_bugs = [
+                bug for bug in bugs if not bug.userCanView(self.grantee)]
+            for bug in inaccessible_bugs:
                 bug.unsubscribe(
                     self.grantee, self.requestor, ignore_permissions=True)
 
-        # Unsubscribe grantee from the specified branches.
+        # Unsubscribe grantee from the specified branches if they can't see the
+        # branch.
         if self.branch_names:
             branches = [
                 getUtility(IBranchLookup).getByUniqueName(branch_name)
                 for branch_name in self.branch_names]
-            for branch in branches:
+            inaccessible_branches = [
+                branch for branch in branches
+                if not branch.visibleByUser(self.grantee)
+            ]
+            for branch in inaccessible_branches:
                 branch.unsubscribe(
                     self.grantee, self.requestor, ignore_permissions=True)
 
@@ -360,6 +368,11 @@
 
         # Do the bugs.
         privacy_filter = get_bug_privacy_filter(self.grantee)
+
+        # Admins can see all bugs so there's nothing to do.
+        if not privacy_filter:
+            return
+
         bug_filter = Not(In(
             Bug.id,
             Select(
@@ -440,7 +453,8 @@
         logger = logging.getLogger()
         logger.info(self.getOperationDescription())
 
-        # Unsubscribe grantee from the specified bugs.
+        # Find all bug subscriptions for which the subscriber cannot see the
+        # bug.
         constraints = [
             BugTaskFlat.bug_id.is_in(self.bug_ids),
             Not(Coalesce(

=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2012-06-08 05:53:21 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-06-08 05:53:21 +0000
@@ -12,6 +12,7 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.code.enums import (
     BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel,
@@ -239,6 +240,13 @@
             information_type=information_type)
         with person_logged_in(owner):
             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])
+
         return bug, owner
 
     def test_unsubscribe_bugs(self):
@@ -254,6 +262,19 @@
         self.assertNotIn(
             grantee, removeSecurityProxy(bug).getDirectSubscribers())
 
+    def test_unsubscribe_bugs_admin(self):
+        # Admins can see all bugs so no unsubscribe occurs.
+        pillar = self.factory.makeDistribution()
+        grantee = getUtility(ILaunchpadCelebrities).admin.teamowner
+        owner = self.factory.makePerson()
+        bug, ignored = self._make_subscribed_bug(grantee, distribution=pillar)
+        getUtility(IRemoveGranteeSubscriptionsJobSource).create(
+            pillar, grantee, owner, bugs=[bug])
+        with block_on_job(self):
+            transaction.commit()
+        self.assertIn(
+            grantee, removeSecurityProxy(bug).getDirectSubscribers())
+
     def _make_subscribed_branch(self, pillar, grantee,
                                 information_type=None):
         owner = self.factory.makePerson()
@@ -278,6 +299,20 @@
         self.assertNotIn(
             grantee, list(removeSecurityProxy(branch).subscribers))
 
+    def test_unsubscribe_branches_admin(self):
+        # Admins can see all branches so no unsubscribe occurs.
+        owner = self.factory.makePerson()
+        pillar = self.factory.makeProduct(owner=owner)
+        grantee = getUtility(ILaunchpadCelebrities).admin.teamowner
+        branch = self._make_subscribed_branch(pillar, grantee)
+        job = getUtility(IRemoveGranteeSubscriptionsJobSource).create(
+            pillar, grantee, owner, branches=[branch])
+        job.run()
+#        with block_on_job(self):
+#            transaction.commit()
+        self.assertIn(
+            grantee, list(removeSecurityProxy(branch).subscribers))
+
     def _assert_unsubscribe_pillar_artifacts_direct_bugs(self,
                                                          pillar=None):
         # All direct pillar bug subscriptions are removed.
@@ -291,13 +326,6 @@
             grantee, product=pillar,
             information_type=InformationType.USERDATA)
 
-        # Subscribing grantee to bugs creates an access grant so we need to
-        # revoke those for our test.
-        accessartifact_source = getUtility(IAccessArtifactSource)
-        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
-        accessartifact_grant_source.revokeByArtifact(
-            accessartifact_source.find([bug1, bug2]), [grantee])
-
         # Now run the job.
         requestor = self.factory.makePerson()
         getUtility(IRemoveGranteeSubscriptionsJobSource).create(
@@ -342,13 +370,6 @@
         with person_logged_in(bug2_owner):
             bug2.subscribe(person_grantee, bug2_owner)
 
-        # Subscribing person_grantee to bugs creates an access grant so we
-        # need to revoke those for our test.
-        accessartifact_source = getUtility(IAccessArtifactSource)
-        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
-        accessartifact_grant_source.revokeByArtifact(
-            accessartifact_source.find([bug1, bug2]), [person_grantee])
-
         # Now run the job.
         requestor = self.factory.makePerson()
         getUtility(IRemoveGranteeSubscriptionsJobSource).create(
@@ -371,32 +392,33 @@
     def test_unsubscribe_artifacts_indirect_bugs_unspecified_pillar(self):
         self._assert_unsubscribe_pillar_artifacts_indirect_bugs()
 
+    def _make_subscribed_bugs(self, person_grantee):
+        # Set up some bugs and subscribe the grantee.
+
+        owner = self.factory.makePerson(name='pillarowner')
+        pillar = self.factory.makeProduct(owner=owner)
+
+        # Make bugs the person_grantee is subscribed to.
+        bug1, ignored = self._make_subscribed_bug(
+            person_grantee, product=pillar,
+            information_type=InformationType.USERDATA)
+
+        bug2, ignored = self._make_subscribed_bug(
+            person_grantee, product=pillar,
+            information_type=InformationType.EMBARGOEDSECURITY)
+
+        return pillar, bug1, bug2
+
     def test_unsubscribe_pillar_artifacts_specific_info_types(self):
         # Only delete pillar artifacts of the specified info type.
 
-        owner = self.factory.makePerson(name='pillarowner')
-        pillar = self.factory.makeProduct(owner=owner)
         person_grantee = self.factory.makePerson(name='grantee')
 
-        # Make bugs the person_grantee is subscribed to.
-        bug1, ignored = self._make_subscribed_bug(
-            person_grantee, product=pillar,
-            information_type=InformationType.USERDATA)
-
-        bug2, ignored = self._make_subscribed_bug(
-            person_grantee, product=pillar,
-            information_type=InformationType.EMBARGOEDSECURITY)
-
-        # Subscribing grantee to bugs creates an access grant so we
-        # need to revoke those for our test.
-        accessartifact_source = getUtility(IAccessArtifactSource)
-        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
-        accessartifact_grant_source.revokeByArtifact(
-            accessartifact_source.find([bug1, bug2]), [person_grantee])
+        pillar, bug1, bug2 = self._make_subscribed_bugs(person_grantee)
 
         # Now run the job, removing access to userdata artifacts.
         getUtility(IRemoveGranteeSubscriptionsJobSource).create(
-            pillar, person_grantee, owner, [InformationType.USERDATA])
+            pillar, person_grantee, pillar.owner, [InformationType.USERDATA])
         with block_on_job(self):
             transaction.commit()
 
@@ -405,6 +427,24 @@
         self.assertIn(
             person_grantee, removeSecurityProxy(bug2).getDirectSubscribers())
 
+    def test_unsubscribe_pillar_artifacts_admin_grantee(self):
+        # For admins, the job is effectively a no-op.
+
+        admin_grantee = getUtility(ILaunchpadCelebrities).admin.teamowner
+
+        pillar, bug1, bug2 = self._make_subscribed_bugs(admin_grantee)
+
+        # Now run the job, removing access to userdata artifacts.
+        getUtility(IRemoveGranteeSubscriptionsJobSource).create(
+            pillar, admin_grantee, pillar.owner, [InformationType.USERDATA])
+        with block_on_job(self):
+            transaction.commit()
+
+        self.assertIn(
+            admin_grantee, removeSecurityProxy(bug1).getDirectSubscribers())
+        self.assertIn(
+            admin_grantee, removeSecurityProxy(bug2).getDirectSubscribers())
+
 
 class TestRunViaCron(TestCaseWithFactory):
     """Sharing jobs run via cron."""