launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04792
[Merge] lp:~sinzui/launchpad/private-bug-4 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/private-bug-4 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/private-bug-4/+merge/73114
Use command groups in the bug mail process loop to control the order.
Launchpad bug:
https://bugs.launchpad.net/bugs/404010
https://bugs.launchpad.net/bugs/531954
https://bugs.launchpad.net/launchpad/+bug/385607
Pre-implementation: jcsackett, lifeless
Collect commands in transactions bunches and reorder before executing
them because some commands fail because order was wrong. eg.
"affects firefox" command comes after the "private yes" command, and the
second bug section won't be considered at all.
If I send an email to new@xxxxxxxxxxxxxxxxxx and I provide " assignee
some-dude" and " affects some-project", and I have the affects directive
*second*, Launchpad refuses to accept the bug
Specifying commands in the wrong order using the email interface can
lead to sensitive information being exposed. Structural subscribers are
notified of security bugs.
--------------------------------------------------------------------
RULES
* Change from a while-loop over the command list to a nest for-loop
over the bug and bugtask groups.
* ADDENDUM: The for-loop was far more complex than the while-loop
because the rules tested in bugs-emailinterface.txt required extra
levels of error control. I chose to add an iterator that orders the
commands for thw while-loop
* Add tests based on the examples in the bug to verify the ordering
issue is fixed.
QA
* Send an email to new@xxxxxxxxxxxxxxxxxx with the example content
from the bug.
security true
affects launchpad
status triaged
importance high
* Ask an admin to run the incoming mail precessing script
* Verfy the bugs are created and updated per the commands.
* Verify structural subscribers did not get the email.
LINT
lib/lp/bugs/mail/handler.py
lib/lp/bugs/mail/tests/test_handler.py
TEST
./bin/test -vv \
-t MaloneHandlerProcessTestCase -t BugCommandGroupTestCase \
-t bugs-emailinterface
IMPLEMENTATION
Added a rule to fix the handling of a misplaced affects lines when it is
100% clear that there can be only one affected target.
Added an interator to return a the bug commands in a controlled order.
Updated the method that assembled the bug commands to use BugCommandGroups.
Added tests based on the examples in the bug to verify the issues are fixed.
lib/lp/bugs/mail/handler.py
lib/lp/bugs/mail/tests/test_handler.py
--
https://code.launchpad.net/~sinzui/launchpad/private-bug-4/+merge/73114
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/private-bug-4 into lp:launchpad.
=== modified file 'lib/lp/bugs/mail/handler.py'
--- lib/lp/bugs/mail/handler.py 2011-08-24 20:53:40 +0000
+++ lib/lp/bugs/mail/handler.py 2011-08-26 22:30:27 +0000
@@ -99,7 +99,20 @@
@property
def groups(self):
- "Return the `BugTaskCommandGroup` in the order they were added."
+ "Return the `BugTaskCommandGroup`s."
+ is_new_bug = (
+ len(self.commands) > 0
+ and self.commands[0].RANK == 0
+ and self.commands[0].string_args == ['new'])
+ has_split_affects = (
+ len(self._groups) == 2
+ and self._groups[0].commands[0].RANK != 0
+ and self._groups[1].commands[0].RANK == 0)
+ if is_new_bug and has_split_affects:
+ # The affects line was in the wrong position and this
+ # exact case can be fixed.
+ self._groups[0]._commands += self._groups[1]._commands
+ del self._groups[1]
return list(self._groups)
def add(self, command_or_group):
@@ -140,6 +153,14 @@
this_bug.add(this_bugtask)
self.add(this_bug)
+ def __iter__(self):
+ for bug_group in self.groups:
+ for command in bug_group.commands:
+ yield command
+ for bugtask_group in bug_group.groups:
+ for command in bugtask_group.commands:
+ yield command
+
def add(self, command_or_group):
"""Add a `BugCommandGroup` to the groups of commands.
@@ -217,7 +238,8 @@
elif to_user.lower() != 'edit':
# Indicate that we didn't handle the mail.
return False, False, None
- return None, add_comment_to_bug, commands
+ bug_commands = [command for command in BugCommandGroups(commands)]
+ return None, add_comment_to_bug, bug_commands
def process(self, signed_msg, to_addr, filealias=None, log=None):
"""See IMailHandler."""
=== modified file 'lib/lp/bugs/mail/tests/test_handler.py'
--- lib/lp/bugs/mail/tests/test_handler.py 2011-08-24 20:10:38 +0000
+++ lib/lp/bugs/mail/tests/test_handler.py 2011-08-26 22:30:27 +0000
@@ -35,6 +35,7 @@
BugTaskCommandGroup,
MaloneHandler,
)
+from lp.bugs.model.bugnotification import BugNotification
from lp.services.mail import stub
from lp.testing import (
celebrity_logged_in,
@@ -204,6 +205,106 @@
"There is no such bug in Launchpad: 1", message)
+class MaloneHandlerProcessTestCase(TestCaseWithFactory):
+ """
+ makeSignedMessage(msgid=None, body=None, subject=None,
+ attachment_contents=None, force_transfer_encoding=False,
+ email_address=None, signing_context=None, to_address=None):
+ """
+ layer = LaunchpadFunctionalLayer
+
+ @staticmethod
+ def getLatestBugNotification():
+ return BugNotification.selectFirst(orderBy='-id')
+
+ def test_new_bug(self):
+ project = self.factory.makeProduct(name='fnord')
+ transaction.commit()
+ handler = MaloneHandler()
+ with person_logged_in(project.owner):
+ msg = self.factory.makeSignedMessage(
+ body='borked\n affects fnord',
+ subject='subject borked',
+ to_address='new@xxxxxxxxxxxxxxxxxx')
+ handler.process(msg, msg['To'])
+ notification = self.getLatestBugNotification()
+ bug = notification.bug
+ self.assertEqual(
+ [project.owner], list(bug.getDirectSubscribers()))
+ self.assertEqual(project.owner, bug.owner)
+ self.assertEqual('subject borked', bug.title)
+ self.assertEqual(1, bug.messages.count())
+ self.assertEqual('borked\n affects fnord', bug.description)
+ self.assertEqual(1, len(bug.bugtasks))
+ self.assertEqual(project, bug.bugtasks[0].target)
+
+ def test_new_bug_with_one_misplaced_affects_line(self):
+ # Affects commands in the wrong position are processed as the user
+ # intended when the bug is new and there is only one affects.
+ project = self.factory.makeProduct(name='fnord')
+ assignee = self.factory.makePerson(name='pting')
+ transaction.commit()
+ handler = MaloneHandler()
+ with person_logged_in(project.owner):
+ msg = self.factory.makeSignedMessage(
+ body='borked\n assignee pting\n affects fnord',
+ subject='affects after assignee',
+ to_address='new@xxxxxxxxxxxxxxxxxx')
+ handler.process(msg, msg['To'])
+ notification = self.getLatestBugNotification()
+ bug = notification.bug
+ self.assertEqual('affects after assignee', bug.title)
+ self.assertEqual(1, len(bug.bugtasks))
+ self.assertEqual(project, bug.bugtasks[0].target)
+ self.assertEqual(assignee, bug.bugtasks[0].assignee)
+
+ def test_new_affect_command_interleaved_with_bug_commands(self):
+ # The bug commands can appear before and after the affects command.
+ project = self.factory.makeProduct(name='fnord')
+ transaction.commit()
+ handler = MaloneHandler()
+ with person_logged_in(project.owner):
+ msg = self.factory.makeSignedMessage(
+ body='unsecure\n security yes\n affects fnord\n tag ajax',
+ subject='unsecure code',
+ to_address='new@xxxxxxxxxxxxxxxxxx')
+ handler.process(msg, msg['To'])
+ notification = self.getLatestBugNotification()
+ bug = notification.bug
+ self.assertEqual('unsecure code', bug.title)
+ self.assertEqual(True, bug.security_related)
+ self.assertEqual(['ajax'], bug.tags)
+ self.assertEqual(1, len(bug.bugtasks))
+ self.assertEqual(project, bug.bugtasks[0].target)
+
+ def test_new_security_bug(self):
+ # Structural subscribers are not notified of security bugs.
+ maintainer = self.factory.makePerson(name='maintainer')
+ project = self.factory.makeProduct(name='fnord', owner=maintainer)
+ subscriber = self.factory.makePerson(name='subscriber')
+ with person_logged_in(subscriber):
+ project.addBugSubscription(subscriber, subscriber)
+ transaction.commit()
+ handler = MaloneHandler()
+ with person_logged_in(project.owner):
+ msg = self.factory.makeSignedMessage(
+ body='bad thing\n security yes\n affects fnord',
+ subject='security issue',
+ to_address='new@xxxxxxxxxxxxxxxxxx')
+ handler.process(msg, msg['To'])
+ notification = self.getLatestBugNotification()
+ bug = notification.bug
+ self.assertEqual('security issue', bug.title)
+ self.assertEqual(True, bug.security_related)
+ self.assertEqual(1, len(bug.bugtasks))
+ self.assertEqual(project, bug.bugtasks[0].target)
+ recipients = set()
+ for notification in BugNotification.select():
+ for recipient in notification.recipients:
+ recipients.add(recipient.person)
+ self.assertContentEqual([maintainer], recipients)
+
+
class BugTaskCommandGroupTestCase(TestCase):
def test_BugTaskCommandGroup_init_with_command(self):
@@ -309,6 +410,22 @@
self.assertFalse(group._groups is group.groups)
self.assertEqual([bugtask_group_1, bugtask_group_2], group.groups)
+ def test_BugCommandGroup_groups_new_bug_with_fixable_affects(self):
+ # A new bug that affects only one target does not require the
+ # affects command to be first.
+ group = BugCommandGroup(
+ BugEmailCommands.get('bug', ['new']))
+ status_command = BugEmailCommands.get('status', ['triaged'])
+ bugtask_group_1 = BugTaskCommandGroup(status_command)
+ group.add(bugtask_group_1)
+ affects_command = BugEmailCommands.get('affects', ['fnord'])
+ bugtask_group_2 = BugTaskCommandGroup(affects_command)
+ group.add(bugtask_group_2)
+ self.assertEqual(1, len(group.groups))
+ self.assertFalse(group._groups is group.groups)
+ self.assertEqual(
+ [affects_command, status_command], group.groups[0].commands)
+
def test_BugCommandGroup__nonzero__false(self):
# A BugCommandGroup is zero is it has no commands or groups.
group = BugCommandGroup()
@@ -480,6 +597,34 @@
self.assertEqual(3, len(ordered_commands.groups))
self.assertEqual(expected, str(ordered_commands))
+ def test_BugCommandGroups__iter_(self):
+ email_commands = [
+ ('bug', '1234'),
+ ('importance', 'high'),
+ ('private', 'yes'),
+ ('bug', 'new'),
+ ('security', 'yes'),
+ ('status', 'triaged'),
+ ('affects', 'fnord'),
+ ]
+ commands = [
+ BugEmailCommands.get(name=name, string_args=[args])
+ for name, args in email_commands]
+ ordered_commands = [
+ command for command in BugCommandGroups(commands)]
+ self.assertEqual(7, len(ordered_commands))
+ expected = [
+ 'bug 1234',
+ 'private yes',
+ 'importance high',
+ 'bug new',
+ 'security yes',
+ 'affects fnord',
+ 'status triaged',
+ ]
+ self.assertEqual(
+ expected, [str(command) for command in ordered_commands])
+
class FakeSignature:
