From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Fri, 25 Nov 2022 08:22:28 +0000 (+0100)
Subject: Fix login loop if login CSRF fails and user is not logged in
X-Git-Tag: v26.0.0beta1~54^2
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=refs%2Fpull%2F35419%2Fhead;p=nextcloud-server.git

Fix login loop if login CSRF fails and user is not logged in

If CSRF fails but the user is logged in that they probably logged in in
another tab. This is fine. We can just redirect.
If CSRF fails and the user is also not logged in then something is
fishy. E.g. because Nextcloud contantly regenrates the session and the
CSRF token and the user is stuck in an endless login loop.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
---

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index 386987842c2..ef1c1fbd94f 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -303,11 +303,23 @@ class LoginController extends Controller {
 							 string $redirect_url = null,
 							 string $timezone = '',
 							 string $timezone_offset = ''): RedirectResponse {
-		// If the user is already logged in and the CSRF check does not pass then
-		// simply redirect the user to the correct page as required. This is the
-		// case when an user has already logged-in, in another tab.
 		if (!$this->request->passesCSRFCheck()) {
-			return $this->generateRedirect($redirect_url);
+			if ($this->userSession->isLoggedIn()) {
+				// If the user is already logged in and the CSRF check does not pass then
+				// simply redirect the user to the correct page as required. This is the
+				// case when a user has already logged-in, in another tab.
+				return $this->generateRedirect($redirect_url);
+			}
+
+			// Clear any auth remnants like cookies to ensure a clean login
+			// For the next attempt
+			$this->userSession->logout();
+			return $this->createLoginFailedResponse(
+				$user,
+				$user,
+				$redirect_url,
+				$this->l10n->t('Please try again')
+			);
 		}
 
 		$data = new LoginData(
diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php
index fae20ae6b74..8a206a429bd 100644
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -511,7 +511,7 @@ class LoginControllerTest extends TestCase {
 		$this->assertEquals($expected, $this->loginController->tryLogin($user, $password));
 	}
 
-	public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() {
+	public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn(): void {
 		/** @var IUser|MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$user->expects($this->any())
@@ -524,7 +524,7 @@ class LoginControllerTest extends TestCase {
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(false);
-		$this->userSession->expects($this->once())
+		$this->userSession
 			->method('isLoggedIn')
 			->with()
 			->willReturn(false);
@@ -532,13 +532,12 @@ class LoginControllerTest extends TestCase {
 			->method('deleteUserValue');
 		$this->userSession->expects($this->never())
 			->method('createRememberMeToken');
-		$this->urlGenerator
-			->expects($this->once())
-			->method('linkToDefaultPageUrl')
-			->willReturn('/default/foo');
 
-		$expected = new RedirectResponse('/default/foo');
-		$this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl));
+		$response = $this->loginController->tryLogin('Jane', $password, $originalUrl);
+
+		$expected = new RedirectResponse('');
+		$expected->throttle(['user' => 'Jane']);
+		$this->assertEquals($expected, $response);
 	}
 
 	public function testLoginWithoutPassedCsrfCheckAndLoggedIn() {
@@ -555,7 +554,7 @@ class LoginControllerTest extends TestCase {
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(false);
-		$this->userSession->expects($this->once())
+		$this->userSession
 			->method('isLoggedIn')
 			->with()
 			->willReturn(true);
@@ -572,8 +571,10 @@ class LoginControllerTest extends TestCase {
 			->with('remember_login_cookie_lifetime')
 			->willReturn(1234);
 
+		$response = $this->loginController->tryLogin('Jane', $password, $originalUrl);
+
 		$expected = new RedirectResponse($redirectUrl);
-		$this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl));
+		$this->assertEquals($expected, $response);
 	}
 
 	public function testLoginWithValidCredentialsAndRedirectUrl() {