diff --git a/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php b/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php index 9f848fbbb7..5cdba0cfff 100644 --- 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); diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 732a1d32ee..759d5ee416 100644 --- 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; } /** diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php index c9a1d5ecb2..62adca53f4 100644 --- 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); } diff --git a/apps/user_ldap/lib/Controller/ConfigAPIController.php b/apps/user_ldap/lib/Controller/ConfigAPIController.php index 7d51b0aafe..54800ef24e 100644 --- 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 ); diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index 4597124897..7a2590094b 100644 --- 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; } /** diff --git a/core/Controller/OCSController.php b/core/Controller/OCSController.php index 1deb5e958b..a709ab7b07 100644 --- 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); } diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php index 539fe016c8..ab3f022c97 100644 --- 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() { diff --git a/tests/Core/Controller/OCSControllerTest.php b/tests/Core/Controller/OCSControllerTest.php index 7241df9317..e6066a8014 100644 --- 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('', '')); }