fix(ratelimit): Allow to bypass rate-limit from bruteforce allowlistbugfix/noid/allow-ratelimit-bypass

Signed-off-by: Joas Schilling <coding@schilljs.com>

3 files changed, 171 insertions, 213 deletions

diff --git a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php
index fddca471215..c42baadcb1c 100644
--- a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php
@@ -11,6 +11,7 @@ namespace Test\AppFramework\Middleware\Security;
 
 use OC\AppFramework\Middleware\Security\RateLimitingMiddleware;
 use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Ip\BruteforceAllowList;
 use OC\Security\RateLimiting\Exception\RateLimitExceededException;
 use OC\Security\RateLimiting\Limiter;
 use OCP\AppFramework\Controller;
@@ -18,6 +19,7 @@ use OCP\AppFramework\Http\Attribute\AnonRateLimit;
 use OCP\AppFramework\Http\Attribute\UserRateLimit;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\IAppConfig;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IUser;
@@ -61,6 +63,8 @@ class RateLimitingMiddlewareTest extends TestCase {
 	private ControllerMethodReflector $reflector;
 	private Limiter|MockObject $limiter;
 	private ISession|MockObject $session;
+	private IAppConfig|MockObject $appConfig;
+	private BruteforceAllowList|MockObject $bruteForceAllowList;
 	private RateLimitingMiddleware $rateLimitingMiddleware;
 
 	protected function setUp(): void {
@@ -71,13 +75,17 @@ class RateLimitingMiddlewareTest extends TestCase {
 		$this->reflector = new ControllerMethodReflector();
 		$this->limiter = $this->createMock(Limiter::class);
 		$this->session = $this->createMock(ISession::class);
+		$this->appConfig = $this->createMock(IAppConfig::class);
+		$this->bruteForceAllowList = $this->createMock(BruteforceAllowList::class);
 
 		$this->rateLimitingMiddleware = new RateLimitingMiddleware(
 			$this->request,
 			$this->userSession,
 			$this->reflector,
 			$this->limiter,
-			$this->session
+			$this->session,
+			$this->appConfig,
+			$this->bruteForceAllowList,
 		);
 	}
 
diff --git a/tests/lib/Security/Bruteforce/ThrottlerTest.php b/tests/lib/Security/Bruteforce/ThrottlerTest.php
deleted file mode 100644
index a94f1849c8a..00000000000
--- a/tests/lib/Security/Bruteforce/ThrottlerTest.php
+++ /dev/null
@@ -1,212 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-/**
- * SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors
- * SPDX-License-Identifier: AGPL-3.0-or-later
- */
-
-namespace Test\Security\Bruteforce;
-
-use OC\Security\Bruteforce\Backend\DatabaseBackend;
-use OC\Security\Bruteforce\Throttler;
-use OCP\AppFramework\Utility\ITimeFactory;
-use OCP\IConfig;
-use OCP\IDBConnection;
-use Psr\Log\LoggerInterface;
-use Test\TestCase;
-
-/**
- * Based on the unit tests from Paragonie's Airship CMS
- * Ref: https://github.com/paragonie/airship/blob/7e5bad7e3c0fbbf324c11f963fd1f80e59762606/test/unit/Engine/Security/AirBrakeTest.php
- *
- * @package Test\Security\Bruteforce
- */
-class ThrottlerTest extends TestCase {
-	/** @var Throttler */
-	private $throttler;
-	/** @var IDBConnection */
-	private $dbConnection;
-	/** @var ITimeFactory */
-	private $timeFactory;
-	/** @var LoggerInterface */
-	private $logger;
-	/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
-	private $config;
-
-	protected function setUp(): void {
-		$this->dbConnection = $this->createMock(IDBConnection::class);
-		$this->timeFactory = $this->createMock(ITimeFactory::class);
-		$this->logger = $this->createMock(LoggerInterface::class);
-		$this->config = $this->createMock(IConfig::class);
-
-		$this->throttler = new Throttler(
-			$this->timeFactory,
-			$this->logger,
-			$this->config,
-			new DatabaseBackend($this->dbConnection)
-		);
-		parent::setUp();
-	}
-
-	public function dataIsIPWhitelisted() {
-		return [
-			[
-				'10.10.10.10',
-				[
-					'whitelist_0' => '10.10.10.0/24',
-				],
-				true,
-			],
-			[
-				'10.10.10.10',
-				[
-					'whitelist_0' => '192.168.0.0/16',
-				],
-				false,
-			],
-			[
-				'10.10.10.10',
-				[
-					'whitelist_0' => '192.168.0.0/16',
-					'whitelist_1' => '10.10.10.0/24',
-				],
-				true,
-			],
-			[
-				'10.10.10.10',
-				[
-					'whitelist_0' => '10.10.10.11/31',
-				],
-				true,
-			],
-			[
-				'10.10.10.10',
-				[
-					'whitelist_0' => '10.10.10.9/31',
-				],
-				false,
-			],
-			[
-				'10.10.10.10',
-				[
-					'whitelist_0' => '10.10.10.15/29',
-				],
-				true,
-			],
-			[
-				'dead:beef:cafe::1',
-				[
-					'whitelist_0' => '192.168.0.0/16',
-					'whitelist_1' => '10.10.10.0/24',
-					'whitelist_2' => 'deaf:beef:cafe:1234::/64'
-				],
-				false,
-			],
-			[
-				'dead:beef:cafe::1',
-				[
-					'whitelist_0' => '192.168.0.0/16',
-					'whitelist_1' => '10.10.10.0/24',
-					'whitelist_2' => 'deaf:beef::/64'
-				],
-				false,
-			],
-			[
-				'dead:beef:cafe::1',
-				[
-					'whitelist_0' => '192.168.0.0/16',
-					'whitelist_1' => '10.10.10.0/24',
-					'whitelist_2' => 'deaf:cafe::/8'
-				],
-				true,
-			],
-			[
-				'dead:beef:cafe::1111',
-				[
-					'whitelist_0' => 'dead:beef:cafe::1100/123',
-
-				],
-				true,
-			],
-			[
-				'invalid',
-				[],
-				false,
-			],
-		];
-	}
-
-	/**
-	 * @param string $ip
-	 * @param string[] $whitelists
-	 * @param bool $isWhiteListed
-	 * @param bool $enabled
-	 */
-	private function isIpWhiteListedHelper($ip,
-		$whitelists,
-		$isWhiteListed,
-		$enabled) {
-		$this->config->method('getAppKeys')
-			->with($this->equalTo('bruteForce'))
-			->willReturn(array_keys($whitelists));
-		$this->config
-			->expects($this->once())
-			->method('getSystemValueBool')
-			->with('auth.bruteforce.protection.enabled', true)
-			->willReturn($enabled);
-
-		$this->config->method('getAppValue')
-			->willReturnCallback(function ($app, $key, $default) use ($whitelists) {
-				if ($app !== 'bruteForce') {
-					return $default;
-				}
-				if (isset($whitelists[$key])) {
-					return $whitelists[$key];
-				}
-				return $default;
-			});
-
-		$this->assertSame(
-			($enabled === false) ? true : $isWhiteListed,
-			self::invokePrivate($this->throttler, 'isBypassListed', [$ip])
-		);
-	}
-
-	/**
-	 * @dataProvider dataIsIPWhitelisted
-	 *
-	 * @param string $ip
-	 * @param string[] $whitelists
-	 * @param bool $isWhiteListed
-	 */
-	public function testIsIpWhiteListedWithEnabledProtection($ip,
-		$whitelists,
-		$isWhiteListed): void {
-		$this->isIpWhiteListedHelper(
-			$ip,
-			$whitelists,
-			$isWhiteListed,
-			true
-		);
-	}
-
-	/**
-	 * @dataProvider dataIsIPWhitelisted
-	 *
-	 * @param string $ip
-	 * @param string[] $whitelists
-	 * @param bool $isWhiteListed
-	 */
-	public function testIsIpWhiteListedWithDisabledProtection($ip,
-		$whitelists,
-		$isWhiteListed): void {
-		$this->isIpWhiteListedHelper(
-			$ip,
-			$whitelists,
-			$isWhiteListed,
-			false
-		);
-	}
-}
diff --git a/tests/lib/Security/Ip/BruteforceAllowListTest.php b/tests/lib/Security/Ip/BruteforceAllowListTest.php
new file mode 100644
index 00000000000..f7ef9e3df72
--- /dev/null
+++ b/tests/lib/Security/Ip/BruteforceAllowListTest.php
@@ -0,0 +1,162 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace Test\Security\Ip;
+
+use OC\Security\Ip\BruteforceAllowList;
+use OC\Security\Ip\Factory;
+use OCP\IAppConfig;
+use OCP\Security\Ip\IFactory;
+use PHPUnit\Framework\MockObject\MockObject;
+use Test\TestCase;
+
+/**
+ * Based on the unit tests from Paragonie's Airship CMS
+ * Ref: https://github.com/paragonie/airship/blob/7e5bad7e3c0fbbf324c11f963fd1f80e59762606/test/unit/Engine/Security/AirBrakeTest.php
+ *
+ * @package Test\Security\Bruteforce
+ */
+class BruteforceAllowListTest extends TestCase {
+	/** @var IAppConfig|MockObject */
+	private $appConfig;
+	/** @var IFactory|MockObject */
+	private $factory;
+	/** @var BruteforceAllowList */
+	private $allowList;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->appConfig = $this->createMock(IAppConfig::class);
+		$this->factory = new Factory();
+
+		$this->allowList = new BruteforceAllowList(
+			$this->appConfig,
+			$this->factory,
+		);
+	}
+
+	public function dataIsBypassListed(): array {
+		return [
+			[
+				'10.10.10.10',
+				[
+					'whitelist_0' => '10.10.10.0/24',
+				],
+				true,
+			],
+			[
+				'10.10.10.10',
+				[
+					'whitelist_0' => '192.168.0.0/16',
+				],
+				false,
+			],
+			[
+				'10.10.10.10',
+				[
+					'whitelist_0' => '192.168.0.0/16',
+					'whitelist_1' => '10.10.10.0/24',
+				],
+				true,
+			],
+			[
+				'10.10.10.10',
+				[
+					'whitelist_0' => '10.10.10.11/31',
+				],
+				true,
+			],
+			[
+				'10.10.10.10',
+				[
+					'whitelist_0' => '10.10.10.9/31',
+				],
+				false,
+			],
+			[
+				'10.10.10.10',
+				[
+					'whitelist_0' => '10.10.10.15/29',
+				],
+				true,
+			],
+			[
+				'dead:beef:cafe::1',
+				[
+					'whitelist_0' => '192.168.0.0/16',
+					'whitelist_1' => '10.10.10.0/24',
+					'whitelist_2' => 'deaf:beef:cafe:1234::/64'
+				],
+				false,
+			],
+			[
+				'dead:beef:cafe::1',
+				[
+					'whitelist_0' => '192.168.0.0/16',
+					'whitelist_1' => '10.10.10.0/24',
+					'whitelist_2' => 'deaf:beef::/64'
+				],
+				false,
+			],
+			[
+				'dead:beef:cafe::1',
+				[
+					'whitelist_0' => '192.168.0.0/16',
+					'whitelist_1' => '10.10.10.0/24',
+					'whitelist_2' => 'deaf:cafe::/8'
+				],
+				true,
+			],
+			[
+				'dead:beef:cafe::1111',
+				[
+					'whitelist_0' => 'dead:beef:cafe::1100/123',
+				],
+				true,
+			],
+			[
+				'invalid',
+				[],
+				false,
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider dataIsBypassListed
+	 *
+	 * @param string[] $allowList
+	 */
+	public function testIsBypassListed(
+		string $ip,
+		array $allowList,
+		bool $isAllowListed,
+	): void {
+		$this->appConfig->method('getKeys')
+			->with($this->equalTo('bruteForce'))
+			->willReturn(array_keys($allowList));
+
+		$this->appConfig->method('getValueString')
+			->willReturnCallback(function ($app, $key, $default) use ($allowList) {
+				if ($app !== 'bruteForce') {
+					return $default;
+				}
+				if (isset($allowList[$key])) {
+					return $allowList[$key];
+				}
+				return $default;
+			});
+
+		$this->assertSame(
+			$isAllowListed,
+			$this->allowList->isBypassListed($ip)
+		);
+	}
+}