Merge pull request #1264 from django-helpdesk/fix_cc_crash_when_duplicate_emails

Fix cc crash when CC email address is assocaited with multiple active User records

docs/configuration.rst

@@ -94,4 +94,12 @@ You may add your own site specific navigation header to be included inside the <
 
     helpdesk/custom_navigation_header.html
 
-2. Update the contents to display your custom navigation.
+2. Update the contents to display your custom navigation.
+
+Suppressible Log Messages
+-------------------------
+Some logging messages support being switched on or off according to deployment preferences.
+
+The following settings variables control emitting log messages for specific scenarios:
+  LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER - there is no user matching the email address in the CC list. Defaults to False
+  LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER - there is more than 1 user matching the email address in the CC list. Defaults to True

helpdesk/email.py

@@ -46,6 +46,7 @@ from typing import List
 # import User model, which may be a custom model
 User = get_user_model()
 
+MULTIPLE_USERS_SAME_EMAIL_MSG = "CC email address is linked to more than 1 active user"
 STRIPPED_SUBJECT_STRINGS = [
     "Re: ",
     "Fw: ",
@@ -531,7 +532,7 @@ def is_autoreply(message):
     return any(any_if_this)
 
 
-def create_ticket_cc(ticket, cc_list):
+def create_ticket_cc(ticket, cc_list, logger):
     if not cc_list:
         return []
     # Local import to deal with non-defined / circular reference problem
@@ -544,16 +545,32 @@ def create_ticket_cc(ticket, cc_list):
         if cced_email == ticket.queue.email_address:
             continue
 
-        user = None
+        user_id = None
 
-        try:
-            user = User.objects.get(email=cced_email)  # @UndefinedVariable
-        except User.DoesNotExist:
-            pass
+        user_list = User.objects.filter(email=cced_email, is_active=True).values_list(
+            "id", flat=True
+        )
+        count = user_list.count()
+        if count == 0:
+            if getattr(
+                helpdesk_settings, "LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER", False
+            ):
+                logger.warning(
+                    f"CC email address is not linked to an active user: {cced_email}"
+                )
+        elif count > 1:
+            if getattr(
+                helpdesk_settings,
+                "LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER",
+                True,
+            ):
+                logger.warning(f"{MULTIPLE_USERS_SAME_EMAIL_MSG}: {cced_email}")
+        else:
+            user_id = user_list[0]
 
         try:
             ticket_cc = subscribe_to_ticket_updates(
-                ticket=ticket, user=user, email=cced_email
+                ticket=ticket, user_id=user_id, email=cced_email
             )
             new_ticket_ccs.append(ticket_cc)
         except ValidationError:
@@ -673,7 +690,7 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger)
     context = safe_template_context(ticket)
 
     new_ticket_ccs = []
-    new_ticket_ccs.append(create_ticket_cc(ticket, to_list + cc_list))
+    new_ticket_ccs.append(create_ticket_cc(ticket, to_list + cc_list, logger))
 
     autoreply = is_autoreply(message)
     if autoreply:

helpdesk/settings.py

@@ -487,3 +487,11 @@ HELPDESK_GET_NEW_TICKET_WEBHOOK_URLS = getattr(
 )
 
 HELPDESK_WEBHOOK_TIMEOUT = getattr(settings, "HELPDESK_WEBHOOK_TIMEOUT", 3)
+
+
+LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER = getattr(
+    settings, "HELPDESK_LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER", False
+)
+LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER = getattr(
+    settings, "HELPDESK_LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER", True
+)

helpdesk/tests/test_ticket_submission.py

@@ -4,7 +4,7 @@ from django.test import TestCase
 from django.test.client import Client
 from django.urls import reverse
 import email
-from helpdesk.email import extract_email_metadata
+from helpdesk.email import extract_email_metadata, MULTIPLE_USERS_SAME_EMAIL_MSG
 from helpdesk.models import (
     CustomField,
     FollowUp,
@@ -14,10 +14,12 @@ from helpdesk.models import (
     Ticket,
     TicketCC,
 )
+from . import utils
 import logging
 from urllib.parse import urlparse
 import uuid
 
+User = get_user_model()
 
 logger = logging.getLogger("helpdesk")
 
@@ -46,7 +48,7 @@ class TicketBasicsTestCase(TestCase):
             "description": "Some Test Ticket",
         }
 
-        self.user = get_user_model().objects.create(
+        self.user = User.objects.create(
             username="User_1",
         )
 
@@ -480,6 +482,40 @@ class EmailInteractionsTestCase(TestCase):
             self.assertTrue(ticket_cc.ticket, ticket)
             self.assertTrue(ticket_cc.email, cc_email)
 
+    def test_create_ticket_from_email_with_cc_duplicate_users_same_email(self):
+        """
+        Verify that the duplicated email warning log is invoked
+        """
+        dup_email = "dup@helpdesk.test"
+        User.objects.create(
+            username="User_dup1",
+            email=dup_email,
+        )
+        User.objects.create(
+            username="User_dup2",
+            email=dup_email,
+        )
+        message, _, _ = utils.generate_text_email(locale="fr_FR")
+
+        cc_list = [dup_email, "alpha@acme.test", "beta@acme.test", "gamma@delta.test"]
+        message_id = uuid.uuid4().hex
+
+        message["Message-ID"] = message_id
+        message["Cc"] = ",".join(cc_list)
+
+        with self.assertLogs("helpdesk", level="WARNING") as dup_warn:
+            extract_email_metadata(str(message), self.queue_public, logger=logger)
+            self.assertTrue(
+                MULTIPLE_USERS_SAME_EMAIL_MSG in dup_warn.output[0],
+                "The duplicated email across user ID's was was not logged.",
+            )
+        # Check that the CC duplicate was still sent a notification email
+        email_count = len(mail.outbox)
+        found = any(dup_email in email.to for email in mail.outbox[: email_count - 1])
+        self.assertTrue(
+            found, "The duplicated email across user ID's was not sent a notification."
+        )
+
     def test_create_followup_from_email_with_valid_message_id_with_no_initial_cc_list(
         self,
     ):

helpdesk/update_ticket.py

@@ -29,7 +29,7 @@ def add_staff_subscription(user: User, ticket: Ticket) -> None:
         and user.is_authenticated
         and return_ticketccstring_and_show_subscribe(user, ticket)[1]
     ):
-        subscribe_to_ticket_updates(ticket, user)
+        subscribe_to_ticket_updates(ticket, user.id)
 
 
 def return_ticketccstring_and_show_subscribe(user, ticket):
@@ -73,16 +73,16 @@ def return_ticketccstring_and_show_subscribe(user, ticket):
 
 
 def subscribe_to_ticket_updates(
-    ticket, user=None, email=None, can_view=True, can_update=False
+    ticket, user_id=None, email=None, can_view=True, can_update=False
 ):
     if ticket is not None:
-        queryset = TicketCC.objects.filter(ticket=ticket, user=user, email=email)
+        queryset = TicketCC.objects.filter(ticket=ticket, user_id=user_id, email=email)
 
         # Don't create duplicate entries for subscribers
         if queryset.count() > 0:
             return queryset.first()
 
-        if user is None and len(email) < 5:
+        if user_id is None and len(email) < 5:
             raise ValidationError(
                 _(
                     "When you add somebody on Cc, you must provide either a User or a valid email. Email: %s"
@@ -91,7 +91,7 @@ def subscribe_to_ticket_updates(
             )
 
         return ticket.ticketcc_set.create(
-            user=user, email=email, can_view=can_view, can_update=can_update
+            user_id=user_id, email=email, can_view=can_view, can_update=can_update
         )
 
 

helpdesk/views/staff.py

@@ -412,7 +412,7 @@ def view_ticket(request, ticket_id):
         ]
 
         if show_subscribe:
-            subscribe_to_ticket_updates(ticket, request.user)
+            subscribe_to_ticket_updates(ticket, request.user.id)
             return HttpResponseRedirect(reverse("helpdesk:view", args=[ticket.id]))
 
     if "close" in request.GET and ticket.status == Ticket.RESOLVED_STATUS:

setup.py

@@ -7,7 +7,7 @@ from setuptools import find_packages, setup
 import sys
 
 
-version = "1.4.0"
+version = "1.5.0"
 
 
 # Provided as an attribute, so you can append to these instead