upstream/nextcloud
1
0
Fork 0
mirror of https://github.com/nextcloud/server.git synced 2026-06-10 09:13:19 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 54

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

Browse source 
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>
This commit is contained in:
Christoph Wurst 2022-11-25 09:22:28 +01:00 committed by backportbot-nextcloud[bot]
parent d2984ed076
commit 197a2ea1f9
2 changed files with 27 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
core/Controller/LoginController.php
View file

@ -300,11 +300,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(

21
tests/Core/Controller/LoginControllerTest.php
View file

@ -498,7 +498,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())
@ -511,7 +511,7 @@ class LoginControllerTest extends TestCase {
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(false);
$this->userSession->expects($this->once())
$this->userSession
->method('isLoggedIn')
->with()
->willReturn(false);
@ -519,13 +519,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() {
@ -542,7 +541,7 @@ class LoginControllerTest extends TestCase {
->expects($this->once())
->method('passesCSRFCheck')
->willReturn(false);
$this->userSession->expects($this->once())
$this->userSession
->method('isLoggedIn')
->with()
->willReturn(true);
@ -559,8 +558,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() {