From bf364e2e63d08864493219d1ed17caf08043624d Mon Sep 17 00:00:00 2001 From: Semih Serhat Karakaya Date: Sun, 10 Nov 2019 20:36:56 +0300 Subject: [PATCH 1/7] use EventDispatcher events instead of legacy OC\User hooks --- lib/Hooks.php | 55 ++++++++++++++++++------------------- tests/unit/HooksTest.php | 58 ++++++++++++++++------------------------ 2 files changed, 49 insertions(+), 64 deletions(-) diff --git a/lib/Hooks.php b/lib/Hooks.php index 3027645..1a26ef1 100644 --- a/lib/Hooks.php +++ b/lib/Hooks.php @@ -25,8 +25,9 @@ namespace OCA\BruteForceProtection; use OC\User\LoginException; -use OCP\IUserManager; use OCP\IRequest; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use Symfony\Component\EventDispatcher\GenericEvent; /** * Class Hooks @@ -34,64 +35,60 @@ */ class Hooks { - /** @var \OC\User\Manager */ - private $userManager; - - /** @var Throttle*/ + /** @var Throttle */ private $throttle; - /** @var IRequest*/ + /** @var IRequest */ private $request; + /** @var EventDispatcherInterface */ + private $eventDispatcher; + /** - * @param \OC\User\Manager $userManager * @param Throttle $throttle * @param IRequest $request + * @param EventDispatcherInterface $eventDispatcher */ public function __construct( - IUserManager $userManager, Throttle $throttle, - IRequest $request + IRequest $request, + EventDispatcherInterface $eventDispatcher ) { - $this->userManager = $userManager; $this->throttle = $throttle; $this->request = $request; + $this->eventDispatcher = $eventDispatcher; } public function register() { - $this->userManager->listen('\OC\User', 'preLogin', function ($uid) { - $this->preLoginCallback($uid); - }); - - $this->userManager->listen('\OC\User', 'failedLogin', function ($uid) { - $this->failedLoginCallback($uid); - }); - - $this->userManager->listen('\OC\User', 'postLogin', function ($user) { - /** @var $user \OCP\IUser */ - $this->postLoginCallback($user->getUID()); - }); + /* Login events */ + $this->eventDispatcher->addListener('user.loginfailed', [$this, 'failedLoginCallback']); + $this->eventDispatcher->addListener('user.afterlogin', [$this, 'postLoginCallback']); + $this->eventDispatcher->addListener('user.beforelogin', [$this, 'preLoginCallback']); } /** - * @param string $uid + * @param GenericEvent $event */ - public function failedLoginCallback($uid) { + public function failedLoginCallback($event) { + $uid = $event->getArgument('user'); $this->throttle->addFailedLoginAttempt($uid, $this->request->getRemoteAddress()); } /** - * @param string $uid + * @param GenericEvent $event */ - public function postLoginCallback($uid) { - $this->throttle->clearSuspiciousAttemptsForUidIpCombination($uid, $this->request->getRemoteAddress()); + public function postLoginCallback($event) { + /** @var \OCP\IUser $user */ + $user = $event->getArgument('user'); + $this->throttle->clearSuspiciousAttemptsForUidIpCombination($user->getUID(), $this->request->getRemoteAddress()); } /** - * @param string $uid + * @param GenericEvent $event * @throws LoginException */ - public function preLoginCallback($uid) { + public function preLoginCallback($event) { + $uid = $event->getArgument('user'); $this->throttle->applyBruteForcePolicy($uid, $this->request->getRemoteAddress()); } } diff --git a/tests/unit/HooksTest.php b/tests/unit/HooksTest.php index 53a2aff..9e47d71 100644 --- a/tests/unit/HooksTest.php +++ b/tests/unit/HooksTest.php @@ -24,76 +24,64 @@ namespace OCA\BruteForceProtection\Tests; -use OC\User\Manager; use OCA\BruteForceProtection\Hooks; use OCA\BruteForceProtection\Throttle; use OCP\IRequest; +use OCP\IUser; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use Symfony\Component\EventDispatcher\GenericEvent; use Test\TestCase; class HooksTest extends TestCase { /** @var Hooks */ private $hooks; - /** - * @var \PHPUnit\Framework\MockObject\MockObject | Manager - */ - private $userManagerMock; - /** - * @var \PHPUnit\Framework\MockObject\MockObject | Throttle - */ + /** @var \PHPUnit\Framework\MockObject\MockObject | Throttle */ private $throttleMock; - /** - * @var \PHPUnit\Framework\MockObject\MockObject | IRequest - */ + /** @var \PHPUnit\Framework\MockObject\MockObject | IRequest */ private $requestMock; + /** @var \PHPUnit\Framework\MockObject\MockObject | EventDispatcherInterface */ + private $eventDispatcherMock; public function setUp(): void { parent::setUp(); - - $this->userManagerMock = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); - - $this->throttleMock = $this->getMockBuilder('OCA\BruteForceProtection\Throttle') - ->disableOriginalConstructor() - ->getMock(); - $this->requestMock = $this->getMockBuilder('OCP\IRequest') - ->disableOriginalConstructor() - ->getMock(); + $this->throttleMock = $this->createMock(Throttle::class); + $this->requestMock = $this->createMock(IRequest::class); + $this->eventDispatcherMock = $this->createMock(EventDispatcherInterface::class); $this->hooks = new Hooks( - $this->userManagerMock, $this->throttleMock, - $this->requestMock); + $this->requestMock, + $this->eventDispatcherMock + ); } public function testRegister() { - $this->userManagerMock->expects($this->exactly(3)) - ->method('listen'); + $this->eventDispatcherMock->expects($this->exactly(3)) + ->method('addListener'); $this->hooks->register(); } public function testFailedLoginCallback() { + $event = new GenericEvent(null, ['user' => 'test']); $this->throttleMock->expects($this->once()) ->method('addFailedLoginAttempt'); - - $this->hooks->failedLoginCallback("test"); - $this->assertTrue(true); + $this->hooks->failedLoginCallback($event); } public function testPostLoginCallback() { + $mockUser = $this->createMock(IUser::class); + $mockUser->method('getUID')->willReturn('test'); + $event = new GenericEvent(null, ['user' => $mockUser]); $this->throttleMock->expects($this->once()) ->method('clearSuspiciousAttemptsForUidIpCombination'); - - $this->hooks->postLoginCallback("test"); - $this->assertTrue(true); + $this->hooks->postLoginCallback($event); } public function testPreLoginCallback() { + $event = new GenericEvent(null, ['user' => 'test']); $this->throttleMock->expects($this->once()) ->method('applyBruteForcePolicy'); - - $this->hooks->preLoginCallback('test'); - $this->assertTrue(true); + $this->hooks->preLoginCallback($event); } } From 674d8231eb34ca5d7fce1f025f2eb128b48843d8 Mon Sep 17 00:00:00 2001 From: Semih Serhat Karakaya Date: Sun, 10 Nov 2019 20:41:16 +0300 Subject: [PATCH 2/7] apply brute force protection for public share links --- appinfo/Migrations/Version20191109111104.php | 60 +++++++ lib/BruteForceProtectionConfig.php | 2 +- lib/Db/FailedLinkAccess.php | 48 +++++ lib/Db/FailedLinkAccessMapper.php | 131 ++++++++++++++ lib/Db/FailedLoginAttemptMapper.php | 6 +- lib/Exceptions/LinkAuthException.php | 27 +++ lib/Hooks.php | 47 ++++- lib/Jobs/ExpireOldAttempts.php | 17 +- lib/Throttle.php | 94 +++++++--- templates/settings-admin.php | 26 +-- .../Controller/SettingsControllerTest.php | 6 +- tests/unit/Db/FailedLinkAccessMapperTest.php | 170 ++++++++++++++++++ .../unit/Db/FailedLoginAttemptMapperTest.php | 19 +- tests/unit/HooksTest.php | 38 +++- tests/unit/Jobs/ExpireOldAttemptsTest.php | 29 +-- tests/unit/ThrottleTest.php | 115 ++++++++---- 16 files changed, 720 insertions(+), 115 deletions(-) create mode 100644 appinfo/Migrations/Version20191109111104.php create mode 100644 lib/Db/FailedLinkAccess.php create mode 100644 lib/Db/FailedLinkAccessMapper.php create mode 100644 lib/Exceptions/LinkAuthException.php create mode 100644 tests/unit/Db/FailedLinkAccessMapperTest.php diff --git a/appinfo/Migrations/Version20191109111104.php b/appinfo/Migrations/Version20191109111104.php new file mode 100644 index 0000000..10b6405 --- /dev/null +++ b/appinfo/Migrations/Version20191109111104.php @@ -0,0 +1,60 @@ + + * + * @copyright Copyright (c) 2020, ownCloud GmbH + * @license GPL-2.0 + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 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 General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ +namespace OCA\brute_force_protection\Migrations; + +use Doctrine\DBAL\Schema\Schema; +use OCP\Migration\ISchemaMigration; + +class Version20191109111104 implements ISchemaMigration { + + /** @var string */ + private $prefix; + + public function changeSchema(Schema $schema, array $options) { + $this->prefix = $options['tablePrefix']; + if (!$schema->hasTable("{$this->prefix}bfp_link_accesses")) { + $table = $schema->createTable("{$this->prefix}bfp_link_accesses"); + $table->addColumn('id', 'integer', [ + 'autoincrement' => true, + 'unsigned' => true, + 'notnull' => true, + 'length' => 11, + ]); + $table->addColumn('ip', 'string', [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('link_token', 'string', [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('attempted_at', 'integer', [ + 'notnull' => true, + ]); + + $table->setPrimaryKey(['id']); + $table->addIndex(['ip'], 'bfp_link_accesses_ip'); + $table->addIndex(['attempted_at'], 'bfp_link_accesses_at'); + } + } +} diff --git a/lib/BruteForceProtectionConfig.php b/lib/BruteForceProtectionConfig.php index 890f412..ee1feaf 100644 --- a/lib/BruteForceProtectionConfig.php +++ b/lib/BruteForceProtectionConfig.php @@ -75,7 +75,7 @@ public function getBruteForceProtectionFailTolerance() { } /** - * Count failed login attempts over how many seconds + * Count failed attempts over how many seconds * * @return int */ diff --git a/lib/Db/FailedLinkAccess.php b/lib/Db/FailedLinkAccess.php new file mode 100644 index 0000000..3b26cb5 --- /dev/null +++ b/lib/Db/FailedLinkAccess.php @@ -0,0 +1,48 @@ + + * + * @copyright Copyright (c) 2019, ownCloud GmbH + * @license GPL-2.0 + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 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 General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +namespace OCA\BruteForceProtection\Db; + +use OCP\AppFramework\Db\Entity; + +/** + * @method int getId() + * @method void setId(\int $id) + * @method string getIp() + * @method void setIp(\string $ip) + * @method string getLinkToken() + * @method void setLinkToken(\string $linkToken) + * @method int getAttemptedAt() + * @method void setAttemptedAt(\int $attemptedAt) + */ +class FailedLinkAccess extends Entity { + + /** @var string $ip */ + protected $ip; + + /** @var string $linkToken */ + protected $linkToken; + + /** @var int $attemptedAt */ + protected $attemptedAt; +} diff --git a/lib/Db/FailedLinkAccessMapper.php b/lib/Db/FailedLinkAccessMapper.php new file mode 100644 index 0000000..b035633 --- /dev/null +++ b/lib/Db/FailedLinkAccessMapper.php @@ -0,0 +1,131 @@ + + * + * @copyright Copyright (c) 2019 ownCloud GmbH + * @license GPL-2.0 + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 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 General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +namespace OCA\BruteForceProtection\Db; + +use OCP\AppFramework\Db\Mapper; +use OCA\BruteForceProtection\BruteForceProtectionConfig; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IDBConnection; + +/** + * Class FailedLinkAccessMapper + * @package OCA\BruteForceProtection\Db + */ +class FailedLinkAccessMapper extends Mapper { + + /** + * @var BruteForceProtectionConfig $config + */ + protected $config; + + /** + * @var ITimeFactory $timeFactory + */ + protected $timeFactory; + + /** + * @var string $tableName + */ + protected $tableName = 'bfp_link_accesses'; + + /** + * FailedLinkAccessMapper constructor. + * + * @param IDBConnection $db + * @param BruteForceProtectionConfig $config + * @param ITimeFactory $timeFactory + */ + public function __construct( + IDBConnection $db, + BruteForceProtectionConfig $config, + ITimeFactory $timeFactory + ) { + parent::__construct($db, $this->tableName); + $this->config = $config; + $this->timeFactory = $timeFactory; + } + + /** + * @param string $token + * @param string $ip + * @return int + */ + public function getFailedAccessCountForTokenIpCombination($token, $ip) { + $builder = $this->db->getQueryBuilder(); + $thresholdTime = $this->getLastFailedAccessTimeForTokenIpCombination($token, $ip) - $this->config->getBruteForceProtectionTimeThreshold(); + $attempts = $builder->selectAlias($builder->createFunction('COUNT(*)'), 'count') + ->from($this->tableName) + ->where($builder->expr()->gt('attempted_at', $builder->createNamedParameter($thresholdTime))) + ->andWhere($builder->expr()->eq('link_token', $builder->createNamedParameter($token))) + ->andWhere($builder->expr()->eq('ip', $builder->createNamedParameter($ip))) + ->execute() + ->fetch(); + return \intval($attempts['count']); + } + + /** + * @param string $token + * @param string $ip + * @return int + */ + public function getLastFailedAccessTimeForTokenIpCombination($token, $ip) { + $builder = $this->db->getQueryBuilder(); + $thresholdTime = $this->timeFactory->getTime() - $this->config->getBruteForceProtectionBanPeriod(); + $lastAttempt = $builder->select('attempted_at') + ->from($this->tableName) + ->where($builder->expr()->gt('attempted_at', $builder->createNamedParameter($thresholdTime))) + ->andWhere($builder->expr()->eq('link_token', $builder->createNamedParameter($token))) + ->andWhere($builder->expr()->eq('ip', $builder->createNamedParameter($ip))) + ->orderBy('attempted_at', 'DESC') + ->setMaxResults(1) + ->execute() + ->fetch(); + return \intval($lastAttempt['attempted_at']); + } + + /** + * @param string $token + * @param string $ip + */ + public function deleteFailedAccessForTokenIpCombination($token, $ip) { + $builder = $this->db->getQueryBuilder(); + $builder->delete($this->tableName) + ->where($builder->expr()->eq('link_token', $builder->createNamedParameter($token))) + ->andWhere($builder->expr()->eq('ip', $builder->createNamedParameter($ip))) + ->execute(); + } + + /** + * It removes entries that were created before the specified threshold seconds. + * + * @param int $threshold the amount of threshold seconds + */ + public function deleteOldFailedAccesses($threshold) { + $builder = $this->db->getQueryBuilder(); + $thresholdTime = $this->timeFactory->getTime() - $threshold; + $builder->delete($this->tableName) + ->where($builder->expr()->lt('attempted_at', $builder->createNamedParameter($thresholdTime))) + ->execute(); + } +} diff --git a/lib/Db/FailedLoginAttemptMapper.php b/lib/Db/FailedLoginAttemptMapper.php index 53962cf..931cb2f 100644 --- a/lib/Db/FailedLoginAttemptMapper.php +++ b/lib/Db/FailedLoginAttemptMapper.php @@ -72,7 +72,7 @@ public function __construct( * @param string $ip * @return int */ - public function getSuspiciousActivityCountForUidIpCombination($uid, $ip) { + public function getFailedLoginCountForUidIpCombination($uid, $ip) { $builder = $this->db->getQueryBuilder(); $thresholdTime = $this->getLastFailedLoginAttemptTimeForUidIpCombination($uid, $ip) - $this->config->getBruteForceProtectionTimeThreshold(); $attempts = $builder->selectAlias($builder->createFunction('COUNT(*)'), 'count') @@ -109,7 +109,7 @@ public function getLastFailedLoginAttemptTimeForUidIpCombination($uid, $ip) { * @param string $uid * @param string $ip */ - public function deleteSuspiciousAttemptsForUidIpCombination($uid, $ip) { + public function deleteFailedLoginAttemptsForUidIpCombination($uid, $ip) { $builder = $this->db->getQueryBuilder(); $builder->delete($this->tableName) ->where($builder->expr()->eq('uid', $builder->createNamedParameter($uid))) @@ -118,7 +118,7 @@ public function deleteSuspiciousAttemptsForUidIpCombination($uid, $ip) { } /** - * It removes entries that created before the specified threshold seconds. + * It removes entries that were created before the specified threshold seconds. * * @param int $threshold the amount of threshold seconds */ diff --git a/lib/Exceptions/LinkAuthException.php b/lib/Exceptions/LinkAuthException.php new file mode 100644 index 0000000..0059d52 --- /dev/null +++ b/lib/Exceptions/LinkAuthException.php @@ -0,0 +1,27 @@ + + * + * @copyright Copyright (c) 2019, ownCloud GmbH + * @license GPL-2.0 + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 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 General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +namespace OCA\BruteForceProtection\Exceptions; + +class LinkAuthException extends \Exception { +} diff --git a/lib/Hooks.php b/lib/Hooks.php index 1a26ef1..098804d 100644 --- a/lib/Hooks.php +++ b/lib/Hooks.php @@ -25,7 +25,10 @@ namespace OCA\BruteForceProtection; use OC\User\LoginException; +use OCA\BruteForceProtection\Exceptions\LinkAuthException; use OCP\IRequest; +use OCP\IUser; +use OCP\Share\IShare; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\GenericEvent; @@ -64,6 +67,11 @@ public function register() { $this->eventDispatcher->addListener('user.loginfailed', [$this, 'failedLoginCallback']); $this->eventDispatcher->addListener('user.afterlogin', [$this, 'postLoginCallback']); $this->eventDispatcher->addListener('user.beforelogin', [$this, 'preLoginCallback']); + + /* Public link share events */ + $this->eventDispatcher->addListener('share.linkaccess', [$this, 'linkAccessCallback']); + $this->eventDispatcher->addListener('share.afterlinkauth', [$this, 'postLinkAuthCallback']); + $this->eventDispatcher->addListener('share.beforelinkauth', [$this, 'preLinkAuthCallback']); } /** @@ -78,9 +86,9 @@ public function failedLoginCallback($event) { * @param GenericEvent $event */ public function postLoginCallback($event) { - /** @var \OCP\IUser $user */ + /** @var IUser $user */ $user = $event->getArgument('user'); - $this->throttle->clearSuspiciousAttemptsForUidIpCombination($user->getUID(), $this->request->getRemoteAddress()); + $this->throttle->clearFailedLoginAttemptsForUidIpCombination($user->getUID(), $this->request->getRemoteAddress()); } /** @@ -88,7 +96,38 @@ public function postLoginCallback($event) { * @throws LoginException */ public function preLoginCallback($event) { - $uid = $event->getArgument('user'); - $this->throttle->applyBruteForcePolicy($uid, $this->request->getRemoteAddress()); + $uid = $event->getArgument('login'); + $this->throttle->applyBruteForcePolicyForLogin($uid, $this->request->getRemoteAddress()); + } + + /** + * @param GenericEvent $event + */ + public function linkAccessCallback($event) { + /** @var IShare $share */ + $share = $event->getArgument('shareObject'); + $errorCode = $event->getArgument('errorCode'); + if ($errorCode === 403) { + $this->throttle->addFailedLinkAccess($share->getToken(), $this->request->getRemoteAddress()); + } + } + + /** + * @param GenericEvent $event + */ + public function postLinkAuthCallback($event) { + /** @var IShare $share */ + $share = $event->getArgument('shareObject'); + $this->throttle->clearFailedLinkAccesses($share->getToken(), $this->request->getRemoteAddress()); + } + + /** + * @param GenericEvent $event + * @throws LinkAuthException + */ + public function preLinkAuthCallback($event) { + /** @var IShare $share */ + $share = $event->getArgument('shareObject'); + $this->throttle->applyBruteForcePolicyForLinkShare($share->getToken(), $this->request->getRemoteAddress()); } } diff --git a/lib/Jobs/ExpireOldAttempts.php b/lib/Jobs/ExpireOldAttempts.php index 4b5d3e5..096b0f9 100644 --- a/lib/Jobs/ExpireOldAttempts.php +++ b/lib/Jobs/ExpireOldAttempts.php @@ -23,25 +23,32 @@ namespace OCA\BruteForceProtection\Jobs; use OC\BackgroundJob\TimedJob; use OCA\BruteForceProtection\BruteForceProtectionConfig; +use OCA\BruteForceProtection\Db\FailedLinkAccessMapper; use OCA\BruteForceProtection\Db\FailedLoginAttemptMapper; class ExpireOldAttempts extends TimedJob { - /** @var FailedLoginAttemptMapper $mapper */ - private $mapper; + /** @var FailedLoginAttemptMapper $failedLoginMapper */ + private $failedLoginMapper; + /** @var FailedLinkAccessMapper $linkAccessMapper */ + private $linkAccessMapper; /** @var BruteForceProtectionConfig $config */ private $config; + public function __construct( - FailedLoginAttemptMapper $mapper, + FailedLoginAttemptMapper $failedLoginMapper, + FailedLinkAccessMapper $linkAccessMapper, BruteForceProtectionConfig $config ) { // Run once a day $this->setInterval(24 * 60 * 60); - $this->mapper = $mapper; + $this->failedLoginMapper = $failedLoginMapper; + $this->linkAccessMapper = $linkAccessMapper; $this->config = $config; } public function run($argument) { $threshold = $this->config->getBruteForceProtectionTimeThreshold() + $this->config->getBruteForceProtectionBanPeriod(); - $this->mapper->deleteOldFailedLoginAttempts($threshold); + $this->failedLoginMapper->deleteOldFailedLoginAttempts($threshold); + $this->linkAccessMapper->deleteOldFailedAccesses($threshold); } } diff --git a/lib/Throttle.php b/lib/Throttle.php index 72c3fa4..ad6d0f3 100644 --- a/lib/Throttle.php +++ b/lib/Throttle.php @@ -25,8 +25,11 @@ namespace OCA\BruteForceProtection; use OC\User\LoginException; +use OCA\BruteForceProtection\Db\FailedLinkAccess; +use OCA\BruteForceProtection\Db\FailedLinkAccessMapper; use OCA\BruteForceProtection\Db\FailedLoginAttempt; use OCA\BruteForceProtection\Db\FailedLoginAttemptMapper; +use OCA\BruteForceProtection\Exceptions\LinkAuthException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IL10N; @@ -36,39 +39,37 @@ */ class Throttle { - /** - * @var FailedLoginAttemptMapper $attemptMapper - */ - protected $attemptMapper; + /** @var FailedLoginAttemptMapper $loginAttemptMapper */ + protected $loginAttemptMapper; - /** - * @var BruteForceProtectionConfig $config - */ + /** @var FailedLinkAccessMapper $linkAccessMapper */ + protected $linkAccessMapper; + + /** @var BruteForceProtectionConfig $config */ protected $config; - /** - * @var IL10N $l - */ + /** @var IL10N $l */ protected $l; - /** - * @var ITimeFactory $timeFactory - */ + /** @var ITimeFactory $timeFactory */ protected $timeFactory; /** - * @param FailedLoginAttemptMapper $attemptMapper + * @param FailedLoginAttemptMapper $loginAttemptMapper + * @param FailedLinkAccessMapper $linkAccessMapper * @param BruteForceProtectionConfig $config * @param IL10N $l * @param ITimeFactory $timeFactory */ public function __construct( - FailedLoginAttemptMapper $attemptMapper, + FailedLoginAttemptMapper $loginAttemptMapper, + FailedLinkAccessMapper $linkAccessMapper, BruteForceProtectionConfig $config, IL10N $l, ITimeFactory $timeFactory ) { - $this->attemptMapper = $attemptMapper; + $this->loginAttemptMapper = $loginAttemptMapper; + $this->linkAccessMapper = $linkAccessMapper; $this->config = $config; $this->l = $l; $this->timeFactory = $timeFactory; @@ -84,7 +85,7 @@ public function addFailedLoginAttempt($uid, $ip) { $attempt->setUid($uid); $attempt->setIp($ip); $attempt->setAttemptedAt($this->timeFactory->getTime()); - $this->attemptMapper->insert($attempt); + $this->loginAttemptMapper->insert($attempt); } /** @@ -92,10 +93,10 @@ public function addFailedLoginAttempt($uid, $ip) { * @param string $ip * @throws LoginException */ - public function applyBruteForcePolicy($uid, $ip) { + public function applyBruteForcePolicyForLogin($uid, $ip) { $banPeriod = $this->config->getBruteForceProtectionBanPeriod(); - $banUntil = $this->attemptMapper->getLastFailedLoginAttemptTimeForUidIpCombination($uid, $ip) + $banPeriod; - if ($this->attemptMapper->getSuspiciousActivityCountForUidIpCombination($uid, $ip) >= + $banUntil = $this->loginAttemptMapper->getLastFailedLoginAttemptTimeForUidIpCombination($uid, $ip) + $banPeriod; + if ($this->loginAttemptMapper->getFailedLoginCountForUidIpCombination($uid, $ip) >= $this->config->getBruteForceProtectionFailTolerance() && $banUntil > $this->timeFactory->getTime()) { throw new LoginException($this->l->t("Too many failed login attempts. Try again in %s.", @@ -109,17 +110,56 @@ public function applyBruteForcePolicy($uid, $ip) { * @param string $ip * @return void */ - public function clearSuspiciousAttemptsForUidIpCombination($uid, $ip) { - $this->attemptMapper->deleteSuspiciousAttemptsForUidIpCombination($uid, $ip); + public function clearFailedLoginAttemptsForUidIpCombination($uid, $ip) { + $this->loginAttemptMapper->deleteFailedLoginAttemptsForUidIpCombination($uid, $ip); + } + + /** + * @param string $token + * @param string $ip + * @return void + */ + public function addFailedLinkAccess($token, $ip) { + $access = new FailedLinkAccess(); + $access->setLinkToken($token); + $access->setIp($ip); + $access->setAttemptedAt($this->timeFactory->getTime()); + $this->linkAccessMapper->insert($access); + } + + /** + * @param string $token + * @param string $ip + * @throws LinkAuthException + */ + public function applyBruteForcePolicyForLinkShare($token, $ip) { + $banPeriod = $this->config->getBruteForceProtectionBanPeriod(); + $banUntil = $this->linkAccessMapper->getLastFailedAccessTimeForTokenIpCombination($token, $ip) + $banPeriod; + if ($this->linkAccessMapper->getFailedAccessCountForTokenIpCombination($token, $ip) >= + $this->config->getBruteForceProtectionFailTolerance() && + $banUntil > $this->timeFactory->getTime()) { + throw new LinkAuthException($this->l->t("Too many failed attempts. Try again in %s.", + $this->parseBanPeriodForHumans($banPeriod)) + ); + } + } + + /** + * @param string $token + * @param string $ip + * @return void + */ + public function clearFailedLinkAccesses($token, $ip) { + $this->linkAccessMapper->deleteFailedAccessForTokenIpCombination($token, $ip); } /** - * @param int $banPeriod + * @param int $seconds * @return string $banPeriodForHumans */ - private function parseBanPeriodForHumans($banPeriod) { - return ($banPeriod / 60 < 60) - ? $this->l->n(' %n minute', ' %n minutes', (int)\ceil($banPeriod/60)) - : $this->l->n(' %n hour', ' %n hours', (int)\ceil(($banPeriod/60)/60)); + private function parseBanPeriodForHumans($seconds) { + return ($seconds / 60 < 60) + ? $this->l->n(' %n minute', ' %n minutes', (int)\ceil($seconds/60)) + : $this->l->n(' %n hour', ' %n hours', (int)\ceil(($seconds/60)/60)); } } diff --git a/templates/settings-admin.php b/templates/settings-admin.php index e3a6014..9235705 100644 --- a/templates/settings-admin.php +++ b/templates/settings-admin.php @@ -26,16 +26,16 @@ /** @var \OCP\IL10N $l */ script('brute_force_protection', 'settings-admin'); ?> -
-

t('Brute Force Protection')); ?>

- -
-
-
-
-
-
-
- -
-
+
+

t('Brute Force Protection')); ?>

+ +
+
+
+
+
+
+
+ +
+
diff --git a/tests/unit/Controller/SettingsControllerTest.php b/tests/unit/Controller/SettingsControllerTest.php index b5e8b14..56d0e87 100644 --- a/tests/unit/Controller/SettingsControllerTest.php +++ b/tests/unit/Controller/SettingsControllerTest.php @@ -39,10 +39,8 @@ class SettingsControllerTest extends TestCase { protected function setUp(): void { parent::setUp(); - $this->request = $this->getMockBuilder(IRequest::class)->getMock(); - $this->config = $this->getMockBuilder(BruteForceProtectionConfig::class) - ->disableOriginalConstructor() - ->getMock(); + $this->request = $this->createMock(IRequest::class); + $this->config = $this->createMock(BruteForceProtectionConfig::class); $this->controller = new SettingsController('brute_force_protection', $this->request, $this->config); } diff --git a/tests/unit/Db/FailedLinkAccessMapperTest.php b/tests/unit/Db/FailedLinkAccessMapperTest.php new file mode 100644 index 0000000..a9ad661 --- /dev/null +++ b/tests/unit/Db/FailedLinkAccessMapperTest.php @@ -0,0 +1,170 @@ + + * + * @copyright Copyright (c) 2019, ownCloud GmbH + * @license GPL-2.0 + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 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 General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +namespace OCA\BruteForceProtection\Tests\Db; + +use OCA\BruteForceProtection\BruteForceProtectionConfig; +use OCA\BruteForceProtection\Db\FailedLinkAccess; +use OCA\BruteForceProtection\Db\FailedLinkAccessMapper; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IDBConnection; +use Test\TestCase; + +/** + * @group DB + */ +class FailedLinkAccessMapperTest extends TestCase { + + /** @var FailedLinkAccess $mapper*/ + private $mapper; + + /** @var IDBConnection $connection*/ + private $connection; + + /** @var BruteForceProtectionConfig | \PHPUnit\Framework\MockObject\MockObject $configMock */ + private $configMock; + + /** @var ITimeFactory | \PHPUnit\Framework\MockObject\MockObject $timeFactory */ + private $timeFactoryMock; + + /** @var int $baseTime */ + private $baseTime; + + /** @var string */ + private $dbTable = 'bfp_link_accesses'; + + /** @var int $thresholdConfigVal */ + private $thresholdConfigVal = 60; + + public function setUp(): void { + parent::setUp(); + $this->baseTime = \time(); + $this->timeFactoryMock = $this->createMock(ITimeFactory::class); + $this->configMock = $this->createMock(BruteForceProtectionConfig::class); + $this->connection = \OC::$server->getDatabaseConnection(); + $this->mapper = new FailedLinkAccessMapper($this->connection, $this->configMock, $this->timeFactoryMock); + + $query = $this->connection->getQueryBuilder()->select('*')->from($this->dbTable); + $result = $query->execute()->fetchAll(); + $this->assertEmpty($result, 'we need to start with a empty bfp_link_accesses table'); + + $this->addInitialTestEntries(); + } + + public function addInitialTestEntries() { + $linkAccess = new FailedLinkAccess(); + $linkAccess->setLinkToken('token1'); + $linkAccess->setIp("192.168.1.1"); + $linkAccess->setAttemptedAt($this->baseTime+20); + $this->mapper->insert($linkAccess); + + $linkAccess = new FailedLinkAccess(); + $linkAccess->setLinkToken('token1'); + $linkAccess->setIp("192.168.1.1"); + $linkAccess->setAttemptedAt($this->baseTime+60); + $this->mapper->insert($linkAccess); + + $linkAccess = new FailedLinkAccess(); + $linkAccess->setLinkToken('token1'); + $linkAccess->setIp("192.168.1.2"); + $linkAccess->setAttemptedAt($this->baseTime+60); + $this->mapper->insert($linkAccess); + + $linkAccess = new FailedLinkAccess(); + $linkAccess->setLinkToken('token2'); + $linkAccess->setIp("192.168.1.1"); + $linkAccess->setAttemptedAt($this->baseTime+60); + $this->mapper->insert($linkAccess); + + $linkAccess = new FailedLinkAccess(); + $linkAccess->setLinkToken('token1'); + $linkAccess->setIp("192.168.1.1"); + $linkAccess->setAttemptedAt($this->baseTime+100); + $this->mapper->insert($linkAccess); + } + + public function tearDown(): void { + parent::tearDown(); + $query = $this->connection->getQueryBuilder()->delete($this->dbTable); + $query->execute(); + } + + public function testGetFailedAccessCountForTokenIpCombination() { + $functionCallTime = $this->baseTime+110; + $this->configMock->expects($this->exactly(3)) + ->method('getBruteForceProtectionTimeThreshold') + ->willReturn($this->thresholdConfigVal); + $this->timeFactoryMock->expects($this->exactly(3)) + ->method('getTime') + ->willReturn($functionCallTime); + + $this->assertEquals(3, $this->mapper->getFailedAccessCountForTokenIpCombination('token1', '192.168.1.1')); + $this->assertEquals(1, $this->mapper->getFailedAccessCountForTokenIpCombination('token1', '192.168.1.2')); + $this->assertEquals(1, $this->mapper->getFailedAccessCountForTokenIpCombination('token2', '192.168.1.1')); + } + + public function testGetLastFailedAccessTimeForTokenIpCombination() { + $lastAttemptTime = $this->baseTime+100; + $this->configMock->expects($this->once()) + ->method('getBruteForceProtectionBanPeriod') + ->willReturn('250'); + $this->timeFactoryMock->expects($this->once()) + ->method('getTime') + ->willReturn($this->baseTime+300); + + $this->assertEquals($this->mapper->getLastFailedAccessTimeForTokenIpCombination('token1', '192.168.1.1'), $lastAttemptTime); + } + + public function testDeleteFailedAccessForTokenIpCombination() { + $builder = $this->connection->getQueryBuilder(); + + $query = $builder->select('*')->from($this->dbTable) + ->Where($builder->expr()->eq('ip', $builder->createNamedParameter("192.168.1.1"))) + ->andWhere($builder->expr()->eq('link_token', $builder->createNamedParameter("token1"))); + $result = $query->execute()->fetchAll(); + $this->assertCount(3, $result); + + $this->mapper->deleteFailedAccessForTokenIpCombination('token1', "192.168.1.1"); + + $query = $builder->select('*')->from($this->dbTable) + ->Where($builder->expr()->eq('ip', $builder->createNamedParameter("192.168.1.1"))) + ->andWhere($builder->expr()->eq('link_token', $builder->createNamedParameter("token1"))); + $result = $query->execute()->fetchAll(); + $this->assertCount(0, $result); + } + + public function testDeleteOldFailedLoginAttempts() { + $builder = $this->connection->getQueryBuilder(); + $functionCallTime = $this->baseTime+130; + $this->timeFactoryMock->expects($this->exactly(1)) + ->method('getTime') + ->willReturn($functionCallTime); + $query = $builder->select('*')->from($this->dbTable); + $result = $query->execute()->fetchAll(); + $this->assertCount(5, $result); + $this->mapper->deleteOldFailedAccesses($this->thresholdConfigVal); + $query = $builder->select('*')->from($this->dbTable); + $result = $query->execute()->fetchAll(); + $this->assertCount(1, $result); + } +} diff --git a/tests/unit/Db/FailedLoginAttemptMapperTest.php b/tests/unit/Db/FailedLoginAttemptMapperTest.php index 434221c..f7ed8ea 100644 --- a/tests/unit/Db/FailedLoginAttemptMapperTest.php +++ b/tests/unit/Db/FailedLoginAttemptMapperTest.php @@ -61,13 +61,9 @@ class FailedLoginAttemptMapperTest extends TestCase { public function setUp(): void { parent::setUp(); $this->baseTime = \time(); - $this->timeFactoryMock = $this->getMockBuilder(ITimeFactory::class) - ->disableOriginalConstructor() - ->getMock(); + $this->timeFactoryMock = $this->createMock(ITimeFactory::class); + $this->configMock = $this->createMock(BruteForceProtectionConfig::class); $this->connection = \OC::$server->getDatabaseConnection(); - $this->configMock = $this->getMockBuilder(BruteForceProtectionConfig::class) - ->disableOriginalConstructor() - ->getMock(); $this->mapper = new FailedLoginAttemptMapper($this->connection, $this->configMock, $this->timeFactoryMock); $query = $this->connection->getQueryBuilder()->select('*')->from($this->dbTable); @@ -123,9 +119,10 @@ public function testGetSuspiciousActivityCountForUidIpCombination() { $this->timeFactoryMock->expects($this->exactly(3)) ->method('getTime') ->willReturn($functionCallTime); - $this->assertEquals(3, $this->mapper->getSuspiciousActivityCountForUidIpCombination('test1', '192.168.1.1')); - $this->assertEquals(1, $this->mapper->getSuspiciousActivityCountForUidIpCombination('test1', '192.168.1.2')); - $this->assertEquals(1, $this->mapper->getSuspiciousActivityCountForUidIpCombination('test2', '192.168.1.1')); + + $this->assertEquals(3, $this->mapper->getFailedLoginCountForUidIpCombination('test1', '192.168.1.1')); + $this->assertEquals(1, $this->mapper->getFailedLoginCountForUidIpCombination('test1', '192.168.1.2')); + $this->assertEquals(1, $this->mapper->getFailedLoginCountForUidIpCombination('test2', '192.168.1.1')); } public function testGetLastFailedLoginAttemptTimeForUidIpCombination() { @@ -140,7 +137,7 @@ public function testGetLastFailedLoginAttemptTimeForUidIpCombination() { $this->assertEquals($this->mapper->getLastFailedLoginAttemptTimeForUidIpCombination('test1', '192.168.1.1'), $lastAttemptTime); } - public function testDeleteSuspiciousAttemptsForUidIpCombination() { + public function testDeleteFailedLoginAttemptsForUidIpCombination() { $builder = $this->connection->getQueryBuilder(); $query = $builder->select('*')->from($this->dbTable) @@ -149,7 +146,7 @@ public function testDeleteSuspiciousAttemptsForUidIpCombination() { $result = $query->execute()->fetchAll(); $this->assertCount(3, $result); - $this->mapper->deleteSuspiciousAttemptsForUidIpCombination('test1', "192.168.1.1"); + $this->mapper->deleteFailedLoginAttemptsForUidIpCombination('test1', "192.168.1.1"); $query = $builder->select('*')->from($this->dbTable) ->Where($builder->expr()->eq('ip', $builder->createNamedParameter("192.168.1.1"))) diff --git a/tests/unit/HooksTest.php b/tests/unit/HooksTest.php index 9e47d71..4b6cdce 100644 --- a/tests/unit/HooksTest.php +++ b/tests/unit/HooksTest.php @@ -28,6 +28,7 @@ use OCA\BruteForceProtection\Throttle; use OCP\IRequest; use OCP\IUser; +use OCP\Share; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\GenericEvent; use Test\TestCase; @@ -57,7 +58,7 @@ public function setUp(): void { } public function testRegister() { - $this->eventDispatcherMock->expects($this->exactly(3)) + $this->eventDispatcherMock->expects($this->exactly(6)) ->method('addListener'); $this->hooks->register(); } @@ -74,14 +75,43 @@ public function testPostLoginCallback() { $mockUser->method('getUID')->willReturn('test'); $event = new GenericEvent(null, ['user' => $mockUser]); $this->throttleMock->expects($this->once()) - ->method('clearSuspiciousAttemptsForUidIpCombination'); + ->method('clearFailedLoginAttemptsForUidIpCombination'); $this->hooks->postLoginCallback($event); } public function testPreLoginCallback() { - $event = new GenericEvent(null, ['user' => 'test']); + $event = new GenericEvent(null, ['login' => 'test']); $this->throttleMock->expects($this->once()) - ->method('applyBruteForcePolicy'); + ->method('applyBruteForcePolicyForLogin'); $this->hooks->preLoginCallback($event); } + + public function testLinkAccessCallback() { + $share = $this->createMock('OCP\Share\IShare'); + $share->method('getShareType')->willReturn(Share::SHARE_TYPE_LINK); + $event = new GenericEvent(null, ['shareObject' => $share, 'errorCode' => 403]); + $this->throttleMock->expects($this->once()) + ->method('addFailedLinkAccess'); + $this->hooks->linkAccessCallback($event); + } + + public function testPostLinkAccessCallback() { + $share = $this->createMock('OCP\Share\IShare'); + $share->method('getShareType')->willReturn(Share::SHARE_TYPE_LINK); + $mockUser = $this->createMock(IUser::class); + $mockUser->method('getUID')->willReturn('test'); + $event = new GenericEvent(null, ['shareObject' => $share]); + $this->throttleMock->expects($this->once()) + ->method('clearFailedLinkAccesses'); + $this->hooks->postLinkAuthCallback($event); + } + + public function testPreLinkAuthCallback() { + $share = $this->createMock('OCP\Share\IShare'); + $share->method('getShareType')->willReturn(Share::SHARE_TYPE_LINK); + $event = new GenericEvent(null, ['shareObject' => $share]); + $this->throttleMock->expects($this->once()) + ->method('applyBruteForcePolicyForLinkShare'); + $this->hooks->preLinkAuthCallback($event); + } } diff --git a/tests/unit/Jobs/ExpireOldAttemptsTest.php b/tests/unit/Jobs/ExpireOldAttemptsTest.php index 443b0f4..b74067c 100644 --- a/tests/unit/Jobs/ExpireOldAttemptsTest.php +++ b/tests/unit/Jobs/ExpireOldAttemptsTest.php @@ -22,6 +22,7 @@ */ namespace OCA\BruteForceProtection\Tests\Jobs; use OCA\BruteForceProtection\BruteForceProtectionConfig; +use OCA\BruteForceProtection\Db\FailedLinkAccessMapper; use OCA\BruteForceProtection\Db\FailedLoginAttemptMapper; use OCA\BruteForceProtection\Jobs\ExpireOldAttempts; use Test\TestCase; @@ -34,7 +35,9 @@ */ class ExpireOldAttemptsTest extends TestCase { /** @var FailedLoginAttemptMapper | \PHPUnit\Framework\MockObject\MockObject $mapper */ - private $mapper; + private $failedLoginMapper; + /** @var FailedLinkAccessMapper | \PHPUnit\Framework\MockObject\MockObject $mapper */ + private $linkAccessMapper; /** @var BruteForceProtectionConfig | \PHPUnit\Framework\MockObject\MockObject $config */ private $config; /** @var int $thresholdConfigVal */ @@ -43,27 +46,33 @@ class ExpireOldAttemptsTest extends TestCase { private $banPeriodConfigVal = 300; /** @var ExpireOldAttempts $expireAttempts */ private $expireAttempts; + public function setUp(): void { - $this->mapper = $this->getMockBuilder(FailedLoginAttemptMapper::class) - ->disableOriginalConstructor() - ->getMock(); - $this->config = $this->getMockBuilder(BruteForceProtectionConfig::class) - ->disableOriginalConstructor() - ->getMock(); - $this->expireAttempts = new ExpireOldAttempts($this->mapper, $this->config); + $this->failedLoginMapper = $this->createMock(FailedLoginAttemptMapper::class); + $this->linkAccessMapper = $this->createMock(FailedLinkAccessMapper::class); + $this->config = $this->createMock(BruteForceProtectionConfig::class); + $this->expireAttempts = new ExpireOldAttempts( + $this->failedLoginMapper, + $this->linkAccessMapper, + $this->config + ); } public function testExecute() { /** @var \OC\BackgroundJob\JobList $jobList */ $argument = []; + $threshold = $this->thresholdConfigVal+$this->banPeriodConfigVal; $this->config->expects($this->exactly(1)) ->method('getBruteForceProtectionTimeThreshold') ->willReturn($this->thresholdConfigVal); $this->config->expects($this->exactly(1)) ->method('getBruteForceProtectionBanPeriod') ->willReturn($this->banPeriodConfigVal); - $this->mapper->expects($this->exactly(1)) + $this->failedLoginMapper->expects($this->exactly(1)) ->method('deleteOldFailedLoginAttempts') - ->with($this->thresholdConfigVal+$this->banPeriodConfigVal); + ->with($threshold); + $this->linkAccessMapper->expects($this->exactly(1)) + ->method('deleteOldFailedAccesses') + ->with($threshold); $this->expireAttempts->run($argument); } } diff --git a/tests/unit/ThrottleTest.php b/tests/unit/ThrottleTest.php index 025af7c..f382aa8 100644 --- a/tests/unit/ThrottleTest.php +++ b/tests/unit/ThrottleTest.php @@ -24,52 +24,47 @@ namespace OCA\BruteForceProtection\Tests; +use OCA\BruteForceProtection\Db\FailedLinkAccessMapper; use OCA\BruteForceProtection\Db\FailedLoginAttemptMapper; use OCA\BruteForceProtection\Throttle; use OCA\BruteForceProtection\BruteForceProtectionConfig; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IL10N; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class ThrottleTest extends TestCase { /** @var Throttle */ private $throttle; - /** - * @var \PHPUnit\Framework\MockObject\MockObject | FailedLoginAttemptMapper - */ - private $attemptMapper; - /** - * @var \PHPUnit\Framework\MockObject\MockObject | BruteForceProtectionConfig - */ + + /** @var MockObject | FailedLoginAttemptMapper */ + private $loginAttemptMapper; + + /** @var MockObject | FailedLinkAccessMapper */ + private $linkAccessMapper; + + /** @var MockObject | BruteForceProtectionConfig */ private $configMock; - /** - * @var \PHPUnit\Framework\MockObject\MockObject | IL10N - */ + + /** @var MockObject | IL10N */ private $lMock; - /** - * @var \PHPUnit\Framework\MockObject\MockObject | ITimeFactory - */ + + /** @var MockObject | ITimeFactory */ private $timeFactoryMock; public function setUp(): void { parent::setUp(); - $this->attemptMapper = $this->getMockBuilder(FailedLoginAttemptMapper::class) - ->disableOriginalConstructor() - ->getMock(); - $this->lMock = $this->getMockBuilder(IL10N::class) - ->disableOriginalConstructor() - ->getMock(); - $this->timeFactoryMock = $this->getMockBuilder(ITimeFactory::class) - ->disableOriginalConstructor() - ->getMock(); - $this->configMock = $this->getMockBuilder(BruteForceProtectionConfig::class) - ->disableOriginalConstructor() - ->getMock(); + $this->loginAttemptMapper = $this->createMock(FailedLoginAttemptMapper::class); + $this->linkAccessMapper = $this->createMock(FailedLinkAccessMapper::class); + $this->lMock = $this->createMock(IL10N::class); + $this->timeFactoryMock = $this->createMock(ITimeFactory::class); + $this->configMock = $this->createMock(BruteForceProtectionConfig::class); $this->throttle = new Throttle( - $this->attemptMapper, + $this->loginAttemptMapper, + $this->linkAccessMapper, $this->configMock, $this->lMock, $this->timeFactoryMock @@ -77,13 +72,13 @@ public function setUp(): void { } public function testAddFailedLoginAttempt() { - $this->attemptMapper->expects($this->once())->method('insert'); + $this->loginAttemptMapper->expects($this->once())->method('insert'); $this->throttle->addFailedLoginAttempt('test', '192.168.1.1'); } /** - * @dataProvider bruteForceTestData + * @dataProvider bruteForceForLoginTestData * @param int $lastAttempt * @param int $attemptCount * @param int $banPeriod @@ -93,12 +88,12 @@ public function testAddFailedLoginAttempt() { public function testApplyBruteForcePolicy($lastAttempt, $attemptCount, $banPeriod, $failTolerance, $time) { $this->expectException(\OC\User\LoginException::class); - $this->attemptMapper->expects($this->once()) + $this->loginAttemptMapper->expects($this->once()) ->method('getLastFailedLoginAttemptTimeForUidIpCombination') ->with('test', '192.168.1.1') ->will($this->returnValue($lastAttempt)); - $this->attemptMapper->expects($this->once()) - ->method('getSuspiciousActivityCountForUidIpCombination') + $this->loginAttemptMapper->expects($this->once()) + ->method('getFailedLoginCountForUidIpCombination') ->with('test', '192.168.1.1') ->will($this->returnValue($attemptCount)); $this->configMock->expects($this->once()) @@ -110,12 +105,66 @@ public function testApplyBruteForcePolicy($lastAttempt, $attemptCount, $banPerio $this->timeFactoryMock->expects($this->once()) ->method('getTime') ->will($this->returnValue($time)); - $this->throttle->applyBruteForcePolicy('test', '192.168.1.1'); + $this->throttle->applyBruteForcePolicyForLogin('test', '192.168.1.1'); } - public function bruteForceTestData() { + public function bruteForceForLoginTestData() { return [ [5, 5, 10, 4, 14], [0, 3, 300, 2, 250] ]; } + + public function testClearFailedLoginAttemptsForUidIpCombination() { + $this->loginAttemptMapper->expects($this->once())->method('deleteFailedLoginAttemptsForUidIpCombination'); + + $this->throttle->clearFailedLoginAttemptsForUidIpCombination('test', '192.168.1.1'); + } + + public function testAddFailedLinkAccess() { + $this->linkAccessMapper->expects($this->once())->method('insert'); + + $this->throttle->addFailedLinkAccess('test', '192.168.1.1'); + } + + /** + * @dataProvider bruteForceForLoginTestData + * @param int $lastAttempt + * @param int $attemptCount + * @param int $banPeriod + * @param int $failTolerance + * @param int $time + */ + public function testApplyBruteForcePolicyForLinkShare($lastAttempt, $attemptCount, $banPeriod, $failTolerance, $time) { + $this->expectException(\OCA\BruteForceProtection\Exceptions\LinkAuthException::class); + + $this->linkAccessMapper->expects($this->once()) + ->method('getLastFailedAccessTimeForTokenIpCombination') + ->with('test', '192.168.1.1') + ->will($this->returnValue($lastAttempt)); + $this->linkAccessMapper->expects($this->once()) + ->method('getFailedAccessCountForTokenIpCombination') + ->with('test', '192.168.1.1') + ->will($this->returnValue($attemptCount)); + $this->configMock->expects($this->once()) + ->method('getBruteForceProtectionBanPeriod') + ->will($this->returnValue($banPeriod)); + $this->configMock->expects($this->once()) + ->method('getBruteForceProtectionFailTolerance') + ->will($this->returnValue($failTolerance)); + $this->timeFactoryMock->expects($this->once()) + ->method('getTime') + ->will($this->returnValue($time)); + $this->throttle->applyBruteForcePolicyForLinkShare('test', '192.168.1.1'); + } + public function bruteForceLinkShareTestData() { + return [ + [5, 5, 10, 4, 14], + [0, 3, 300, 2, 250] + ]; + } + + public function testClearFailedLinkAccesses() { + $this->linkAccessMapper->expects($this->once())->method('deleteFailedAccessForTokenIpCombination'); + $this->throttle->clearFailedLinkAccesses('test', '192.168.1.1'); + } } From b3fb1302cb421ae7b2b2ee30a6427192680f811d Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Tue, 19 May 2020 14:05:51 +0545 Subject: [PATCH 3/7] Fix phpstan errors --- lib/Db/FailedLinkAccess.php | 6 +++--- lib/Throttle.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Db/FailedLinkAccess.php b/lib/Db/FailedLinkAccess.php index 3b26cb5..b825cdb 100644 --- a/lib/Db/FailedLinkAccess.php +++ b/lib/Db/FailedLinkAccess.php @@ -29,11 +29,11 @@ * @method int getId() * @method void setId(\int $id) * @method string getIp() - * @method void setIp(\string $ip) + * @method void setIp(string $ip) * @method string getLinkToken() - * @method void setLinkToken(\string $linkToken) + * @method void setLinkToken(string $linkToken) * @method int getAttemptedAt() - * @method void setAttemptedAt(\int $attemptedAt) + * @method void setAttemptedAt(int $attemptedAt) */ class FailedLinkAccess extends Entity { diff --git a/lib/Throttle.php b/lib/Throttle.php index ad6d0f3..1f2fcde 100644 --- a/lib/Throttle.php +++ b/lib/Throttle.php @@ -139,7 +139,7 @@ public function applyBruteForcePolicyForLinkShare($token, $ip) { $this->config->getBruteForceProtectionFailTolerance() && $banUntil > $this->timeFactory->getTime()) { throw new LinkAuthException($this->l->t("Too many failed attempts. Try again in %s.", - $this->parseBanPeriodForHumans($banPeriod)) + [$this->parseBanPeriodForHumans($banPeriod)]) ); } } From 5705d98b593932c51ea2b5fd771ad880e7ef1e94 Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Tue, 19 May 2020 14:39:42 +0545 Subject: [PATCH 4/7] Bump minor version so migration runs --- appinfo/info.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index 433d3d1..a3d8375 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -21,7 +21,7 @@ See the [2-Factor Authentication](https://marketplace.owncloud.com/apps/twofacto Prevent attackers from guessing user passwords GPLv2 Semih Serhat Karakaya - 1.0.1 + 1.1.0 BruteForceProtection true From bf745876c2784a3bd1c34d2dd636d98dbebb9d1c Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Tue, 19 May 2020 14:42:08 +0545 Subject: [PATCH 5/7] Handle getLastFailedAccessTimeForTokenIpCombination case when there is no LastFailedAccessTime --- lib/Db/FailedLinkAccessMapper.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/Db/FailedLinkAccessMapper.php b/lib/Db/FailedLinkAccessMapper.php index b035633..e9074c0 100644 --- a/lib/Db/FailedLinkAccessMapper.php +++ b/lib/Db/FailedLinkAccessMapper.php @@ -101,6 +101,9 @@ public function getLastFailedAccessTimeForTokenIpCombination($token, $ip) { ->setMaxResults(1) ->execute() ->fetch(); + if ($lastAttempt === false) { + return 0; + } return \intval($lastAttempt['attempted_at']); } From 8764f0765df89ebad6bd8e3a507c0152cd562c82 Mon Sep 17 00:00:00 2001 From: HariBhandari Date: Tue, 17 Mar 2020 12:07:19 +0545 Subject: [PATCH 6/7] [Tests-only] Add acceptance tests to protect public share from bruteforce --- tests/acceptance/config/behat.yml | 2 ++ .../bruteforceprotection.feature | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/tests/acceptance/config/behat.yml b/tests/acceptance/config/behat.yml index fae61eb..c608984 100644 --- a/tests/acceptance/config/behat.yml +++ b/tests/acceptance/config/behat.yml @@ -25,6 +25,8 @@ default: - BruteForceProtectionContext: - IpContext: - FeatureContext: *common_feature_context_params + - OccContext: + - PublicWebDavContext: extensions: jarnaiz\JUnitFormatter\JUnitFormatterExtension: diff --git a/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature b/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature index c5ae5cf..8a234d0 100644 --- a/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature +++ b/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature @@ -87,3 +87,13 @@ Feature: brute force protection And user "Alice" sends HTTP method "GET" to URL "/remote.php/webdav/welcome.txt" with password "notvalid" And user "Alice" sends HTTP method "GET" to URL "/index.php/apps/files" Then the HTTP status code should be "403" + + Scenario: access to public link is not blocked after too many invalid requests + Given user "user1" has uploaded file with content "user1 file" to "/PARENT/randomfile.txt" + When user "user1" creates a public link share using the sharing API with settings + | path | PARENT | + | password | %public% | + And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401" + And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401" + And the public download of the last publicly shared file using the new public WebDAV API with password "123455" should fail with HTTP status code "401" + And the public should be able to download file "/randomfile.txt" from inside the last public shared folder using the new public WebDAV API with password "%public%" and the content should be "user1 file" From a11cde8d9dfec63bbe7162101f5ac7ec3e295936 Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Tue, 19 May 2020 17:03:42 +0545 Subject: [PATCH 7/7] Adjust new test for username Alice --- .../apiBruteForceProtection/bruteforceprotection.feature | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature b/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature index 8a234d0..94fc33f 100644 --- a/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature +++ b/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature @@ -89,8 +89,8 @@ Feature: brute force protection Then the HTTP status code should be "403" Scenario: access to public link is not blocked after too many invalid requests - Given user "user1" has uploaded file with content "user1 file" to "/PARENT/randomfile.txt" - When user "user1" creates a public link share using the sharing API with settings + Given user "Alice" has uploaded file with content "user1 file" to "/PARENT/randomfile.txt" + When user "Alice" creates a public link share using the sharing API with settings | path | PARENT | | password | %public% | And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401"