Use enum for different possible statuses

cirrusidentity · May 9, 2024 · b9f9a10 · b9f9a10
1 parent 6b0567e
commit b9f9a10
Show file tree

Hide file tree

Showing 10 changed files with 53 additions and 39 deletions.
diff --git a/src/Auth/Source/RateLimitUserPass.php b/src/Auth/Source/RateLimitUserPass.php
@@ -18,6 +18,7 @@
     UsernameLimiter,
     UserPassLimiter,
 };
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use SimpleSAML\Store\{StoreFactory, StoreInterface};
 
 use function get_class;
@@ -42,7 +43,10 @@ class RateLimitUserPass extends UserPassBase
      */
     private array $rateLimiters = [];
 
-    /** @psalm-suppress MissingClassConstType */
+    /**
+     * @var array
+     * psalm-suppress MissingClassConstType
+     */
     private const DEFAULT_CONFIG = [
         0 => [
             'device',
@@ -207,16 +211,14 @@ public function allowLoginAttempt(string $username, string $password): bool
                 continue;
             }
             switch ($result) {
-                case 'allow':
+                case PreAuthStatusEnum::ALLOW:
                     Logger::debug(sprintf('User \'%s\' login attempt allowed by %s', $username, get_class($limiter)));
                     return true;
-                case 'block':
+                case PreAuthStatusEnum::BLOCK:
                     Logger::stats(sprintf('User \'%s\' login attempt blocked by %s', $username, get_class($limiter)));
                     return false;
-                case 'continue':
+                case PreAuthStatusEnum::CONTINUE:
                     continue 2;
-                default:
-                    Logger::warning("Unrecognized ratelimit allow() value '$result'");
             }
         }
 

diff --git a/src/Limiters/DeviceCookieLimiter.php b/src/Limiters/DeviceCookieLimiter.php
@@ -5,6 +5,7 @@
 namespace SimpleSAML\Module\ratelimit\Limiters;
 
 use SimpleSAML\{Configuration, Logger};
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use SimpleSAML\Utils\HTTP;
 
 use function sprintf;
@@ -31,18 +32,18 @@ public function getRateLimitKey(string $username, string $password): string
         return "device-" . $this->checkForDeviceCookie();
     }
 
-    public function allow(string $username, string $password): string
+    public function allow(string $username, string $password): PreAuthStatusEnum
     {
         if (!$this->hasDeviceCookieSet()) {
-            return UserPassBaseLimiter::PREAUTH_CONTINUE;
+            return PreAuthStatusEnum::CONTINUE;
         }
         $key = $this->getRateLimitKey($username, $password);
         /** @var array|null $ret */
         $ret = $this->getStore()->get('array', "ratelimit-$key");
         if ($ret === null) {
-            return UserPassBaseLimiter::PREAUTH_CONTINUE;
+            return PreAuthStatusEnum::CONTINUE;
         }
-        return $ret['user'] === $username ? UserPassBaseLimiter::PREAUTH_ALLOW : UserPassBaseLimiter::PREAUTH_CONTINUE;
+        return $ret['user'] === $username ? PreAuthStatusEnum::ALLOW : PreAuthStatusEnum::CONTINUE;
     }
 
     public function postFailure(string $username, string $password): int

diff --git a/src/Limiters/IpLimiter.php b/src/Limiters/IpLimiter.php
@@ -5,6 +5,7 @@
 namespace SimpleSAML\Module\ratelimit\Limiters;
 
 use SimpleSAML\{Configuration, Logger};
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use Symfony\Component\HttpFoundation\{IpUtils, Request};
 
 /**
@@ -32,10 +33,10 @@ public function __construct(Configuration $config)
         $this->clientIpAddress = $ip;
     }
 
-    public function allow(string $username, string $password): string
+    public function allow(string $username, string $password): PreAuthStatusEnum
     {
         if ($this->isIpWhiteListed($this->clientIpAddress)) {
-            return UserPassBaseLimiter::PREAUTH_CONTINUE;
+            return PreAuthStatusEnum::CONTINUE;
         }
         return parent::allow($username, $password);
     }

diff --git a/src/Limiters/UserPassBaseLimiter.php b/src/Limiters/UserPassBaseLimiter.php
@@ -6,6 +6,7 @@
 
 use SimpleSAML\Assert\Assert;
 use SimpleSAML\Configuration;
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use SimpleSAML\Store\{StoreFactory, StoreInterface};
 use SimpleSAML\Utils\Time;
 
@@ -14,15 +15,6 @@
 
 abstract class UserPassBaseLimiter implements UserPassLimiter
 {
-    /** @psalm-suppress MissingClassConstType */
-    protected const PREAUTH_ALLOW = 'allow';
-
-    /** @psalm-suppress MissingClassConstType */
-    protected const PREAUTH_BLOCK = 'block';
-
-    /** @psalm-suppress MissingClassConstType */
-    protected const PREAUTH_CONTINUE = 'continue';
-
     /**
      * @var int $limit The limit of attempts
      */
@@ -54,16 +46,16 @@ public function __construct(Configuration $config, string $defaultWindow = 'PT5M
      * Called prior to verifying the credentials to determine if the attempt is allowed.
      * @param string $username The username to check
      * @param string $password The password to check
-     * @return string allow|block|continue
+     * @return \SimpleSAML\Module\ratelimit\PreAuthStatusEnum
      */
-    public function allow(string $username, string $password): string
+    public function allow(string $username, string $password): PreAuthStatusEnum
     {
         $key = $this->getRateLimitKey($username, $password);
         $count = $this->getCurrentCount($key);
         if ($count >= $this->limit) {
-            return UserPassBaseLimiter::PREAUTH_BLOCK;
+            return PreAuthStatusEnum::BLOCK;
         }
-        return UserPassBaseLimiter::PREAUTH_CONTINUE;
+        return PreAuthStatusEnum::CONTINUE;
     }
 
     /**

diff --git a/src/Limiters/UserPassLimiter.php b/src/Limiters/UserPassLimiter.php
@@ -4,6 +4,8 @@
 
 namespace SimpleSAML\Module\ratelimit\Limiters;
 
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
+
 /**
  * Allow limiting of user password authentications
  * @package SimpleSAML\Module\ratelimit
@@ -14,9 +16,9 @@ interface UserPassLimiter
      * Called prior to verifying the credentials to determine if the attempt is allowed.
      * @param string $username The username to check
      * @param string $password The password to check
-     * @return string allow|block|continue
+     * @return \SimpleSAML\Module\ratelimit\PreAuthStatusEnum
      */
-    public function allow(string $username, string $password): string;
+    public function allow(string $username, string $password): PreAuthStatusEnum;
 
     /**
      * Called after a successful authentication

diff --git a/src/PreAuthStatusEnum.php b/src/PreAuthStatusEnum.php
@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SimpleSAML\Module\ratelimit;
+
+enum PreAuthStatusEnum
+{
+    case ALLOW;
+    case BLOCK;
+    case CONTINUE;
+}
diff --git a/tests/Utils/BaseLimitTest.php b/tests/Utils/BaseLimitTest.php
@@ -7,6 +7,7 @@
 use PHPUnit\Framework\TestCase;
 use SimpleSAML\Configuration;
 use SimpleSAML\Module\ratelimit\Limiters\UserPassBaseLimiter;
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use SimpleSAML\Store\StoreFactory;
 use SimpleSAML\TestUtils\InMemoryStore;
 
@@ -65,20 +66,20 @@ public function testAllowAndFailure(): void
         $password = 'Beer';
         for ($i = 1; $i <= 3; $i++) {
             // First 3 attempts should not be blocked
-            $this->assertEquals('continue', $limiter->allow($username, $password), "Attempt $i");
+            $this->assertEquals(PreAuthStatusEnum::CONTINUE, $limiter->allow($username, $password), "Attempt $i");
             $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('block', $limiter->allow($username, $password));
+        $this->assertEquals(PreAuthStatusEnum::BLOCK, $limiter->allow($username, $password));
 
         // Sleep until the next window, and counter should be reset
         usleep(4020000);
         $this->assertNull(
             $this->getStoreValueFor($limiter->getRateLimitKey($username, $password)),
             'Value not expected in store'
         );
-        $this->assertEquals('continue', $limiter->allow($username, $password));
+        $this->assertEquals(PreAuthStatusEnum::CONTINUE, $limiter->allow($username, $password));
     }
 
     /**

diff --git a/tests/src/Limiters/DeviceCookieLimiterTest.php b/tests/src/Limiters/DeviceCookieLimiterTest.php
@@ -9,6 +9,7 @@
 use PHPUnit\Framework\TestCase;
 use SimpleSAML\{Configuration, Utils};
 use SimpleSAML\Module\ratelimit\Limiters\DeviceCookieLimiter;
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use SimpleSAML\TestUtils\InMemoryStore;
 
 #[CoversClass(DeviceCookieLimiter::class)]
@@ -93,27 +94,27 @@ public function testDeviceCookieAllowsAuth(): void
         $limiter = $this->getLimiter([]);
 
         //expect: auth with out device cookie is continue
-        $this->assertEquals('continue', $limiter->allow('me', 'a'));
+        $this->assertEquals(PreAuthStatusEnum::CONTINUE, $limiter->allow('me', 'a'));
 
         //given: an existing cookie in the stroe
         $limiter->postSuccess('me', 'pass');
         $deviceCookie = $this->getDeviceCookieFromMock();
         $_COOKIE['deviceCookie'] = $deviceCookie;
 
         $this->assertEquals(
-            'allow',
+            PreAuthStatusEnum::ALLOW,
             $limiter->allow('me', 'a'),
             'User matches device cookie, so is allowed'
         );
 
         $this->assertEquals(
-            'continue',
+            PreAuthStatusEnum::CONTINUE,
             $limiter->allow('typoe', 'a'),
             'User does not match device cookie, so go to other rules'
         );
         $_COOKIE['deviceCookie'] = 'not-in-store';
         $this->assertEquals(
-            'continue',
+            PreAuthStatusEnum::CONTINUE,
             $limiter->allow('me', 'a'),
             'Device cookie not in store means go to other rules'
         );
@@ -145,7 +146,7 @@ public function testFailedAttemptsRemoveCookie(): void
         // Expect: failure for use increments count
         $this->assertEquals(1, $limiter->postFailure('u', 'p'));
         //sanity check that allow method still returns allow
-        $this->assertEquals('allow', $limiter->allow('u', 'p'));
+        $this->assertEquals(PreAuthStatusEnum::ALLOW, $limiter->allow('u', 'p'));
 
         $key = $limiter->getRateLimitKey('u', 'p');
         /** @var array{count: int, user: string}|null $result */
@@ -157,7 +158,7 @@ public function testFailedAttemptsRemoveCookie(): void
         $this->assertEquals(2, $limiter->postFailure('u', 'p'));
         $this->assertNull($store->get('array', 'ratelimit-' . $key));
         //sanity check that allow method not returns continue
-        $this->assertEquals('continue', $limiter->allow('u', 'p'));
+        $this->assertEquals(PreAuthStatusEnum::CONTINUE, $limiter->allow('u', 'p'));
     }
 
     private function getDeviceCookieFromMock(): ?string

diff --git a/tests/src/Limiters/ExceptionThrowingLimiter.php b/tests/src/Limiters/ExceptionThrowingLimiter.php
@@ -6,17 +6,18 @@
 
 use Exception;
 use SimpleSAML\Module\ratelimit\Limiters\UserPassLimiter;
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 
 class ExceptionThrowingLimiter implements UserPassLimiter
 {
     /**
      * Called prior to verifying the credentials to determine if the attempt is allowed.
      * @param string $username The username to check
      * @param string $password The password to check
-     * @return string allow|block|continue
+     * @return \SimpleSAML\Module\ratelimit\PreAuthStatusEnum
      * @throws \Exception always thrown
      */
-    public function allow(string $username, string $password): string
+    public function allow(string $username, string $password): PreAuthStatusEnum
     {
         throw new Exception('Boom!');
     }

diff --git a/tests/src/Limiters/IpLimiterTest.php b/tests/src/Limiters/IpLimiterTest.php
@@ -7,6 +7,7 @@
 use PHPUnit\Framework\Attributes\{CoversClass, DataProvider};
 use SimpleSAML\Configuration;
 use SimpleSAML\Module\ratelimit\Limiters\{IpLimiter, UserPassBaseLimiter};
+use SimpleSAML\Module\ratelimit\PreAuthStatusEnum;
 use SimpleSAML\Test\Module\ratelimit\Utils\BaseLimitTest;
 
 #[CoversClass(IpLimiter::class)]
@@ -41,7 +42,7 @@ public function testIpWhitelist(string $ip, bool $ignoreExpected): void
 
         $limiter = $this->getLimiter($config);
         $this->assertEquals($ignoreExpected ? 0 : 1, $limiter->postFailure('u', 'p'));
-        $this->assertEquals($ignoreExpected ? 'continue' : 'block', $limiter->allow('u', 'p'));
+        $this->assertEquals($ignoreExpected ? PreAuthStatusEnum::CONTINUE : PreAuthStatusEnum::BLOCK, $limiter->allow('u', 'p'));
     }
 
     public static function ipWhitelistProvider(): array