launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07684
lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-4 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-4 into lp:launchpad with lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-part3 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #992315 in Launchpad itself: "When revoking access to a project or artifact, user needs to be unsubscribed also"
https://bugs.launchpad.net/launchpad/+bug/992315
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/revoke-access-delete-subscriptions-job-4/+merge/105158
== Implementation ==
Simply create the RemoveSubscriptionsJob when the sharing service api is used to revoke access. The celery stuff takes care of running the job.
This work cannot be landed will celery is in prod.
== Tests ==
Add and update tests in TestSharingService test case.
- shareeUnsubscribedWhenDeleted
- shareeUnsubscribedWhenDeletedSelectedPolicies
- _assert_revokeAccessGrants (updated)
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
--
https://code.launchpad.net/~wallyworld/launchpad/revoke-access-delete-subscriptions-job-4/+merge/105158
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-4 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-04-13 01:06:53 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-05-09 01:42:22 +0000
@@ -103,22 +103,25 @@
"""
@export_write_operation()
+ @call_with(user=REQUEST_USER)
@operation_parameters(
pillar=Reference(IPillar, title=_('Pillar'), required=True),
sharee=Reference(IPerson, title=_('Sharee'), required=True),
information_types=List(
Choice(vocabulary=InformationType), required=False))
@operation_for_version('devel')
- def deletePillarSharee(pillar, sharee, information_types):
+ def deletePillarSharee(pillar, sharee, user, information_types):
"""Remove a sharee from a pillar.
:param pillar: the pillar from which to remove access
+ :param user: the user making the request
:param sharee: the person or team to remove
:param information_types: if None, remove all access, otherwise just
remove the specified access_policies
"""
@export_write_operation()
+ @call_with(user=REQUEST_USER)
@operation_parameters(
pillar=Reference(IPillar, title=_('Pillar'), required=True),
sharee=Reference(IPerson, title=_('Sharee'), required=True),
@@ -127,10 +130,11 @@
branches=List(
Reference(schema=IBranch), title=_('Branches'), required=False))
@operation_for_version('devel')
- def revokeAccessGrants(pillar, sharee, branches=None, bugs=None):
+ def revokeAccessGrants(pillar, user, sharee, branches=None, bugs=None):
"""Remove a sharee's access to the specified artifacts.
:param pillar: the pillar from which to remove access
+ :param user: the user making the request
:param sharee: the person or team for whom to revoke access
:param bugs: the bugs for which to revoke access
:param branches: the branches for which to revoke access
=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py 2012-05-09 01:42:21 +0000
+++ lib/lp/registry/model/sharingjob.py 2012-05-09 01:42:22 +0000
@@ -24,8 +24,8 @@
from sqlobject import SQLObjectNotFound
from storm.expr import (
And,
+ In,
Not,
- In,
Select,
)
from storm.locals import (
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-04-26 07:54:34 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-05-09 01:42:22 +0000
@@ -2,6 +2,7 @@
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Classes for pillar and artifact sharing service."""
+from lp.registry.interfaces.sharingjob import IRemoveSubscriptionsJobSource
__metaclass__ = type
__all__ = [
@@ -217,7 +218,7 @@
return sharee
@available_with_permission('launchpad.Edit', 'pillar')
- def deletePillarSharee(self, pillar, sharee,
+ def deletePillarSharee(self, pillar, user, sharee,
information_types=None):
"""See `ISharingService`."""
@@ -253,8 +254,14 @@
IAccessArtifactGrantSource)
accessartifact_grant_source.revokeByArtifact(to_delete)
+ # Create a job to remove subscriptions for artifacts the sharee can no
+ # longer see.
+ getUtility(IRemoveSubscriptionsJobSource).create(
+ pillar, sharee, user, information_types=information_types)
+
@available_with_permission('launchpad.Edit', 'pillar')
- def revokeAccessGrants(self, pillar, sharee, branches=None, bugs=None):
+ def revokeAccessGrants(self, pillar, user, sharee, branches=None,
+ bugs=None):
"""See `ISharingService`."""
if not self.write_enabled:
@@ -272,3 +279,8 @@
accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
accessartifact_grant_source.revokeByArtifact(
artifacts_to_delete, [sharee])
+
+ # Create a job to remove subscriptions for artifacts the sharee can no
+ # longer see.
+ getUtility(IRemoveSubscriptionsJobSource).create(
+ pillar, sharee, user, bugs=bugs, branches=branches)
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-05-02 05:25:11 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-05-09 01:42:22 +0000
@@ -13,6 +13,10 @@
from zope.traversing.browser.absoluteurl import absoluteURL
from lp.app.interfaces.services import IService
+from lp.code.enums import (
+ BranchSubscriptionNotificationLevel,
+ CodeReviewNotificationLevel,
+ )
from lp.registry.enums import (
InformationType,
SharingPermission,
@@ -25,6 +29,7 @@
)
from lp.registry.services.sharingservice import SharingService
from lp.services.features.testing import FeatureFixture
+from lp.services.job.tests import block_on_job
from lp.services.webapp.interaction import ANONYMOUS
from lp.services.webapp.interfaces import ILaunchpadRoot
from lp.services.webapp.publisher import canonical_url
@@ -38,7 +43,7 @@
)
from lp.testing.layers import (
AppServerLayer,
- DatabaseFunctionalLayer,
+ CeleryJobLayer,
)
from lp.testing.matchers import HasQueryCount
from lp.testing.pages import LaunchpadWebServiceCaller
@@ -46,14 +51,15 @@
WRITE_FLAG = {
'disclosure.enhanced_sharing.writable': 'true',
- 'disclosure.enhanced_sharing_details.enabled': 'true'}
+ 'disclosure.enhanced_sharing_details.enabled': 'true',
+ 'jobs.celery.enabled_classes': 'RemoveSubscriptionsJob'}
DETAILS_FLAG = {'disclosure.enhanced_sharing_details.enabled': 'true'}
class TestSharingService(TestCaseWithFactory):
"""Tests for the SharingService."""
- layer = DatabaseFunctionalLayer
+ layer = CeleryJobLayer
def setUp(self):
super(TestSharingService, self).setUp()
@@ -516,7 +522,8 @@
self.factory.makeAccessPolicyGrant(access_policies[0], another)
# Delete data for a specific information type.
with FeatureFixture(WRITE_FLAG):
- self.service.deletePillarSharee(pillar, grantee, types_to_delete)
+ self.service.deletePillarSharee(
+ pillar, pillar.owner, grantee, types_to_delete)
# Assemble the expected data for the remaining access grants for
# grantee.
expected_data = []
@@ -572,7 +579,7 @@
with FeatureFixture(WRITE_FLAG):
self.assertRaises(
Unauthorized, self.service.deletePillarSharee,
- pillar, [InformationType.USERDATA])
+ pillar, pillar.owner, [InformationType.USERDATA])
def test_deletePillarShareeAnonymous(self):
# Anonymous users are not allowed.
@@ -595,7 +602,69 @@
login_person(owner)
self.assertRaises(
Unauthorized, self.service.deletePillarSharee,
- product, [InformationType.USERDATA])
+ product, product.owner, [InformationType.USERDATA])
+
+ def _assert_deleteShareeRemoveSubscriptions(self,
+ types_to_delete=None):
+ product = self.factory.makeProduct()
+ access_policies = getUtility(IAccessPolicySource).findByPillar(
+ (product,))
+ information_types = [ap.type for ap in access_policies]
+ grantee = self.factory.makePerson()
+ # Make some access policy grants for our sharee.
+ for access_policy in access_policies:
+ self.factory.makeAccessPolicyGrant(access_policy, grantee)
+
+ login_person(product.owner)
+ # Make some bug artifact grants for our sharee.
+ # Branches will be done when information_type attribute is supported.
+ bugs = []
+ for access_policy in access_policies:
+ bug = self.factory.makeBug(
+ product=product, owner=product.owner,
+ information_type=access_policy.type)
+ bugs.append(bug)
+ artifact = self.factory.makeAccessArtifact(concrete=bug)
+ self.factory.makeAccessArtifactGrant(artifact, grantee)
+
+ # Make some access policy grants for another sharee.
+ another = self.factory.makePerson()
+ self.factory.makeAccessPolicyGrant(access_policies[0], another)
+
+ # Subscribe the grantee and other person to the artifacts.
+ for person in [grantee, another]:
+ for bug in bugs:
+ bug.subscribe(person, product.owner)
+
+ # Delete data for specified information types or all.
+ with FeatureFixture(WRITE_FLAG):
+ self.service.deletePillarSharee(
+ product, product.owner, grantee, types_to_delete)
+ with block_on_job(self):
+ transaction.commit()
+
+ expected_information_types = []
+ if types_to_delete is not None:
+ expected_information_types = (
+ set(information_types).difference(types_to_delete))
+ # Check that grantee is unsubscribed.
+ for bug in bugs:
+ if bug.information_type in expected_information_types:
+ self.assertIn(grantee, bug.getDirectSubscribers())
+ else:
+ self.assertNotIn(grantee, bug.getDirectSubscribers())
+ self.assertIn(another, bug.getDirectSubscribers())
+
+ def test_shareeUnsubscribedWhenDeleted(self):
+ # The sharee is unsubscribed from any inaccessible artifacts when their
+ # access is revoked.
+ self._assert_deleteShareeRemoveSubscriptions()
+
+ def test_shareeUnsubscribedWhenDeletedSelectedPolicies(self):
+ # The sharee is unsubscribed from any inaccessible artifacts when their
+ # access to selected policies is revoked.
+ self._assert_deleteShareeRemoveSubscriptions(
+ [InformationType.USERDATA])
def _assert_revokeAccessGrants(self, pillar, bugs, branches):
artifacts = []
@@ -619,6 +688,15 @@
artifact=access_artifact, grantee=person,
grantor=pillar.owner)
+ # Subscribe the grantee and other person to the artifacts.
+ for person in [grantee, someone]:
+ for bug in bugs or []:
+ bug.subscribe(person, pillar.owner)
+ for branch in branches or []:
+ branch.subscribe(grantee,
+ BranchSubscriptionNotificationLevel.NOEMAIL, None,
+ CodeReviewNotificationLevel.NOEMAIL, pillar.owner)
+
# Check that grantee has expected access grants.
accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
grants = accessartifact_grant_source.findByArtifact(
@@ -628,17 +706,29 @@
with FeatureFixture(WRITE_FLAG):
self.service.revokeAccessGrants(
- pillar, grantee, bugs=bugs, branches=branches)
+ pillar, pillar.owner, grantee, bugs=bugs, branches=branches)
+ with block_on_job(self):
+ transaction.commit()
# The grantee now has no access to anything.
permission_info = apgfs.findGranteePermissionsByPolicy(
[policy], [grantee])
self.assertEqual(0, permission_info.count())
+ # Check that the grantee's subscriptions have been removed.
+ # Branches will be done once they have the information_type attribute.
+ for bug in bugs:
+ self.assertNotIn(grantee, bug.getDirectSubscribers())
+
# Someone else still has access to the bugs and branches.
grants = accessartifact_grant_source.findByArtifact(
access_artifacts, [someone])
self.assertEqual(1, grants.count())
+ # Someone else still has subscriptions to the bugs and branches.
+ for bug in bugs:
+ self.assertIn(someone, bug.getDirectSubscribers())
+ for branch in branches:
+ self.assertIn(someone, branch.subscribers)
def test_revokeAccessGrantsBugs(self):
# Users with launchpad.Edit can delete all access for a sharee.
@@ -669,7 +759,7 @@
with FeatureFixture(WRITE_FLAG):
self.assertRaises(
Unauthorized, self.service.revokeAccessGrants,
- product, sharee, bugs=[bug])
+ product, product.owner, sharee, bugs=[bug])
def test_revokeAccessGrantsAnonymous(self):
# Anonymous users are not allowed.
@@ -693,7 +783,7 @@
login_person(owner)
self.assertRaises(
Unauthorized, self.service.revokeAccessGrants,
- product, sharee, bugs=[bug])
+ product, product.owner, sharee, bugs=[bug])
class ApiTestMixin:
Follow ups
