launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04935
[Merge] lp:~wallyworld/launchpad/bug-subscribers-after-private-475775 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-subscribers-after-private-475775 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #475775 in Launchpad itself: "project subscription becomes direct bug subscription when made private"
https://bugs.launchpad.net/launchpad/+bug/475775
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-subscribers-after-private-475775/+merge/74908
This branch changes the implementation of setPrivate() and setSecurityRelated() on bug, to ensure that when a bug is marked as private or security_related, the correct direct subscriptions are in place.
== Implementation ==
The core problem - when a bug was marked as private, all indirect subscriptions were converted to direct subscriptions, creating a security hole and allowing sensitive information to leak out to those who should not see it. The new behaviour is to not touch any indirect subscriptions, and actually remove those direct subscriptions which are not allowed for the new state of the bug.
A new setPrivacyAndSecurityRelated() method is introduced. The business logic in the individual setPrivate() and setSecurityRelated() methods is moved to this new consolidated method, and the individual setPrivate() and setSecurityRelated() methods just delegate to the new method.
The new method:
- determines the required subscribers given the new private and security_related values
- determines the allowed subscribers given the new private and security_related values
- determines any subscribers which should be removed (auto removed subscribers)
- subscribes the required subscribers
- unsubscribes any existing direct subscribers who are not in the allowed set or who should be auto removed
A new parameter was added to the existing setSecurityRelated() method: "who". This is the same as the "who" parameter already used in the setPrivate() method.
It is important to note: some direct subscribers may now be unsubscribed as a result of updating the privacy/security status of a bug. This did not previously happen. To facilitate this, the bug unsubscribe() method was changed to allow the canBeUnsubscribedByUser() check to be bypassed when invoked as part of this workflow, since the user marking the bug private may not ordinarily have permission to unsubscribe someone.
The rules for determining the required subscribers are:
security=true, private=true/false ->
subscribers = the reporter + security contact for each task
security=false, private=true ->
subscribers = the reporter + bug supervisor for each task
The allowed subscribers (the ones who if already subscribed will not be unsubscribed) are:
- bug owner
- bugtask owners
- bugtask pillar owners, drivers
The above means that if a bug is not security_related, security contacts will be unsubscribed, and if a bug is not private, then bug supervisors will be unsubscribed.
If a bug is marked as security=false and private=false, then any direct subscriptions are left untouched, except for:
- if a bugtask security contact were subscribed to a bug, then they would be unsubscribed.
== Tests ==
A new test case was created in lp.bugs.model.tests.test_bugs: TestBugPrivateAndSecurityRelatedUpdates
Two existing tests were moved:
- test_setPrivate_subscribes_person_who_makes_bug_private
- test_setPrivate_does_not_subscribe_member_of_subscribed_team
And new tests added:
- test_setPrivateTrueAndSecurityRelatedTrue
- test_setPrivateFalseAndSecurityRelatedTrue
- test_setPrivateTrueAndSecurityRelatedFalse
- test_setPrivateFalseAndSecurityRelatedFalse
Other tests were tweaked to reflect the new behaviour and to update calls to setSecurityRelated() to take the new "who" parameter.
The existing BugSecrecyEditView tests are sufficient to ensure the new consolidated method gets correctly invoked when setting privacy/security via the ui.
== Lint ==
There's a bit of noise from the doc tests.
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/configure.zcml
lib/lp/bugs/browser/bug.py
lib/lp/bugs/browser/tests/test_bug_views.py
lib/lp/bugs/doc/bug-heat.txt
lib/lp/bugs/doc/bug.txt
lib/lp/bugs/doc/bugnotification-email.txt
lib/lp/bugs/doc/bugnotification-sending.txt
lib/lp/bugs/doc/security-teams.txt
lib/lp/bugs/interfaces/bug.py
lib/lp/bugs/mail/commands.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/model/tests/test_bug.py
lib/lp/bugs/model/tests/test_bugsummary.py
lib/lp/bugs/scripts/bugimport.py
lib/lp/bugs/tests/test_bugchanges.py
./lib/lp/bugs/doc/bug-heat.txt
296: narrative exceeds 78 characters.
./lib/lp/bugs/doc/bug.txt
457: source exceeds 78 characters.
1129: narrative exceeds 78 characters.
1164: want exceeds 78 characters.
1165: want exceeds 78 characters.
1166: want exceeds 78 characters.
./lib/lp/bugs/doc/bugnotification-email.txt
1: narrative uses a moin header.
31: source exceeds 78 characters.
39: narrative uses a moin header.
90: source exceeds 78 characters.
116: narrative uses a moin header.
174: source exceeds 78 characters.
196: want has trailing whitespace.
207: source exceeds 78 characters.
213: source exceeds 78 characters.
230: source exceeds 78 characters.
236: source exceeds 78 characters.
280: narrative uses a moin header.
347: narrative uses a moin header.
417: narrative uses a moin header.
482: narrative uses a moin header.
./lib/lp/bugs/doc/bugnotification-sending.txt
286: want exceeds 78 characters.
./lib/lp/bugs/scripts/bugimport.py
29: redefinition of unused 'ET' from line 27
--
https://code.launchpad.net/~wallyworld/launchpad/bug-subscribers-after-private-475775/+merge/74908
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-subscribers-after-private-475775 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2011-08-31 10:40:04 +0000
+++ lib/lp/bugs/browser/bug.py 2011-09-12 04:40:38 +0000
@@ -847,8 +847,9 @@
data = dict(data)
# We handle privacy changes by hand instead of leaving it to
- # the usual machinery because we must use bug.setPrivate() to
- # ensure auditing information is recorded.
+ # the usual machinery because we must use
+ # bug.setPrivacyAndSecurityRelated() to ensure auditing information is
+ # recorded.
bug = self.context.bug
bug_before_modification = Snapshot(
bug, providing=providedBy(bug))
@@ -856,9 +857,9 @@
user_will_be_subscribed = (
private and bug.getSubscribersForPerson(self.user).is_empty())
security_related = data.pop('security_related')
- private_changed = bug.setPrivate(
- private, getUtility(ILaunchBag).user)
- security_related_changed = bug.setSecurityRelated(security_related)
+ user = getUtility(ILaunchBag).user
+ (private_changed, security_related_changed) = (
+ bug.setPrivacyAndSecurityRelated(private, security_related, user))
if private_changed or security_related_changed:
changed_fields = []
if private_changed:
=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py 2011-08-31 10:40:04 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py 2011-09-12 04:40:38 +0000
@@ -216,7 +216,7 @@
layer = DatabaseFunctionalLayer
def createInitializedSecrecyView(self, person=None, bug=None,
- request=None):
+ request=None, security_related=False):
"""Create and return an initialized BugSecrecyView."""
if person is None:
person = self.factory.makePerson()
@@ -226,7 +226,8 @@
view = create_initialized_view(
bug.default_bugtask, name='+secrecy', form={
'field.private': 'on',
- 'field.security_related': '',
+ 'field.security_related':
+ 'on' if security_related else 'off',
'field.actions.change': 'Change',
},
request=request)
@@ -274,7 +275,7 @@
# subscription information resulting from the update to the bug
# privacy.
person = self.factory.makePerson()
- bug = self.factory.makeBug()
+ bug = self.factory.makeBug(owner=person)
with person_logged_in(person):
bug.subscribe(person, person)
@@ -283,7 +284,7 @@
method='POST', form={
'field.actions.change': 'Change',
'field.private': 'on',
- 'field.security_related': 'ff'},
+ 'field.security_related': 'off'},
**extra)
view = self.createInitializedSecrecyView(person, bug, request)
cache_data = simplejson.loads(view.render())
@@ -297,3 +298,12 @@
subscription_data['person_link'])
self.assertEqual(
'Discussion', subscription_data['bug_notification_level'])
+
+ def test_set_security_related(self):
+ # Test that the bug attribute 'security_related' can be updated
+ # using the view.
+ owner = self.factory.makePerson()
+ bug = self.factory.makeBug(owner=owner)
+ self.createInitializedSecrecyView(bug=bug, security_related=True)
+ with person_logged_in(owner):
+ self.assertTrue(bug.security_related)
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-09-08 22:50:59 +0000
+++ lib/lp/bugs/configure.zcml 2011-09-12 04:40:38 +0000
@@ -853,6 +853,7 @@
removeWatch
setPrivate
setSecurityRelated
+ setPrivacyAndSecurityRelated
setStatus
subscribe
unlinkBranch
=== modified file 'lib/lp/bugs/doc/bug-heat.txt'
--- lib/lp/bugs/doc/bug-heat.txt 2011-08-03 11:20:50 +0000
+++ lib/lp/bugs/doc/bug-heat.txt 2011-09-12 04:40:38 +0000
@@ -84,7 +84,7 @@
Setting the bug as security related adds another 250 heat points.
- >>> changed = bug.setSecurityRelated(True)
+ >>> changed = bug.setSecurityRelated(True, bug_owner)
>>> bug.heat
406
@@ -96,7 +96,7 @@
And marking it not security-related removes 250 points.
- >>> changed = bug.setSecurityRelated(False)
+ >>> changed = bug.setSecurityRelated(False, bug_owner)
>>> bug.heat
6
=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt 2011-08-01 05:25:59 +0000
+++ lib/lp/bugs/doc/bug.txt 2011-09-12 04:40:38 +0000
@@ -750,7 +750,8 @@
>>> firefox_bug.security_related
False
- >>> changed = firefox_bug.setSecurityRelated(True)
+ >>> changed = firefox_bug.setSecurityRelated(True,
+ ... getUtility(ILaunchBag).user)
>>> bug_security_changed = ObjectModifiedEvent(
... firefox_bug, bug_before_modification, ["security_related"])
=== modified file 'lib/lp/bugs/doc/bugnotification-email.txt'
--- lib/lp/bugs/doc/bugnotification-email.txt 2011-07-18 03:05:04 +0000
+++ lib/lp/bugs/doc/bugnotification-email.txt 2011-09-12 04:40:38 +0000
@@ -87,7 +87,7 @@
New security related bugs are sent with a prominent warning:
- >>> changed = bug_four.setSecurityRelated(True)
+ >>> changed = bug_four.setSecurityRelated(True, getUtility(ILaunchBag).user)
>>> subject, body = generate_bug_add_email(bug_four)
>>> subject
@@ -204,7 +204,7 @@
>>> edited_bug.setPrivate(True, getUtility(ILaunchBag).user)
True
- >>> changed = edited_bug.setSecurityRelated(True)
+ >>> changed = edited_bug.setSecurityRelated(True, getUtility(ILaunchBag).user)
>>> bug_delta = BugDelta(
... bug=edited_bug,
... bugurl="http://www.example.com/bugs/2";,
@@ -227,7 +227,7 @@
>>> edited_bug.setPrivate(False, getUtility(ILaunchBag).user)
True
- >>> changed = edited_bug.setSecurityRelated(False)
+ >>> changed = edited_bug.setSecurityRelated(False, getUtility(ILaunchBag).user)
>>> bug_delta = BugDelta(
... bug=edited_bug,
... bugurl="http://www.example.com/bugs/2";,
=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt 2011-06-28 15:04:29 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt 2011-09-12 04:40:38 +0000
@@ -768,8 +768,12 @@
>>> bug_three.private
True
+When the bug is made private, the bugtask owners and supervisors are
+subscribed in addition any other direct subscribers which already exist.
+
>>> for message in trigger_and_get_email_messages(bug_three):
... print message['To'], message.get_all('X-Launchpad-Bug-Private')
+ foo.bar@xxxxxxxxxxxxx ['yes']
test@xxxxxxxxxxxxx ['yes']
@@ -786,13 +790,14 @@
>>> for message in trigger_and_get_email_messages(bug_three):
... print message['To'], (
... message.get_all('X-Launchpad-Bug-Security-Vulnerability'))
+ foo.bar@xxxxxxxxxxxxx ['no']
test@xxxxxxxxxxxxx ['no']
The presence of the security flag on a bug is, surprise, denoted by a
simple "yes":
>>> with lp_dbuser():
- ... bug_three.setSecurityRelated(True)
+ ... bug_three.setSecurityRelated(True, getUtility(ILaunchBag).user)
True
>>> bug_three.security_related
True
@@ -800,6 +805,7 @@
>>> for message in trigger_and_get_email_messages(bug_three):
... print message['To'], (
... message.get_all('X-Launchpad-Bug-Security-Vulnerability'))
+ foo.bar@xxxxxxxxxxxxx ['yes']
test@xxxxxxxxxxxxx ['yes']
@@ -810,8 +816,8 @@
people who have ever commented on the bug. It's a space-separated
list.
- >>> for message in trigger_and_get_email_messages(bug_three):
- ... print message.get('X-Launchpad-Bug-Commenters')
+ >>> message = trigger_and_get_email_messages(bug_three)[0]
+ >>> print message.get('X-Launchpad-Bug-Commenters')
name12
>>> from lp.registry.interfaces.person import IPersonSet
@@ -822,8 +828,8 @@
... ignored = getUtility(IBugMessageSet).createMessage(
... 'Hungry', bug_three, foo_bar, "Make me a sandwich.")
- >>> for message in trigger_and_get_email_messages(bug_three):
- ... print message.get('X-Launchpad-Bug-Commenters')
+ >>> message = trigger_and_get_email_messages(bug_three)[0]
+ >>> print message.get('X-Launchpad-Bug-Commenters')
name12 name16
It only lists each user once, no matter how many comments they've
@@ -833,8 +839,8 @@
... ignored = getUtility(IBugMessageSet).createMessage(
... 'Hungry', bug_three, foo_bar, "Make me a sandwich.")
- >>> for message in trigger_and_get_email_messages(bug_three):
- ... print message.get('X-Launchpad-Bug-Commenters')
+ >>> message = trigger_and_get_email_messages(bug_three)[0]
+ >>> print message.get('X-Launchpad-Bug-Commenters')
name12 name16
@@ -844,8 +850,8 @@
The X-Launchpad-Bug-Reporter header contains information about the Launchpad
user who originally reported the bug and opened the bug's first bug task.
- >>> for message in trigger_and_get_email_messages(bug_three):
- ... print message.get('X-Launchpad-Bug-Reporter')
+ >>> message = trigger_and_get_email_messages(bug_three)[0]
+ >>> print message.get('X-Launchpad-Bug-Reporter')
Foo Bar (name16)
@@ -1176,7 +1182,7 @@
----------------------------------------------------------------------
To: owner@xxxxxxxxxxx
...
- You received this bug notification because you are a member of
+ You received this bug notification because you are a member of
Addressless Team, which is subscribed to the bug report.
...
----------------------------------------------------------------------
@@ -1240,7 +1246,7 @@
----------------------------------------------------------------------
To: owner@xxxxxxxxxxx
...
- You received this bug notification because you are a member of
+ You received this bug notification because you are a member of
Addressless Team, which is subscribed to the bug report.
...
----------------------------------------------------------------------
@@ -1293,7 +1299,7 @@
no comment for no-priv.
<BLANKLINE>
--
- You received this bug notification because you are a member of
+ You received this bug notification because you are a member of
Addressless Team, which is subscribed to the bug report.
...
----------------------------------------------------------------------
@@ -1357,7 +1363,7 @@
----------------------------------------------------------------------
To: owner@xxxxxxxxxxx
...
- You received this bug notification because you are a member of
+ You received this bug notification because you are a member of
Addressless Team, which is subscribed to the bug report.
...
----------------------------------------------------------------------
@@ -1412,7 +1418,7 @@
+ Whatever else
<BLANKLINE>
--
- You received this bug notification because you are a member of
+ You received this bug notification because you are a member of
Addressless Team, which is subscribed to the bug report.
http://bugs.launchpad.dev/bugs/1
...
@@ -1479,7 +1485,7 @@
----------------------------------------------------------------------
To: owner@xxxxxxxxxxx
...
- You received this bug notification because you are a member of
+ You received this bug notification because you are a member of
Addressless Team, which is subscribed to the bug report.
...
----------------------------------------------------------------------
=== modified file 'lib/lp/bugs/doc/security-teams.txt'
--- lib/lp/bugs/doc/security-teams.txt 2011-08-01 05:25:59 +0000
+++ lib/lp/bugs/doc/security-teams.txt 2011-09-12 04:40:38 +0000
@@ -265,7 +265,7 @@
>>> subscriber_names(bug)
[u'name12', u'name16']
- >>> bug.setSecurityRelated(False)
+ >>> bug.setSecurityRelated(False, getUtility(ILaunchBag).user)
True
>>> bug.security_related
@@ -296,7 +296,7 @@
>>> bug.addTask(owner=reporter, target=distribution)
<BugTask ...>
>>> old_state = Snapshot(bug, providing=IBug)
- >>> bug.setSecurityRelated(True)
+ >>> bug.setSecurityRelated(True, getUtility(ILaunchBag).user)
True
>>> notify(ObjectModifiedEvent(bug, old_state, ['security_related']))
>>> for subscriber_name in sorted(
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2011-08-15 12:09:48 +0000
+++ lib/lp/bugs/interfaces/bug.py 2011-09-12 04:40:38 +0000
@@ -849,11 +849,13 @@
@mutator_for(security_related)
@operation_parameters(security_related=copy_field(security_related))
+ @call_with(who=REQUEST_USER)
@export_write_operation()
- def setSecurityRelated(security_related):
+ def setSecurityRelated(security_related, who):
"""Set bug security.
:security_related: True/False.
+ :who: The IPerson who is making the change.
This may also cause the security contact to be subscribed
if one is registered and if the bug is not private.
@@ -861,6 +863,23 @@
Return True if a change is made, False otherwise.
"""
+ @operation_parameters(
+ private=copy_field(private),
+ security_related=copy_field(security_related),
+ )
+ @call_with(who=REQUEST_USER)
+ @export_write_operation()
+ @operation_for_version("devel")
+ def setPrivacyAndSecurityRelated(private, security_related, who):
+ """Set bug privacy and security .
+
+ :private: True/False.
+ :security_related: True/False.
+ :who: The IPerson who is making the change.
+
+ Return (private_changed, security_related_changed) tuple.
+ """
+
def getBugTask(target):
"""Return the bugtask with the specified target.
=== modified file 'lib/lp/bugs/mail/commands.py'
--- lib/lp/bugs/mail/commands.py 2011-08-24 21:02:55 +0000
+++ lib/lp/bugs/mail/commands.py 2011-09-12 04:40:38 +0000
@@ -241,13 +241,13 @@
context, providing=providedBy(context))
# Apply requested changes.
+ user = getUtility(ILaunchBag).user
if security_related:
- user = getUtility(ILaunchBag).user
if context.setPrivate(True, user):
edited = True
edited_fields.add('private')
if context.security_related != security_related:
- context.setSecurityRelated(security_related)
+ context.setSecurityRelated(security_related, user)
edited = True
edited_fields.add('security_related')
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2011-08-30 19:25:53 +0000
+++ lib/lp/bugs/model/bug.py 2011-09-12 04:40:38 +0000
@@ -811,7 +811,7 @@
self.updateHeat()
return sub
- def unsubscribe(self, person, unsubscribed_by):
+ def unsubscribe(self, person, unsubscribed_by, **kwargs):
"""See `IBug`."""
# Drop cached subscription info.
clear_property_cache(self)
@@ -821,9 +821,11 @@
if person is None:
person = unsubscribed_by
+ ignore_permissions = kwargs.get('ignore_permissions', False)
for sub in self.subscriptions:
if sub.person.id == person.id:
- if not sub.canBeUnsubscribedByUser(unsubscribed_by):
+ if (not ignore_permissions
+ and not sub.canBeUnsubscribedByUser(unsubscribed_by)):
raise UserCannotUnsubscribePerson(
'%s does not have permission to unsubscribe %s.' % (
unsubscribed_by.displayname,
@@ -1644,28 +1646,19 @@
return bugtask
- def setPrivate(self, private, who):
- """See `IBug`.
-
- We also record who made the change and when the change took
- place.
- """
+ def setPrivacyAndSecurityRelated(self, private, security_related, who):
+ """ See `IBug`."""
+ private_changed = False
+ security_related_changed = False
+
+ # Before we update the privacy or security_related status, we need to
+ # reconcile the subscribers to avoid leaking private information.
+ if (self.private != private
+ or self.security_related != security_related):
+ self.reconcileSubscribers(private, security_related, who)
+
if self.private != private:
- if private:
- # Change indirect subscribers into direct subscribers
- # *before* setting private because
- # getIndirectSubscribers() behaves differently when
- # the bug is private.
- for person in self.getIndirectSubscribers():
- self.subscribe(person, who)
- subscribers_for_who = self.getSubscribersForPerson(who)
- if subscribers_for_who.is_empty():
- # We also add `who` as a subscriber, if they're not
- # already directly subscribed or part of a team
- # that's directly subscribed, so that they can
- # see the bug they've just marked private.
- self.subscribe(who, who)
-
+ private_changed = True
self.private = private
if private:
@@ -1680,25 +1673,121 @@
for attachment in self.attachments_unpopulated:
attachment.libraryfile.restricted = private
- # Correct the heat for the bug immediately, so that we don't have
- # to wait for the next calculation job for the adjusted heat.
- self.updateHeat()
- return True # Changed.
- else:
- return False # Not changed.
-
- def setSecurityRelated(self, security_related):
- """Setter for the `security_related` property."""
if self.security_related != security_related:
+ security_related_changed = True
self.security_related = security_related
+ if private_changed or security_related_changed:
# Correct the heat for the bug immediately, so that we don't have
# to wait for the next calculation job for the adjusted heat.
self.updateHeat()
- return True # Changed
- else:
- return False # Unchanged
+ return private_changed, security_related_changed
+
+ def setPrivate(self, private, who):
+ """See `IBug`.
+
+ We also record who made the change and when the change took
+ place.
+ """
+ return self.setPrivacyAndSecurityRelated(
+ private, self.security_related, who)[0]
+
+ def setSecurityRelated(self, security_related, who):
+ """Setter for the `security_related` property."""
+ return self.setPrivacyAndSecurityRelated(
+ self.private, security_related, who)[1]
+
+ def getRequiredSubscribers(self, for_private, for_security_related, who):
+ """Return the mandatory subscribers for a bug with given attributes.
+
+ When a bug is marked as private or security related, it is required
+ that certain people be subscribed so they can access details about the
+ bug. The rules are:
+ security=true, private=true/false ->
+ subscribers = the reporter + security contact for each task
+ security=false, private=true ->
+ subscribers = the reporter + bug supervisor for each task
+ security=false, private=false ->
+ subscribers = ()
+
+ If bug supervisor or security contact is unset, fallback to bugtask
+ reporter/owner.
+ """
+ if not for_private and not for_security_related:
+ return set()
+ result = set()
+ for bugtask in self.bugtasks:
+ result.add(bugtask.owner)
+ if for_security_related:
+ result.add(bugtask.pillar.security_contact or bugtask.owner)
+ elif for_private:
+ result.add(bugtask.pillar.bug_supervisor or bugtask.owner)
+ if for_private:
+ subscribers_for_who = self.getSubscribersForPerson(who)
+ if subscribers_for_who.is_empty():
+ result.add(who)
+ return result
+
+ def getAutoRemovedSubscribers(self, for_private, for_security_related):
+ """Return the to be removed subscribers for bug with given attributes.
+
+ When a bug's privacy or security related attributes change, some
+ existing subscribers may need to be automatically removed.
+ The rules are:
+ security=false, private=false ->
+ auto removed subscribers = (bugtask security contacts)
+
+ """
+ result = set()
+ for bugtask in self.bugtasks:
+ if not for_private and not for_security_related:
+ if bugtask.pillar.security_contact:
+ result.add(bugtask.pillar.security_contact)
+ return result
+
+ def reconcileSubscribers(self, for_private, for_security_related, who):
+ """ Ensure only appropriate people are subscribed to private bugs.
+
+ When a bug is marked as either private = True or security_related =
+ True, we need to ensure that only people who are authorised to know
+ about the privileged contents of the bug remain directly subscribed
+ to it. So we:
+ 1. Get the required subscribers depending on the bug status.
+ 2. Get the auto removed subscribers depending on the bug status.
+ eg security contacts when a bug is updated to security related =
+ false.
+ 3. Get the allowed subscribers = required subscribers
+ + bugtask owners
+ 4. Remove any current direct subscribers who are not allowed or are
+ to be auto removed.
+ 5. Add any subscribers who are required.
+ """
+ current_direct_subscribers = (
+ self.getSubscriptionInfo().direct_subscribers)
+ required_subscribers = self.getRequiredSubscribers(
+ for_private, for_security_related, who)
+ auto_removed_subscribers = self.getAutoRemovedSubscribers(
+ for_private, for_security_related)
+ allowed_subscribers = set()
+ allowed_subscribers.add(self.owner)
+ for bugtask in self.bugtasks:
+ allowed_subscribers.add(bugtask.owner)
+ allowed_subscribers.add(bugtask.pillar.owner)
+ allowed_subscribers.update(set(bugtask.pillar.drivers))
+ allowed_subscribers = required_subscribers.union(allowed_subscribers)
+
+ subscribers_to_remove = auto_removed_subscribers
+ if for_private or for_security_related:
+ subscribers_to_remove = auto_removed_subscribers.union(
+ current_direct_subscribers.difference(allowed_subscribers))
+ for subscriber in subscribers_to_remove:
+ self.unsubscribe(subscriber, who, ignore_permissions=True)
+
+ subscribers_to_add = (
+ required_subscribers.difference(current_direct_subscribers))
+ for subscriber in subscribers_to_add:
+ self.subscribe(subscriber, who)
def getBugTask(self, target):
"""See `IBug`."""
=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py 2011-08-16 16:12:05 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2011-09-12 04:40:38 +0000
@@ -6,6 +6,7 @@
from storm.store import Store
from testtools.testcase import ExpectedException
from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.enum import (
@@ -434,27 +435,6 @@
info = bug.getSubscriptionInfo(BugNotificationLevel.METADATA)
self.assertEqual(BugNotificationLevel.METADATA, info.level)
- def test_setPrivate_subscribes_person_who_makes_bug_private(self):
- # When setPrivate(True) is called on a bug, the person who is
- # marking the bug private is subscribed to the bug.
- bug = self.factory.makeBug()
- person = self.factory.makePerson()
- with person_logged_in(person):
- bug.setPrivate(True, person)
- self.assertTrue(bug.personIsDirectSubscriber(person))
-
- def test_setPrivate_does_not_subscribe_member_of_subscribed_team(self):
- # When setPrivate(True) is called on a bug, the person who is
- # marking the bug private will not be subscribed if they're
- # already a member of a team which is a direct subscriber.
- bug = self.factory.makeBug()
- team = self.factory.makeTeam()
- person = team.teamowner
- with person_logged_in(person):
- bug.subscribe(team, person)
- bug.setPrivate(True, person)
- self.assertFalse(bug.personIsDirectSubscriber(person))
-
def test_getVisibleLinkedBranches_doesnt_rtn_inaccessible_branches(self):
# If a Bug has branches linked to it that the current user
# cannot access, those branches will not be returned in its
@@ -485,6 +465,172 @@
self.assertNotIn(private_branch, linked_branches)
+class TestBugPrivateAndSecurityRelatedUpdates(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_setPrivate_subscribes_person_who_makes_bug_private(self):
+ # When setPrivate(True) is called on a bug, the person who is
+ # marking the bug private is subscribed to the bug.
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ bug.setPrivate(True, person)
+ self.assertTrue(bug.personIsDirectSubscriber(person))
+
+ def test_setPrivate_does_not_subscribe_member_of_subscribed_team(self):
+ # When setPrivate(True) is called on a bug, the person who is
+ # marking the bug private will not be subscribed if they're
+ # already a member of a team which is a direct subscriber.
+ bug = self.factory.makeBug()
+ team = self.factory.makeTeam()
+ person = team.teamowner
+ with person_logged_in(person):
+ bug.subscribe(team, person)
+ bug.setPrivate(True, person)
+ self.assertFalse(bug.personIsDirectSubscriber(person))
+
+ def createBugTasksAndSubscribers(self, private_security_related=False):
+ # Used with the various setPrivateAndSecurityRelated tests to create
+ # a bug and add some initial subscribers.
+ bug_owner = self.factory.makePerson(name='bugowner')
+ bug = self.factory.makeBug(
+ owner=bug_owner,
+ private=private_security_related,
+ security_related=private_security_related)
+ owner_a = self.factory.makePerson(name='ownera')
+ security_contact_a = self.factory.makePerson(name='securitycontacta')
+ bug_supervisor_a = self.factory.makePerson(name='bugsupervisora')
+ driver_a = self.factory.makePerson(name='drivera')
+ product_a = self.factory.makeProduct(
+ owner=owner_a,
+ security_contact=security_contact_a,
+ bug_supervisor=bug_supervisor_a,
+ driver=driver_a)
+ owner_b = self.factory.makePerson(name='ownerb')
+ security_contact_b = self.factory.makePerson(name='securitycontactb')
+ product_b = self.factory.makeProduct(
+ owner=owner_b,
+ security_contact=security_contact_b)
+ bugtask_a = self.factory.makeBugTask(bug=bug, target=product_a)
+ bugtask_b = self.factory.makeBugTask(bug=bug, target=product_b)
+ naked_bugtask_a = removeSecurityProxy(bugtask_a)
+ naked_bugtask_b = removeSecurityProxy(bugtask_b)
+ return bug, bug_owner, naked_bugtask_a, naked_bugtask_b
+
+ def test_setPrivateTrueAndSecurityRelatedTrue(self):
+ # When a bug is marked as private=true and security_related=true, the
+ # only direct subscribers should be:
+ # - the bugtask reporters
+ # - the bugtask pillar security contacts (if set)
+ # - the person changing the state
+ # - and bug/pillar owners, drivers if they are already subscribed
+
+ (bug, bug_owner, bugtask_a, bugtask_b) = (
+ self.createBugTasksAndSubscribers())
+ initial_subscribers = set(
+ (self.factory.makePerson(), bug_owner,
+ bugtask_a.pillar.security_contact, bugtask_a.pillar.driver))
+
+ with person_logged_in(bug_owner):
+ for subscriber in initial_subscribers:
+ bug.subscribe(subscriber, bug_owner)
+ who = self.factory.makePerson()
+ bug.setPrivacyAndSecurityRelated(
+ private=True, security_related=True, who=who)
+ subscribers = bug.getDirectSubscribers()
+ self.assertContentEqual(
+ set((bugtask_a.pillar.security_contact,
+ bugtask_a.pillar.driver,
+ bugtask_b.pillar.security_contact,
+ bugtask_a.owner, bugtask_b.owner,
+ bug_owner, who)),
+ subscribers
+ )
+
+ def test_setPrivateTrueAndSecurityRelatedFalse(self):
+ # When a bug is marked as private=true and security_related=false, the
+ # only direct subscribers should be:
+ # - the bugtask reporters
+ # - the bugtask pillar bug supervisors (if set)
+ # - the person changing the state
+ # - and bug/pillar owners, drivers if they are already subscribed
+
+ (bug, bug_owner, bugtask_a, bugtask_b) = (
+ self.createBugTasksAndSubscribers())
+ initial_subscribers = set(
+ (self.factory.makePerson(), bug_owner,
+ bugtask_a.pillar.security_contact, bugtask_a.pillar.driver))
+
+ with person_logged_in(bug_owner):
+ for subscriber in initial_subscribers:
+ bug.subscribe(subscriber, bug_owner)
+ who = self.factory.makePerson()
+ bug.setPrivacyAndSecurityRelated(
+ private=True, security_related=False, who=who)
+ subscribers = bug.getDirectSubscribers()
+ self.assertContentEqual(
+ set((bugtask_a.pillar.bug_supervisor,
+ bugtask_a.pillar.driver,
+ bugtask_a.owner, bugtask_b.owner,
+ bug_owner, who)),
+ subscribers
+ )
+
+ def test_setPrivateFalseAndSecurityRelatedTrue(self):
+ # When a bug is marked as private=false and security_related=true, the
+ # only direct subscribers should be:
+ # - the bugtask reporters
+ # - the bugtask pillar security contacts (if set)
+ # - and bug/pillar owners, drivers if they are already subscribed
+
+ (bug, bug_owner, bugtask_a, bugtask_b) = (
+ self.createBugTasksAndSubscribers())
+ initial_subscribers = set(
+ (self.factory.makePerson(), bug_owner,
+ bugtask_a.pillar.security_contact, bugtask_a.pillar.driver,
+ bugtask_a.pillar.bug_supervisor))
+
+ with person_logged_in(bug_owner):
+ for subscriber in initial_subscribers:
+ bug.subscribe(subscriber, bug_owner)
+ who = self.factory.makePerson()
+ bug.setPrivacyAndSecurityRelated(
+ private=False, security_related=True, who=who)
+ subscribers = bug.getDirectSubscribers()
+ self.assertContentEqual(
+ set((bugtask_a.pillar.security_contact,
+ bugtask_a.pillar.driver,
+ bugtask_b.pillar.security_contact,
+ bugtask_a.owner, bugtask_b.owner,
+ bug_owner)),
+ subscribers
+ )
+
+ def test_setPrivateFalseAndSecurityRelatedFalse(self):
+ # When a bug is marked as private=false and security_related=false,
+ # any existing subscriptions are left alone apart from any security
+ # contacts being unsubscribed.
+
+ (bug, bug_owner, bugtask_a, bugtask_b) = (
+ self.createBugTasksAndSubscribers(private_security_related=True))
+ initial_subscribers = set(
+ (self.factory.makePerson(), bug_owner,
+ bugtask_a.pillar.security_contact, bugtask_a.pillar.driver))
+
+ with person_logged_in(bug_owner):
+ for subscriber in initial_subscribers:
+ bug.subscribe(subscriber, bug_owner)
+ who = self.factory.makePerson()
+ expected_direct_subscribers = set(bug.getDirectSubscribers())
+ bug.setPrivacyAndSecurityRelated(
+ private=False, security_related=False, who=who)
+ subscribers = bug.getDirectSubscribers()
+ expected_direct_subscribers.remove(bugtask_a.pillar.security_contact)
+ for subscriber in expected_direct_subscribers:
+ self.assertTrue(subscriber in subscribers)
+
+
class TestBugAutoConfirmation(TestCaseWithFactory):
"""Tests for auto confirming bugs"""
=== modified file 'lib/lp/bugs/model/tests/test_bugsummary.py'
--- lib/lp/bugs/model/tests/test_bugsummary.py 2011-08-22 13:23:43 +0000
+++ lib/lp/bugs/model/tests/test_bugsummary.py 2011-09-12 04:40:38 +0000
@@ -245,11 +245,11 @@
3 - count)
def test_makePrivate(self):
- product = self.factory.makeProduct()
- bug = self.factory.makeBug(product=product)
-
person_a = self.factory.makePerson()
person_b = self.factory.makePerson()
+ product = self.factory.makeProduct()
+ bug = self.factory.makeBug(product=product, owner=person_b)
+
bug.subscribe(person=person_a, subscribed_by=person_a)
# Make the bug private. We have to use the Python API to ensure
@@ -263,10 +263,10 @@
0)
self.assertEqual(
self.getCount(person_a, BugSummary.product == product),
+ 0)
+ self.assertEqual(
+ self.getCount(person_b, BugSummary.product == product),
1)
- self.assertEqual(
- self.getCount(person_b, BugSummary.product == product),
- 0)
# Confirm implicit subscriptions work too.
self.assertEqual(
self.getCount(bug.owner, BugSummary.product == product),
=== modified file 'lib/lp/bugs/scripts/bugimport.py'
--- lib/lp/bugs/scripts/bugimport.py 2011-05-27 19:53:20 +0000
+++ lib/lp/bugs/scripts/bugimport.py 2011-09-12 04:40:38 +0000
@@ -328,9 +328,9 @@
self.createAttachments(bug, msg, commentnode)
# set up bug
- bug.setPrivate(get_value(bugnode, 'private') == 'True', owner)
- bug.setSecurityRelated(
- get_value(bugnode, 'security_related') == 'True')
+ private = get_value(bugnode, 'private') == 'True'
+ security_related = get_value(bugnode, 'security_related') == 'True'
+ bug.setPrivacyAndSecurityRelated(private, security_related, owner)
bug.name = get_value(bugnode, 'nickname')
description = get_value(bugnode, 'description')
if description:
=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py 2011-08-03 11:00:11 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py 2011-09-12 04:40:38 +0000
@@ -648,7 +648,7 @@
def test_mark_as_security_vulnerability(self):
# Marking a bug as a security vulnerability adds to the bug's
# activity log and sends a notification.
- self.bug.setSecurityRelated(False)
+ self.bug.setSecurityRelated(False, self.user)
self.changeAttribute(self.bug, 'security_related', True)
security_change_activity = {
@@ -672,7 +672,7 @@
def test_unmark_as_security_vulnerability(self):
# Unmarking a bug as a security vulnerability adds to the
# bug's activity log and sends a notification.
- self.bug.setSecurityRelated(True)
+ self.bug.setSecurityRelated(True, self.user)
self.changeAttribute(self.bug, 'security_related', False)
security_change_activity = {
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-09-06 05:58:25 +0000
+++ lib/lp/testing/factory.py 2011-09-12 04:40:38 +0000
@@ -1627,8 +1627,8 @@
return branch.createBranchRevision(sequence, revision)
def makeBug(self, product=None, owner=None, bug_watch_url=None,
- private=False, date_closed=None, title=None,
- date_created=None, description=None, comment=None,
+ private=False, security_related=False, date_closed=None,
+ title=None, date_created=None, description=None, comment=None,
status=None, distribution=None, milestone=None, series=None,
tags=None, sourcepackagename=None):
"""Create and return a new, arbitrary Bug.
@@ -1679,6 +1679,7 @@
sourcepackagename=sourcepackagename)
create_bug_params = CreateBugParams(
owner, title, comment=comment, private=private,
+ security_related=security_related,
datecreated=date_created, description=description,
status=status, tags=tags)
create_bug_params.setBugTarget(
