From 446ebd35a9813988e96dd1c38d7655119870e334 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 30 Mar 2023 15:02:51 +0200 Subject: [PATCH 1/3] fix(dav): Abort requests with 429 instead of waiting Signed-off-by: Joas Schilling --- .../composer/composer/autoload_classmap.php | 1 + .../dav/composer/composer/autoload_static.php | 1 + apps/dav/lib/Connector/Sabre/Auth.php | 6 ++ .../Sabre/Exception/TooManyRequests.php | 55 +++++++++++++++++++ lib/private/User/Session.php | 4 +- tests/lib/User/SessionTest.php | 10 ++-- 6 files changed, 70 insertions(+), 7 deletions(-) create mode 100644 apps/dav/lib/Connector/Sabre/Exception/TooManyRequests.php diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index a29e7e72327cb..0f0bdb8f14f5e 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -158,6 +158,7 @@ 'OCA\\DAV\\Connector\\Sabre\\Exception\\Forbidden' => $baseDir . '/../lib/Connector/Sabre/Exception/Forbidden.php', 'OCA\\DAV\\Connector\\Sabre\\Exception\\InvalidPath' => $baseDir . '/../lib/Connector/Sabre/Exception/InvalidPath.php', 'OCA\\DAV\\Connector\\Sabre\\Exception\\PasswordLoginForbidden' => $baseDir . '/../lib/Connector/Sabre/Exception/PasswordLoginForbidden.php', + 'OCA\\DAV\\Connector\\Sabre\\Exception\\TooManyRequests' => $baseDir . '/../lib/Connector/Sabre/Exception/TooManyRequests.php', 'OCA\\DAV\\Connector\\Sabre\\Exception\\UnsupportedMediaType' => $baseDir . '/../lib/Connector/Sabre/Exception/UnsupportedMediaType.php', 'OCA\\DAV\\Connector\\Sabre\\FakeLockerPlugin' => $baseDir . '/../lib/Connector/Sabre/FakeLockerPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\File' => $baseDir . '/../lib/Connector/Sabre/File.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 1a7e239acd1cf..33115a921c2df 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -173,6 +173,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\Exception\\Forbidden' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Exception/Forbidden.php', 'OCA\\DAV\\Connector\\Sabre\\Exception\\InvalidPath' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Exception/InvalidPath.php', 'OCA\\DAV\\Connector\\Sabre\\Exception\\PasswordLoginForbidden' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Exception/PasswordLoginForbidden.php', + 'OCA\\DAV\\Connector\\Sabre\\Exception\\TooManyRequests' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Exception/TooManyRequests.php', 'OCA\\DAV\\Connector\\Sabre\\Exception\\UnsupportedMediaType' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Exception/UnsupportedMediaType.php', 'OCA\\DAV\\Connector\\Sabre\\FakeLockerPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/FakeLockerPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\File' => __DIR__ . '/..' . '/../lib/Connector/Sabre/File.php', diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php index 1610c554b9bba..1d9952354832d 100644 --- a/apps/dav/lib/Connector/Sabre/Auth.php +++ b/apps/dav/lib/Connector/Sabre/Auth.php @@ -37,10 +37,13 @@ use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OC\Authentication\TwoFactorAuth\Manager; use OC\Security\Bruteforce\Throttler; +use OC\User\LoginException; use OC\User\Session; use OCA\DAV\Connector\Sabre\Exception\PasswordLoginForbidden; +use OCA\DAV\Connector\Sabre\Exception\TooManyRequests; use OCP\IRequest; use OCP\ISession; +use OCP\Security\Bruteforce\MaxDelayReached; use Psr\Log\LoggerInterface; use Sabre\DAV\Auth\Backend\AbstractBasic; use Sabre\DAV\Exception\NotAuthenticated; @@ -122,6 +125,9 @@ protected function validateUserPass($username, $password) { } catch (PasswordLoginForbiddenException $ex) { $this->session->close(); throw new PasswordLoginForbidden(); + } catch (MaxDelayReached $ex) { + $this->session->close(); + throw new TooManyRequests(); } } } diff --git a/apps/dav/lib/Connector/Sabre/Exception/TooManyRequests.php b/apps/dav/lib/Connector/Sabre/Exception/TooManyRequests.php new file mode 100644 index 0000000000000..1110797aed4f3 --- /dev/null +++ b/apps/dav/lib/Connector/Sabre/Exception/TooManyRequests.php @@ -0,0 +1,55 @@ + + * + * @author Joas Schilling + * + * @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 . + * + */ + +namespace OCA\DAV\Connector\Sabre\Exception; + +use DOMElement; +use Sabre\DAV\Exception\NotAuthenticated; +use Sabre\DAV\Server; + +class TooManyRequests extends NotAuthenticated { + public const NS_OWNCLOUD = 'http://owncloud.org/ns'; + + public function getHTTPCode() { + return 429; + } + + /** + * This method allows the exception to include additional information + * into the WebDAV error response + * + * @param Server $server + * @param DOMElement $errorNode + * @return void + */ + public function serialize(Server $server, DOMElement $errorNode) { + + // set ownCloud namespace + $errorNode->setAttribute('xmlns:o', self::NS_OWNCLOUD); + + $error = $errorNode->ownerDocument->createElementNS('o:', 'o:hint', 'too many requests'); + $errorNode->appendChild($error); + } +} diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 84548c4b9376c..0030e3601fcd4 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -428,7 +428,7 @@ public function logClientIn($user, IRequest $request, OC\Security\Bruteforce\Throttler $throttler) { $remoteAddress = $request->getRemoteAddress(); - $currentDelay = $throttler->sleepDelay($remoteAddress, 'login'); + $currentDelay = $throttler->sleepDelayOrThrowOnMax($remoteAddress, 'login'); if ($this->manager instanceof PublicEmitter) { $this->manager->emit('\OC\User', 'preLogin', [$user, $password]); @@ -488,7 +488,7 @@ private function handleLoginFailed(IThrottler $throttler, int $currentDelay, str $this->dispatcher->dispatchTyped(new OC\Authentication\Events\LoginFailed($user)); if ($currentDelay === 0) { - $throttler->sleepDelay($remoteAddress, 'login'); + $throttler->sleepDelayOrThrowOnMax($remoteAddress, 'login'); } } diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 4928744ed1c4c..bc916b2e5747c 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -362,7 +362,7 @@ public function testLogClientInNoTokenPasswordWith2fa() { ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('192.168.0.1'); $this->throttler ->expects($this->any()) @@ -427,7 +427,7 @@ public function testLogClientInWithTokenPassword() { ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('192.168.0.1'); $this->throttler ->expects($this->any()) @@ -472,7 +472,7 @@ public function testLogClientInNoTokenPasswordNo2fa() { ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('192.168.0.1'); $this->throttler ->expects($this->any()) @@ -1085,7 +1085,7 @@ public function testLogClientInThrottlerUsername() { ->willReturn('192.168.0.1'); $this->throttler ->expects($this->exactly(2)) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('192.168.0.1'); $this->throttler ->expects($this->any()) @@ -1135,7 +1135,7 @@ public function testLogClientInThrottlerEmail() { ->willReturn('192.168.0.1'); $this->throttler ->expects($this->exactly(2)) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('192.168.0.1'); $this->throttler ->expects($this->any()) From b2776bf58628d22b4555eb3bacd93ae1c616fce2 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Mon, 10 Jul 2023 14:28:19 +0200 Subject: [PATCH 2/3] fix: use handleLoginFailed for invalid email address Signed-off-by: Daniel Kesselberg --- lib/private/User/Session.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 0030e3601fcd4..ba0d9bbc91e26 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -458,15 +458,7 @@ public function logClientIn($user, } $users = $this->manager->getByEmail($user); if (!(\count($users) === 1 && $this->login($users[0]->getUID(), $password))) { - $this->logger->warning('Login failed: \'' . $user . '\' (Remote IP: \'' . \OC::$server->getRequest()->getRemoteAddress() . '\')', ['app' => 'core']); - - $throttler->registerAttempt('login', $request->getRemoteAddress(), ['user' => $user]); - - $this->dispatcher->dispatchTyped(new OC\Authentication\Events\LoginFailed($user)); - - if ($currentDelay === 0) { - $throttler->sleepDelay($request->getRemoteAddress(), 'login'); - } + $this->handleLoginFailed($throttler, $currentDelay, $remoteAddress, $user, $password); return false; } } From ae691e0986ea243a8a431f6758a873bd4cf3771e Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Mon, 10 Jul 2023 14:54:02 +0200 Subject: [PATCH 3/3] chore: fix codestyle for \OC\Files\Node\Folder Signed-off-by: Daniel Kesselberg --- lib/private/Files/Node/Folder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 5669f2a4b4599..beb2ac60d559e 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -326,7 +326,7 @@ protected function getAppDataDirectoryName(): string { * @param int $id * @return array */ - protected function getByIdInRootMount(int $id): array { + protected function getByIdInRootMount(int $id): array { if (!method_exists($this->root, 'createNode')) { // Always expected to be false. Being a method of Folder, this is // always implemented. For it is an internal method and should not