Moves newsletter hash generating logic into Security helper class

Updates Links class to use Security helper's hash generating method

lib/Models/Newsletter.php

@@ -19,7 +19,6 @@ class Newsletter extends Model {
   const STATUS_SENT = 'sent';
   // automatic newsletters status
   const STATUS_ACTIVE = 'active';
-  const NEWSLETTER_HASH_LENGTH = 6;
 
   function __construct() {
     parent::__construct();
@@ -80,7 +79,7 @@ class Newsletter extends Model {
     $this->set('hash',
       ($this->hash)
       ? $this->hash
-      : self::generateHash()
+      : Security::generateHash()
     );
     return parent::save();
   }
@@ -787,12 +786,4 @@ class Newsletter extends Model {
     return parent::where('hash', $hash)
       ->findOne();
   }
-
-  static function generateHash() {
-    return substr(
-      md5(AUTH_KEY . Security::generateRandomString(15)),
-      0,
-      self::NEWSLETTER_HASH_LENGTH
-    );
-  }
 }

lib/Newsletter/Links/Links.php

@@ -12,7 +12,6 @@ use MailPoet\Util\Security;
 class Links {
   const DATA_TAG_CLICK = '[mailpoet_click_data]';
   const DATA_TAG_OPEN = '[mailpoet_open_data]';
-  const HASH_LENGTH = 5;
 
   const LINK_TYPE_SHORTCODE = 'shortcode';
   const LINK_TYPE_LINK = 'link';
@@ -72,7 +71,7 @@ class Links {
   static function hash($extracted_links) {
     $processed_links = array();
     foreach($extracted_links as $extracted_link) {
-      $hash = Security::generateRandomString(self::HASH_LENGTH);
+      $hash = Security::generateRandomString();
       // Use URL as a key to map between extracted and processed links
       // regardless of their sequential position (useful for link skips etc.)
       $key = $extracted_link['link'];
@@ -216,4 +215,4 @@ class Links {
     $transformed_data['preview'] = (!empty($data[4])) ? $data[4] : false;
     return $transformed_data;
   }
-}
+}

lib/Router/Endpoints/Track.php

@@ -49,7 +49,9 @@ class Track {
       Newsletter::findOne($data->queue->newsletter_id) :
       false;
     if(!empty($data->link_hash)) {
-      $data->link = NewsletterLink::getByHash($data->link_hash);
+      $data->link = NewsletterLink::where('queue_id', $data->queue_id)
+        ->where('hash', $data->link_hash)
+        ->findOne();
     }
     return $this->_validateTrackData($data);
   }

lib/Util/Security.php

@@ -5,6 +5,8 @@ if(!defined('ABSPATH')) exit;
 require_once(ABSPATH . 'wp-includes/pluggable.php');
 
 class Security {
+  const HASH_LENGTH = 6;
+
   static function generateToken($action = 'mailpoet_token') {
     return wp_create_nonce($action);
   }
@@ -17,4 +19,13 @@ class Security {
       min(max(5, (int)$length), 32)
     );
   }
+
+  static function generateHash($length = false) {
+    $length = ($length) ? $length : self::HASH_LENGTH;
+    return substr(
+      md5(AUTH_KEY . self::generateRandomString(15)),
+      0,
+      $length
+    );
+  }
 }

tests/unit/Models/NewsletterTest.php

@@ -10,6 +10,7 @@ use MailPoet\Models\NewsletterOption;
 use MailPoet\Models\StatisticsOpens;
 use MailPoet\Models\StatisticsClicks;
 use MailPoet\Models\StatisticsUnsubscribes;
+use MailPoet\Util\Security;
 
 class NewsletterTest extends MailPoetTest {
   function _before() {
@@ -358,21 +359,21 @@ class NewsletterTest extends MailPoetTest {
 
   function testItGeneratesHashOnNewsletterSave() {
     expect(strlen($this->newsletter->hash))
-      ->equals(Newsletter::NEWSLETTER_HASH_LENGTH);
+      ->equals(Security::HASH_LENGTH);
   }
 
   function testItRegeneratesHashOnNewsletterDuplication() {
     $duplicate_newsletter = $this->newsletter->duplicate();
     expect($duplicate_newsletter->hash)->notEquals($this->newsletter->hash);
     expect(strlen($duplicate_newsletter->hash))
-      ->equals(Newsletter::NEWSLETTER_HASH_LENGTH);
+      ->equals(Security::HASH_LENGTH);
   }
 
   function testItRegeneratesHashOnNotificationHistoryCreation() {
     $notification_history = $this->newsletter->createNotificationHistory();
     expect($notification_history->hash)->notEquals($this->newsletter->hash);
     expect(strlen($notification_history->hash))
-      ->equals(Newsletter::NEWSLETTER_HASH_LENGTH);
+      ->equals(Security::HASH_LENGTH);
   }
 
   function testItGetsQueueFromNewsletter() {

tests/unit/Util/SecurityTest.php

@@ -29,4 +29,15 @@ class SecurityTest extends MailPoetTest {
     expect(ctype_alnum($short_hash))->true();
     expect(ctype_alnum($long_hash))->true();
   }
+
+  function testItGeneratesRandomHash() {
+    $hash_1 = Security::generateHash();
+    $hash_2 = Security::generateHash();
+    expect($hash_1)->notEquals($hash_2);
+    expect(strlen($hash_1))->equals(Security::HASH_LENGTH);
+  }
+
+  function testItGeneratesRandomHashWithCustomLength() {
+    expect(strlen(Security::generateHash(10)))->equals(10);
+  }
 }