Ensure In-Reply-To mail header is unique accross instances (#3957)

* Ensure In-Reply-To mail header is unique accross instances; closes #3836

* fix

* changelog
Co-authored-by: Cédric Anne <cedric.anne@gmail.com>

CHANGELOG.md

@@ -21,6 +21,7 @@ The present file will list all changes made to the project; according to the
 #### Added
 
 #### Changes
+- Format of `Message-Id` header sent in Tickets notifications changed to match format used by other items.
 
 #### Deprecated
 - Usage of `GLPI_FORCE_EMPTY_SQL_MODE` constant

inc/mailcollector.class.php

@@ -968,10 +968,15 @@ class MailCollector  extends CommonDBTM {
       }
 
       if ($this->isMessageSentByGlpi($message)) {
-         // Message was sent by GLPI.
+         // Message was sent by current instance of GLPI.
          // Message is blacklisted to avoid infinite loop (where GLPI creates a ticket from its own notification).
          $tkt['_blacklisted'] = true;
          return $tkt;
+      } else if ($this->isResponseToMessageSentByAnotherGlpi($message)) {
+         // Message is a response to a message sent by another GLPI.
+         // Message is blacklisted as we consider that the other instance of GLPI is responsible to handle this thread.
+         $tkt['_blacklisted'] = true;
+         return $tkt;
       }
 
       // manage blacklist
@@ -1957,14 +1962,18 @@ class MailCollector  extends CommonDBTM {
     * @return CommonDBTM|null
     */
    public function getItemFromHeaders(Message $message): ?CommonDBTM {
+      if ($this->isResponseToMessageSentByAnotherGlpi($message)) {
+         return null;
+      }
+
       $pattern = $this->getMessageIdExtractPattern();
 
       foreach (['in_reply_to', 'references'] as $header_name) {
          $matches = [];
          if ($message->getHeaders()->has($header_name)
              && preg_match($pattern, $message->getHeader($header_name)->getFieldValue(), $matches)) {
-            $itemtype = $matches['itemtype'];
-            $items_id = $matches['items_id'];
+            $itemtype = $matches['itemtype'] ?? '';
+            $items_id = $matches['items_id'] ?? '';
 
             // Handle old format MessageId where itemtype was not in header
             if (empty($itemtype) && !empty($items_id)) {
@@ -2010,22 +2019,70 @@ class MailCollector  extends CommonDBTM {
          return false;
       }
 
-      return true;
+      $uuid = $matches['uuid'] ?? '';
+      if (empty($uuid)) {
+         // message-id corresponds to old format, without uuid.
+         // We assume that in most environments this message have been sent by this instance of GLPI,
+         // as only one instance of GLPI will be installed.
+         return true;
+      }
+
+      return $uuid == Config::getUuid('notification');
+   }
+
+   /**
+    * Check if message is a response to a message sent by another Glpi instance.
+    * Responses to GLPI messages should contains a InReplyTo or a References header
+    * that matches the MessageId from original message.
+    *
+    * @param Message $message
+    *
+    * @since x.x.x
+    *
+    * @return bool
+    */
+   public function isResponseToMessageSentByAnotherGlpi(Message $message): bool {
+      $pattern = $this->getMessageIdExtractPattern();
+
+      $has_uuid_from_another_glpi = false;
+      $has_uuid_from_current_glpi = false;
+      foreach (['in-reply-to', 'references'] as $header_name) {
+         $matches = [];
+         if ($message->getHeaders()->has($header_name)
+             && preg_match($pattern, $message->getHeader($header_name)->getFieldValue(), $matches)) {
+            if (empty($matches['uuid'])) {
+               continue;
+            }
+            if ($matches['uuid'] == Config::getUuid('notification')) {
+               $has_uuid_from_current_glpi = true;
+            } else if ($matches['uuid'] != Config::getUuid('notification')) {
+               $has_uuid_from_another_glpi = true;
+            }
+         }
+      }
+
+      // Matches if one of following conditions matches:
+      // - no UUID found matching current GLPI instance;
+      // - at least one unknown UUID.
+      return !$has_uuid_from_current_glpi && $has_uuid_from_another_glpi;
    }
 
    /**
-    * Get pattern that can be used to extract informations from a GLPI MessageId (itemtype and items_id).
+    * Get pattern that can be used to extract informations from a GLPI MessageId (uuid, itemtype and items_id).
     *
     * @see NotificationTarget::getMessageID()
     *
     * @return string
     */
    private function getMessageIdExtractPattern(): string {
-      // old format:           GLPI-{$items_id}.{$time}.{$rand}@{$uname}
-      // without related item: GLPI.{$time}.{$rand}@{$uname}
-      // with related item:    GLPI-{$itemtype}-{$items_id}.{$time}.{$rand}@{$uname}
+      // old format for tickets:           GLPI-{$items_id}.{$time}.{$rand}@{$uname}
+      // old format without related item:  GLPI.{$time}.{$rand}@{$uname}
+      // old format with related item:     GLPI-{$itemtype}-{$items_id}.{$time}.{$rand}@{$uname}
+      // new format without related item:  GLPI_{$uuid}.{$time}.{$rand}@{$uname}
+      // new format with related item:     GLPI_{$uuid}-{$itemtype}-{$items_id}.{$time}.{$rand}@{$uname}
 
       return '/GLPI'
+         . '(_(?<uuid>[a-z0-9]+))?' // uuid was not be present in old format
          . '(-(?<itemtype>[a-z]+))?' // itemtype is not present if notification is not related to any object and was not present in old format
          . '(-(?<items_id>[0-9]+))?' // items_id is not present if notification is not related to any object
          . '\.[0-9]+' // time()

inc/notificationeventmailing.class.php

@@ -127,8 +127,21 @@ class NotificationEventMailing extends NotificationEventAbstract implements Noti
          }
 
          // Add custom header for mail grouping in reader
-         $mmail->AddCustomHeader("In-Reply-To: <GLPI-".$current->fields["itemtype"]."-".
-                                 $current->fields["items_id"].">");
+         $mmail->AddCustomHeader(
+            str_replace(
+               [
+                  '%uuid',
+                  '%itemtype',
+                  '%items_id'
+               ],
+               [
+                  Config::getUuid('notification'),
+                  $current->fields['itemtype'],
+                  $current->fields['items_id']
+               ],
+               "In-Reply-To: <GLPI-%uuid-%itemtype-%items_id>"
+            )
+         );
 
          $mmail->SetFrom($current->fields['sender'], $current->fields['sendername']);
 

inc/notificationtarget.class.php

@@ -243,7 +243,8 @@ class NotificationTarget extends CommonDBChild {
    function getMessageID() {
       if ($this->obj instanceof CommonDBTM) {
          return sprintf(
-            'GLPI-%s-%s.%s.%s@%s',
+            'GLPI_%s-%s-%s.%s.%s@%s',
+            Config::getUuid('notification'),
             $this->obj->getType(),
             $this->obj->getField('id'),
             time(),
@@ -253,7 +254,8 @@ class NotificationTarget extends CommonDBChild {
       }
 
       return sprintf(
-         'GLPI.%s.%s@%s',
+         'GLPI_%s.%s.%s@%s',
+         Config::getUuid('notification'),
          time(),
          rand(),
          php_uname('n')

inc/notificationtargetticket.class.php

@@ -101,16 +101,6 @@ class NotificationTargetTicket extends NotificationTargetCommonITILObject {
    }
 
 
-   /**
-    * @since 0.84
-    *
-    * @return string
-   **/
-   function getMessageID() {
-      return "GLPI-".$this->obj->getField('id').".".time().".".rand(). "@".php_uname('n');
-   }
-
-
    /**
     * Get item associated with the object on which the event was raised
     *

tests/emails-tests/04.1-ticket-followup-from-inreplyto.eml → tests/emails-tests/04.1-ticket-followup-from-oldformat-inreplyto.eml

@@ -13,5 +13,5 @@ Content-Type: text/plain; charset=utf-8
 Content-Language: fr-FR
 Content-Transfer-Encoding: quoted-printable
 
-This is a reply that references Ticket 100 in In-Reply-To header.
+This is a reply that references Ticket 100 in In-Reply-To header (old format).
 It should be added as followup.

tests/emails-tests/04.2-ticket-followup-from-references.eml → tests/emails-tests/04.2-ticket-followup-from-oldformat-references.eml

@@ -13,5 +13,5 @@ Content-Type: text/plain; charset=utf-8
 Content-Language: fr-FR
 Content-Transfer-Encoding: quoted-printable
 
-This is a reply that references Ticket 100 in References header.
+This is a reply that references Ticket 100 in References header (old format).
 It should be added as followup.

tests/emails-tests/04.4-ticket-followup-from-newformat-inreplyto.eml

+Return-Path: tech@glpi-project.org
+Received: from mail.glpi-project.org (localhost [127.0.0.1])
+	by mail.glpi-project.org (Postfix) with ESMTP id B74527E805E8
+	for <unittests@glpi-project.org>; Wed,  3 Apr 2019 11:48:35 +0200 (CEST)
+Subject: Re: New database issue
+From: Tech Ni Cian <tech@glpi-project.org>
+To: GLPI debug <unittests@glpi-project.org>
+Message-ID: <961c2885-94e6-2343-b000-280c809fcdb1@glpi-project.org>
+Date: Wed, 3 Apr 2019 11:48:25 +0200
+MIME-Version: 1.0
+In-Reply-To: <GLPI_t3StN0t1f1c4tiOnUUID-Ticket-100.1554270788.1601670865@mycomputer>
+Content-Type: text/plain; charset=utf-8
+Content-Language: fr-FR
+Content-Transfer-Encoding: quoted-printable
+
+This is a reply that references Ticket 100 in In-Reply-To header (new format).
+It should be added as followup.

tests/emails-tests/04.5-ticket-followup-from-newformat-references.eml

+Return-Path: tech@glpi-project.org
+Received: from mail.glpi-project.org (localhost [127.0.0.1])
+	by mail.glpi-project.org (Postfix) with ESMTP id B74527E805E8
+	for <unittests@glpi-project.org>; Wed,  3 Apr 2019 11:48:35 +0200 (CEST)
+Subject: Re: New database issue
+References: <GLPI_t3StN0t1f1c4tiOnUUID-Ticket-100.1554270788.1601670865@mycomputer>
+From: Tech Ni Cian <tech@glpi-project.org>
+To: GLPI debug <unittests@glpi-project.org>
+Message-ID: <961c2885-94e6-2343-b000-280c809fcdb1@glpi-project.org>
+Date: Wed, 3 Apr 2019 11:48:25 +0200
+MIME-Version: 1.0
+Content-Type: text/plain; charset=utf-8
+Content-Language: fr-FR
+Content-Transfer-Encoding: quoted-printable
+
+This is a reply that references Ticket 100 in References header (new format).
+It should be added as followup.

tests/emails-tests/22.2-notification-from-glpi-new-messageid-format.eml

+Return-Path: noreply@glpi-project.org
+Received: from mail.glpi-project.org (localhost [127.0.0.1])
+    by mail.glpi-project.org (Postfix) with ESMTP id B74527E805E8
+    for <tech@glpi-project.org>; Wed,  3 Apr 2019 11:48:35 +0200 (CEST)
+Subject: [GLPI #0000001] New ticket
+From: noreply@glpi-project.org
+To: tech@glpi-project.org
+Message-ID: <GLPI_t3StN0t1f1c4tiOnUUID-Ticket-1.1611674092.1756143577@localhost>
+Date: Wed, 3 Apr 2019 11:48:25 +0200
+MIME-Version: 1.0
+In-Reply-To: <GLPI-t3StN0t1f1c4tiOnUUID-Ticket-1>
+Content-Type: text/plain; charset=utf-8
+Content-Language: fr-FR
+Content-Transfer-Encoding: quoted-printable
+
+This is a notification for a new ticket.
+It should be blacklisted to prevent infinite loop.

tests/emails-tests/23-uuid-from-another-glpi.eml

+Return-Path: tech@glpi-project.org
+Received: from mail.glpi-project.org (localhost [127.0.0.1])
+    by mail.glpi-project.org (Postfix) with ESMTP id B74527E805E8
+    for <tech@glpi-project.org>; Wed,  3 Apr 2019 11:48:35 +0200 (CEST)
+Subject: Re: [GLPI #0000001] New ticket
+References: <GLPI_1pAReXsxIoFd4pZEJhXZyrPGvef1ju3TMQcTTavt-Ticket-1.1611674092.1756143577@ae1a6b5a3a9c>
+From: Tech Ni Cian <tech@glpi-project.org>
+To: tech@glpi-project.org
+Message-ID: <961c2885-94e6-2343-b000-280c809fcdb1@glpi-project.org>
+Date: Wed, 3 Apr 2019 11:48:25 +0200
+MIME-Version: 1.0
+In-Reply-To: <GLPI_1pAReXsxIoFd4pZEJhXZyrPGvef1ju3TMQcTTavt-Ticket-1.1611674092.1756143577@ae1a6b5a3a9c>
+Content-Type: text/plain; charset=utf-8
+Content-Language: fr-FR
+Content-Transfer-Encoding: quoted-printable
+
+Hi,
+
+This is a response to a ticket notification from another GLPI instance.
+It should be blacklisted as we consider that current instance of GLPI is not responsible of this thread.
+
+Regards.

tests/imap/MailCollector.php

@@ -32,6 +32,7 @@
 
 namespace tests\units;
 
+use Config;
 use DbTestCase;
 use ITILFollowup;
 use Laminas\Mail\Storage\Message;
@@ -203,6 +204,12 @@ class MailCollector extends DbTestCase {
       $soft_notif = new NotificationTargetSoftwareLicense($root_ent_id, 'test_event', getItemByTypeName('SoftwareLicense', '_test_softlic_1'));
       $base_notif = new NotificationTarget();
 
+      $uuid = Config::getUuid('notification');
+
+      $time = time();
+      $rand = rand();
+      $uname = 'localhost';
+
       return [
          [
             'headers'  => [],
@@ -216,7 +223,25 @@ class MailCollector extends DbTestCase {
          ],
          [
             'headers'  => [
-               'message-id' => $ticket_notif->getMessageID(), // ticket format
+               'message-id' => "GLPI-1.{$time}.{$rand}@{$uname}", // old ticket format
+            ],
+            'expected' => true,
+         ],
+         [
+            'headers'  => [
+               'message-id' => "GLPI-SoftwareLicence-1.{$time}.{$rand}@{$uname}", // old format with object relation
+            ],
+            'expected' => true,
+         ],
+         [
+            'headers'  => [
+               'message-id' => "GLPI.{$time}.{$rand}@{$uname}", // old format without object relation
+            ],
+            'expected' => true,
+         ],
+         [
+            'headers'  => [
+               'message-id' => $ticket_notif->getMessageID(), // new format for ticket
             ],
             'expected' => true,
          ],
@@ -232,6 +257,18 @@ class MailCollector extends DbTestCase {
             ],
             'expected' => true,
          ],
+         [
+            'headers'  => [
+               'message-id' => "GLPI_notmyuuid-Ticket-1.{$time}.{$rand}@{$uname}", // new format with object relation
+            ],
+            'expected' => false,
+         ],
+         [
+            'headers'  => [
+               'message-id' => "GLPI_notmyuuid.{$time}.{$rand}@{$uname}", // new format without object relation
+            ],
+            'expected' => false,
+         ],
       ];
    }
 
@@ -260,6 +297,8 @@ class MailCollector extends DbTestCase {
       $soft_id   = getItemByTypeName('SoftwareLicense', '_test_softlic_1', true);
       $soft_notif = new NotificationTargetSoftwareLicense($root_ent_id, 'test_event', getItemByTypeName('SoftwareLicense', '_test_softlic_1'));
 
+      $uuid = Config::getUuid('notification');
+
       $time1 = time() - 548;
       $time2 = $time1 - 1567;
       $rand1 = rand();
@@ -278,10 +317,10 @@ class MailCollector extends DbTestCase {
             'expected_items_id' => null,
             'accepted'          => true,
          ],
-         // ticket header format - found item
+         // old ticket format - found item
          [
             'headers'           => [
-               'in-reply-to' => $ticket_notif->getMessageID(),
+               'in-reply-to' => "GLPI-{$ticket_id}.{$time1}.{$rand1}@{$uname1}",
             ],
             'expected_itemtype' => Ticket::class,
             'expected_items_id' => $ticket_id,
@@ -289,13 +328,13 @@ class MailCollector extends DbTestCase {
          ],
          [
             'headers'           => [
-               'references'  => $ticket_notif->getMessageID(),
+               'references'  => "GLPI-{$ticket_id}.{$time1}.{$rand1}@{$uname2}",
             ],
             'expected_itemtype' => Ticket::class,
             'expected_items_id' => $ticket_id,
             'accepted'          => true,
          ],
-         // ticket header format - invalid items_id
+         // old ticket format - invalid items_id
          [
             'headers'           => [
                'in-reply-to' => "GLPI-9999999.{$time2}.{$rand2}@{$uname1}",
@@ -304,15 +343,42 @@ class MailCollector extends DbTestCase {
             'expected_items_id' => null,
             'accepted'          => true,
          ],
-         // other items header format - found item
+         // old items header format - found item
          [
             'headers'           => [
-               'in-reply-to' => $soft_notif->getMessageID(),
+               'in-reply-to' => "GLPI-SoftwareLicense-{$soft_id}.{$time1}.{$rand2}@{$uname2}",
             ],
             'expected_itemtype' => SoftwareLicense::class,
             'expected_items_id' => $soft_id,
             'accepted'          => true,
          ],
+         // old items header format - invalid itemtype
+         [
+            'headers'           => [
+               'references'  => "GLPI-UnknownType-{$soft_id}.{$time2}.{$rand2}@{$uname1}",
+            ],
+            'expected_itemtype' => null,
+            'expected_items_id' => null,
+            'accepted'          => true,
+         ],
+         // old items header format - invalid items_id
+         [
+            'headers'           => [
+               'references'  => "GLPI-SoftwareLicense-9999999.{$time1}.{$rand1}@{$uname2}",
+            ],
+            'expected_itemtype' => null,
+            'expected_items_id' => null,
+            'accepted'          => true,
+         ],
+         // new header format - found item
+         [
+            'headers'           => [
+               'in-reply-to' => $ticket_notif->getMessageID(),
+            ],
+            'expected_itemtype' => Ticket::class,
+            'expected_items_id' => $ticket_id,
+            'accepted'          => true,
+         ],
          [
             'headers'           => [
                'references'  => $soft_notif->getMessageID(),
@@ -339,24 +405,41 @@ class MailCollector extends DbTestCase {
             'expected_items_id' => $soft_id,
             'accepted'          => true,
          ],
-         // other items header format - invalid itemtype
+         // new header format - invalid itemtype
          [
             'headers'           => [
-               'references'  => "GLPI-UnknownType-{$soft_id}.{$time2}.{$rand2}@{$uname1}",
+               'references'  => "GLPI_{$uuid}-UnknownType-{$ticket_id}.{$time2}.{$rand2}@{$uname1}",
             ],
             'expected_itemtype' => null,
             'expected_items_id' => null,
             'accepted'          => true,
          ],
-         // other items header format - invalid items_id
+         // new header format - invalid items_id
          [
             'headers'           => [
-               'references'  => "GLPI-SoftwareLicense-9999999.{$time1}.{$rand1}@{$uname2}",
+               'references'  => "GLPI_{$uuid}-Ticket-9999999.{$time1}.{$rand1}@{$uname1}",
             ],
             'expected_itemtype' => null,
             'expected_items_id' => null,
             'accepted'          => true,
          ],
+         // new header format - uuid from another GLPI instance
+         [
+            'headers'           => [
+               'in-reply-to' => "GLPI_notmyuuid-Ticket-{$ticket_id}.{$time1}.{$rand1}@{$uname2}",
+            ],
+            'expected_itemtype' => null,
+            'expected_items_id' => null,
+            'accepted'          => false,
+         ],
+         [
+            'headers'           => [
+               'references'  => "GLPI_notmyuuid-Ticket-{$ticket_id}.{$time2}.{$rand2}@{$uname1}",
+            ],
+            'expected_itemtype' => null,
+            'expected_items_id' => null,
+            'accepted'          => false,
+         ],
       ];
    }
 
@@ -388,6 +471,27 @@ class MailCollector extends DbTestCase {
       }
    }
 
+   /**
+    * @dataProvider itemReferenceHeaderProvider
+    */
+   public function testIsResponseToMessageSentByAnotherGlpi(
+      array $headers,
+      ?string $expected_itemtype,
+      ?int $expected_items_id,
+      bool $accepted
+   ) {
+      $this->newTestedInstance();
+
+      $message = new Message(
+         [
+            'headers' => $headers,
+            'content' => 'Message contents...',
+         ]
+      );
+
+      $this->boolean($this->testedInstance->isResponseToMessageSentByAnotherGlpi($message))->isEqualTo(!$accepted);
+   }
+
    private function doConnect() {
       if (null === $this->collector) {
          $this->newTestedInstance();
@@ -424,6 +528,9 @@ class MailCollector extends DbTestCase {
       global $DB;
       $_SESSION['glpicronuserrunning'] = 'cron_phpunit';
 
+      // Force notification_uuid
+      Config::setConfigurationValues('core', ['notification_uuid' => 't3StN0t1f1c4tiOnUUID']);
+
       //assign email to user
       $nuid = getItemByTypeName('User', 'normal', true);
       $uemail = new \UserEmail();
@@ -451,7 +558,7 @@ class MailCollector extends DbTestCase {
       $total_count                     = count(glob(GLPI_ROOT . '/tests/emails-tests/*.eml'));
       $expected_refused_count          = 2;
       $expected_error_count            = 2;
-      $expected_blacklist_count        = 1;
+      $expected_blacklist_count        = 3;
       $expected_expected_already_seen  = 0;
 
       $this->variable($msg)->isIdenticalTo(
@@ -635,18 +742,28 @@ class MailCollector extends DbTestCase {
          [
             'items_id' => 100,
             'users_id' => $tuid,
-            'content'  => 'This is a reply that references Ticket 100 in In-Reply-To header.<br />It should be added as followup.',
+            'content'  => 'This is a reply that references Ticket 100 in In-Reply-To header (old format).<br />It should be added as followup.',
          ],
          [
             'items_id' => 100,
             'users_id' => $tuid,
-            'content'  => 'This is a reply that references Ticket 100 in References header.<br />It should be added as followup.',
+            'content'  => 'This is a reply that references Ticket 100 in References header (old format).<br />It should be added as followup.',
          ],
          [
             'items_id' => 101,
             'users_id' => $tuid,
             'content'  => 'This is a reply that references Ticket 101 in its subject.<br />It should be added as followup.',
-         ]
+         ],
+         [
+            'items_id' => 100,
+            'users_id' => $tuid,
+            'content'  => 'This is a reply that references Ticket 100 in In-Reply-To header (new format).<br />It should be added as followup.',
+         ],
+         [
+            'items_id' => 100,
+            'users_id' => $tuid,
+            'content'  => 'This is a reply that references Ticket 100 in References header (new format).<br />It should be added as followup.',
+         ],
       ];
 
       foreach ($expected_followups as $expected_followup) {