From 4b6bbccf1c998f6632c630af966a1c5c17a79b51 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 14 Jan 2021 02:28:21 -0800 Subject: [PATCH 1/3] (NFC) BaseMailingSystemTest - Tweak debug message for assertion --- tests/phpunit/CRM/Mailing/BaseMailingSystemTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/phpunit/CRM/Mailing/BaseMailingSystemTest.php b/tests/phpunit/CRM/Mailing/BaseMailingSystemTest.php index 3e3a9c540277..87670df9ff48 100644 --- a/tests/phpunit/CRM/Mailing/BaseMailingSystemTest.php +++ b/tests/phpunit/CRM/Mailing/BaseMailingSystemTest.php @@ -325,7 +325,6 @@ public function urlTrackingExamples() { * @throws \CRM_Core_Exception */ public function testUrlTracking($inputHtml, $htmlUrlRegex, $textUrlRegex, $params) { - $caseName = print_r(['inputHtml' => $inputHtml, 'params' => $params], 1); $allMessages = $this->runMailingSuccess($params + [ 'subject' => 'Example Subject', @@ -341,11 +340,13 @@ public function testUrlTracking($inputHtml, $htmlUrlRegex, $textUrlRegex, $param list($textPart, $htmlPart) = $message->body->getParts(); if ($htmlUrlRegex) { + $caseName = print_r(['inputHtml' => $inputHtml, 'params' => $params, 'htmlUrlRegex' => $htmlUrlRegex, 'htmlPart' => $htmlPart->text], 1); $this->assertEquals('html', $htmlPart->subType, "Should have HTML part in case: $caseName"); $this->assertRegExp($htmlUrlRegex, $htmlPart->text, "Should have correct HTML in case: $caseName"); } if ($textUrlRegex) { + $caseName = print_r(['inputHtml' => $inputHtml, 'params' => $params, 'textUrlRegex' => $textUrlRegex, 'textPart' => $textPart->text], 1); $this->assertEquals('plain', $textPart->subType, "Should have text part in case: $caseName"); $this->assertRegExp($textUrlRegex, $textPart->text, "Should have correct text in case: $caseName"); } From ba75e300ac0de014c1fdbc5bcf2caad4754bb998 Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Wed, 9 Dec 2020 10:12:36 +0000 Subject: [PATCH 2/3] Enable tracking of urls with tokens in Flexmailer --- .../src/ClickTracker/BaseClickTracker.php | 82 +++++++++++ .../src/ClickTracker/HtmlClickTracker.php | 19 ++- .../src/ClickTracker/TextClickTracker.php | 15 +- .../Civi/FlexMailer/ClickTrackerTest.php | 139 ++++++++++++++++++ .../Civi/FlexMailer/FlexMailerSystemTest.php | 13 ++ 5 files changed, 258 insertions(+), 10 deletions(-) create mode 100644 ext/flexmailer/src/ClickTracker/BaseClickTracker.php create mode 100644 ext/flexmailer/tests/phpunit/Civi/FlexMailer/ClickTrackerTest.php diff --git a/ext/flexmailer/src/ClickTracker/BaseClickTracker.php b/ext/flexmailer/src/ClickTracker/BaseClickTracker.php new file mode 100644 index 000000000000..970b52fbd05c --- /dev/null +++ b/ext/flexmailer/src/ClickTracker/BaseClickTracker.php @@ -0,0 +1,82 @@ +installMe(__DIR__) + ->apply(); + } + + public function setUp() { + // Mock the getTrackerURL call; we don't need to test creating a row in a table. + BaseClickTracker::$getTrackerURL = function($a, $b, $c) { + return 'http://example.com/extern?u=1&qid=1'; + }; + + parent::setUp(); + } + + public function tearDown() { + // Reset the class. + BaseClickTracker::$getTrackerURL = ['CRM_Mailing_BAO_TrackableURL', 'getTrackerURL']; + parent::tearDown(); + } + + /** + * Example: Test that a link without any tokens works. + */ + public function testLinkWithoutTokens() { + $filter = new TextClickTracker(); + $msg = 'See this: https://example.com/foo/bar?a=b&c=d#frag'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: http://example.com/extern?u=1&qid=1', $result); + } + + /** + * Example: Test that a link with tokens in the query works. + */ + public function testLinkWithTokensInQueryWithStaticParams() { + $filter = new TextClickTracker(); + $msg = 'See this: https://example.com/foo/bar?a=b&cid={contact.id}'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cid={contact.id}', $result); + } + + /** + * Example: Test that a link with tokens in the query works. + */ + public function testLinkWithTokensInQueryWithMultipleStaticParams() { + $filter = new TextClickTracker(); + $msg = 'See this: https://example.com/foo/bar?cs={contact.checksum}&a=b&cid={contact.id}'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cs={contact.checksum}&cid={contact.id}', $result); + } + + /** + * Example: Test that a link with tokens in the query works. + */ + public function testLinkWithTokensInQueryWithMultipleStaticParamsHtml() { + $filter = new HtmlClickTracker(); + $msg = 'See this'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this', $result); + } + + /** + * Example: Test that a link with tokens in the query works. + */ + public function testLinkWithTokensInQueryWithoutStaticParams() { + $filter = new TextClickTracker(); + $msg = 'See this: https://example.com/foo/bar?cid={contact.id}'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cid={contact.id}', $result); + } + + /** + * Example: Test that a link with tokens in the fragment works. + * + * Seems browsers maintain the fragment when they receive a redirect, so a + * token here might still work. + */ + public function testLinkWithTokensInFragment() { + $filter = new TextClickTracker(); + $msg = 'See this: https://example.com/foo/bar?a=b#cid={contact.id}'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: http://example.com/extern?u=1&qid=1#cid={contact.id}', $result); + } + + /** + * Example: Test that a link with tokens in the fragment works. + * + * Seems browsers maintain the fragment when they receive a redirect, so a + * token here might still work. + */ + public function testLinkWithTokensInQueryAndFragment() { + $filter = new TextClickTracker(); + $msg = 'See this: https://example.com/foo/bar?a=b&cid={contact.id}#cid={contact.id}'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cid={contact.id}#cid={contact.id}', $result); + } + + /** + * We can't handle tokens in the domain so it should not be tracked. + */ + public function testLinkWithTokensInDomainFails() { + $filter = new TextClickTracker(); + $msg = 'See this: https://{some.domain}.com/foo/bar'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: https://{some.domain}.com/foo/bar', $result); + } + + /** + * We can't handle tokens in the path so it should not be tracked. + */ + public function testLinkWithTokensInPathFails() { + $filter = new TextClickTracker(); + $msg = 'See this: https://example.com/{some.path}'; + $result = $filter->filterContent($msg, 1, 1); + $this->assertEquals('See this: https://example.com/{some.path}', $result); + } + +} diff --git a/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php b/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php index d592787e8d0f..5e4504ca702a 100644 --- a/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php +++ b/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php @@ -110,6 +110,19 @@ public function testUrlTracking( parent::testUrlTracking($inputHtml, $htmlUrlRegex, $textUrlRegex, $params); } + /** + * + * This takes CiviMail's own ones, but removes one that tested for a + * non-feature (i.e. that tokenised links are not handled). + * + * @return array + */ + public function urlTrackingExamples() { + $cases = parent::urlTrackingExamples(); + unset($cases[6]); + return $cases; + } + public function testBasicHeaders() { parent::testBasicHeaders(); } From 6ad859f1b83295522d94e5392dd1cb00d5cd3775 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 14 Jan 2021 02:29:49 -0800 Subject: [PATCH 3/3] Simplify tracking of URLs In the prior commit, it needed to conceptually change the behavior of `getTrackerURL($oldURL): $trackedURL`, but it danced around it (adding more classes and similar-sounding-methods, but distributed elsewhere). This re-consolidates to keep a cleaner separation of responsibilities between `{Html,Text}ClickTracker.php` (*parsing an HTML or text message for URLS*) and `TrackableURL.php` (*translating the URL*). --- CRM/Mailing/BAO/TrackableURL.php | 68 +++++++++++++++ .../src/ClickTracker/BaseClickTracker.php | 82 ------------------- .../src/ClickTracker/HtmlClickTracker.php | 20 +---- .../src/ClickTracker/TextClickTracker.php | 16 +--- .../Civi/FlexMailer/ClickTrackerTest.php | 15 ++-- .../Civi/FlexMailer/FlexMailerSystemTest.php | 11 ++- 6 files changed, 94 insertions(+), 118 deletions(-) delete mode 100644 ext/flexmailer/src/ClickTracker/BaseClickTracker.php diff --git a/CRM/Mailing/BAO/TrackableURL.php b/CRM/Mailing/BAO/TrackableURL.php index 0113e53bb9b7..ea6d032db4d5 100644 --- a/CRM/Mailing/BAO/TrackableURL.php +++ b/CRM/Mailing/BAO/TrackableURL.php @@ -38,7 +38,15 @@ public function __construct() { * The redirect/tracking url */ public static function getTrackerURL($url, $mailing_id, $queue_id) { + if (strpos($url, '{') !== FALSE) { + return self::getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id); + } + else { + return self::getBasicTrackerURL($url, $mailing_id, $queue_id); + } + } + private static function getBasicTrackerURL($url, $mailing_id, $queue_id) { static $urlCache = []; if (array_key_exists($mailing_id . $url, $urlCache)) { @@ -87,6 +95,66 @@ public static function getTrackerURL($url, $mailing_id, $queue_id) { return $returnUrl; } + /** + * Create a trackable URL for a URL with tokens. + * + * @param string $url + * @param int $mailing_id + * @param int|string $queue_id + * + * @return string + */ + private static function getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id) { + + // Parse the URL. + // (not using parse_url because it's messy to reassemble) + if (!preg_match('/^([^?#]+)([?][^#]*)?(#.*)?$/', $url, $parsed)) { + // Failed to parse it, give up and don't track it. + return $url; + } + + // If we have a token in the URL + path section, we can't tokenise. + if (strpos($parsed[1], '{') !== FALSE) { + return $url; + } + + $trackable_url = $parsed[1]; + + // Process the query parameters, if there are any. + $tokenised_params = []; + $static_params = []; + if (!empty($parsed[2])) { + $query_key_value_pairs = explode('&', substr($parsed[2], 1)); + + // Separate the tokenised from the static parts. + foreach ($query_key_value_pairs as $_) { + if (strpos($_, '{') === FALSE) { + $static_params[] = $_; + } + else { + $tokenised_params[] = $_; + } + } + // Add the static params to the trackable part. + if ($static_params) { + $trackable_url .= '?' . implode('&', $static_params); + } + } + + // Get trackable URL. + $data = self::getBasicTrackerURL($trackable_url, $mailing_id, $queue_id); + + // Append the tokenised bits and the fragment. + if ($tokenised_params) { + // We know the URL will already have the '?' + $data .= '&' . implode('&', $tokenised_params); + } + if (!empty($parsed[3])) { + $data .= $parsed[3]; + } + return $data; + } + /** * @param $url * @param $mailing_id diff --git a/ext/flexmailer/src/ClickTracker/BaseClickTracker.php b/ext/flexmailer/src/ClickTracker/BaseClickTracker.php deleted file mode 100644 index 970b52fbd05c..000000000000 --- a/ext/flexmailer/src/ClickTracker/BaseClickTracker.php +++ /dev/null @@ -1,82 +0,0 @@ -Foo

', + ';

Foo

;', + ';\\[1\\] .*(extern/url.php|civicrm/mailing/url)[\?&]u=\d+.*&id=\d+.*;', + ['url_tracking' => 1], + ]; + return $cases; }