launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04763
[Merge] lp:~sinzui/launchpad/private-bug-3 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/private-bug-3 into lp:launchpad with lp:~sinzui/launchpad/private-bug-2 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/private-bug-3/+merge/72791
Introduce command groups to sort and group bug email commands.
Launchpad bug: https://bugs.launchpad.net/bugs/404010
Pre-implementation: lifeless, jcsackett
My goal is to fix a cluster of bugs where email commands are processed in
the wrong order or without context which causes security and privacy
leaks.
This branch introduces classes to organise the commands. My next branch
will use the groups in the bug email command processing loop to process
them in the correct order and context.
--------------------------------------------------------------------
RULES
* Update bug email commands with an attribute that can be used to
sort them so that they are collected/executed in a predicable
order.
* Collect commands in transaction bunches and reorder before executing
them.
* Create a class to group bugtask commands
* Create a class to group bug commands and one or more bugtasks
* Create a class that builds the groups.
QA
* None, no code uses these classes
LINT
lib/lp/bugs/mail/commands.py
lib/lp/bugs/mail/handler.py
lib/lp/bugs/mail/tests/test_handler.py
lib/lp/services/mail/interfaces.py
TEST
./bin/test -vv --layer=UnitTests lp.bugs.mail.tests.test_handler
IMPLEMENTATION
Added the RANK attribute to bug email commands so that they can be sorted
by order of precedence. Zero (0) is the most important because it is used
to get to create a bug or bugtask. Bug commands and bugtask commands are
ranked separately since operations on them are independent.
lib/lp/services/mail/interfaces.py
lib/lp/bugs/mail/commands.py
Added classes to group and sort the bug email commands.
lib/lp/bugs/mail/handler.py
lib/lp/bugs/mail/tests/test_handler.py
--
https://code.launchpad.net/~sinzui/launchpad/private-bug-3/+merge/72791
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/private-bug-3 into lp:launchpad.
=== modified file 'lib/lp/bugs/mail/commands.py'
--- lib/lp/bugs/mail/commands.py 2011-08-16 17:05:07 +0000
+++ lib/lp/bugs/mail/commands.py 2011-08-24 21:12:26 +0000
@@ -85,6 +85,7 @@
implements(IBugEmailCommand)
_numberOfArguments = 1
+ RANK = 0
def execute(self, parsed_msg, filealias):
"""See IBugEmailCommand."""
@@ -160,6 +161,7 @@
implements(IBugEditEmailCommand)
_numberOfArguments = 1
+ RANK = 3
def execute(self, context, current_event):
"""See `IEmailCommand`. Much of this method has been lifted from
@@ -206,6 +208,7 @@
implements(IBugEditEmailCommand)
_numberOfArguments = 1
+ RANK = 2
def execute(self, context, current_event):
"""See `IEmailCommand`.
@@ -260,6 +263,7 @@
"""Subscribes someone to the bug."""
implements(IBugEditEmailCommand)
+ RANK = 7
def execute(self, bug, current_event):
"""See IEmailCommand."""
@@ -300,6 +304,7 @@
"""Unsubscribes someone from the bug."""
implements(IBugEditEmailCommand)
+ RANK = 8
def execute(self, bug, current_event):
"""See IEmailCommand."""
@@ -335,6 +340,7 @@
implements(IBugEditEmailCommand)
_numberOfArguments = 1
+ RANK = 1
def execute(self, bug, current_event):
"""See IEmailCommand."""
@@ -365,6 +371,7 @@
implements(IBugEditEmailCommand)
_numberOfArguments = 1
+ RANK = 6
def execute(self, context, current_event):
"""See IEmailCommand."""
@@ -405,6 +412,7 @@
implements(IBugEditEmailCommand)
_numberOfArguments = 1
+ RANK = 5
def execute(self, bug, current_event):
"""See IEmailCommand."""
@@ -422,6 +430,7 @@
implements(IBugTaskEmailCommand)
_numberOfArguments = 1
+ RANK = 0
@classmethod
def _splitPath(cls, path):
@@ -633,6 +642,7 @@
implements(IBugTaskEditEmailCommand)
_numberOfArguments = 1
+ RANK = 2
def convertArguments(self, context):
"""See EmailCommand."""
@@ -655,6 +665,7 @@
implements(IBugTaskEditEmailCommand)
_numberOfArguments = 1
+ RANK = 3
def convertArguments(self, context):
"""See EmailCommand."""
@@ -741,6 +752,7 @@
class StatusEmailCommand(DBSchemaEditEmailCommand):
"""Changes a bug task's status."""
dbschema = BugTaskStatus
+ RANK = 4
def setAttributeValue(self, context, attr_name, attr_value):
"""See EmailCommand."""
@@ -758,11 +770,13 @@
class ImportanceEmailCommand(DBSchemaEditEmailCommand):
"""Changes a bug task's importance."""
dbschema = BugTaskImportance
+ RANK = 5
class ReplacedByImportanceCommand(EmailCommand):
"""This command has been replaced by the 'importance' command."""
implements(IBugTaskEditEmailCommand)
+ RANK = 1
def execute(self, context, current_event):
raise EmailProcessingError(
@@ -776,6 +790,7 @@
"""Assigns a tag to or removes a tag from bug."""
implements(IBugEditEmailCommand)
+ RANK = 4
def execute(self, bug, current_event):
"""See `IEmailCommand`."""
=== modified file 'lib/lp/bugs/mail/handler.py'
--- lib/lp/bugs/mail/handler.py 2011-08-24 21:12:25 +0000
+++ lib/lp/bugs/mail/handler.py 2011-08-24 21:12:26 +0000
@@ -8,6 +8,7 @@
"MaloneHandler",
]
+from operator import attrgetter
import os
from lazr.lifecycle.event import ObjectCreatedEvent
@@ -54,6 +55,101 @@
error_templates = os.path.join(os.path.dirname(__file__), 'errortemplates')
+class BugTaskCommandGroup:
+
+ def __init__(self, command=None):
+ self._commands = []
+ if command is not None:
+ self._commands.append(command)
+
+ def __nonzero__(self):
+ return len(self._commands) > 0
+
+ def __str__(self):
+ text_commands = [str(cmd) for cmd in self.commands]
+ return '\n'.join(text_commands).strip()
+
+ @property
+ def commands(self):
+ "Return the `EmailCommand`s ordered by their rank."
+ return sorted(self._commands, key=attrgetter('RANK'))
+
+ def add(self, command):
+ "Add an `EmailCommand` to the commands."
+ self._commands.append(command)
+
+
+class BugCommandGroup(BugTaskCommandGroup):
+
+ def __init__(self, command=None):
+ super(BugCommandGroup, self).__init__(command=command)
+ self._groups = []
+
+ def __nonzero__(self):
+ if len(self._groups) > 0:
+ return True
+ else:
+ return super(BugCommandGroup, self).__nonzero__()
+
+ def __str__(self):
+ text_commands = [super(BugCommandGroup, self).__str__()]
+ for group in self.groups:
+ text_commands += [str(group)]
+ return '\n'.join(text_commands).strip()
+
+ @property
+ def groups(self):
+ "Return the `BugTaskCommandGroup` in the order they were added."
+ return list(self._groups)
+
+ def add(self, command_or_group):
+ """Add an `EmailCommand` or `BugTaskCommandGroup` to the commands.
+
+ Empty BugTaskCommandGroup are ignored.
+ """
+ if isinstance(command_or_group, BugTaskCommandGroup):
+ if command_or_group:
+ self._groups.append(command_or_group)
+ else:
+ super(BugCommandGroup, self).add(command_or_group)
+
+
+class BugCommandGroups(BugCommandGroup):
+
+ def __init__(self, commands):
+ super(BugCommandGroups, self).__init__(command=None)
+ self._groups = []
+ this_bug = BugCommandGroup()
+ this_bugtask = BugTaskCommandGroup()
+ for command in commands:
+ if IBugEmailCommand.providedBy(command) and command.RANK == 0:
+ # Multiple bugs are being edited.
+ this_bug.add(this_bugtask)
+ self.add(this_bug)
+ this_bug = BugCommandGroup(command)
+ this_bugtask = BugTaskCommandGroup()
+ elif IBugEditEmailCommand.providedBy(command):
+ this_bug.add(command)
+ elif (IBugTaskEmailCommand.providedBy(command)
+ and command.RANK == 0):
+ # Multiple or explicit bugtasks are being edited.
+ this_bug.add(this_bugtask)
+ this_bugtask = BugTaskCommandGroup(command)
+ elif IBugTaskEditEmailCommand.providedBy(command):
+ this_bugtask.add(command)
+ this_bug.add(this_bugtask)
+ self.add(this_bug)
+
+ def add(self, command_or_group):
+ """Add a `BugCommandGroup` to the groups of commands.
+
+ Empty BugCommandGroups are ignored.
+ """
+ if isinstance(command_or_group, BugCommandGroup):
+ if command_or_group:
+ self._groups.append(command_or_group)
+
+
class MaloneHandler:
"""Handles emails sent to Malone.
=== modified file 'lib/lp/bugs/mail/tests/test_handler.py'
--- lib/lp/bugs/mail/tests/test_handler.py 2011-08-12 14:39:51 +0000
+++ lib/lp/bugs/mail/tests/test_handler.py 2011-08-24 21:12:26 +0000
@@ -25,13 +25,22 @@
LaunchpadZopelessLayer,
)
from lp.bugs.interfaces.bug import IBugSet
-from lp.bugs.mail.commands import BugEmailCommand
-from lp.bugs.mail.handler import MaloneHandler
+from lp.bugs.mail.commands import (
+ BugEmailCommand,
+ BugEmailCommands,
+ )
+from lp.bugs.mail.handler import (
+ BugCommandGroup,
+ BugCommandGroups,
+ BugTaskCommandGroup,
+ MaloneHandler,
+ )
from lp.services.mail import stub
from lp.testing import (
celebrity_logged_in,
login,
person_logged_in,
+ TestCase,
TestCaseWithFactory,
)
from lp.testing.factory import GPGSigningContext
@@ -195,6 +204,283 @@
"There is no such bug in Launchpad: 1", message)
+class BugTaskCommandGroupTestCase(TestCase):
+
+ def test_BugTaskCommandGroup_init_with_command(self):
+ # BugTaskCommandGroup can be inited with a BugEmailCommands.
+ command = BugEmailCommands.get('status', ['triaged'])
+ group = BugTaskCommandGroup(command)
+ self.assertEqual([command], group._commands)
+
+ def test_BugTaskCommandGroup_add(self):
+ # BugEmailCommands can be added to the group.
+ command_1 = BugEmailCommands.get('affects', ['fnord'])
+ command_2 = BugEmailCommands.get('status', ['triaged'])
+ group = BugTaskCommandGroup()
+ group.add(command_1)
+ group.add(command_2)
+ self.assertEqual([command_1, command_2], group._commands)
+
+ def test_BugTaskCommandGroup_sorted_commands(self):
+ # Commands are sorted by the Command's Rank.
+ command_3 = BugEmailCommands.get('importance', ['low'])
+ command_2 = BugEmailCommands.get('status', ['triaged'])
+ command_1 = BugEmailCommands.get('affects', ['fnord'])
+ group = BugTaskCommandGroup()
+ group.add(command_3)
+ group.add(command_2)
+ group.add(command_1)
+ self.assertEqual(0, command_1.RANK)
+ self.assertEqual(4, command_2.RANK)
+ self.assertEqual(5, command_3.RANK)
+ self.assertEqual(
+ [command_1, command_2, command_3], group.commands)
+
+ def test_BugTaskCommandGroup__nonzero__false(self):
+ # A BugTaskCommandGroup is zero is it has no commands.
+ group = BugTaskCommandGroup()
+ self.assertEqual(0, len(group._commands))
+ self.assertFalse(bool(group))
+
+ def test_BugTaskCommandGroup__nonzero__true(self):
+ # A BugTaskCommandGroup is non-zero is it has commands.
+ group = BugTaskCommandGroup(
+ BugEmailCommands.get('affects', ['fnord']))
+ self.assertEqual(1, len(group._commands))
+ self.assertTrue(bool(group))
+
+ def test_BugTaskCommandGroup__str__(self):
+ # The str of a BugTaskCommandGroup is the ideal order of the
+ # text commands in the email.
+ command_1 = BugEmailCommands.get('affects', ['fnord'])
+ command_2 = BugEmailCommands.get('status', ['triaged'])
+ group = BugTaskCommandGroup()
+ group.add(command_1)
+ group.add(command_2)
+ self.assertEqual(
+ 'affects fnord\nstatus triaged', str(group))
+
+
+class BugCommandGroupTestCase(TestCase):
+
+ def test_BugCommandGroup_init_with_command(self):
+ # A BugCommandGroup can be inited with a BugEmailCommand.
+ command = BugEmailCommands.get('private', ['true'])
+ group = BugCommandGroup(command)
+ self.assertEqual([command], group._commands)
+ self.assertEqual([], group._groups)
+
+ def test_BugCommandGroup_add_command(self):
+ # A BugEmailCommand can be added to a BugCommandGroup.
+ command = BugEmailCommands.get('private', ['true'])
+ group = BugCommandGroup()
+ group.add(command)
+ self.assertEqual([], group._groups)
+ self.assertEqual([command], group._commands)
+
+ def test_BugCommandGroup_add_bugtask_empty_group(self):
+ # Empty BugTaskCommandGroups are ignored.
+ bugtask_group = BugTaskCommandGroup()
+ group = BugCommandGroup()
+ group.add(bugtask_group)
+ self.assertEqual([], group._commands)
+ self.assertEqual([], group._groups)
+
+ def test_BugCommandGroup_add_bugtask_non_empty_group(self):
+ # Non-empty BugTaskCommandGroups are added.
+ bugtask_group = BugTaskCommandGroup(
+ BugEmailCommands.get('affects', ['fnord']))
+ group = BugCommandGroup()
+ group.add(bugtask_group)
+ self.assertEqual([], group._commands)
+ self.assertEqual([bugtask_group], group._groups)
+
+ def test_BugCommandGroup_groups(self):
+ # The groups property returns a copy _groups list in the order that
+ # that they were added.
+ bugtask_group_1 = BugTaskCommandGroup(
+ BugEmailCommands.get('affects', ['fnord']))
+ group = BugCommandGroup()
+ group.add(bugtask_group_1)
+ bugtask_group_2 = BugTaskCommandGroup(
+ BugEmailCommands.get('affects', ['pting']))
+ group.add(bugtask_group_2)
+ self.assertEqual(group._groups, group.groups)
+ self.assertFalse(group._groups is group.groups)
+ self.assertEqual([bugtask_group_1, bugtask_group_2], group.groups)
+
+ def test_BugCommandGroup__nonzero__false(self):
+ # A BugCommandGroup is zero is it has no commands or groups.
+ group = BugCommandGroup()
+ self.assertEqual(0, len(group._commands))
+ self.assertEqual(0, len(group._groups))
+ self.assertFalse(bool(group))
+
+ def test_BugCommandGroup__nonzero__true_commands(self):
+ # A BugCommandGroup is not zero is it has a command.
+ group = BugCommandGroup(
+ BugEmailCommands.get('private', ['true']))
+ self.assertEqual(1, len(group._commands))
+ self.assertEqual(0, len(group._groups))
+ self.assertTrue(bool(group))
+
+ def test_BugCommandGroup__nonzero__true_groups(self):
+ # A BugCommandGroup is not zero is it has a group.
+ group = BugCommandGroup()
+ group.add(BugTaskCommandGroup(
+ BugEmailCommands.get('affects', ['fnord'])))
+ self.assertEqual(0, len(group._commands))
+ self.assertEqual(1, len(group._groups))
+ self.assertTrue(bool(group))
+
+ def test_BugCommandGroup__str__(self):
+ # The str of a BugCommandGroup is the ideal order of the
+ # text commands in the email.
+ bug_group = BugCommandGroup(
+ BugEmailCommands.get('private', ['true']))
+ bug_group.add(
+ BugEmailCommands.get('security', ['false']))
+ bugtask_group = BugTaskCommandGroup(
+ BugEmailCommands.get('affects', ['fnord']))
+ bug_group.add(bugtask_group)
+ self.assertEqual(
+ 'security false\nprivate true\naffects fnord', str(bug_group))
+
+
+class BugCommandGroupsTestCase(TestCase):
+
+ def test_BugCommandGroups_add_bug_email_command(self):
+ # BugEmailCommands are ignored.
+ group = BugCommandGroups([])
+ group.add(
+ BugEmailCommands.get('private', ['true']))
+ self.assertEqual([], group._commands)
+ self.assertEqual([], group._groups)
+
+ def test_BugCommandGroups_add_bug_empty_group(self):
+ # Empty BugCommandGroups are ignored.
+ group = BugCommandGroups([])
+ group.add(
+ BugCommandGroup())
+ self.assertEqual([], group._commands)
+ self.assertEqual([], group._groups)
+
+ def test_BugCommandGroup_add_bug_non_empty_group(self):
+ # Non-empty BugCommandGroups are added.
+ group = BugCommandGroups([])
+ bug_group = BugCommandGroup(
+ BugEmailCommands.get('private', ['true']))
+ group.add(bug_group)
+ self.assertEqual([], group._commands)
+ self.assertEqual([bug_group], group._groups)
+
+ def test_BugCommandGroups__init__no_commands(self):
+ # Emails may not contain any commands to group.
+ ordered_commands = BugCommandGroups([])
+ self.assertEqual(0, len(ordered_commands.groups))
+ self.assertEqual('', str(ordered_commands))
+
+ def test_BugCommandGroups__init__one_bug_no_bugtasks(self):
+ # Commands can operate on one bug.
+ email_commands = [
+ ('bug', '1234'),
+ ('private', 'true'),
+ ]
+ commands = [
+ BugEmailCommands.get(name=name, string_args=[args])
+ for name, args in email_commands]
+ ordered_commands = BugCommandGroups(commands)
+ expected = '\n'.join([
+ 'bug 1234',
+ 'private true',
+ ])
+ self.assertEqual(1, len(ordered_commands.groups))
+ self.assertEqual(2, len(ordered_commands.groups[0].commands))
+ self.assertEqual(0, len(ordered_commands.groups[0].groups))
+ self.assertEqual(expected, str(ordered_commands))
+
+ def test_BugCommandGroups__init__one_bug_one_bugtask(self):
+ # Commands can operate on one bug and one bugtask.
+ email_commands = [
+ ('bug', 'new'),
+ ('affects', 'fnord'),
+ ('importance', 'high'),
+ ('private', 'true'),
+ ]
+ commands = [
+ BugEmailCommands.get(name=name, string_args=[args])
+ for name, args in email_commands]
+ ordered_commands = BugCommandGroups(commands)
+ expected = '\n'.join([
+ 'bug new',
+ 'private true',
+ 'affects fnord',
+ 'importance high',
+ ])
+ self.assertEqual(1, len(ordered_commands.groups))
+ self.assertEqual(2, len(ordered_commands.groups[0].commands))
+ self.assertEqual(1, len(ordered_commands.groups[0].groups))
+ self.assertEqual(
+ 2, len(ordered_commands.groups[0].groups[0].commands))
+ self.assertEqual(expected, str(ordered_commands))
+
+ def test_BugCommandGroups__init__one_bug_many_bugtask(self):
+ # Commands can operate on one bug and one bugtask.
+ email_commands = [
+ ('bug', 'new'),
+ ('affects', 'fnord'),
+ ('importance', 'high'),
+ ('private', 'true'),
+ ('affects', 'pting'),
+ ('importance', 'low'),
+ ]
+ commands = [
+ BugEmailCommands.get(name=name, string_args=[args])
+ for name, args in email_commands]
+ ordered_commands = BugCommandGroups(commands)
+ expected = '\n'.join([
+ 'bug new',
+ 'private true',
+ 'affects fnord',
+ 'importance high',
+ 'affects pting',
+ 'importance low',
+ ])
+ self.assertEqual(1, len(ordered_commands.groups))
+ self.assertEqual(2, len(ordered_commands.groups[0].commands))
+ self.assertEqual(2, len(ordered_commands.groups[0].groups))
+ self.assertEqual(
+ 2, len(ordered_commands.groups[0].groups[0].commands))
+ self.assertEqual(
+ 2, len(ordered_commands.groups[0].groups[1].commands))
+ self.assertEqual(expected, str(ordered_commands))
+
+ def test_BugCommandGroups_init_many_bugs(self):
+ # Commands can operate on many bugs.
+ email_commands = [
+ ('bug', '1234'),
+ ('importance', 'high'),
+ ('bug', '5678'),
+ ('importance', 'low'),
+ ('bug', '4321'),
+ ('importance', 'medium'),
+ ]
+ commands = [
+ BugEmailCommands.get(name=name, string_args=[args])
+ for name, args in email_commands]
+ ordered_commands = BugCommandGroups(commands)
+ expected = '\n'.join([
+ 'bug 1234',
+ 'importance high',
+ 'bug 5678',
+ 'importance low',
+ 'bug 4321',
+ 'importance medium',
+ ])
+ self.assertEqual(3, len(ordered_commands.groups))
+ self.assertEqual(expected, str(ordered_commands))
+
+
class FakeSignature:
def __init__(self, timestamp):
=== modified file 'lib/lp/services/mail/interfaces.py'
--- lib/lp/services/mail/interfaces.py 2011-08-11 20:33:47 +0000
+++ lib/lp/services/mail/interfaces.py 2011-08-24 21:12:26 +0000
@@ -137,6 +137,9 @@
class IBugEmailCommand(IEmailCommand):
"""An email command specific to getting or creating a bug."""
+ RANK = Attribute(
+ "The int used to determine the order of execution of many commands.")
+
def execute(parsed_msg, filealias):
"""Either create or get an exiting bug.
@@ -150,6 +153,9 @@
class IBugTaskEmailCommand(IEmailCommand):
"""An email command specific to getting or creating a bug task."""
+ RANK = Attribute(
+ "The int used to determine the order of execution of many commands.")
+
def execute(bug):
"""Either create or get an exiting bug task.
@@ -160,6 +166,9 @@
class IBugEditEmailCommand(IEmailCommand):
"""An email command specific to editing a bug."""
+ RANK = Attribute(
+ "The int used to determine the order of execution of many commands.")
+
def execute(bug, current_event):
"""Execute the command in the context of the bug.
@@ -170,6 +179,9 @@
class IBugTaskEditEmailCommand(IEmailCommand):
"""An email command specific to editing a bug task."""
+ RANK = Attribute(
+ "The int used to determine the order of execution of many commands.")
+
def execute(bugtask, current_event):
"""Execute the command in the context of the bug task.
