From 8963fa694afca43bcbb3a11fdfce80e5521c3f86 Mon Sep 17 00:00:00 2001 From: Garret Wassermann Date: Wed, 19 Apr 2017 23:47:58 -0400 Subject: [PATCH] Prevent adding Users or Queue email to the CC list --- helpdesk/management/commands/get_email.py | 11 +- helpdesk/tests/test_get_email.py | 222 ++++++++++++---------- 2 files changed, 133 insertions(+), 100 deletions(-) diff --git a/helpdesk/management/commands/get_email.py b/helpdesk/management/commands/get_email.py index a7baa956..fda915df 100644 --- a/helpdesk/management/commands/get_email.py +++ b/helpdesk/management/commands/get_email.py @@ -372,7 +372,16 @@ def ticket_from_message(message, queue, logger): if cc: # get list of currently CC'd emails current_cc = TicketCC.objects.filter(ticket=ticket) - current_cc = set([x.email for x in current_cc]) + current_cc_emails = [x.email for x in current_cc] + # get emails of any Users CC'd to email + current_cc_users = [x.user.email for x in current_cc] + # ensure submitter, assigned user, queue email not added + other_emails = [queue.email_address] + if t.submitter_email: + other_emails.append(t.submitter_email) + if t.assigned_to: + other_emails.append(t.assigned_to.email) + current_cc = set(current_cc_emails + current_cc_users + other_emails) # add any email in cc that's not already in current_cc (set difference) new_cc = cc.difference(current_cc) # add emails alphabetically, makes testing easy diff --git a/helpdesk/tests/test_get_email.py b/helpdesk/tests/test_get_email.py index 60cd5a4e..f6d51a1e 100644 --- a/helpdesk/tests/test_get_email.py +++ b/helpdesk/tests/test_get_email.py @@ -7,6 +7,8 @@ from django.test import TestCase from django.core.management import call_command from django.utils import six from django.shortcuts import get_object_or_404 +from django.contrib.auth.models import User +from django.contrib.auth.hashers import make_password import itertools from shutil import rmtree import sys @@ -44,13 +46,13 @@ class GetEmailCommonTests(TestCase): class GetEmailParametricTemplate(object): - """TestCase that checks email functionality across methods and socks configs.""" + """TestCase that checks basic email functionality across methods and socks configs.""" def setUp(self): self.temp_logdir = mkdtemp() kwargs = { - "title": 'Queue 1', + "title": 'Basic Queue', "slug": 'QQ', "allow_public_submission": True, "allow_email_submission": True, @@ -221,100 +223,6 @@ class GetEmailParametricTemplate(object): self.assertEqual(ticket2.title, test_email_subject) self.assertEqual(ticket2.description, test_email_body) - def test_read_email_cc(self): - """Tests reading plain text emails from a queue and creating tickets, - particularly to test appropriate handling of CC'd emails. - For each email source supported, we mock the backend to provide - authentically formatted responses containing our test data.""" - - # example email text from Django docs: https://docs.djangoproject.com/en/1.10/ref/unicode/ - test_email_from = "Arnbjörg Ráðormsdóttir " - # NOTE: CC emails are in alphabetical order and must be tested as such! - # implementation uses sets, so only way to ensure tickets created - # in right order is to change set to list and sort it - test_email_cc_one = "Alice Ráðormsdóttir " - test_email_cc_two = "nobody@example.com" - test_email_cc_three = "other@example.com" - test_email_cc_four = "someone@example.com" - test_email_subject = "My visit to Sør-Trøndelag" - test_email_body = "Unicode helpdesk comment with an s-hat (ŝ) via email." - test_email = "To: helpdesk@example.com\nCc: " + test_email_cc_one + ", " + test_email_cc_one + ", " + test_email_cc_two + ", " + test_email_cc_three + "\nCC: " + test_email_cc_one + ", " + test_email_cc_three + ", " + test_email_cc_four + "\nFrom: " + test_email_from + "\nSubject: " + test_email_subject + "\n\n" + test_email_body - test_mail_len = len(test_email) - - if self.socks: - from socks import ProxyConnectionError - with self.assertRaisesRegexp(ProxyConnectionError, '%s:%s' % (unrouted_socks_server, unused_port)): - call_command('get_email') - - else: - # Test local email reading - if self.method == 'local': - with mock.patch('helpdesk.management.commands.get_email.listdir') as mocked_listdir, \ - mock.patch('helpdesk.management.commands.get_email.isfile') as mocked_isfile, \ - mock.patch('builtins.open' if six.PY3 else '__builtin__.open', mock.mock_open(read_data=test_email)): - mocked_isfile.return_value = True - mocked_listdir.return_value = ['filename1', 'filename2'] - - call_command('get_email') - - mocked_listdir.assert_called_with('/var/lib/mail/helpdesk/') - mocked_isfile.assert_any_call('/var/lib/mail/helpdesk/filename1') - mocked_isfile.assert_any_call('/var/lib/mail/helpdesk/filename2') - - elif self.method == 'pop3': - # mock poplib.POP3's list and retr methods to provide responses as per RFC 1939 - pop3_emails = { - '1': ("+OK", test_email.split('\n')), - '2': ("+OK", test_email.split('\n')), - } - pop3_mail_list = ("+OK 2 messages", ("1 %d" % test_mail_len, "2 %d" % test_mail_len)) - mocked_poplib_server = mock.Mock() - mocked_poplib_server.list = mock.Mock(return_value=pop3_mail_list) - mocked_poplib_server.retr = mock.Mock(side_effect=lambda x: pop3_emails[x]) - with mock.patch('helpdesk.management.commands.get_email.poplib', autospec=True) as mocked_poplib: - mocked_poplib.POP3 = mock.Mock(return_value=mocked_poplib_server) - call_command('get_email') - - elif self.method == 'imap': - # mock imaplib.IMAP4's search and fetch methods with responses from RFC 3501 - imap_emails = { - "1": ("OK", (("1", test_email),)), - "2": ("OK", (("2", test_email),)), - } - imap_mail_list = ("OK", ("1 2",)) - mocked_imaplib_server = mock.Mock() - mocked_imaplib_server.search = mock.Mock(return_value=imap_mail_list) - - # we ignore the second arg as the data item/mime-part is constant (RFC822) - mocked_imaplib_server.fetch = mock.Mock(side_effect=lambda x, _: imap_emails[x]) - with mock.patch('helpdesk.management.commands.get_email.imaplib', autospec=True) as mocked_imaplib: - mocked_imaplib.IMAP4 = mock.Mock(return_value=mocked_imaplib_server) - call_command('get_email') - - ticket1 = get_object_or_404(Ticket, pk=1) - self.assertEqual(ticket1.ticket_for_url, "QQ-%s" % ticket1.id) - self.assertEqual(ticket1.title, test_email_subject) - self.assertEqual(ticket1.description, test_email_body) - cc1 = get_object_or_404(TicketCC, pk=1) - self.assertEqual(cc1.email, test_email_cc_one) - cc2 = get_object_or_404(TicketCC, pk=2) - self.assertEqual(cc2.email, test_email_cc_two) - cc3 = get_object_or_404(TicketCC, pk=3) - self.assertEqual(cc3.email, test_email_cc_three) - cc4 = get_object_or_404(TicketCC, pk=4) - # the second CC has an extra test_email_cc_one and three that - # should not be saved again, so the 4th email should be - # test_email_cc_four - self.assertEqual(cc4.email, test_email_cc_four) - # ensure these 4 CCs are the only ones created - # (otherwise, there were duplicates) - self.assertEqual(len(TicketCC.objects.filter(ticket=1)), 4) - - ticket2 = get_object_or_404(Ticket, pk=2) - self.assertEqual(ticket2.ticket_for_url, "QQ-%s" % ticket2.id) - self.assertEqual(ticket2.title, test_email_subject) - self.assertEqual(ticket2.description, test_email_body) - def test_read_html_multipart_email(self): """Tests reading multipart MIME (HTML body and plain text alternative) emails from a queue and creating tickets. @@ -447,6 +355,121 @@ class GetEmailParametricTemplate(object): self.assertEqual(attach2.followup.id, 2) self.assertEqual(attach2.filename, 'email_html_body.html') +class GetEmailCCHandling(TestCase): + """TestCase that checks CC handling in email. Needs its own test harness.""" + + def setUp(self): + + self.temp_logdir = mkdtemp() + + kwargs = { + "title": 'CC Queue', + "slug": 'CC', + "allow_public_submission": True, + "allow_email_submission": True, + "email_box_type": 'local', + "email_box_local_dir": '/var/lib/mail/helpdesk/', + "logging_dir": self.temp_logdir, + "logging_type": 'none' + } + self.queue_public = Queue.objects.create(**kwargs) + + user1_kwargs = { + 'username': 'staff', + 'email': 'staff@example.com', + 'password': make_password('Test1234'), + 'is_staff': True, + 'is_superuser': False, + 'is_active': True + } + self.staff_user = User.objects.create(**user1_kwargs) + + user2_kwargs = { + 'username': 'assigned', + 'email': 'assigned@example.com', + 'password': make_password('Test1234'), + 'is_staff': True, + 'is_superuser': False, + 'is_active': True + } + self.assigned_user = User.objects.create(**user2_kwargs) + + ticket_kwargs = { + 'title': 'Original Ticket', + 'queue': self.queue_public, + 'submitter_email': 'submitter@example.com', + 'assigned_to': self.assigned_user, + 'status': 1 + } + self.original_ticket = Ticket.objects.create(**ticket_kwargs) + + cc_kwargs = { + 'ticket': self.original_ticket, + 'user': self.staff_user, + 'can_view': True, + 'can_update': True + } + self.original_cc = TicketCC.objects.create(**cc_kwargs) + + + def tearDown(self): + + rmtree(self.temp_logdir) + + def test_read_email_cc(self): + """Tests reading plain text emails from a queue and adding to a ticket, + particularly to test appropriate handling of CC'd emails.""" + + # first, check that test ticket exists + ticket1 = get_object_or_404(Ticket, pk=1) + self.assertEqual(ticket1.ticket_for_url, "CC-1") + self.assertEqual(ticket1.title, "Original Ticket") + # only the staff_user is CC'd for now + self.assertEqual(len(TicketCC.objects.filter(ticket=1)), 1) + + # example email text from Django docs: https://docs.djangoproject.com/en/1.10/ref/unicode/ + test_email_from = "Arnbjörg Ráðormsdóttir " + # NOTE: CC emails are in alphabetical order and must be tested as such! + # implementation uses sets, so only way to ensure tickets created + # in right order is to change set to list and sort it + test_email_cc_one = "Alice Ráðormsdóttir " + test_email_cc_two = "nobody@example.com" + test_email_cc_three = "other@example.com" + test_email_cc_four = "someone@example.com" + ticket_user_emails = "assigned@example.com, staff@example.com, submitter@example.com" + test_email_subject = "[CC-1] My visit to Sør-Trøndelag" + test_email_body = "Unicode helpdesk comment with an s-hat (ŝ) via email." + test_email = "To: helpdesk@example.com\nCc: " + test_email_cc_one + ", " + test_email_cc_one + ", " + test_email_cc_two + ", " + test_email_cc_three + "\nCC: " + test_email_cc_one + ", " + test_email_cc_three + ", " + test_email_cc_four + ", " + ticket_user_emails + "\nFrom: " + test_email_from + "\nSubject: " + test_email_subject + "\n\n" + test_email_body + test_mail_len = len(test_email) + + with mock.patch('helpdesk.management.commands.get_email.listdir') as mocked_listdir, \ + mock.patch('helpdesk.management.commands.get_email.isfile') as mocked_isfile, \ + mock.patch('builtins.open' if six.PY3 else '__builtin__.open', mock.mock_open(read_data=test_email)): + mocked_isfile.return_value = True + mocked_listdir.return_value = ['filename1'] + + call_command('get_email') + + mocked_listdir.assert_called_with('/var/lib/mail/helpdesk/') + mocked_isfile.assert_any_call('/var/lib/mail/helpdesk/filename1') + + # next we make sure no duplicates were added, and the + # staff users nor submitter were not re-added as email TicketCCs + cc1 = get_object_or_404(TicketCC, pk=2) + self.assertEqual(cc1.email, test_email_cc_one) + cc2 = get_object_or_404(TicketCC, pk=3) + self.assertEqual(cc2.email, test_email_cc_two) + cc3 = get_object_or_404(TicketCC, pk=4) + self.assertEqual(cc3.email, test_email_cc_three) + cc4 = get_object_or_404(TicketCC, pk=5) + self.assertEqual(cc4.email, test_email_cc_four) + # ensure these 4 CCs (test_email_cc one thru four) are the only ones + # created and added to the existing staff_user that was CC'd, + # and that submitter and assignee are not added as CC either + # (in other words, even though everyone was CC'd to this email, + # we should come out with only 5 CCs after filtering) + self.assertEqual(len(TicketCC.objects.filter(ticket=1)), 5) + # build matrix of test cases case_methods = [c[0] for c in Queue._meta.get_field('email_box_type').choices] @@ -466,7 +489,8 @@ for method, socks in case_matrix: test_name = str( "TestGetEmail%s%s" % (method.capitalize(), socks_str)) - cl = type(test_name, (GetEmailParametricTemplate, TestCase,), { - "method": method, - "socks": socks}) + cl = type(test_name, + (GetEmailParametricTemplate,TestCase,), + { "method": method, "socks": socks} + ) setattr(thismodule, test_name, cl)