From cc1b616f241052e909ade6edd2c48e96bdd28f33 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Tue, 21 Feb 2023 22:45:37 +0100 Subject: [PATCH] chore: use local variable for remote address Signed-off-by: Daniel Kesselberg --- lib/private/User/Session.php | 16 +++++- tests/lib/User/SessionTest.php | 97 ++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index acf95b1e27125..84548c4b9376c 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -59,6 +59,7 @@ use OCP\IUser; use OCP\IUserSession; use OCP\Lockdown\ILockdownManager; +use OCP\Security\Bruteforce\IThrottler; use OCP\Security\ISecureRandom; use OCP\Session\Exceptions\SessionNotAvailableException; use OCP\User\Events\PostLoginEvent; @@ -426,7 +427,8 @@ public function logClientIn($user, $password, IRequest $request, OC\Security\Bruteforce\Throttler $throttler) { - $currentDelay = $throttler->sleepDelay($request->getRemoteAddress(), 'login'); + $remoteAddress = $request->getRemoteAddress(); + $currentDelay = $throttler->sleepDelay($remoteAddress, 'login'); if ($this->manager instanceof PublicEmitter) { $this->manager->emit('\OC\User', 'preLogin', [$user, $password]); @@ -451,6 +453,7 @@ public function logClientIn($user, // Failed, maybe the user used their email address if (!filter_var($user, FILTER_VALIDATE_EMAIL)) { + $this->handleLoginFailed($throttler, $currentDelay, $remoteAddress, $user, $password); return false; } $users = $this->manager->getByEmail($user); @@ -478,6 +481,17 @@ public function logClientIn($user, return true; } + private function handleLoginFailed(IThrottler $throttler, int $currentDelay, string $remoteAddress, string $user, ?string $password) { + $this->logger->warning("Login failed: '" . $user . "' (Remote IP: '" . $remoteAddress . "')", ['app' => 'core']); + + $throttler->registerAttempt('login', $remoteAddress, ['user' => $user]); + $this->dispatcher->dispatchTyped(new OC\Authentication\Events\LoginFailed($user)); + + if ($currentDelay === 0) { + $throttler->sleepDelay($remoteAddress, 'login'); + } + } + protected function supportsCookies(IRequest $request) { if (!is_null($request->getCookie('cookie_test'))) { return true; diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 735a3b3d06a8b..4928744ed1c4c 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -9,6 +9,7 @@ namespace Test\User; use OC\AppFramework\Http\Request; +use OC\Authentication\Events\LoginFailed; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OC\Authentication\Token\IProvider; @@ -1057,4 +1058,100 @@ public function testUpdateTokens() { $this->userSession->updateTokens('uid', 'pass'); } + + public function testLogClientInThrottlerUsername() { + $manager = $this->createMock(Manager::class); + $session = $this->createMock(ISession::class); + $request = $this->createMock(IRequest::class); + + /** @var Session $userSession */ + $userSession = $this->getMockBuilder(Session::class) + ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher]) + ->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser']) + ->getMock(); + + $userSession->expects($this->once()) + ->method('isTokenPassword') + ->willReturn(true); + $userSession->expects($this->once()) + ->method('login') + ->with('john', 'I-AM-AN-PASSWORD') + ->willReturn(false); + + $session->expects($this->never()) + ->method('set'); + $request + ->method('getRemoteAddress') + ->willReturn('192.168.0.1'); + $this->throttler + ->expects($this->exactly(2)) + ->method('sleepDelay') + ->with('192.168.0.1'); + $this->throttler + ->expects($this->any()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(0); + + $this->throttler + ->expects($this->once()) + ->method('registerAttempt') + ->with('login', '192.168.0.1', ['user' => 'john']); + $this->dispatcher + ->expects($this->once()) + ->method('dispatchTyped') + ->with(new LoginFailed('john', 'I-AM-AN-PASSWORD')); + + $this->assertFalse($userSession->logClientIn('john', 'I-AM-AN-PASSWORD', $request, $this->throttler)); + } + + public function testLogClientInThrottlerEmail() { + $manager = $this->createMock(Manager::class); + $session = $this->createMock(ISession::class); + $request = $this->createMock(IRequest::class); + + /** @var Session $userSession */ + $userSession = $this->getMockBuilder(Session::class) + ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher]) + ->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser']) + ->getMock(); + + $userSession->expects($this->once()) + ->method('isTokenPassword') + ->willReturn(true); + $userSession->expects($this->once()) + ->method('login') + ->with('john@foo.bar', 'I-AM-AN-PASSWORD') + ->willReturn(false); + $manager + ->method('getByEmail') + ->with('john@foo.bar') + ->willReturn([]); + + $session->expects($this->never()) + ->method('set'); + $request + ->method('getRemoteAddress') + ->willReturn('192.168.0.1'); + $this->throttler + ->expects($this->exactly(2)) + ->method('sleepDelay') + ->with('192.168.0.1'); + $this->throttler + ->expects($this->any()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(0); + + $this->throttler + ->expects($this->once()) + ->method('registerAttempt') + ->with('login', '192.168.0.1', ['user' => 'john@foo.bar']); + $this->dispatcher + ->expects($this->once()) + ->method('dispatchTyped') + ->with(new LoginFailed('john@foo.bar', 'I-AM-AN-PASSWORD')); + + $this->assertFalse($userSession->logClientIn('john@foo.bar', 'I-AM-AN-PASSWORD', $request, $this->throttler)); + } }