Adjust existing bruteforce protection code

- Moves code to annotation - Adds the `throttle()` call on the responses on existing annotations Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
9 years ago · 727688ebd9
8 changed files with 43 additions and 72 deletions
--- a/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php
+++ b/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php
@ -148,10 +148,12 @@ class MountPublicLinkController extends Controller {
 		$authenticated = $this->session->get('public_link_authenticated') === $share->getId() ||
 			$this->shareManager->checkPassword($share, $password);
 		if (!empty($storedPassword) && !$authenticated ) {
-			return new JSONResponse(
+			$response = new JSONResponse(
 				['message' => 'No permission to access the share'],
 				Http::STATUS_BAD_REQUEST
 			);
+			$response->throttle();
+			return $response;
 		}

 		$share->setSharedWith($shareWith);
--- a/apps/files_sharing/lib/Controller/ShareController.php
+++ b/apps/files_sharing/lib/Controller/ShareController.php
@ -182,7 +182,9 @@ class ShareController extends Controller {
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.showShare', array('token' => $token)));
 		}

-		return new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest');
+		$response = new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest');
+		$response->throttle();
+		return $response;
 	}

 	/**
--- a/apps/files_sharing/tests/Controller/ShareControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php
@ -280,6 +280,7 @@ class ShareControllerTest extends \Test\TestCase {

 		$response = $this->shareController->authenticate('token', 'invalidpassword');
 		$expectedResponse =  new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest');
+		$expectedResponse->throttle();
 		$this->assertEquals($expectedResponse, $response);
 	}

--- a/apps/user_ldap/lib/Controller/ConfigAPIController.php
+++ b/apps/user_ldap/lib/Controller/ConfigAPIController.php
@ -25,7 +25,6 @@ namespace OCA\User_LDAP\Controller;

 use OC\CapabilitiesManager;
 use OC\Core\Controller\OCSController;
-use OC\Security\Bruteforce\Throttler;
 use OC\Security\IdentityProof\Manager;
 use OCA\User_LDAP\Configuration;
 use OCA\User_LDAP\Helper;
@ -52,7 +51,6 @@ class ConfigAPIController extends OCSController {
 		CapabilitiesManager $capabilitiesManager,
 		IUserSession $userSession,
 		IUserManager $userManager,
-		Throttler $throttler,
 		Manager $keyManager,
 		Helper $ldapHelper,
 		ILogger $logger
@ -63,7 +61,6 @@ class ConfigAPIController extends OCSController {
 			$capabilitiesManager,
 			$userSession,
 			$userManager,
-			$throttler,
 			$keyManager
 		);

--- a/core/Controller/LostController.php
+++ b/core/Controller/LostController.php
@ -32,6 +32,7 @@ namespace OC\Core\Controller;

 use OCA\Encryption\Exceptions\PrivateKeyMissingException;
 use \OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\JSONResponse;
 use \OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Defaults;
@ -207,17 +208,21 @@ class LostController extends Controller {
 	 * @BruteForceProtection(action=passwordResetEmail)
 	 *
 	 * @param string $user
-	 * @return array
+	 * @return JSONResponse
 	 */
 	public function email($user){
 		// FIXME: use HTTP error codes
 		try {
 			$this->sendEmail($user);
 		} catch (\Exception $e){
-			return $this->error($e->getMessage());
+			$response = new JSONResponse($this->error($e->getMessage()));
+			$response->throttle();
+			return $response;
 		}

-		return $this->success();
+		$response = new JSONResponse($this->success());
+		$response->throttle();
+		return $response;
 	}

 	/**
--- a/core/Controller/OCSController.php
+++ b/core/Controller/OCSController.php
@ -22,7 +22,6 @@
 namespace OC\Core\Controller;

 use OC\CapabilitiesManager;
-use OC\Security\Bruteforce\Throttler;
 use OC\Security\IdentityProof\Manager;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\IRequest;
@ -39,8 +38,6 @@ class OCSController extends \OCP\AppFramework\OCSController {
 	private $userManager;
 	/** @var Manager */
 	private $keyManager;
-	/** @var Throttler */
-	private $throttler;

 	/**
 	 * OCSController constructor.
@ -50,7 +47,6 @@ class OCSController extends \OCP\AppFramework\OCSController {
 	 * @param CapabilitiesManager $capabilitiesManager
 	 * @param IUserSession $userSession
 	 * @param IUserManager $userManager
-	 * @param Throttler $throttler
 	 * @param Manager $keyManager
 	 */
 	public function __construct($appName,
@ -58,13 +54,11 @@ class OCSController extends \OCP\AppFramework\OCSController {
 								CapabilitiesManager $capabilitiesManager,
 								IUserSession $userSession,
 								IUserManager $userManager,
-								Throttler $throttler,
 								Manager $keyManager) {
 		parent::__construct($appName, $request);
 		$this->capabilitiesManager = $capabilitiesManager;
 		$this->userSession = $userSession;
 		$this->userManager = $userManager;
-		$this->throttler = $throttler;
 		$this->keyManager = $keyManager;
 	}

@ -107,6 +101,7 @@ class OCSController extends \OCP\AppFramework\OCSController {

 	/**
 	 * @PublicPage
+	 * @BruteForceProtection(action=login)
 	 *
 	 * @param string $login
 	 * @param string $password
@ -114,7 +109,6 @@ class OCSController extends \OCP\AppFramework\OCSController {
 	 */
 	public function personCheck($login = '', $password = '') {
 		if ($login !== '' && $password !== '') {
-			$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
 			if ($this->userManager->checkPassword($login, $password)) {
 				return new DataResponse([
 					'person' => [
@ -122,8 +116,10 @@ class OCSController extends \OCP\AppFramework\OCSController {
 					]
 				]);
 			}
-			$this->throttler->registerAttempt('login', $this->request->getRemoteAddress());
-			return new DataResponse(null, 102);
+
+			$response = new DataResponse(null, 102);
+			$response->throttle();
+			return $response;
 		}
 		return new DataResponse(null, 101);
 	}
--- a/tests/Core/Controller/LostControllerTest.php
+++ b/tests/Core/Controller/LostControllerTest.php
@ -23,6 +23,7 @@ namespace Tests\Core\Controller;

 use OC\Core\Controller\LostController;
 use OC\Mail\Message;
+use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Defaults;
@ -245,7 +246,7 @@ class LostControllerTest extends \Test\TestCase {
 		$this->assertEquals($expectedResponse, $response);
 	}

-	public function testEmailUnsucessful() {
+	public function testEmailUnsuccessful() {
 		$existingUser = 'ExistingUser';
 		$nonExistingUser = 'NonExistingUser';
 		$this->userManager
@ -258,11 +259,12 @@ class LostControllerTest extends \Test\TestCase {

 		// With a non existing user
 		$response = $this->lostController->email($nonExistingUser);
-		$expectedResponse = [
+		$expectedResponse = new JSONResponse([
 			'status' => 'error',
 			'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
-		];
-		$this->assertSame($expectedResponse, $response);
+		]);
+		$expectedResponse->throttle();
+		$this->assertEquals($expectedResponse, $response);

 		// With no mail address
 		$this->config
@ -271,11 +273,12 @@ class LostControllerTest extends \Test\TestCase {
 			->with($existingUser, 'settings', 'email')
 			->will($this->returnValue(null));
 		$response = $this->lostController->email($existingUser);
-		$expectedResponse = [
+		$expectedResponse = new JSONResponse([
 			'status' => 'error',
 			'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
-		];
-		$this->assertSame($expectedResponse, $response);
+		]);
+		$expectedResponse->throttle();
+		$this->assertEquals($expectedResponse, $response);
 	}

 	public function testEmailSuccessful() {
@ -355,8 +358,9 @@ class LostControllerTest extends \Test\TestCase {
 			)->willReturn('encryptedToken');

 		$response = $this->lostController->email('ExistingUser');
-		$expectedResponse = array('status' => 'success');
-		$this->assertSame($expectedResponse, $response);
+		$expectedResponse = new JSONResponse(['status' => 'success']);
+		$expectedResponse->throttle();
+		$this->assertEquals($expectedResponse, $response);
 	}

 	public function testEmailWithMailSuccessful() {
@ -441,8 +445,9 @@ class LostControllerTest extends \Test\TestCase {
 			)->willReturn('encryptedToken');

 		$response = $this->lostController->email('test@example.com');
-		$expectedResponse = array('status' => 'success');
-		$this->assertSame($expectedResponse, $response);
+		$expectedResponse = new JSONResponse(['status' => 'success']);
+		$expectedResponse->throttle();
+		$this->assertEquals($expectedResponse, $response);
 	}

 	public function testEmailCantSendException() {
@ -522,8 +527,9 @@ class LostControllerTest extends \Test\TestCase {
 			)->willReturn('encryptedToken');

 		$response = $this->lostController->email('ExistingUser');
-		$expectedResponse = ['status' => 'error', 'msg' => 'Couldn\'t send reset email. Please contact your administrator.'];
-		$this->assertSame($expectedResponse, $response);
+		$expectedResponse = new JSONResponse(['status' => 'error', 'msg' => 'Couldn\'t send reset email. Please contact your administrator.']);
+		$expectedResponse->throttle();
+		$this->assertEquals($expectedResponse, $response);
 	}

 	public function testSetPasswordUnsuccessful() {
@ -692,8 +698,9 @@ class LostControllerTest extends \Test\TestCase {
 			->willReturn($user);

 		$response = $this->lostController->email('ExistingUser');
-		$expectedResponse = ['status' => 'error', 'msg' => 'Could not send reset email because there is no email address for this username. Please contact your administrator.'];
-		$this->assertSame($expectedResponse, $response);
+		$expectedResponse = new JSONResponse(['status' => 'error', 'msg' => 'Could not send reset email because there is no email address for this username. Please contact your administrator.']);
+		$expectedResponse->throttle();
+		$this->assertEquals($expectedResponse, $response);
 	}

 	public function testSetPasswordEncryptionDontProceed() {
--- a/tests/Core/Controller/OCSControllerTest.php
+++ b/tests/Core/Controller/OCSControllerTest.php
@ -42,8 +42,6 @@ class OCSControllerTest extends TestCase {
 	private $userSession;
 	/** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */
 	private $userManager;
-	/** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */
-	private $throttler;
 	/** @var Manager|\PHPUnit_Framework_MockObject_MockObject */
 	private $keyManager;
 	/** @var OCSController */
@ -56,7 +54,6 @@ class OCSControllerTest extends TestCase {
 		$this->capabilitiesManager = $this->createMock(CapabilitiesManager::class);
 		$this->userSession = $this->createMock(IUserSession::class);
 		$this->userManager = $this->createMock(IUserManager::class);
-		$this->throttler = $this->createMock(Throttler::class);
 		$this->keyManager = $this->createMock(Manager::class);

 		$this->controller = new OCSController(
@ -65,7 +62,6 @@ class OCSControllerTest extends TestCase {
 			$this->capabilitiesManager,
 			$this->userSession,
 			$this->userManager,
-			$this->throttler,
 			$this->keyManager
 		);
 	}
@ -117,16 +113,6 @@ class OCSControllerTest extends TestCase {
 	}

 	public function testPersonCheckValid() {
-		$this->request->method('getRemoteAddress')
-			->willReturn('1.2.3.4');
-
-		$this->throttler->expects($this->once())
-			->method('sleepDelay')
-			->with('1.2.3.4');
-
-		$this->throttler->expects($this->never())
-			->method('registerAttempt');
-
 		$this->userManager->method('checkPassword')
 			->with(
 				$this->equalTo('user'),
@ -138,25 +124,10 @@ class OCSControllerTest extends TestCase {
 				'personid' => 'user'
 			]
 		]);
-
 		$this->assertEquals($expected, $this->controller->personCheck('user', 'pass'));
 	}

 	public function testPersonInvalid() {
-		$this->request->method('getRemoteAddress')
-			->willReturn('1.2.3.4');
-
-		$this->throttler->expects($this->once())
-			->method('sleepDelay')
-			->with('1.2.3.4');
-
-		$this->throttler->expects($this->once())
-			->method('registerAttempt')
-			->with(
-				$this->equalTo('login'),
-				$this->equalTo('1.2.3.4')
-			);
-
 		$this->userManager->method('checkPassword')
 			->with(
 				$this->equalTo('user'),
@ -164,20 +135,11 @@ class OCSControllerTest extends TestCase {
 			)->willReturn(false);

 		$expected = new DataResponse(null, 102);
-
+		$expected->throttle();
 		$this->assertEquals($expected, $this->controller->personCheck('user', 'wrongpass'));
 	}

 	public function testPersonNoLogin() {
-		$this->request->method('getRemoteAddress')
-			->willReturn('1.2.3.4');
-
-		$this->throttler->expects($this->never())
-			->method('sleepDelay');
-
-		$this->throttler->expects($this->never())
-			->method('registerAttempt');
-
 		$this->userManager->method('checkPassword')
 			->with(
 				$this->equalTo('user'),
@ -185,7 +147,6 @@ class OCSControllerTest extends TestCase {
 			)->willReturn(false);

 		$expected = new DataResponse(null, 101);
-
 		$this->assertEquals($expected, $this->controller->personCheck('', ''));
 	}