Skip to content

Commit

Permalink
feat: don't count failed CSRF as failed login attempt
Browse files Browse the repository at this point in the history
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
  • Loading branch information
Altahrim committed Jul 10, 2024
1 parent b97c7df commit 916e091
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 5 deletions.
16 changes: 12 additions & 4 deletions core/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ private function setPasswordResetInitialState(?string $username): void {
$this->canResetPassword($passwordLink, $user)
);
}

/**
* Sets the initial state of whether or not a user is allowed to login with their email
* initial state is passed in the array of 1 for email allowed and 0 for not allowed
Expand Down Expand Up @@ -299,7 +299,8 @@ public function tryLogin(Chain $loginChain,
$user,
$user,
$redirect_url,
self::LOGIN_MSG_CSRFCHECKFAILED
self::LOGIN_MSG_CSRFCHECKFAILED,
false,
);
}

Expand Down Expand Up @@ -349,7 +350,12 @@ public function tryLogin(Chain $loginChain,
* @return RedirectResponse
*/
private function createLoginFailedResponse(
$user, $originalUser, $redirect_url, string $loginMessage) {
$user,
$originalUser,
$redirect_url,
string $loginMessage,
bool $throttle = true,
) {
// Read current user and append if possible we need to
// return the unmodified user otherwise we will leak the login name
$args = $user !== null ? ['user' => $originalUser, 'direct' => 1] : [];
Expand All @@ -359,7 +365,9 @@ private function createLoginFailedResponse(
$response = new RedirectResponse(
$this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)
);
$response->throttle(['user' => substr($user, 0, 64)]);
if ($throttle) {
$response->throttle(['user' => substr($user, 0, 64)]);
}
$this->session->set('loginMessages', [
[$loginMessage], []
]);
Expand Down
1 change: 0 additions & 1 deletion tests/Core/Controller/LoginControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,6 @@ public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn(): void {
$response = $this->loginController->tryLogin($loginChain, 'Jane', $password, $originalUrl);

$expected = new RedirectResponse('');
$expected->throttle(['user' => 'Jane']);
$this->assertEquals($expected, $response);
}

Expand Down

0 comments on commit 916e091

Please sign in to comment.