launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/revoke-membership-delete-subscriptions-996882 into lp:launchpad with lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-4 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #996882 in Launchpad itself: "When revoking team membership, a user may lose access to pillar artifacts and needs tro be unsubscribed"
  https://bugs.launchpad.net/launchpad/+bug/996882

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/revoke-membership-delete-subscriptions-996882/+merge/105264

== Implementation ==

In IPerson.retractTeamMembership(), create the RemoveSubscriptionsJob to remove any required subscriptions.
Update RemoveSubscriptionsJob class to account for the fact that the pillar attribute is now optional.

== Tests ==

Update sharing job tests to account for the pillar being optional.
Add new test case to test_teammembership: TestTeamMembershipJobs
New test:
- test_retract_unsubscribes_former_member

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/person.py
  lib/lp/registry/model/sharingjob.py
  lib/lp/registry/tests/test_sharingjob.py
  lib/lp/registry/tests/test_teammembership.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/revoke-membership-delete-subscriptions-996882/+merge/105264
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/revoke-membership-delete-subscriptions-996882 into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-05-04 12:02:44 +0000
+++ lib/lp/registry/model/person.py	2012-05-09 20:43:23 +0000
@@ -198,6 +198,7 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.interfaces.role import IPersonRoles
+from lp.registry.interfaces.sharingjob import IRemoveSubscriptionsJobSource
 from lp.registry.interfaces.ssh import (
     ISSHKey,
     ISSHKeySet,
@@ -1731,6 +1732,11 @@
             new_status = active_and_transitioning[tm.status]
             tm.setStatus(new_status, user, comment=comment)
 
+        # A person has left the team so they may no longer have access to some
+        # artifacts shared with the team. We need to run a job to remove any
+        # subscriptions to such artifacts.
+        getUtility(IRemoveSubscriptionsJobSource).create(None, team, user)
+
     def renewTeamMembership(self, team):
         """Renew the TeamMembership for this person on the given team.
 

=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py	2012-05-09 20:43:22 +0000
+++ lib/lp/registry/model/sharingjob.py	2012-05-09 20:43:23 +0000
@@ -164,7 +164,7 @@
         return '<%(job_type)s job for %(grantee)s and %(pillar)s>' % {
             'job_type': self.context.job_type.name,
             'grantee': self.grantee.displayname,
-            'pillar': self.pillar.displayname,
+            'pillar': self.pillar_text,
             }
 
     @property
@@ -175,6 +175,10 @@
             return self.distro
 
     @property
+    def pillar_text(self):
+        return self.pillar.displayname if self.pillar else 'all pillars'
+
+    @property
     def log_name(self):
         return self.__class__.__name__
 
@@ -284,18 +288,16 @@
 
     def getErrorRecipients(self):
         # If something goes wrong we want to let the requestor know as well
-        # as the pillar maintainer.
+        # as the pillar maintainer (if there is a pillar).
         result = set()
         result.add(format_address_for_person(self.requestor))
-        if self.pillar.owner.preferredemail:
+        if self.pillar and self.pillar.owner.preferredemail:
             result.add(format_address_for_person(self.pillar.owner))
         return list(result)
 
     def getOperationDescription(self):
         return ('removing subscriptions for artifacts '
-            'for %s on %s' %
-            (self.grantee.displayname,
-             self.pillar.displayname))
+            'for %s on %s' % (self.grantee.displayname, self.pillar_text))
 
     def run(self):
         """See `IRemoveSubscriptionsJob`."""

=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2012-05-09 20:43:22 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-05-09 20:43:23 +0000
@@ -165,7 +165,7 @@
 
     def test_create(self):
         # Create an instance of RemoveSubscriptionsJob that stores
-        # the notification information.
+        # the information type and artifact information.
         self.assertIs(
             True,
             IRemoveSubscriptionsJobSource.providedBy(RemoveSubscriptionsJob))
@@ -189,6 +189,21 @@
         self.assertContentEqual([bug.id], naked_job.bug_ids)
         self.assertContentEqual([branch.unique_name], naked_job.branch_names)
 
+    def test_create_no_pillar(self):
+        # Create an instance of RemoveSubscriptionsJob that stores
+        # the information type and artifact information but with no pillar.
+        grantee = self.factory.makePerson()
+        requestor = self.factory.makePerson()
+        job = getUtility(IRemoveSubscriptionsJobSource).create(
+            None, grantee, requestor)
+        naked_job = removeSecurityProxy(job)
+        self.assertIsInstance(job, RemoveSubscriptionsJob)
+        self.assertEqual(None, job.pillar)
+        self.assertEqual(grantee, job.grantee)
+        self.assertEqual(requestor.id, naked_job.requestor_id)
+        self.assertIn('all pillars', repr(job))
+        self.assertEqual(1, len(job.getErrorRecipients()))
+
     def _make_subscribed_bug(self, grantee, product=None, distribution=None,
                              information_type=InformationType.USERDATA):
         owner = self.factory.makePerson()
@@ -235,10 +250,9 @@
         self.assertNotIn(
             grantee, list(removeSecurityProxy(branch).subscribers))
 
-    def test_unsubscribe_pillar_artifacts_direct_bugs(self):
+    def _assert_unsubscribe_pillar_artifacts_direct_bugs(self,
+                                                         pillar=None):
         # All direct pillar bug subscriptions are removed.
-        owner = self.factory.makePerson()
-        pillar = self.factory.makeProduct(owner=owner)
         grantee = self.factory.makePerson()
 
         # Make some bugs subscribed to by grantee.
@@ -257,8 +271,9 @@
             accessartifact_source.find([bug1, bug2]), [grantee])
 
         # Now run the job.
+        requestor = self.factory.makePerson()
         getUtility(IRemoveSubscriptionsJobSource).create(
-            pillar, grantee, owner)
+            pillar, grantee, requestor)
         with block_on_job(self):
             transaction.commit()
 
@@ -267,12 +282,19 @@
         self.assertNotIn(
             grantee, removeSecurityProxy(bug2).getDirectSubscribers())
 
-    def test_unsubscribe_pillar_artifacts_indirect_bugs(self):
+    def test_unsubscribe_pillar_artifacts_direct_bugs(self):
+        pillar = self.factory.makeProduct()
+        self._assert_unsubscribe_pillar_artifacts_direct_bugs(pillar)
+
+    def test_unsubscribe_artifacts_direct_bugs_unspecified_pillar(
+                                                                  self):
+        self._assert_unsubscribe_pillar_artifacts_direct_bugs()
+
+    def _assert_unsubscribe_pillar_artifacts_indirect_bugs(self,
+                                                           pillar=None):
         # Do not delete subscriptions to bugs a user has indirect access to
         # because they belong to a team which has an artifact grant on the bug.
 
-        owner = self.factory.makePerson(name='pillarowner')
-        pillar = self.factory.makeProduct(owner=owner)
         person_grantee = self.factory.makePerson(name='grantee')
 
         # Make a bug the person_grantee is subscribed to.
@@ -301,8 +323,9 @@
             accessartifact_source.find([bug1, bug2]), [person_grantee])
 
         # Now run the job.
+        requestor = self.factory.makePerson()
         getUtility(IRemoveSubscriptionsJobSource).create(
-            pillar, person_grantee, owner)
+            pillar, person_grantee, requestor)
         with block_on_job(self):
             transaction.commit()
 
@@ -314,6 +337,14 @@
         self.assertIn(
             person_grantee, removeSecurityProxy(bug2).getDirectSubscribers())
 
+    def test_unsubscribe_pillar_artifacts_indirect_bugs(self):
+        pillar = self.factory.makeProduct()
+        self._assert_unsubscribe_pillar_artifacts_indirect_bugs(pillar)
+
+    def test_unsubscribe_artifacts_indirect_bugs_unspecified_pillar(
+                                                                        self):
+        self._assert_unsubscribe_pillar_artifacts_indirect_bugs()
+
     def test_unsubscribe_pillar_artifacts_specific_info_types(self):
         # Only delete pillar artifacts of the specified info type.
 

=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py	2012-02-27 07:09:17 +0000
+++ lib/lp/registry/tests/test_teammembership.py	2012-05-09 20:43:23 +0000
@@ -23,6 +23,11 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.registry.enums import InformationType
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactSource,
+    IAccessArtifactGrantSource,
+    )
 from lp.registry.interfaces.person import (
     IPersonSet,
     TeamMembershipRenewalPolicy,
@@ -53,7 +58,9 @@
     flush_database_updates,
     sqlvalues,
     )
+from lp.services.features.testing import FeatureFixture
 from lp.services.log.logger import BufferLogger
+from lp.services.job.tests import block_on_job
 from lp.testing import (
     login,
     login_celebrity,
@@ -65,6 +72,7 @@
     )
 from lp.testing.dbuser import dbuser
 from lp.testing.layers import (
+    CeleryJobLayer,
     DatabaseFunctionalLayer,
     DatabaseLayer,
     LaunchpadZopelessLayer,
@@ -984,6 +992,70 @@
         self.assertEqual(TeamMembershipStatus.DEACTIVATED, tm.status)
 
 
+class TestTeamMembershipJobs(TestCaseWithFactory):
+    """Test jobs asscoiated with managing team membership."""
+    layer = CeleryJobLayer
+
+    def setUp(self):
+        self.useFixture(FeatureFixture({
+            'jobs.celery.enabled_classes': 'RemoveSubscriptionsJob',
+        }))
+        super(TestTeamMembershipJobs, self).setUp()
+
+    def _make_subscribed_bug(self, grantee, product=None, distribution=None,
+                             information_type=InformationType.USERDATA):
+        owner = self.factory.makePerson()
+        bug = self.factory.makeBug(
+            owner=owner, product=product, distribution=distribution,
+            information_type=information_type)
+        with person_logged_in(owner):
+            bug.subscribe(grantee, owner)
+        return bug, owner
+
+    def test_retract_unsubscribes_former_member(self):
+        # When a team member is removed, any subscriptions to artifacts they
+        # can no longer see are removed also.
+        person_grantee = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        # Make a bug the person_grantee is subscribed to.
+        bug1, ignored = self._make_subscribed_bug(
+            person_grantee, product=product,
+            information_type=InformationType.USERDATA)
+
+        # Make another bug and grant access to a team.
+        team_owner = self.factory.makePerson()
+        team_grantee = self.factory.makeTeam(
+            owner=team_owner,
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED,
+            members=[person_grantee])
+        bug2, bug2_owner = self._make_subscribed_bug(
+            team_grantee, product=product,
+            information_type=InformationType.EMBARGOEDSECURITY)
+        # Add a subscription for the person_grantee.
+        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 the one to bug2 for our test.
+        accessartifact_source = getUtility(IAccessArtifactSource)
+        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
+        accessartifact_grant_source.revokeByArtifact(
+            accessartifact_source.find([bug2]), [person_grantee])
+
+        with person_logged_in(team_owner):
+            team_grantee.retractTeamMembership(person_grantee, team_owner)
+        with block_on_job(self):
+            transaction.commit()
+
+        # person_grantee is still subscribed to bug1.
+        self.assertIn(
+            person_grantee, removeSecurityProxy(bug1).getDirectSubscribers())
+        # person_grantee is not to bug2 because they no longer have access
+        # via a team.
+        self.assertNotIn(
+            person_grantee, removeSecurityProxy(bug2).getDirectSubscribers())
+
+
 class TestTeamMembershipSendExpirationWarningEmail(TestCaseWithFactory):
     """Test the behaviour of sendExpirationWarningEmail()."""
     layer = DatabaseFunctionalLayer