From f22101d4213768066d3dcbde81898dd64ce46445 Mon Sep 17 00:00:00 2001
From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Fri, 25 Nov 2022 09:22:28 +0100
Subject: [PATCH] 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>
---
 core/Controller/LoginController.php           | 20 ++++++++++++++----
 tests/Core/Controller/LoginControllerTest.php | 21 ++++++++++---------
 2 files changed, 27 insertions(+), 14 deletions(-)

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() {
-- 
2.39.5