From 8149945a916447b4e7dae8182dbf0c354e7d19e8 Mon Sep 17 00:00:00 2001
From: Lukas Reschke <lukas@statuscode.ch>
Date: Thu, 13 Apr 2017 22:50:44 +0200
Subject: [PATCH] Make BruteForceProtection annotation more clever
This makes the new `@BruteForceProtection` annotation more clever and moves the relevant code into it's own middleware.
Basically you can now set `@BruteForceProtection(action=$key)` as annotation and that will make the controller bruteforce protected. However, the difference to before is that you need to call `$responmse->throttle()` to increase the counter. Before the counter was increased every time which leads to all kind of unexpected problems.
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
---
core/Controller/LoginController.php | 36 +---
lib/composer/composer/autoload_classmap.php | 1 +
lib/composer/composer/autoload_static.php | 1 +
.../DependencyInjection/DIContainer.php | 17 +-
.../Security/BruteForceMiddleware.php | 83 ++++++++
.../Security/SecurityMiddleware.php | 15 +-
lib/public/AppFramework/Http/Response.php | 19 ++
tests/Core/Controller/LoginControllerTest.php | 133 +-----------
tests/lib/AppFramework/Http/ResponseTest.php | 5 +
.../Security/BruteForceMiddlewareTest.php | 190 ++++++++++++++++++
.../Security/SecurityMiddlewareTest.php | 76 +------
11 files changed, 329 insertions(+), 247 deletions(-)
create mode 100644 lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
create mode 100644 tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index 59abf47dc80..9f8b2b75fd0 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -29,7 +29,6 @@
namespace OC\Core\Controller;
use OC\Authentication\TwoFactorAuth\Manager;
-use OC\Security\Bruteforce\Throttler;
use OC\User\Session;
use OC_App;
use OC_Util;
@@ -64,8 +63,6 @@ class LoginController extends Controller {
private $logger;
/** @var Manager */
private $twoFactorManager;
- /** @var Throttler */
- private $throttler;
/**
* @param string $appName
@@ -77,7 +74,6 @@ class LoginController extends Controller {
* @param IURLGenerator $urlGenerator
* @param ILogger $logger
* @param Manager $twoFactorManager
- * @param Throttler $throttler
*/
public function __construct($appName,
IRequest $request,
@@ -87,8 +83,7 @@ class LoginController extends Controller {
IUserSession $userSession,
IURLGenerator $urlGenerator,
ILogger $logger,
- Manager $twoFactorManager,
- Throttler $throttler) {
+ Manager $twoFactorManager) {
parent::__construct($appName, $request);
$this->userManager = $userManager;
$this->config = $config;
@@ -97,7 +92,6 @@ class LoginController extends Controller {
$this->urlGenerator = $urlGenerator;
$this->logger = $logger;
$this->twoFactorManager = $twoFactorManager;
- $this->throttler = $throttler;
}
/**
@@ -203,6 +197,7 @@ class LoginController extends Controller {
* @PublicPage
* @UseSession
* @NoCSRFRequired
+ * @BruteForceProtection(action=login)
*
* @param string $user
* @param string $password
@@ -216,8 +211,6 @@ class LoginController extends Controller {
if(!is_string($user)) {
throw new \InvalidArgumentException('Username must be string');
}
- $currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'login');
- $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
// If the user is already logged in and the CSRF check does not pass then
// simply redirect the user to the correct page as required. This is the
@@ -245,16 +238,14 @@ class LoginController extends Controller {
}
}
if ($loginResult === false) {
- $this->throttler->registerAttempt('login', $this->request->getRemoteAddress(), ['user' => $originalUser]);
- if($currentDelay === 0) {
- $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
- }
+ // Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name
+ $args = !is_null($user) ? ['user' => $originalUser] : [];
+ $response = new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args));
+ $response->throttle();
$this->session->set('loginMessages', [
['invalidpassword'], []
]);
- // Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name
- $args = !is_null($user) ? ['user' => $originalUser] : [];
- return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args));
+ return $response;
}
// TODO: remove password checks from above and let the user session handle failures
// requires https://github.com/owncloud/core/pull/24616
@@ -305,6 +296,7 @@ class LoginController extends Controller {
/**
* @NoAdminRequired
* @UseSession
+ * @BruteForceProtection(action=sudo)
*
* @license GNU AGPL version 3 or any later version
*
@@ -312,18 +304,12 @@ class LoginController extends Controller {
* @return DataResponse
*/
public function confirmPassword($password) {
- $currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'sudo');
- $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo');
-
$loginName = $this->userSession->getLoginName();
$loginResult = $this->userManager->checkPassword($loginName, $password);
if ($loginResult === false) {
- $this->throttler->registerAttempt('sudo', $this->request->getRemoteAddress(), ['user' => $loginName]);
- if ($currentDelay === 0) {
- $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo');
- }
-
- return new DataResponse([], Http::STATUS_FORBIDDEN);
+ $response = new DataResponse([], Http::STATUS_FORBIDDEN);
+ $response->throttle();
+ return $response;
}
$confirmTimestamp = time();
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
index 51f822a0e71..29eab9c124b 100644
--- a/lib/composer/composer/autoload_classmap.php
+++ b/lib/composer/composer/autoload_classmap.php
@@ -290,6 +290,7 @@ return array(
'OC\\AppFramework\\Http\\Request' => $baseDir . '/lib/private/AppFramework/Http/Request.php',
'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php',
'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php',
+ 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',
diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php
index 29c90798485..61741bf3252 100644
--- a/lib/composer/composer/autoload_static.php
+++ b/lib/composer/composer/autoload_static.php
@@ -320,6 +320,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
'OC\\AppFramework\\Http\\Request' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Request.php',
'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php',
'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php',
+ 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',
diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php
index d279140afbb..04747485c13 100644
--- a/lib/private/AppFramework/DependencyInjection/DIContainer.php
+++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php
@@ -224,12 +224,22 @@ class DIContainer extends SimpleContainer implements IAppContainer {
$app->isAdminUser(),
$server->getContentSecurityPolicyManager(),
$server->getCsrfTokenManager(),
- $server->getContentSecurityPolicyNonceManager(),
- $server->getBruteForceThrottler()
+ $server->getContentSecurityPolicyNonceManager()
);
});
+ $this->registerService('BruteForceMiddleware', function($c) use ($app) {
+ /** @var \OC\Server $server */
+ $server = $app->getServer();
+
+ return new OC\AppFramework\Middleware\Security\BruteForceMiddleware(
+ $c['ControllerMethodReflector'],
+ $server->getBruteForceThrottler(),
+ $server->getRequest()
+ );
+ });
+
$this->registerService('RateLimitingMiddleware', function($c) use ($app) {
/** @var \OC\Server $server */
$server = $app->getServer();
@@ -281,7 +291,8 @@ class DIContainer extends SimpleContainer implements IAppContainer {
$dispatcher->registerMiddleware($c['CORSMiddleware']);
$dispatcher->registerMiddleware($c['OCSMiddleware']);
$dispatcher->registerMiddleware($c['SecurityMiddleware']);
- $dispatcher->registerMiddleWare($c['TwoFactorMiddleware']);
+ $dispatcher->registerMiddleware($c['TwoFactorMiddleware']);
+ $dispatcher->registerMiddleware($c['BruteForceMiddleware']);
$dispatcher->registerMiddleware($c['RateLimitingMiddleware']);
foreach($middleWares as $middleWare) {
diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
new file mode 100644
index 00000000000..b361f453bdb
--- /dev/null
+++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
@@ -0,0 +1,83 @@
+<?php
+/**
+ * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OC\AppFramework\Middleware\Security;
+
+use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Bruteforce\Throttler;
+use OCP\AppFramework\Http\Response;
+use OCP\AppFramework\Middleware;
+use OCP\IRequest;
+
+/**
+ * Class BruteForceMiddleware performs the bruteforce protection for controllers
+ * that are annotated with @BruteForceProtection(action=$action) whereas $action
+ * is the action that should be logged within the database.
+ *
+ * @package OC\AppFramework\Middleware\Security
+ */
+class BruteForceMiddleware extends Middleware {
+ /** @var ControllerMethodReflector */
+ private $reflector;
+ /** @var Throttler */
+ private $throttler;
+ /** @var IRequest */
+ private $request;
+
+ /**
+ * @param ControllerMethodReflector $controllerMethodReflector
+ * @param Throttler $throttler
+ * @param IRequest $request
+ */
+ public function __construct(ControllerMethodReflector $controllerMethodReflector,
+ Throttler $throttler,
+ IRequest $request) {
+ $this->reflector = $controllerMethodReflector;
+ $this->throttler = $throttler;
+ $this->request = $request;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function beforeController($controller, $methodName) {
+ parent::beforeController($controller, $methodName);
+
+ if($this->reflector->hasAnnotation('BruteForceProtection')) {
+ $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
+ $this->throttler->sleepDelay($this->request->getRemoteAddress(), $action);
+ }
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function afterController($controller, $methodName, Response $response) {
+ if($this->reflector->hasAnnotation('BruteForceProtection') && $response->isThrottled()) {
+ $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
+ $ip = $this->request->getRemoteAddress();
+ $this->throttler->sleepDelay($ip, $action);
+ $this->throttler->registerAttempt($action, $ip);
+ }
+
+ return parent::afterController($controller, $methodName, $response);
+ }
+}
diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
index d4d0598c94f..e420a9dacc0 100644
--- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
+++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
@@ -36,7 +36,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException;
use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException;
use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException;
use OC\AppFramework\Utility\ControllerMethodReflector;
-use OC\Security\Bruteforce\Throttler;
use OC\Security\CSP\ContentSecurityPolicyManager;
use OC\Security\CSP\ContentSecurityPolicyNonceManager;
use OC\Security\CSRF\CsrfTokenManager;
@@ -88,8 +87,6 @@ class SecurityMiddleware extends Middleware {
private $csrfTokenManager;
/** @var ContentSecurityPolicyNonceManager */
private $cspNonceManager;
- /** @var Throttler */
- private $throttler;
/**
* @param IRequest $request
@@ -104,7 +101,6 @@ class SecurityMiddleware extends Middleware {
* @param ContentSecurityPolicyManager $contentSecurityPolicyManager
* @param CSRFTokenManager $csrfTokenManager
* @param ContentSecurityPolicyNonceManager $cspNonceManager
- * @param Throttler $throttler
*/
public function __construct(IRequest $request,
ControllerMethodReflector $reflector,
@@ -117,8 +113,7 @@ class SecurityMiddleware extends Middleware {
$isAdminUser,
ContentSecurityPolicyManager $contentSecurityPolicyManager,
CsrfTokenManager $csrfTokenManager,
- ContentSecurityPolicyNonceManager $cspNonceManager,
- Throttler $throttler) {
+ ContentSecurityPolicyNonceManager $cspNonceManager) {
$this->navigationManager = $navigationManager;
$this->request = $request;
$this->reflector = $reflector;
@@ -131,10 +126,8 @@ class SecurityMiddleware extends Middleware {
$this->contentSecurityPolicyManager = $contentSecurityPolicyManager;
$this->csrfTokenManager = $csrfTokenManager;
$this->cspNonceManager = $cspNonceManager;
- $this->throttler = $throttler;
}
-
/**
* This runs all the security checks before a method call. The
* security checks are determined by inspecting the controller method
@@ -191,12 +184,6 @@ class SecurityMiddleware extends Middleware {
}
}
- if($this->reflector->hasAnnotation('BruteForceProtection')) {
- $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
- $this->throttler->sleepDelay($this->request->getRemoteAddress(), $action);
- $this->throttler->registerAttempt($action, $this->request->getRemoteAddress());
- }
-
/**
* FIXME: Use DI once available
* Checks if app is enabled (also includes a check whether user is allowed to access the resource)
diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php
index 051e68f3144..087522386be 100644
--- a/lib/public/AppFramework/Http/Response.php
+++ b/lib/public/AppFramework/Http/Response.php
@@ -81,6 +81,8 @@ class Response {
/** @var ContentSecurityPolicy|null Used Content-Security-Policy */
private $contentSecurityPolicy = null;
+ /** @var bool */
+ private $throttled = false;
/**
* Caches the response
@@ -322,5 +324,22 @@ class Response {
return $this;
}
+ /**
+ * Marks the response as to throttle. Will be throttled when the
+ * @BruteForceProtection annotation is added.
+ *
+ * @since 12.0.0
+ */
+ public function throttle() {
+ $this->throttled = true;
+ }
+ /**
+ * Whether the current response is throttled.
+ *
+ * @since 12.0.0
+ */
+ public function isThrottled() {
+ return $this->throttled;
+ }
}
diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php
index 9387e69c405..c9ab8e7476d 100644
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -55,8 +55,6 @@ class LoginControllerTest extends TestCase {
private $logger;
/** @var Manager|\PHPUnit_Framework_MockObject_MockObject */
private $twoFactorManager;
- /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */
- private $throttler;
public function setUp() {
parent::setUp();
@@ -68,7 +66,6 @@ class LoginControllerTest extends TestCase {
$this->urlGenerator = $this->createMock(IURLGenerator::class);
$this->logger = $this->createMock(ILogger::class);
$this->twoFactorManager = $this->createMock(Manager::class);
- $this->throttler = $this->createMock(Throttler::class);
$this->loginController = new LoginController(
'core',
@@ -79,8 +76,7 @@ class LoginControllerTest extends TestCase {
$this->userSession,
$this->urlGenerator,
$this->logger,
- $this->twoFactorManager,
- $this->throttler
+ $this->twoFactorManager
);
}
@@ -287,27 +283,10 @@ class LoginControllerTest extends TestCase {
$password = 'secret';
$loginPageUrl = 'some url';
- $this->request
- ->expects($this->exactly(5))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(true);
- $this->throttler
- ->expects($this->exactly(2))
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(0);
- $this->throttler
- ->expects($this->once())
- ->method('registerAttempt')
- ->with('login', '192.168.0.1', ['user' => 'MyUserName']);
$this->userManager->expects($this->once())
->method('checkPasswordNoLogging')
->will($this->returnValue(false));
@@ -324,6 +303,7 @@ class LoginControllerTest extends TestCase {
->method('deleteUserValue');
$expected = new \OCP\AppFramework\Http\RedirectResponse($loginPageUrl);
+ $expected->throttle();
$this->assertEquals($expected, $this->loginController->tryLogin($user, $password, ''));
}
@@ -340,23 +320,10 @@ class LoginControllerTest extends TestCase {
$password = 'secret';
$indexPageUrl = \OC_Util::getDefaultPageUrl();
- $this->request
- ->expects($this->exactly(2))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(true);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
$this->userManager->expects($this->once())
->method('checkPasswordNoLogging')
->will($this->returnValue($user));
@@ -400,23 +367,10 @@ class LoginControllerTest extends TestCase {
$password = 'secret';
$indexPageUrl = \OC_Util::getDefaultPageUrl();
- $this->request
- ->expects($this->exactly(2))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(true);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
$this->userManager->expects($this->once())
->method('checkPasswordNoLogging')
->will($this->returnValue($user));
@@ -450,23 +404,10 @@ class LoginControllerTest extends TestCase {
$password = 'secret';
$originalUrl = 'another%20url';
- $this->request
- ->expects($this->exactly(2))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(false);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
$this->userSession->expects($this->once())
->method('isLoggedIn')
->with()
@@ -490,23 +431,10 @@ class LoginControllerTest extends TestCase {
$originalUrl = 'another%20url';
$redirectUrl = 'http://localhost/another url';
- $this->request
- ->expects($this->exactly(2))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(false);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
$this->userSession->expects($this->once())
->method('isLoggedIn')
->with()
@@ -534,23 +462,10 @@ class LoginControllerTest extends TestCase {
$originalUrl = 'another%20url';
$redirectUrl = 'http://localhost/another url';
- $this->request
- ->expects($this->exactly(2))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(true);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
$this->userManager->expects($this->once())
->method('checkPasswordNoLogging')
->with('Jane', $password)
@@ -584,23 +499,10 @@ class LoginControllerTest extends TestCase {
$challengeUrl = 'challenge/url';
$provider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
- $this->request
- ->expects($this->exactly(2))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(true);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
$this->userManager->expects($this->once())
->method('checkPasswordNoLogging')
->will($this->returnValue($user));
@@ -651,23 +553,10 @@ class LoginControllerTest extends TestCase {
$provider1 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
$provider2 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
- $this->request
- ->expects($this->exactly(2))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(true);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
$this->userManager->expects($this->once())
->method('checkPasswordNoLogging')
->will($this->returnValue($user));
@@ -731,33 +620,17 @@ class LoginControllerTest extends TestCase {
->method('linkToRoute')
->with('core.login.showLoginForm', ['user' => 'john@doe.com'])
->will($this->returnValue(''));
- $this->request
- ->expects($this->exactly(3))
- ->method('getRemoteAddress')
- ->willReturn('192.168.0.1');
$this->request
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(true);
- $this->throttler
- ->expects($this->once())
- ->method('getDelay')
- ->with('192.168.0.1')
- ->willReturn(200);
- $this->throttler
- ->expects($this->once())
- ->method('sleepDelay')
- ->with('192.168.0.1');
- $this->throttler
- ->expects($this->once())
- ->method('registerAttempt')
- ->with('login', '192.168.0.1', ['user' => 'john@doe.com']);
$this->config->expects($this->never())
->method('deleteUserValue');
$this->userSession->expects($this->never())
->method('createRememberMeToken');
$expected = new RedirectResponse('');
+ $expected->throttle();
$this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', 'just wrong', null));
}
}
diff --git a/tests/lib/AppFramework/Http/ResponseTest.php b/tests/lib/AppFramework/Http/ResponseTest.php
index 4840a5f94c3..d8959face89 100644
--- a/tests/lib/AppFramework/Http/ResponseTest.php
+++ b/tests/lib/AppFramework/Http/ResponseTest.php
@@ -264,4 +264,9 @@ class ResponseTest extends \Test\TestCase {
}
+ public function testThrottle() {
+ $this->assertFalse($this->childResponse->isThrottled());
+ $this->childResponse->throttle();
+ $this->assertTrue($this->childResponse->isThrottled());
+ }
}
diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
new file mode 100644
index 00000000000..14d3b796846
--- /dev/null
+++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
@@ -0,0 +1,190 @@
+<?php
+/**
+ * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Test\AppFramework\Middleware\Security;
+
+use OC\AppFramework\Middleware\Security\BruteForceMiddleware;
+use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Bruteforce\Throttler;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\Response;
+use OCP\Http\Client\IResponse;
+use OCP\IRequest;
+use Test\TestCase;
+
+class BruteForceMiddlewareTest extends TestCase {
+ /** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject */
+ private $reflector;
+ /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */
+ private $throttler;
+ /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
+ private $request;
+ /** @var BruteForceMiddleware */
+ private $bruteForceMiddleware;
+
+ public function setUp() {
+ parent::setUp();
+
+ $this->reflector = $this->createMock(ControllerMethodReflector::class);
+ $this->throttler = $this->createMock(Throttler::class);
+ $this->request = $this->createMock(IRequest::class);
+
+ $this->bruteForceMiddleware = new BruteForceMiddleware(
+ $this->reflector,
+ $this->throttler,
+ $this->request
+ );
+ }
+
+ public function testBeforeControllerWithAnnotation() {
+ $this->reflector
+ ->expects($this->once())
+ ->method('hasAnnotation')
+ ->with('BruteForceProtection')
+ ->willReturn(true);
+ $this->reflector
+ ->expects($this->once())
+ ->method('getAnnotationParameter')
+ ->with('BruteForceProtection', 'action')
+ ->willReturn('login');
+ $this->request
+ ->expects($this->once())
+ ->method('getRemoteAddress')
+ ->willReturn('127.0.0.1');
+ $this->throttler
+ ->expects($this->once())
+ ->method('sleepDelay')
+ ->with('127.0.0.1', 'login');
+
+ /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+ $controller = $this->createMock(Controller::class);
+ $this->bruteForceMiddleware->beforeController($controller, 'testMethod');
+ }
+
+ public function testBeforeControllerWithoutAnnotation() {
+ $this->reflector
+ ->expects($this->once())
+ ->method('hasAnnotation')
+ ->with('BruteForceProtection')
+ ->willReturn(false);
+ $this->reflector
+ ->expects($this->never())
+ ->method('getAnnotationParameter');
+ $this->request
+ ->expects($this->never())
+ ->method('getRemoteAddress');
+ $this->throttler
+ ->expects($this->never())
+ ->method('sleepDelay');
+
+ /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+ $controller = $this->createMock(Controller::class);
+ $this->bruteForceMiddleware->beforeController($controller, 'testMethod');
+ }
+
+ public function testAfterControllerWithAnnotationAndThrottledRequest() {
+ /** @var Response|\PHPUnit_Framework_MockObject_MockObject $response */
+ $response = $this->createMock(Response::class);
+ $this->reflector
+ ->expects($this->once())
+ ->method('hasAnnotation')
+ ->with('BruteForceProtection')
+ ->willReturn(true);
+ $response
+ ->expects($this->once())
+ ->method('isThrottled')
+ ->willReturn(true);
+ $this->reflector
+ ->expects($this->once())
+ ->method('getAnnotationParameter')
+ ->with('BruteForceProtection', 'action')
+ ->willReturn('login');
+ $this->request
+ ->expects($this->once())
+ ->method('getRemoteAddress')
+ ->willReturn('127.0.0.1');
+ $this->throttler
+ ->expects($this->once())
+ ->method('sleepDelay')
+ ->with('127.0.0.1', 'login');
+ $this->throttler
+ ->expects($this->once())
+ ->method('registerAttempt')
+ ->with('login', '127.0.0.1');
+
+ /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+ $controller = $this->createMock(Controller::class);
+ $this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response);
+ }
+
+ public function testAfterControllerWithAnnotationAndNotThrottledRequest() {
+ /** @var Response|\PHPUnit_Framework_MockObject_MockObject $response */
+ $response = $this->createMock(Response::class);
+ $this->reflector
+ ->expects($this->once())
+ ->method('hasAnnotation')
+ ->with('BruteForceProtection')
+ ->willReturn(true);
+ $response
+ ->expects($this->once())
+ ->method('isThrottled')
+ ->willReturn(false);
+ $this->reflector
+ ->expects($this->never())
+ ->method('getAnnotationParameter');
+ $this->request
+ ->expects($this->never())
+ ->method('getRemoteAddress');
+ $this->throttler
+ ->expects($this->never())
+ ->method('sleepDelay');
+ $this->throttler
+ ->expects($this->never())
+ ->method('registerAttempt');
+
+ /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+ $controller = $this->createMock(Controller::class);
+ $this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response);
+ }
+
+ public function testAfterControllerWithoutAnnotation() {
+ $this->reflector
+ ->expects($this->once())
+ ->method('hasAnnotation')
+ ->with('BruteForceProtection')
+ ->willReturn(false);
+ $this->reflector
+ ->expects($this->never())
+ ->method('getAnnotationParameter');
+ $this->request
+ ->expects($this->never())
+ ->method('getRemoteAddress');
+ $this->throttler
+ ->expects($this->never())
+ ->method('sleepDelay');
+
+ /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */
+ $controller = $this->createMock(Controller::class);
+ /** @var Response|\PHPUnit_Framework_MockObject_MockObject $response */
+ $response = $this->createMock(Response::class);
+ $this->bruteForceMiddleware->afterController($controller, 'testMethod' ,$response);
+ }
+}
diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
index 164ea48de70..17ac30b8fe4 100644
--- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
@@ -20,8 +20,6 @@
*
*/
-
-
namespace Test\AppFramework\Middleware\Security;
use OC\AppFramework\Http;
@@ -34,7 +32,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException;
use OC\AppFramework\Middleware\Security\SecurityMiddleware;
use OC\AppFramework\Utility\ControllerMethodReflector;
-use OC\Security\Bruteforce\Throttler;
use OC\Security\CSP\ContentSecurityPolicy;
use OC\Security\CSP\ContentSecurityPolicyManager;
use OC\Security\CSP\ContentSecurityPolicyNonceManager;
@@ -54,7 +51,6 @@ use OCP\ISession;
use OCP\IURLGenerator;
use OCP\Security\ISecureRandom;
-
class SecurityMiddlewareTest extends \Test\TestCase {
/** @var SecurityMiddleware|\PHPUnit_Framework_MockObject_MockObject */
@@ -83,8 +79,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
private $csrfTokenManager;
/** @var ContentSecurityPolicyNonceManager|\PHPUnit_Framework_MockObject_MockObject */
private $cspNonceManager;
- /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */
- private $bruteForceThrottler;
protected function setUp() {
parent::setUp();
@@ -99,7 +93,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class);
$this->csrfTokenManager = $this->createMock(CsrfTokenManager::class);
$this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class);
- $this->bruteForceThrottler = $this->getMockBuilder(Throttler::class)->disableOriginalConstructor()->getMock();
$this->middleware = $this->getMiddleware(true, true);
$this->secException = new SecurityException('hey', false);
$this->secAjaxException = new SecurityException('hey', true);
@@ -123,8 +116,7 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$isAdminUser,
$this->contentSecurityPolicyManager,
$this->csrfTokenManager,
- $this->cspNonceManager,
- $this->bruteForceThrottler
+ $this->cspNonceManager
);
}
@@ -657,70 +649,4 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response));
}
-
- /**
- * @dataProvider dataTestBeforeControllerBruteForce
- */
- public function testBeforeControllerBruteForce($bruteForceProtectionEnabled) {
- /** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject $reader */
- $reader = $this->getMockBuilder(ControllerMethodReflector::class)->disableOriginalConstructor()->getMock();
-
- $middleware = new SecurityMiddleware(
- $this->request,
- $reader,
- $this->navigationManager,
- $this->urlGenerator,
- $this->logger,
- $this->session,
- 'files',
- false,
- false,
- $this->contentSecurityPolicyManager,
- $this->csrfTokenManager,
- $this->cspNonceManager,
- $this->bruteForceThrottler
- );
-
- $reader->expects($this->any())->method('hasAnnotation')
- ->willReturnCallback(
- function($annotation) use ($bruteForceProtectionEnabled) {
-
- switch ($annotation) {
- case 'BruteForceProtection':
- return $bruteForceProtectionEnabled;
- case 'PasswordConfirmationRequired':
- case 'StrictCookieRequired':
- return false;
- case 'PublicPage':
- case 'NoCSRFRequired':
- return true;
- }
-
- return true;
- }
- );
-
- $reader->expects($this->any())->method('getAnnotationParameter')->willReturn('action');
- $this->request->expects($this->any())->method('getRemoteAddress')->willReturn('remoteAddress');
-
- if ($bruteForceProtectionEnabled) {
- $this->bruteForceThrottler->expects($this->once())->method('sleepDelay')
- ->with('remoteAddress', 'action');
- $this->bruteForceThrottler->expects($this->once())->method('registerAttempt')
- ->with('action', 'remoteAddress');
- } else {
- $this->bruteForceThrottler->expects($this->never())->method('sleepDelay');
- $this->bruteForceThrottler->expects($this->never())->method('registerAttempt');
- }
-
- $middleware->beforeController($this->controller, 'test');
-
- }
-
- public function dataTestBeforeControllerBruteForce() {
- return [
- [true],
- [false]
- ];
- }
}
--
GitLab