generateFileHash() and validateFileHash() should be colocated

The two functions (`generateFileHash()` and `validateFileHash()`) are
tightly-coupled.  Most changes to one would require a matching change in the
other.  So they should be parallel.

It'd be OK to say "the hash formula is a general utility for any party using
file APIs" (so put `generateFileHash()` and `validateFileHash()` in `CRM_Core_BAO_File`).

It'd be OK to say "the hash formula is specific to the end-point/page which
serves files" (so put `generateFileHash()` and `validateFileHash()` in
`CRM_Core_Page_File`).

The former feels a bit more accurate, so I pushed it toward that.

diff --git a/CRM/Core/BAO/File.php b/CRM/Core/BAO/File.php
index a1aba721068fe90b5028f6b85902eba821428935..4468dd8c0f2da465d58767675902703c09170074 100644 (file)
--- a/CRM/Core/BAO/File.php
+++ b/CRM/Core/BAO/File.php
@@ -789,4 +789,28 @@ AND       CEF.entity_id    = %2";
     return "{$cs}_{$genTs}_{$life}";
   }
 
+  /**
+   * Validate a file Hash
+   * @param string $hash
+   * @param int $eid Entity Id the file is attached to
+   * @param int $fid File Id
+   * @return bool
+   */
+  public static function validateFileHash($hash, $eid, $fid) {
+    $input = CRM_Utils_System::explode('_', $hash, 3);
+    $inputTs = CRM_Utils_Array::value(1, $input);
+    $inputLF = CRM_Utils_Array::value(2, $input);
+    $testHash = CRM_Core_BAO_File::generateFileHash($eid, $fid, $inputTs, $inputLF);
+    if (hash_equals($testHash, $hash)) {
+      $now = time();
+      if ($inputTs + ($inputLF * 60 * 60) >= $now) {
+        return TRUE;
+      }
+      else {
+        return FALSE;
+      }
+    }
+    return FALSE;
+  }
+
 }

diff --git a/CRM/Core/Page/File.php b/CRM/Core/Page/File.php
index 3f0e894193457f0a6f6b939ec6d8003f10b141f4..dec65cc44c3c01a60971e56c02499d65c6526864 100644 (file)
--- a/CRM/Core/Page/File.php
+++ b/CRM/Core/Page/File.php
@@ -46,7 +46,7 @@ class CRM_Core_Page_File extends CRM_Core_Page {
     $fid = CRM_Utils_Request::retrieve('fid', 'Positive', $this, FALSE);
     $id = CRM_Utils_Request::retrieve('id', 'Positive', $this, TRUE);
     $hash = CRM_Utils_Request::retrieve('fcs', 'Alphanumeric', $this);
-    if (!self::validateFileHash($hash, $eid, $fid)) {
+    if (!CRM_Core_BAO_File::validateFileHash($hash, $eid, $fid)) {
       CRM_Core_Error::statusBounce('URL for file is not valid');
     }
 
@@ -84,28 +84,4 @@ class CRM_Core_Page_File extends CRM_Core_Page {
     }
   }
 
-  /**
-   * Validate a file Hash
-   * @param string $hash
-   * @param int $eid Entity Id the file is attached to
-   * @param int $fid File Id
-   * @return bool
-   */
-  public static function validateFileHash($hash, $eid, $fid) {
-    $input = CRM_Utils_System::explode('_', $hash, 3);
-    $inputTs = CRM_Utils_Array::value(1, $input);
-    $inputLF = CRM_Utils_Array::value(2, $input);
-    $testHash = CRM_Core_BAO_File::generateFileHash($eid, $fid, $inputTs, $inputLF);
-    if (hash_equals($testHash, $hash)) {
-      $now = time();
-      if ($inputTs + ($inputLF * 60 * 60) >= $now) {
-        return TRUE;
-      }
-      else {
-        return FALSE;
-      }
-    }
-    return FALSE;
-  }
-
 }

diff --git a/tests/phpunit/api/v3/AttachmentTest.php b/tests/phpunit/api/v3/AttachmentTest.php
index b02abf868e2ad5fbdcee302555b78b6c92f4557d..f2721ec852fc3751dd53cf58aacd4f5ddf5904c3 100644 (file)
--- a/tests/phpunit/api/v3/AttachmentTest.php
+++ b/tests/phpunit/api/v3/AttachmentTest.php
@@ -510,7 +510,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
       $queryResult = [];
       $parsedURl = parse_url($result['url']);
       $parsedQuery = parse_str($parsedURl['query'], $queryResult);
-      $this->assertTrue(CRM_Core_Page_File::validateFileHash($queryResult['fcs'], $queryResult['eid'], $queryResult['id']));
+      $this->assertTrue(CRM_Core_BAO_File::validateFileHash($queryResult['fcs'], $queryResult['eid'], $queryResult['id']));
     }
 
     sort($actualNames);