launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16669
[Merge] lp:~wgrant/launchpad/blargh-userdata into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/blargh-userdata into lp:launchpad.
Commit message:
Stop subscribing the pillar driver, bug supervisor and/or owner (except for an Ubuntu special case, because why not) when a bug transitions to USERDATA. We have +sharing now, so direct subscriptions aren't necessary to retain access.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/blargh-userdata/+merge/217531
This branch removes some Ubuntu and USERDATA special cases from Bug.transitionToInformationType, simplifying the logic considerably.
There were rules left over from before modern privacy, where we'd subscribe the driver, bug supervisor or owner in some situations to ensure that the project's developers retained access to a newly private bug. This didn't work for projects with complex structures, so Ubuntu was special-cased out of this behaviour. This all became redundant in 2012, when +sharing gave project owners the ability to grant principals configurable access to all of their private artifacts. It's no longer possible for them to be locked out, so the special cases to retain access by subscription can die.
Now we just ensure that the bug reporter and the user performing the transition will be able to see the bug afterwards. No pillar driver, bug supervisor or owner involvement, and certainly no Ubuntu special cases.
--
https://code.launchpad.net/~wgrant/launchpad/blargh-userdata/+merge/217531
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/blargh-userdata into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py 2012-12-05 09:55:39 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py 2014-04-29 01:03:20 +0000
@@ -388,13 +388,14 @@
self.assertContentEqual([], view.request.response.notifications)
def _assert_secrecy_view_ajax_render(self, bug, new_type,
- validate_change):
+ validate_change, person=None):
# When the bug secrecy view is called from an ajax request, it should
# provide a json encoded dict when rendered. The dict contains bug
# subscription information resulting from the update to the bug
# privacy as well as information used to populate the updated
# subscribers list.
- person = bug.owner
+ if person is None:
+ person = bug.owner
with person_logged_in(person):
bug.subscribe(person, person)
@@ -426,10 +427,12 @@
# An information type change request is processed as expected when the
# bug remains visible to someone and visibility check is performed.
bug = self.factory.makeBug()
+ with person_logged_in(bug.owner):
+ bug.unsubscribe(bug.owner, bug.owner)
result_data = self._assert_secrecy_view_ajax_render(
- bug, 'USERDATA', True)
+ bug, 'USERDATA', True, person=self.factory.makePerson())
[subscriber_data] = result_data['subscription_data']
- subscriber = removeSecurityProxy(bug).default_bugtask.pillar.owner
+ subscriber = removeSecurityProxy(bug).owner
self.assertEqual(
subscriber.name, subscriber_data['subscriber']['name'])
self.assertEqual('Discussion', subscriber_data['subscription_level'])
=== modified file 'lib/lp/bugs/doc/bug-heat.txt'
--- lib/lp/bugs/doc/bug-heat.txt 2013-06-20 05:50:00 +0000
+++ lib/lp/bugs/doc/bug-heat.txt 2014-04-29 01:03:20 +0000
@@ -46,38 +46,36 @@
>>> bug.heat
6
-When the bug is marked private it gains a subscriber - the owner of the
-product against which it's filed, whose subscription is converted from
-an indirect to a direct subscription.
-
Marking a bug as private also gives it an extra 150 heat points.
>>> changed = bug.setPrivate(True, bug_owner)
>>> bug.heat
- 158
+ 156
Setting the bug as security related adds another 250 heat points.
>>> changed = bug.setSecurityRelated(True, bug_owner)
>>> bug.heat
- 408
+ 406
Marking the bug public removes 150 heat points.
>>> changed = bug.setPrivate(False, bug_owner)
>>> bug.heat
- 258
+ 256
And marking it not security-related removes 250 points.
>>> changed = bug.setSecurityRelated(False, bug_owner)
>>> bug.heat
- 8
+ 6
Adding a subscriber to the bug increases its heat by 2 points.
>>> new_subscriber = factory.makePerson()
+ >>> more_subscriber = factory.makePerson()
>>> subscription = bug.subscribe(new_subscriber, new_subscriber)
+ >>> subscription = bug.subscribe(more_subscriber, more_subscriber)
>>> bug.heat
10
=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt 2012-10-15 03:31:26 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt 2014-04-29 01:03:20 +0000
@@ -730,8 +730,6 @@
>>> for message in trigger_and_get_email_messages(bug_three):
... print_message_header_details(message)
- foo.bar@xxxxxxxxxxxxx ['yes'] ['no'] ['Private']
- mark@xxxxxxxxxxx ['yes'] ['no'] ['Private']
test@xxxxxxxxxxxxx ['yes'] ['no'] ['Private']
Now transition the bug to private security:
@@ -745,8 +743,6 @@
>>> for message in trigger_and_get_email_messages(bug_three):
... print_message_header_details(message)
- foo.bar@xxxxxxxxxxxxx ['yes'] ['yes'] ['Private Security']
- mark@xxxxxxxxxxx ['yes'] ['yes'] ['Private Security']
test@xxxxxxxxxxxxx ['yes'] ['yes'] ['Private Security']
=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2012-10-16 20:37:16 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2014-04-29 01:03:20 +0000
@@ -340,44 +340,11 @@
>>> linux_source_bug.getSubscribersFromDuplicates()
()
-When a bug is marked private, specific people like the person marking private
-will be subscribed. People without pre-existing access and aren't one of the
-special users will be removed.
-
- >>> from zope.event import notify
-
- >>> from lazr.lifecycle.event import ObjectModifiedEvent
- >>> from lazr.lifecycle.snapshot import Snapshot
- >>> from lp.bugs.interfaces.bug import IBug
-
- >>> print_displayname(linux_source_bug.getDirectSubscribers())
- Foo Bar
- Mark Shuttleworth
-
- >>> bug_before_modification = Snapshot(linux_source_bug, providing=IBug)
- >>> linux_source_bug.setPrivate(True, getUtility(ILaunchBag).user)
- True
-
- >>> notify(
- ... ObjectModifiedEvent(
- ... linux_source_bug, bug_before_modification, ["private"]))
-
- >>> print_displayname(linux_source_bug.getDirectSubscribers())
- Foo Bar
- Mark Shuttleworth
- Robert Collins
-
Direct subscriptions always take precedence over indirect subscriptions.
-So, if we unmark the above bug as private, indirect_subscribers will include
-any subscribers not converted to direct subscribers.
-
- >>> linux_source_bug.setPrivate(False, getUtility(ILaunchBag).user)
- True
>>> print_displayname(linux_source_bug.getDirectSubscribers())
Foo Bar
Mark Shuttleworth
- Robert Collins
>>> print_displayname(linux_source_bug.getIndirectSubscribers())
No Privileges Person
@@ -402,7 +369,6 @@
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
('no-priv@xxxxxxxxxxxxx', u'Subscriber (linux-source-2.6.15 in Ubuntu)'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
('test@xxxxxxxxxxxxx', 'Assignee')]
If IBug.getBugNotificationRecipients() is passed a BugNotificationLevel
@@ -415,7 +381,6 @@
>>> [(address, recipients.getReason(address)[1]) for address in addresses]
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
('test@xxxxxxxxxxxxx', 'Assignee')]
When Sample Person is unsubscribed from linux_source_bug, he is no
@@ -429,7 +394,6 @@
>>> [(address, recipients.getReason(address)[1]) for address in addresses]
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
('test@xxxxxxxxxxxxx', 'Assignee')]
...but remains included for the level LIFECYCLE.
@@ -442,7 +406,6 @@
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
('no-priv@xxxxxxxxxxxxx', u'Subscriber (linux-source-2.6.15 in Ubuntu)'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
('test@xxxxxxxxxxxxx', 'Assignee')]
To find out if someone is already directly subscribed to a bug, call
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2014-02-28 11:12:31 +0000
+++ lib/lp/bugs/model/bug.py 2014-04-29 01:03:20 +0000
@@ -1748,11 +1748,9 @@
if information_type in PRIVATE_INFORMATION_TYPES:
self.who_made_private = who
self.date_made_private = UTC_NOW
- required_subscribers = set([who, self.owner])
else:
self.who_made_private = None
self.date_made_private = None
- 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:
@@ -1762,59 +1760,31 @@
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 and 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:
- for pillar in pillars:
- if pillar.driver in subscribers:
- required_subscribers.add(pillar.driver)
- if pillar != ubuntu:
- if pillar.bug_supervisor is not None:
- required_subscribers.add(pillar.bug_supervisor)
- else:
- 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 the new type is private, some people may no longer have
+ # access to the bug. We ensure that the bug reporter and the
+ # person changing the privacy can see the bug, and then remove
+ # subscriptions for anyone who can't see it any more.
if information_type in PRIVATE_INFORMATION_TYPES:
- if subscribers:
- # If we're switching to private types, and the driver is
- # subscribed for a pillar (except ubuntu), we need to make
- # sure the driver maintains access.
- for pillar in pillars:
- if pillar.driver in subscribers and pillar != ubuntu:
- required_subscribers.add(pillar.driver)
- service = getUtility(IService, 'sharing')
- if len(required_subscribers):
+ service = getUtility(IService, 'sharing')
+ for person in (who, self.owner):
+ bugs, ignored, ignored = service.getVisibleArtifacts(
+ person, bugs=[self], ignore_permissions=True)
+ if not bugs:
+ # subscribe() isn't sufficient if a subscription
+ # already exists, as it will do nothing even if
+ # there is no corresponding access grant. We
+ # explicitly ensureAccessGrants() to ensure that the
+ # subscription is retained.
service.ensureAccessGrants(
- required_subscribers, who, bugs=[self],
- ignore_permissions=True)
+ [person], who, bugs=[self], ignore_permissions=True)
+ self.subscribe(person, who)
- # 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)
+ # And now fire off a job to remove any subscribers that can
+ # no longer see the bug.
+ getUtility(IRemoveArtifactSubscriptionsJobSource).create(
+ who, [self])
update_bug_heat([self.id])
-
- # As a result of the transition, some subscribers may no longer
- # have access to the bug. We need to run a job to remove any such
- # subscriptions.
- getUtility(IRemoveArtifactSubscriptionsJobSource).create(who, [self])
-
return True
def getBugTask(self, target):
=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py 2012-10-11 19:32:53 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2014-04-29 01:03:20 +0000
@@ -676,13 +676,12 @@
# should include:
# - the bug reporter
# - the person changing the state
- # - and bug/pillar owners, drivers if they are already subscribed
(bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
self.createBugTasksAndSubscribers())
initial_subscribers = set((
self.factory.makePerson(name='subscriber'), bugtask_a.owner,
- bug_owner, bugtask_a.pillar.driver))
+ bug_owner))
initial_subscribers.update(bug.getDirectSubscribers())
with person_logged_in(bug_owner):
@@ -692,22 +691,18 @@
bug.transitionToInformationType(
InformationType.PRIVATESECURITY, who=who)
subscribers = bug.getDirectSubscribers(filter_visible=True)
- expected_subscribers = set((
- default_bugtask.pillar.driver, bug_owner, who))
- self.assertContentEqual(expected_subscribers, subscribers)
+ self.assertContentEqual([bug_owner, who], subscribers)
def test_transition_to_USERDATA_information_type(self):
# When a bug is marked as USERDATA, the direct subscribers should
# include:
# - the bug reporter
# - the person changing the state
- # - and bug/pillar owners, drivers if they are already subscribed
(bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
self.createBugTasksAndSubscribers())
initial_subscribers = set((
- self.factory.makePerson(name='subscriber'), bug_owner,
- bugtask_a.pillar.driver))
+ self.factory.makePerson(name='subscriber'), bug_owner))
with person_logged_in(bug_owner):
for subscriber in initial_subscribers:
@@ -715,12 +710,7 @@
who = self.factory.makePerson(name='who')
bug.transitionToInformationType(InformationType.USERDATA, who)
subscribers = bug.getDirectSubscribers(filter_visible=True)
- expected_subscribers = set((
- default_bugtask.pillar.bug_supervisor,
- default_bugtask.pillar.driver,
- bug_owner,
- who))
- self.assertContentEqual(expected_subscribers, subscribers)
+ self.assertContentEqual([bug_owner, who], subscribers)
def test_transition_to_PUBLICSECURITY_information_type(self):
# When a security bug is unembargoed, direct subscribers should
@@ -766,38 +756,6 @@
subscribers_before_public,
subscribers_after_public)
- def test_setPillarOwnerSubscribedIfNoBugSupervisor(self):
- # The pillar owner is subscribed if the bug supervisor is not set and
- # the bug is marked as USERDATA.
-
- bug_owner = self.factory.makePerson(name='bugowner')
- bug = self.factory.makeBug(owner=bug_owner)
- with person_logged_in(bug_owner):
- who = self.factory.makePerson()
- bug.transitionToInformationType(InformationType.USERDATA, who)
- subscribers = bug.getDirectSubscribers()
- naked_bugtask = removeSecurityProxy(bug).default_bugtask
- self.assertContentEqual(
- set((naked_bugtask.pillar.owner, bug_owner, who)),
- subscribers)
-
- def test_structural_bug_supervisor_becomes_direct_on_private(self):
- # If a bug supervisor has a structural subscription to the bug, and
- # the bug is marked as private, the supervisor should get a direct
- # subscription. Otherwise they should be removed, per other tests.
- bug_supervisor = self.factory.makePerson()
- product = self.factory.makeProduct(bug_supervisor=bug_supervisor)
- bug_owner = self.factory.makePerson()
- bug = self.factory.makeBug(owner=bug_owner, target=product)
- with person_logged_in(product.owner):
- product.addSubscription(bug_supervisor, bug_supervisor)
-
- self.assertFalse(bug_supervisor in bug.getDirectSubscribers())
- with person_logged_in(bug_owner):
- who = self.factory.makePerson(name="who")
- bug.transitionToInformationType(InformationType.USERDATA, who)
- self.assertTrue(bug_supervisor in bug.getDirectSubscribers())
-
class TestBugPrivacy(TestCaseWithFactory):
Follow ups
