From b9baa624bc80db613839affe02a7a1ee01bf056f 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 b68f91f986e92..6df88726afc4d 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -311,11 +311,23 @@ public function tryLogin(string $user,
 							 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 30a625a612b68..9884255a80e69 100644
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -500,7 +500,7 @@ public function testLoginWithValidCredentials() {
 		$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())
@@ -513,7 +513,7 @@ public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() {
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(false);
-		$this->userSession->expects($this->once())
+		$this->userSession
 			->method('isLoggedIn')
 			->with()
 			->willReturn(false);
@@ -521,13 +521,12 @@ public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() {
 			->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() {
@@ -544,7 +543,7 @@ public function testLoginWithoutPassedCsrfCheckAndLoggedIn() {
 			->expects($this->once())
 			->method('passesCSRFCheck')
 			->willReturn(false);
-		$this->userSession->expects($this->once())
+		$this->userSession
 			->method('isLoggedIn')
 			->with()
 			->willReturn(true);
@@ -561,8 +560,10 @@ public function testLoginWithoutPassedCsrfCheckAndLoggedIn() {
 			->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() {