Merge pull request #458 from keevan/457-round-expiry-times

Round down s3 expiry to nearest minute, add more comments #457
catalyst · Dec 23, 2021 · 329e8ee · 329e8ee
2 parents 35951fd + 6fe586e
commit 329e8ee
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 32 deletions.
diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php
@@ -571,26 +571,32 @@ public function get_expiration_time($now, $expires) {
             // Convert to a valid timestamp.
             $expires = strtotime($expires);
         }
+        // Invalid or already expired:
         // If it's set to 0 or strtotime() returned false,
         // set it to default + 1 min as a healthy margin.
         if (empty($expires)) {
             $expires = $now + $this->expirationtime + MINSECS;
         }
-        // If it's already expired or expires less than in a minute,
-        // set it to 1 minute.
-        if ($expires < $now + MINSECS) {
-            $expires = $now + MINSECS;
+        // Expiry too short, push it out to the next 2 minutes (will round down later):
+        // If it's already expired or expires less than 2 minutes, set it to 2
+        // minutes. This works together with rounding down later on to ensure a
+        // non zero expiry time, and a minimum expiry of 1 minute.
+        if ($expires < $now + (2 * MINSECS)) {
+            $expires = $now + (2 * MINSECS);
         }
+        // Checks upper bound:
         // The expiration date of a signature version 4 presigned URL must be
         // less than one week. So if it's greater than a week set it to 1 week.
         // Use MINSECS as a healthy margin of error.
         if ($expires - $now > WEEKSECS - MINSECS) {
             $expires = $now + WEEKSECS - MINSECS;
         }
-        if (is_null($expires) || false === $expires) {
-            // Invalid date format use default instead.
-            $expires = $now + $this->expirationtime;
-        }
+        // Rounds (down) to nearest minute:
+        // With our new expiry time, ensure we round down to the nearest minute
+        // (#457) to ensure expiry of potentially the same file will use the
+        // same URL, and will result in less duplicate requests.
+        $expires -= ($expires % MINSECS);
+
         return $expires;
     }
 

diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php
@@ -713,36 +713,44 @@ public function test_presigned_url_should_redirect_method_with_data_provider($en
      */
     public function test_get_expiration_time_method_if_supported_provider() {
         $now = time();
+
+        // Seconds after the minute from X.
+        $secondsafternow = ($now % MINSECS);
+        $secondsafternowsub100 = $secondsafternow;
+        $secondsafternowadd30 = $secondsafternow;
+        $secondsafternowadd100 = $secondsafternow;
+        $secondsafternowaddweek = ($now + WEEKSECS) % MINSECS;
+
         return [
             // Default Pre-Signed URL expiration time and int-like 'Expires' header.
-            [7200, $now, 0, $now + 7200 + MINSECS],
-            [7200, $now, $now - 100, $now + MINSECS],
-            [7200, $now, $now + 30, $now + MINSECS],
-            [7200, $now, $now + 100, $now + 100],
-            [7200, $now, $now + WEEKSECS + HOURSECS, $now + WEEKSECS - MINSECS],
+            [7200, $now, 0, $now + 7200 + MINSECS - $secondsafternow],
+            [7200, $now, $now - 100, $now + (2 * MINSECS) - $secondsafternowsub100],
+            [7200, $now, $now + 30, $now + (2 * MINSECS) - $secondsafternowadd30],
+            [7200, $now, $now + 100, $now + (2 * MINSECS) - $secondsafternowadd100],
+            [7200, $now, $now + WEEKSECS + HOURSECS, $now + WEEKSECS - MINSECS - $secondsafternowaddweek],
 
             // Default Pre-Signed URL expiration time and string-like 'Expires' header.
-            [7200, $now, 'Thu, 01 Jan 1970 00:00:00 GMT', $now + 7200 + MINSECS],
-            [7200, $now, userdate($now - 100, '%a, %d %b %Y %H:%M:%S'), $now + MINSECS],
-            [7200, $now, userdate($now + 30, '%a, %d %b %Y %H:%M:%S'), $now + MINSECS],
-            [7200, $now, userdate($now + 100, '%a, %d %b %Y %H:%M:%S'), $now + 100],
-            [7200, $now, userdate($now + WEEKSECS + HOURSECS, '%a, %d %b %Y %H:%M:%S'), $now + WEEKSECS - MINSECS],
+            [7200, $now, 'Thu, 01 Jan 1970 00:00:00 GMT', $now + 7200 + MINSECS - $secondsafternow],
+            [7200, $now, userdate($now - 100, '%a, %d %b %Y %H:%M:%S'), $now + (2 * MINSECS) - $secondsafternowsub100],
+            [7200, $now, userdate($now + 30, '%a, %d %b %Y %H:%M:%S'), $now + (2 * MINSECS) - $secondsafternowadd30],
+            [7200, $now, userdate($now + 100, '%a, %d %b %Y %H:%M:%S'), $now + (2 * MINSECS) - $secondsafternowadd100],
+            [7200, $now, userdate($now + WEEKSECS + HOURSECS, '%a, %d %b %Y %H:%M:%S'), $now + WEEKSECS - MINSECS - $secondsafternowaddweek],
 
             // Custom Pre-Signed URL expiration time and int-like 'Expires' header.
-            [0, $now, 0, $now + MINSECS],
-            [600, $now, 0, $now + 600 + MINSECS],
-            [600, $now, $now - 100, $now + MINSECS],
-            [600, $now, $now + 30, $now + MINSECS],
-            [600, $now, $now + 100, $now + 100],
-            [600, $now, $now + WEEKSECS + HOURSECS, $now + WEEKSECS - MINSECS],
+            [0, $now, 0, $now + (2 * MINSECS) - $secondsafternow],
+            [600, $now, 0, $now + 600 + MINSECS - $secondsafternow],
+            [600, $now, $now - 100, $now + (2 * MINSECS) - $secondsafternowsub100],
+            [600, $now, $now + 30, $now + (2 * MINSECS) - $secondsafternowadd30],
+            [600, $now, $now + 100, $now + (2 * MINSECS) - $secondsafternowadd100],
+            [600, $now, $now + WEEKSECS + HOURSECS, $now + WEEKSECS - MINSECS - $secondsafternowaddweek],
 
             // Custom Pre-Signed URL expiration time and string-like 'Expires' header.
-            [0, $now, 'Thu, 01 Jan 1970 00:00:00 GMT', $now + MINSECS],
-            [600, $now, 'Thu, 01 Jan 1970 00:00:00 GMT', $now + 600 + MINSECS],
-            [600, $now, userdate($now - 100, '%a, %d %b %Y %H:%M:%S'), $now + MINSECS],
-            [600, $now, userdate($now + 30, '%a, %d %b %Y %H:%M:%S'), $now + MINSECS],
-            [600, $now, userdate($now + 100, '%a, %d %b %Y %H:%M:%S'), $now + 100],
-            [600, $now, userdate($now + WEEKSECS + HOURSECS, '%a, %d %b %Y %H:%M:%S'), $now + WEEKSECS - MINSECS],
+            [0, $now, 'Thu, 01 Jan 1970 00:00:00 GMT', $now + (2 * MINSECS) - $secondsafternow],
+            [600, $now, 'Thu, 01 Jan 1970 00:00:00 GMT', $now + 600 + MINSECS - $secondsafternow],
+            [600, $now, userdate($now - 100, '%a, %d %b %Y %H:%M:%S'), $now + (2 * MINSECS) - $secondsafternowsub100],
+            [600, $now, userdate($now + 30, '%a, %d %b %Y %H:%M:%S'), $now + (2 * MINSECS) - $secondsafternowadd30],
+            [600, $now, userdate($now + 100, '%a, %d %b %Y %H:%M:%S'), $now + (2 * MINSECS) - $secondsafternowadd100],
+            [600, $now, userdate($now + WEEKSECS + HOURSECS, '%a, %d %b %Y %H:%M:%S'), $now + WEEKSECS - MINSECS - $secondsafternowaddweek],
         ];
     }
 

diff --git a/version.php b/version.php
@@ -25,8 +25,8 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2021101100;      // The current plugin version (Date: YYYYMMDDXX).
-$plugin->release   = 2021101100;      // Same as version.
+$plugin->version   = 2021122300;      // The current plugin version (Date: YYYYMMDDXX).
+$plugin->release   = 2021122300;      // Same as version.
 $plugin->requires  = 2013111811;      // Requires Filesystem API.
 $plugin->component = "tool_objectfs";
 $plugin->maturity  = MATURITY_STABLE;