launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07659
lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-part3 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-part3 into lp:launchpad with lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-2 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-part3/+merge/105028
== Implementation ==
Add more required functionality to the sharing RemoveSubscriptionsJob.
The remaining 2 use cases for bugs are implemented:
1. remove user subscriptions for all inaccessible bugs for a pillar
2. remove user subscriptions for all inaccessible bugs or a given type for a pillar
The next branch will wire up the job so that it is invoked when the sharing service is used to remove access for bugs.
When branches get the info_type attribute, the job will be updated to handle branches.
== Tests ==
Add tests to RemoveSubscriptionsJobTestCase:
- test_unsubscribe_pillar_artifacts_direct_bugs
- test_unsubscribe_pillar_artifacts_indirect_bugs
- test_unsubscribe_pillar_artifacts_specific_info_types
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/model/bugtasksearch.py
lib/lp/code/model/branch.py
lib/lp/registry/model/sharingjob.py
lib/lp/registry/tests/test_sharingjob.py
--
https://code.launchpad.net/~wallyworld/launchpad/revoke-access-delete-subscriptions-job-part3/+merge/105028
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-part3 into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2012-05-04 13:12:41 +0000
+++ database/schema/security.cfg 2012-05-08 05:33:27 +0000
@@ -1881,13 +1881,17 @@
[sharing-jobs]
groups=script
public.accessartifactgrant = SELECT, UPDATE, DELETE
+public.accesspolicy = SELECT
public.accesspolicygrant = SELECT, UPDATE, DELETE
+public.accesspolicygrantflat = SELECT
public.branch = SELECT
public.branchsubscription = SELECT, UPDATE, DELETE
public.bug = SELECT, UPDATE
public.bugactivity = SELECT, INSERT
public.bugbranch = SELECT
public.bugsubscription = SELECT, UPDATE, DELETE
+public.bugtask = SELECT
+public.bugtaskflat = SELECT
public.distribution = SELECT
public.emailaddress = SELECT
public.job = SELECT, INSERT, UPDATE
=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py 2012-05-04 05:43:28 +0000
+++ lib/lp/bugs/model/bugtasksearch.py 2012-05-08 05:33:27 +0000
@@ -1379,9 +1379,10 @@
# Privacy restrictions
-def get_bug_privacy_filter(user, private_only=False):
+def get_bug_privacy_filter(user, private_only=False, use_flat=False):
"""An SQL filter for search results that adds privacy-awareness."""
- return _get_bug_privacy_filter_with_decorator(user, private_only)[0]
+ return _get_bug_privacy_filter_with_decorator(
+ user, private_only, use_flat)[0]
def _nocache_bug_decorator(obj):
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-05-04 04:52:24 +0000
+++ lib/lp/code/model/branch.py 2012-05-08 05:33:27 +0000
@@ -825,13 +825,15 @@
"""See `IBranch`."""
return self.getSubscription(person) is not None
- def unsubscribe(self, person, unsubscribed_by):
+ def unsubscribe(self, person, unsubscribed_by, **kwargs):
"""See `IBranch`."""
subscription = self.getSubscription(person)
if subscription is None:
# Silent success seems order of the day (like bugs).
return
- if not subscription.canBeUnsubscribedByUser(unsubscribed_by):
+ ignore_permissions = kwargs.get('ignore_permissions', False)
+ if (not ignore_permissions
+ and not subscription.canBeUnsubscribedByUser(unsubscribed_by)):
raise UserCannotUnsubscribePerson(
'%s does not have permission to unsubscribe %s.' % (
unsubscribed_by.displayname,
=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py 2012-05-08 05:33:26 +0000
+++ lib/lp/registry/model/sharingjob.py 2012-05-08 05:33:27 +0000
@@ -18,10 +18,16 @@
from lazr.enum import (
DBEnumeratedType,
DBItem,
+ enumerated_type_registry,
)
import simplejson
from sqlobject import SQLObjectNotFound
-from storm.expr import And
+from storm.expr import (
+ And,
+ Not,
+ In,
+ Select,
+ )
from storm.locals import (
Int,
Reference,
@@ -33,10 +39,14 @@
classProvides,
implements,
)
-from zope.security.proxy import removeSecurityProxy
from lp.bugs.interfaces.bug import IBugSet
+from lp.bugs.model.bug import Bug
+from lp.bugs.model.bugsubscription import BugSubscription
+from lp.bugs.model.bugtaskflat import BugTaskFlat
+from lp.bugs.model.bugtasksearch import get_bug_privacy_filter
from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.registry.enums import InformationType
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.sharingjob import (
@@ -67,10 +77,11 @@
"""Values that ISharingJob.job_type can take."""
REMOVE_SUBSCRIPTIONS = DBItem(0, """
- Remove subscriptions when access is revoked.
+ Remove subscriptions of artifacts which are inaccessible.
This job removes subscriptions to artifacts when access is
- revoked for a particular information type or artifact.
+ no longer possible because a user no longer has an access
+ grant (either direct or indirect via team membership).
""")
@@ -227,7 +238,8 @@
config = config.sharing_jobs
@classmethod
- def create(cls, pillar, grantee, requestor, bugs=None, branches=None):
+ def create(cls, pillar, grantee, requestor, information_types=None,
+ bugs=None, branches=None):
"""See `IRemoveSubscriptionsJob`."""
bug_ids = [
@@ -236,9 +248,13 @@
branch_names = [
branch.unique_name for branch in branches or []
]
+ information_types = [
+ info_type.value for info_type in information_types or []
+ ]
metadata = {
'bug_ids': bug_ids,
'branch_names': branch_names,
+ 'information_types': information_types,
'requestor.id': requestor.id
}
return super(RemoveSubscriptionsJob, cls).create(
@@ -260,6 +276,12 @@
def branch_names(self):
return self.metadata['branch_names']
+ @property
+ def information_types(self):
+ return [
+ enumerated_type_registry[InformationType.name].items[value]
+ for value in self.metadata['information_types']]
+
def getErrorRecipients(self):
# If something goes wrong we want to let the requestor know as well
# as the pillar maintainer.
@@ -285,8 +307,8 @@
if self.bug_ids:
bugs = getUtility(IBugSet).getByNumbers(self.bug_ids)
for bug in bugs:
- removeSecurityProxy(bug).unsubscribe(
- self.grantee, self.requestor)
+ bug.unsubscribe(
+ self.grantee, self.requestor, ignore_permissions=True)
# Unsubscribe grantee from the specified branches.
if self.branch_names:
@@ -294,4 +316,38 @@
getUtility(IBranchLookup).getByUniqueName(branch_name)
for branch_name in self.branch_names]
for branch in branches:
- branch.unsubscribe(self.grantee, self.requestor)
+ branch.unsubscribe(
+ self.grantee, self.requestor, ignore_permissions=True)
+
+ # If required, unsubscribe all pillar artifacts.
+ if not self.bug_ids and not self.branch_names:
+ self._unsubscribe_pillar_artifacts(self.information_types)
+
+ def _unsubscribe_pillar_artifacts(self, only_information_types):
+ # Unsubscribe grantee from pillar artifacts to which they no longer
+ # have access. If only_information_types is specified, filter by the
+ # specified information types, else unsubscribe from all artifacts.
+
+ # Branches are not handled until information_type is supported.
+
+ # Do the bugs.
+ privacy_filter = get_bug_privacy_filter(self.grantee, use_flat=True)
+ bug_filter = Not(In(
+ Bug.id,
+ Select(
+ (BugTaskFlat.bug_id,),
+ where=privacy_filter)))
+ if only_information_types:
+ bug_filter = And(
+ bug_filter,
+ Bug.information_type.is_in(only_information_types)
+ )
+ store = IStore(BugSubscription)
+ subscribed_invisible_bugs = store.find(
+ Bug,
+ BugSubscription.bug_id == Bug.id,
+ BugSubscription.person == self.grantee,
+ bug_filter)
+ for bug in subscribed_invisible_bugs:
+ bug.unsubscribe(
+ self.grantee, self.requestor, ignore_permissions=True)
=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py 2012-05-08 05:33:26 +0000
+++ lib/lp/registry/tests/test_sharingjob.py 2012-05-08 05:33:27 +0000
@@ -14,6 +14,12 @@
BranchSubscriptionNotificationLevel,
CodeReviewNotificationLevel,
)
+from lp.registry.enums import InformationType
+from lp.registry.interfaces.accesspolicy import (
+ IAccessArtifactSource,
+ IAccessArtifactGrantSource,
+ )
+from lp.registry.interfaces.person import TeamSubscriptionPolicy
from lp.registry.interfaces.sharingjob import (
IRemoveSubscriptionsJobSource,
ISharingJob,
@@ -171,44 +177,173 @@
requestor = self.factory.makePerson()
bug = self.factory.makeBug(product=pillar)
branch = self.factory.makeBranch(product=pillar)
+ info_type = InformationType.USERDATA
job = getUtility(IRemoveSubscriptionsJobSource).create(
- pillar, grantee, requestor, [bug], [branch])
+ pillar, grantee, requestor, [info_type], [bug], [branch])
naked_job = removeSecurityProxy(job)
self.assertIsInstance(job, RemoveSubscriptionsJob)
self.assertEqual(pillar, job.pillar)
self.assertEqual(grantee, job.grantee)
self.assertEqual(requestor.id, naked_job.requestor_id)
+ self.assertContentEqual([info_type], naked_job.information_types)
self.assertContentEqual([bug.id], naked_job.bug_ids)
self.assertContentEqual([branch.unique_name], naked_job.branch_names)
+ 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_unsubscribe_bugs(self):
# The requested bug subscriptions are removed.
pillar = self.factory.makeDistribution()
grantee = self.factory.makePerson()
owner = self.factory.makePerson()
- bug = self.factory.makeBug(owner=owner, distribution=pillar)
- with person_logged_in(owner):
- bug.subscribe(grantee, owner)
- self.assertContentEqual([owner, grantee], bug.getDirectSubscribers())
+ bug, ignored = self._make_subscribed_bug(grantee, distribution=pillar)
getUtility(IRemoveSubscriptionsJobSource).create(
- pillar, grantee, owner, [bug])
+ pillar, grantee, owner, bugs=[bug])
with block_on_job(self):
transaction.commit()
- self.assertContentEqual([owner], bug.getDirectSubscribers())
+ self.assertNotIn(
+ grantee, removeSecurityProxy(bug).getDirectSubscribers())
- def test_unsubscribe_branches(self):
- # The requested branch subscriptions are removed.
- pillar = self.factory.makeProduct()
- grantee = self.factory.makePerson()
+ def _make_subscribed_branch(self, pillar, grantee, private=False):
owner = self.factory.makePerson()
- branch = self.factory.makeBranch(owner=owner, product=pillar)
+ branch = self.factory.makeBranch(
+ owner=owner, product=pillar, private=private)
with person_logged_in(owner):
branch.subscribe(grantee,
BranchSubscriptionNotificationLevel.NOEMAIL, None,
CodeReviewNotificationLevel.NOEMAIL, owner)
- self.assertContentEqual([owner, grantee], list(branch.subscribers))
+ return branch
+
+ def test_unsubscribe_branches(self):
+ # The requested branch subscriptions are removed.
+ owner = self.factory.makePerson()
+ pillar = self.factory.makeProduct(owner=owner)
+ grantee = self.factory.makePerson()
+ branch = self._make_subscribed_branch(pillar, grantee)
getUtility(IRemoveSubscriptionsJobSource).create(
pillar, grantee, owner, branches=[branch])
with block_on_job(self):
transaction.commit()
- self.assertContentEqual([owner], list(branch.subscribers))
+ self.assertNotIn(
+ grantee, list(removeSecurityProxy(branch).subscribers))
+
+ def test_unsubscribe_pillar_artifacts_direct_bugs(self):
+ # 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.
+ bug1, ignored = self._make_subscribed_bug(
+ grantee, product=pillar,
+ information_type=InformationType.EMBARGOEDSECURITY)
+ bug2, ignored = self._make_subscribed_bug(
+ 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.
+ getUtility(IRemoveSubscriptionsJobSource).create(
+ pillar, grantee, owner)
+ with block_on_job(self):
+ transaction.commit()
+
+ self.assertNotIn(
+ grantee, removeSecurityProxy(bug1).getDirectSubscribers())
+ self.assertNotIn(
+ grantee, removeSecurityProxy(bug2).getDirectSubscribers())
+
+ def test_unsubscribe_pillar_artifacts_indirect_bugs(self):
+ # 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.
+ bug1, ignored = self._make_subscribed_bug(
+ person_grantee, product=pillar,
+ information_type=InformationType.USERDATA)
+
+ # Make another bug and grant access to a team.
+ team_owner = self.factory.makePerson(name='teamowner')
+ 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=pillar,
+ 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 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.
+ getUtility(IRemoveSubscriptionsJobSource).create(
+ pillar, person_grantee, owner)
+ with block_on_job(self):
+ transaction.commit()
+
+ # person_grantee is not longer subscribed to bug1.
+ self.assertNotIn(
+ person_grantee, removeSecurityProxy(bug1).getDirectSubscribers())
+ # person_grantee is still subscribed to bug2 because they have access
+ # via a team.
+ self.assertIn(
+ person_grantee, removeSecurityProxy(bug2).getDirectSubscribers())
+
+ 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])
+
+ # Now run the job, removing access to userdata artifacts.
+ getUtility(IRemoveSubscriptionsJobSource).create(
+ pillar, person_grantee, owner, [InformationType.USERDATA])
+ with block_on_job(self):
+ transaction.commit()
+
+ self.assertNotIn(
+ person_grantee, removeSecurityProxy(bug1).getDirectSubscribers())
+ self.assertIn(
+ person_grantee, removeSecurityProxy(bug2).getDirectSubscribers())
Follow ups
