From: Eileen McNaughton Date: Thu, 7 Sep 2023 08:08:17 +0000 (+1200) Subject: dev/core#2800 Fix bounce processing to handle verp emails X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=2efc3b177d554c13a65c755dacc549b652184f3f;p=civicrm-core.git dev/core#2800 Fix bounce processing to handle verp emails This fixes 2 issues when combining bounce processing with verp emails 1) the emails being processed are being matched to email they come from - ie the verp email. Hence the created activities are not linked to the actual contact and instead create endless variants of the sender. This happens with both the job.process_activities job and with job.fetch_bounces with is_create_activities = TRUE. This fix adapts the handling to identify verp emails (using existing regex) and look up the contact ID from the mailing_event_queue. This is used as the source (I didn't add a target or assignee at this stage cos I wasn't sure if that was just data-cruft or useful stuff - but it would be retrieved from the mailing) 2) this second one is a doozy & probably only showed up in tests because the tests mostly cover obscure email formats. The create activities code does a call to get the body and attachments using a function that looks like it came from a tutorial on how to best use ezcmail. It seems to parse all the email variants and attachments and, importantly, moves those attachments to the civicrm files as part of creating the activity. The code used to getBody() for bounces looks like it was ... written by us. It does a subset of the processing in earlier call to get body and in many of our test cases falls back on a blunt generateBody(). This call parses the email, including the attachments, which are no longer there cos they got moved.... so it fatals. I dug into the 2 functions and I feel pretty sure the bounce one doesn't add any additional value but DOES have less capability than the other. So I removed that code, Unfortunately the person received weird email from Exchange 2003 did not log the headers https://issues.civicrm.org/jira/browse/CRM-9361 - but even if the preferred version of the function doesn't parse that there is default text so it won't fail & it seems like the risk of having a bit less info about an (untested) obscure email bounce body is not that probematic --- diff --git a/CRM/Utils/Mail/EmailProcessor.php b/CRM/Utils/Mail/EmailProcessor.php index d004a808d6..eb4f991f1c 100644 --- a/CRM/Utils/Mail/EmailProcessor.php +++ b/CRM/Utils/Mail/EmailProcessor.php @@ -77,12 +77,13 @@ class CRM_Utils_Mail_EmailProcessor { */ private static function _process($civiMail, $dao, $is_create_activities) { // 0 = activities; 1 = bounce; - $usedfor = $dao->is_default; - if ($usedfor == 0) { - // create an array of all of to, from, cc, bcc that are in use for this Mail Account, so we don't create contacts for emails we aren't adding to the activity - $emailFields = array_filter(array_unique(array_merge(explode(",", $dao->activity_targets), explode(",", $dao->activity_assignees), explode(",", $dao->activity_source)))); - $createContact = !($dao->is_contact_creation_disabled_if_no_match ?? FALSE); - } + $isBounceProcessing = $dao->is_default; + $targetFields = array_filter(explode(',', $dao->activity_targets)); + $assigneeFields = array_filter(explode(",", $dao->activity_assignees)); + $sourceFields = array_filter(explode(",", $dao->activity_source)); + // create an array of all of to, from, cc, bcc that are in use for this Mail Account, so we don't create contacts for emails we aren't adding to the activity. + $emailFields = array_merge($targetFields, $assigneeFields, $sourceFields); + $createContact = !($dao->is_contact_creation_disabled_if_no_match); // retrieve the emails try { @@ -104,8 +105,8 @@ class CRM_Utils_Mail_EmailProcessor { $queue = $incomingMail->getQueueID(); $hash = $incomingMail->getHash(); - // preseve backward compatibility - if ($usedfor == 0 || $is_create_activities) { + // preserve backward compatibility + if (!$isBounceProcessing || $is_create_activities) { // Mail account may have 'Skip emails which do not have a Case ID // or Case hash' option, if its enabled and email is not related // to cases - then we need to put email to ignored folder. @@ -117,34 +118,39 @@ class CRM_Utils_Mail_EmailProcessor { // if its the activities that needs to be processed .. try { - $mailParams = CRM_Utils_Mail_Incoming::parseMailingObject($mail, $createContact, FALSE, $emailFields); + $mailParams = CRM_Utils_Mail_Incoming::parseMailingObject($mail, $incomingMail->getAttachments(), $createContact, $emailFields); $activityParams = [ 'activity_type_id' => (int) $dao->activity_type_id, 'campaign_id' => $dao->campaign_id ? (int) $dao->campaign_id : NULL, 'status_id' => $dao->activity_status, + 'subject' => $incomingMail->getSubject(), + 'activity_date_time' => $incomingMail->getDate(), + 'details' => $incomingMail->getBody(), ]; + if ($incomingMail->isVerp()) { + $activityParams['source_contact_id'] = $incomingMail->lookup('Queue', 'contact_id'); + } + else { + $activityParams['source_contact_id'] = $mailParams[$dao->activity_source][0]['id']; - $activityParams['source_contact_id'] = $mailParams[$dao->activity_source][0]['id']; - - $activityContacts = ['target_contact_id' => 'activity_targets', 'assignee_contact_id' => 'activity_assignees']; - foreach ($activityContacts as $activityContact => $daoName) { - $activityParams[$activityContact] = []; - $activityKeys = array_filter(explode(",", $dao->$daoName)); - foreach ($activityKeys as $activityKey) { - if (is_array($mailParams[$activityKey])) { - foreach ($mailParams[$activityKey] as $keyValue) { - if (!empty($keyValue['id'])) { - $activityParams[$activityContact][] = $keyValue['id']; + $activityContacts = [ + 'target_contact_id' => $targetFields, + 'assignee_contact_id' => $assigneeFields, + ]; + foreach ($activityContacts as $activityContact => $activityKeys) { + $activityParams[$activityContact] = []; + foreach ($activityKeys as $activityKey) { + if (is_array($mailParams[$activityKey])) { + foreach ($mailParams[$activityKey] as $keyValue) { + if (!empty($keyValue['id'])) { + $activityParams[$activityContact][] = $keyValue['id']; + } } } } } } - $activityParams['subject'] = $mailParams['subject']; - $activityParams['activity_date_time'] = $mailParams['date']; - $activityParams['details'] = $mailParams['body']; - $numAttachments = Civi::settings()->get('max_attachments_backend') ?? CRM_Core_BAO_File::DEFAULT_MAX_ATTACHMENTS_BACKEND; for ($i = 1; $i <= $numAttachments; $i++) { if (isset($mailParams["attachFile_$i"])) { @@ -184,54 +190,13 @@ class CRM_Utils_Mail_EmailProcessor { switch ($action) { case 'b': - $text = ''; - if ($mail->body instanceof ezcMailText) { - $text = $mail->body->text; - } - elseif ($mail->body instanceof ezcMailMultipart) { - $text = self::getTextFromMultipart($mail->body); - } - elseif ($mail->body instanceof ezcMailFile) { - $text = file_get_contents($mail->body->__get('fileName')); - } - - if ( - empty($text) && - $mail->subject === 'Delivery Status Notification (Failure)' - ) { - // Exchange error - CRM-9361 - foreach ($mail->body->getParts() as $part) { - if ($part instanceof ezcMailDeliveryStatus) { - foreach ($part->recipients as $rec) { - if ($rec['Status'] === '5.1.1') { - if (isset($rec['Description'])) { - $text = $rec['Description']; - } - else { - $text = $rec['Status'] . ' Delivery to the following recipients failed'; - } - break; - } - } - } - } - } - - if (empty($text)) { - // If bounce processing fails, just take the raw body. Cf. CRM-11046 - $text = $mail->generateBody(); - - // if text is still empty, lets fudge a blank text so the api call below will succeed - if (empty($text)) { - $text = ts('We could not extract the mail body from this bounce message.'); - } - } + $text = $incomingMail->getBody(); $activityParams = [ 'job_id' => $job, 'event_queue_id' => $queue, 'hash' => $hash, - 'body' => $text, + 'body' => $text ?: ts('We could not extract the mail body from this bounce message.'), 'version' => 3, // Setting is_transactional means it will rollback if // it crashes part way through creating the bounce. diff --git a/CRM/Utils/Mail/Incoming.php b/CRM/Utils/Mail/Incoming.php index 7f27bab670..11f0b23809 100644 --- a/CRM/Utils/Mail/Incoming.php +++ b/CRM/Utils/Mail/Incoming.php @@ -287,14 +287,14 @@ class CRM_Utils_Mail_Incoming { /** * @param $mail - * @param $createContact - * @param $requireContact + * @param $attachments + * @param bool $createContact * @param array $emailFields * Which fields to process and create contacts for of from, to, cc, bcc * * @return array */ - public static function parseMailingObject(&$mail, $createContact = TRUE, $requireContact = TRUE, $emailFields = ['from', 'to', 'cc', 'bcc']) { + public static function parseMailingObject(&$mail, $attachments, $createContact = TRUE, $emailFields = ['from', 'to', 'cc', 'bcc']) { // Sometimes $mail->from is unset because ezcMail didn't handle format // of From header. CRM-19215. @@ -314,15 +314,6 @@ class CRM_Utils_Mail_Incoming { } self::parseAddresses($value, $field, $params, $mail, $createContact); } - - // define other parameters - $params['subject'] = $mail->subject; - $params['date'] = date("YmdHi00", - strtotime($mail->getHeader("Date")) - ); - $attachments = []; - $params['body'] = self::formatMailPart($mail->body, $attachments); - // format and move attachments to the civicrm area if (!empty($attachments)) { $date = date('YmdHis'); diff --git a/CRM/Utils/Mail/IncomingMail.php b/CRM/Utils/Mail/IncomingMail.php index 4da3997eee..abf9f055c0 100644 --- a/CRM/Utils/Mail/IncomingMail.php +++ b/CRM/Utils/Mail/IncomingMail.php @@ -9,6 +9,9 @@ +--------------------------------------------------------------------+ */ +use Civi\API\EntityLookupTrait; +use Civi\Api4\MailingJob; + /** * Incoming mail class. * @@ -19,6 +22,8 @@ */ class CRM_Utils_Mail_IncomingMail { + use EntityLookupTrait; + /** * @var \ezcMail */ @@ -44,6 +49,27 @@ class CRM_Utils_Mail_IncomingMail { */ private $hash; + /** + * @var string|null + */ + private $body; + + /** + * @return string|null + */ + public function getBody(): ?string { + return $this->body; + } + + /** + * @return array + */ + public function getAttachments(): array { + return $this->attachments; + } + + private $attachments = []; + public function getAction() : ?string { return $this->action; } @@ -69,6 +95,17 @@ class CRM_Utils_Mail_IncomingMail { return $this->hash; } + /** + * @return string + */ + public function getSubject(): string { + return (string) $this->mail->subject; + } + + public function getDate(): string { + return date('YmdHis', strtotime($this->mail->getHeader('Date'))); + } + /** * Is this a verp email. * @@ -90,6 +127,7 @@ class CRM_Utils_Mail_IncomingMail { */ public function __construct(ezcMail $mail, string $emailDomain, string $emailLocalPart) { $this->mail = $mail; + $this->body = CRM_Utils_Mail_Incoming::formatMailPart($mail->body, $this->attachments); $verpSeparator = preg_quote(\Civi::settings()->get('verpSeparator') ?: ''); $emailDomain = preg_quote($emailDomain); @@ -145,6 +183,26 @@ class CRM_Utils_Mail_IncomingMail { if (!$matches && preg_match($regex, ($mail->getHeader('Delivered-To') ?? ''), $matches)) { [, $this->action, $this->jobID, $this->queueID, $this->hash] = $matches; } + if ($this->isVerp()) { + $queue = CRM_Mailing_Event_BAO_MailingEventQueue::verify($this->getJobID(), $this->getQueueID(), $this->getHash()); + if (!$queue) { + throw new CRM_Core_Exception('Contact could not be found from civimail response'); + } + $this->define('Queue', 'Queue', [ + 'id' => $queue->id, + 'hash' => $queue->hash, + 'contact_id' => $queue->contact_id, + 'job_id' => $queue->contact_id, + ]); + $this->define('Mailing', 'Mailing', [ + 'id' => MailingJob::get(FALSE) + ->addWhere('id', '=', $this->getJobID()) + ->addSelect('mailing_id') + ->execute() + ->first()['mailing_id'], + ]); + } + } } diff --git a/tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php b/tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php index de3a686dcd..e893939998 100644 --- a/tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php +++ b/tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php @@ -113,9 +113,10 @@ class CRM_Utils_Mail_EmailProcessorTest extends CiviUnitTestCase { $mail = 'test_sample_message.eml'; copy(__DIR__ . '/data/bounces/' . $mail, __DIR__ . '/data/mail/' . $mail); - $this->callAPISuccess('job', 'fetch_bounces', []); + $this->callAPISuccess('job', 'fetch_bounces', ['is_create_activities' => TRUE]); $this->assertFileDoesNotExist(__DIR__ . '/data/mail/' . $mail); $this->checkMailingBounces(1); + $this->callAPISuccessGetSingle('Activity', ['source_contact_id' => $this->contactID, 'activity_type_id' => 'Inbound Email']); } /**