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 386987842c2ebab97a56d406dd21cefc3612e484..ef1c1fbd94f5076ce77134e13b61b87f60377b02 100644 (file)
--- 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 fae20ae6b741be14c41b5297f04b7de6df75700d..8a206a429bd3b24133b1988ca79b6bcfce1142d9 100644 (file)
--- 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() {