Merge pull request #35 from cirrusidentity/timing_fix_for_tests

Fix for window timing issues #30
cirrusidentity · May 16, 2024 · 838fabf · 838fabf
2 parents ae2ea3e + da831cf
commit 838fabf
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 8 deletions.
diff --git a/tests/Utils/BaseLimitTest.php b/tests/Utils/BaseLimitTest.php
@@ -5,13 +5,14 @@
 namespace SimpleSAML\Test\Module\ratelimit\Utils;
 
 use PHPUnit\Framework\TestCase;
+use SimpleSAML\Assert\Assert;
 use SimpleSAML\Configuration;
 use SimpleSAML\Module\ratelimit\Limiters\UserPassBaseLimiter;
 use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use SimpleSAML\Store\StoreFactory;
 use SimpleSAML\TestUtils\InMemoryStore;
 
-use function usleep;
+use function sleep;
 
 abstract class BaseLimitTest extends TestCase
 {
@@ -29,6 +30,49 @@ protected function tearDown(): void
 
     abstract protected function getLimiter(array $config): UserPassBaseLimiter;
 
+    /**
+     * If the current rate limit window has less than $miTime seconds left, sleep till the
+     * next window.
+     * @param UserPassBaseLimiter $limiter
+     * @param int $minTime minimum seconds left in the window
+     * @return void
+     */
+    public function waitTillWindowHasAtLeastMinTime(UserPassBaseLimiter $limiter, int $minTime): void
+    {
+        $time = time();
+        $startingWindow = $limiter->determineWindowExpiration($time);
+        $windowTimeLeft = $startingWindow - $time;
+        $this->assertGreaterThanOrEqual(0, $windowTimeLeft);
+        if ($windowTimeLeft < $minTime) {
+            echo "Waiting for new wait limit window " . $windowTimeLeft;
+            /** @psalm-suppress InvalidArgument */
+            sleep($windowTimeLeft);
+            $this->assertNotEquals(
+                $startingWindow,
+                $limiter->determineWindowExpiration(time()),
+                "Unable to start new window"
+            );
+        } else {
+            echo "$windowTimeLeft seconds left in window $startingWindow";
+        }
+    }
+
+    /**
+     * Some tests require sleeping until the next time window starts.
+     * @param int $currentWindow The current time window
+     * @param UserPassBaseLimiter $limiter
+     * @return void
+     */
+    public function sleepTillNextWindow(int $currentWindow, UserPassBaseLimiter $limiter): void
+    {
+        while ($currentWindow == $limiter->determineWindowExpiration(time())) {
+            sleep(1);
+        }
+        // Pending discussion from https://github.com/simplesamlphp/simplesamlphp-test-framework/issues/5
+        // Sleep an extra second since inMemoryStore considers data expired 1 second after expiration date
+        sleep(1);
+    }
+
     /**
      *  Test window calculation
      */
@@ -64,17 +108,26 @@ public function testAllowAndFailure(): void
         $limiter = $this->getLimiter($config);
         $username = 'Homer';
         $password = 'Beer';
+        $this->waitTillWindowHasAtLeastMinTime($limiter, 2);
+        $startingWindow = $limiter->determineWindowExpiration(time());
         for ($i = 1; $i <= 3; $i++) {
             // First 3 attempts should not be blocked
             $this->assertEquals(PreAuthStatusEnum::CONTINUE, $limiter->allow($username, $password), "Attempt $i");
+            $currentWindow = $limiter->determineWindowExpiration(time());
+            $this->assertEquals($startingWindow, $currentWindow, 'Cache window changed during test');
             $this->assertEquals($i, $limiter->postFailure($username, $password));
             $this->assertEquals($i, $this->getStoreValueFor($limiter->getRateLimitKey($username, $password)));
         }
         // After 3 failed attempts it should be blocked
         $this->assertEquals(PreAuthStatusEnum::BLOCK, $limiter->allow($username, $password));
 
         // Sleep until the next window, and counter should be reset
-        usleep(4020000);
+        $this->sleepTillNextWindow($startingWindow, $limiter);
+        $this->assertNotEquals(
+            $startingWindow,
+            $limiter->determineWindowExpiration(time()),
+            'Next cache window expected'
+        );
         $this->assertNull(
             $this->getStoreValueFor($limiter->getRateLimitKey($username, $password)),
             'Value not expected in store'

diff --git a/tests/src/Limiters/IpLimiterTest.php b/tests/src/Limiters/IpLimiterTest.php
@@ -42,7 +42,10 @@ public function testIpWhitelist(string $ip, bool $ignoreExpected): void
 
         $limiter = $this->getLimiter($config);
         $this->assertEquals($ignoreExpected ? 0 : 1, $limiter->postFailure('u', 'p'));
-        $this->assertEquals($ignoreExpected ? PreAuthStatusEnum::CONTINUE : PreAuthStatusEnum::BLOCK, $limiter->allow('u', 'p'));
+        $this->assertEquals(
+            $ignoreExpected ? PreAuthStatusEnum::CONTINUE : PreAuthStatusEnum::BLOCK,
+            $limiter->allow('u', 'p')
+        );
     }
 
     public static function ipWhitelistProvider(): array

diff --git a/tests/src/Limiters/PasswordStuffingLimiterTest.php b/tests/src/Limiters/PasswordStuffingLimiterTest.php
@@ -11,8 +11,6 @@
 use SimpleSAML\TestUtils\InMemoryStore;
 use SimpleSAML\Test\Module\ratelimit\Utils\BaseLimitTest;
 
-use function sleep;
-
 #[CoversClass(PasswordStuffingLimiter::class)]
 class PasswordStuffingLimiterTest extends BaseLimitTest
 {
@@ -27,18 +25,20 @@ protected function getLimiter(array $config): UserPassBaseLimiter
     public function testKeyVariesWithWindow(): void
     {
         $config = [
-          'window' => 'PT2S'
+          'window' => 'PT3S'
         ];
 
         $limiter = $this->getLimiter($config);
 
         $password = 'abcXYZ123';
-        //TODO: adjust time to be the start of a window
+        $this->waitTillWindowHasAtLeastMinTime($limiter, 1);
+        $startingWindow = $limiter->determineWindowExpiration(time());
         $result = $limiter->getRateLimitKey('efg', $password);
         $this->assertEquals($result, $limiter->getRateLimitKey('xyz', $password));
         $this->assertEquals($result, $limiter->getRateLimitKey('abc', $password));
 
-        sleep(3);
+        $this->sleepTillNextWindow($startingWindow, $limiter);
+        $this->waitTillWindowHasAtLeastMinTime($limiter, 1);
         // Next window should have different keys
         $newKey = $limiter->getRateLimitKey('efg', $password);
         $this->assertNotEquals($result, $newKey, 'Key should vary with window');