User login page broken when there are more than 5 failed login attempts for an account

That seems to have changed in #2983395: user module's flood controls should do better logging.

Confirmed that the change in theming for the flood control access denied response(s) was introduced in #2983395: user module's flood controls should do better logging.

As I mentioned there, I can't recall if the change was intentional. FWIW #2989985: User module's flood controls should do better logging [D7 backport] still produces a normal-looking themed page 403 with the flood message displayed.

I'd argue it's not necessarily "broken" to return a minimal message here; along similar lines to fast 404, in many cases the response will be returned to automated tools trying to brute force logins, in which case it's a good thing not to expend resources theming a full page. That will not always be the case though, and perhaps to a human user struggling with their password the response will look a bit broken.

If we want to change this, looks like we'll need to work out how to theme the \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException which is thrown from \Drupal\user\Controller\UserAuthenticationController::floodControl, or work out a different implementation of that 403 response.

Actually I think in this case I'm wrong about the \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException - it looks like it's this new Response we're looking at in \Drupal\user\Form\UserLoginForm::validateFinal:

      if ($flood_control_triggered = $form_state->get('flood_control_triggered')) {
        if ($flood_control_triggered == 'user') {
          $message = $this->formatPlural($flood_config->get('user_limit'), 'There has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', 'There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => Url::fromRoute('user.pass')->toString()]);
        }
        else {
          // We did not find a uid, so the limit is IP-based.
          $message = $this->t('Too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => Url::fromRoute('user.pass')->toString()]);
        }
        $form_state->setResponse(new Response($message, 403));
      }

@mcdruid Yes, that Response is what led me to #2983395: user module's flood controls should do better logging.

A few examples where core does something similar:

lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php:85:          $safe_response = new Response($message, 400);
lib/Drupal/Core/EventSubscriber/FinalExceptionSubscriber.php:129:    $response = new Response($content, 500, ['Content-Type' => $content_type]);
lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php:54:        $response = new Response('', 200, ['Allow' => implode(', ', $methods)]);
lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php:77:        $response = new Response($fast_404_html, Response::HTTP_NOT_FOUND);
lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php:117:          $response = new Response($this->getSiteMaintenanceMessage(), 503, ['Content-Type' => 'text/plain']);
modules/ban/src/BanMiddleware.php:48:      return new Response(new FormattableMarkup('@ip has been banned', ['@ip' => $ip]), 403);

I'm not sure that FormattableMarkup provides a great deal more than what we already have.

\Drupal\Core\EventSubscriber\MaintenanceModeSubscriber::onKernelRequestMaintenance might be a good example to look at.

I'm not sure if this is the approach we want, but this patch uses the bareHtmlPageRenderer to present the "too many failed logins" message as a themed maintenance page.

So that's still fairly cheap (I'd think?) and looks less "broken".

Needed to remove Symfony\Component\HttpFoundation\Response as it's no longer being used.

Can we add an assert to the existing functional test that asserts there's some markup output in this scenario

Thanks!

Good idea.

Including a test only patch which should fail without the maintenance page markup (come to think of it, might need to remove the change to core/modules/user/tests/src/Kernel/Form/UserLoginFormTest.php from the test only patch too?)

I also fixed a couple of deprecated assertion method calls within the same test per https://www.drupal.org/node/3129738 ... hopefully that's not scope creep here.

The test-only failure was as expected.

Do we need anything else?

you can use one patches from the list all are same by these patches login failed message will be show in wrapper

drupal.org/files/issues/2021-02-12/3198010-8.patch
https://www.drupal.org/files/issues/2021-02-12/3198010-9.patch
https://www.drupal.org/files/issues/2021-02-19/3198010-13.patch

There are 2 screenshot before and after apply the patch

patches apply successfully sharing screenshot

Reroll of #13 for 9.2.x and 9.3.x.

Verified and tested patch#20 on the drupal 9.3.x-dev version and Drupal 9.2.x-dev version. Patch applied successfully and looks good to me.

Testing Steps:

Install drupal 9.3.x-dev version and drupal 9.2.x-dev version.
Enter 5-time wrong password and observe the 5 failed login attempts error screen is broken.
Now apply the patch and verify that after the patch proper error message displayed on the drupal 9.3.x-dev version and Drupal 9.2.x-de version.

Testing Results:
After applying the patch on the drupal 9.3.x-dev version and Drupal 9.2.x-dev version proper 5 failed login attempts error screen displayed.

We can move this ticket to RTBC.

Same problem in D9.2.0 :

There have been more than 5 failed login attempts for this account. It is temporarily blocked....

I already make a new password request, and I can login with the link in the mail to reset the password.
But if I logoff, I cannot login again : error msg "5 failed login" ...

Thanks

I restore a D9.1.10 backup.
Same problem if I enter a bad password 5x, even after a password recovery, I cannot login again.
Thanks

Verified and tested patch #8.
Patch applied successfully and looks good to me.

Testing Steps:
# Enter 5-time wrong password and observe the 5 failed login attempts error screen is broken.
# Apply the patch and verify that after the patch proper error message displayed on the drupal 9.3.x-dev version

Expected Results:
# User login page should not be broken when there are more than 5 failed login attempts for an account

Actual Results:
# User login page broken when there are more than 5 failed login attempts for an account

Note:
As a User, I sent a new password request, and I can log in with the link in the mail to reset the password.
But if I log off, I cannot log in again
Displaying error message: error msg "5 failed login"

Not working as expected
can be a move to Needs work.

Why are you testing 8 when there are 4 newer patches since then? Also it doesn't look broken in your after screenshot.

@Didier Misson and @chetanbharambe - I think the issue you're experiencing is #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password, this issue is only about the appearance of the error message. Setting back to needs review.

same problem with 9.20
As a User, I sent a new password request, and I can log in with the link in the mail to reset the password.
But if I log off, I cannot log in again

I came to offer a review.

The Issue Summary needs and update, it makes it easier to review if the template is added. In particular, the proposed resolution needs to be there so I know what to look for. Also, it looks like this needs before and after screenshots, also to be put in the Issue Summary.

I then read through the issue. There is a fail and success patch in #13. There are screenshots in #16 but reading the comment I do not know which patch was used to make the screenshots. Followed by another set of screenshots. Given my lack of understanding of #16 and no comment in #18 I am not confident that the patch in #13 was used. Then there is a reroll and that patch is tested, in #20, there are screenshots with details of what patch was applied and the steps followed. That is useful, but should be in the Issue Summary. This is followed by comments that show confusion about what this issue is trying to solve. That brings me back to this needs an Issue Summary update.

I've added the template to the IS , hoping that helps.

I then looked at the patch and could not find a fault. At first I though changing the constructor would be a problem but then I saw that the form is @internal. I applied the patch and tested and it works as expected. The screenshots in #20 reflect my results. This is almost ready for a committer.

Setting to NW for an IS update. When that is done ping me and I'll review.

Edit: s/RTBC/review. in the last sentence

@mstrelan, thank you for updates!

This time I compared the the result here with Drupal 7. In Drupal 7 the flood message is displayed using the default theme. This patch is an improvement but we still have a regression because of the theme change. Does the comment in #8 mean that this is best that can be done or does this need a followup? Whatever the answer is I think it should be in the Issue Summary.

Thanks for the reviews and for pushing this one along.

I'm not sure whether this suggestion follows the letter of the law in the terms of the Backport Policy, but I'd argue it'd be better to backport the "cheap" theming for the flood control message to D7 than to make the D8/9/10 version more expensive.

A lot of the time these messages will be presented to bots attempting to brute force sites. For that reason, I'd suggest we think of this a bit like the fast404 functionality whereby it's best to expend as little effort as possible theming the response.

I'm happy to file and work on a backport issue for D7 where we look at using the maintenance theme for the flood control response. I'd prefer to go that route than change the approach in this issue.

Happy to hear other opinions on this, of course.

Another thought is that I've not actually measured how much "cheaper" the bareHtml / maintenance theming actually is. Maybe it'd be best to compare it with Umami rather than a bare default theme (not looked at the details of how the login page is themed / setup in either case though). I will try to have a go at this unless anyone beats me to it.

Hmm, I had a look at whether we can backport the cheap themed response to D7 and I'm not seeing an obvious way to do it cleanly.

AFAICS the closest equivalent we have to the bareHtmlPageRenderer is something like this:

https://git.drupalcode.org/project/drupal/-/blob/7.81/includes/theme.mai...

/**
 * Returns HTML for the update page.
 *
 * Note: this function is not themeable.
 *
 * @param $variables
 *   An associative array containing:
 *   - content: The page content to show.
 *   - show_messages: Whether to output status and error messages.
 *     FALSE can be useful to postpone the messages to a subsequent page.
 */
function theme_update_page($variables) {
  drupal_add_http_header('Content-Type', 'text/html; charset=utf-8');
  return theme('maintenance_page', $variables);
}

I think we'd have to jump through some hoops to do something like that when the flood control response is being finalised:

https://git.drupalcode.org/project/drupal/-/blob/7.81/modules/user/user....

...and if we did, the result would be an un-themeable page.

I don't think that calling drupal_maintenance_theme() helps; that'd typically bail out early without doing anything as the theme system will already be loaded.

So I'm not sure that a backport is viable (but will get at least a 2nd opinion on that).

In which case, I think we may be in the situation where we need to ask the D8/9/10 Framework Managers to confirm that the proposed fix here does not represent a regression because D7 presents the 403 "too many login attempts" response as a fully themed page in the default theme. (This is sort of the Backport Policy in reverse).

I think there's a strong argument to use the bareHtmlPageRenderer when flood control has been triggered by an unsuccessful login; much like fast404 the response will often not be consumed by a human and it'd be ideal if it's as cheap as possible.

We can't do the same thing in D7 easily, but it'd be a shame not to use D8/9's enhanced rendering capabilities for that reason alone.

I did have a look at some measurements of what we're actually looking at performance-wise with the "cheap" bareHtmlPageRenderer vs. a fully themed page.

Of course, to measure that properly we'd need to actually implement the 403 "more than 5 failed login attempts for this account. It is temporarily blocked" response as a fully themed page, which is what I'm arguing we should avoid doing.

So I don't have any particularly scientific measurements, but I did look at the user/login page in Umami and compare normal responses to unsuccessful POSTs to that with the response we get when login is blocked and the bareHtmlPageRenderer kicks in.

Not an especially fair comparison as the POSTs which were not blocked will have actually tried to authenticate with the password etc.. but FWIW, the response times for the POSTs to the login form averaged out to 622ms:

...and the POSTs when flood control has been triggered, with patch #20 applied so that the bareHtmlPageRenderer is used averaged 355ms:

Those numbers aren't hugely conclusive (and the variance in the 2nd set of results is a little odd), but I think they suggest that the bareHtml page is usually significantly cheaper, as we'd expect.

Finally, here's what the bareHtml response actually looks like in Umami:

I think that's okay.

Back to NR and "Needs framework manager review" tag added per previous comment.

note about BareHtmlPageRenderer and D7 added to IS

I agree that making this as cheap as possible to render as possible is a good idea, because we're likely dealing with a brute-force attempt and we don't want to expend resources on it.

Reroll of #20 for 9.3.x and 9.2.x

Removing credit for #18 which provides renamed copies of the same screenshots provided in #16

Finally back to this one.

I tested on Drupal 9.3.x, standard install, with the patch in #39. As before it worked just fine. Since the patch has changed I made new screenshots and added them to the IS. I compared the patches in #20 and #39 and the changes are to the assertions look fine. I've uploaded a diff (the interdiff failed).

We have performance numbers in #35 and agreement from larowlan on the approach. Therefor RTBC.

Random test failure, #3203712: [random test failure] EntityDisplayTest::testExtraFields()
Retesting

Tests passing, restoring RTBC.

Edit: fix typo

1. +++ b/core/modules/user/src/Form/UserLoginForm.php
   @@ -59,8 +66,10 @@ class UserLoginForm extends FormBase {
   +  public function __construct($user_flood_control, UserStorageInterface $user_storage, UserAuthInterface $user_auth, RendererInterface $renderer, BareHtmlPageRendererInterface $bare_html_renderer) {
   
   @@ -69,6 +78,7 @@ public function __construct($user_flood_control, UserStorageInterface $user_stor
   +    $this->bareHtmlPageRenderer = $bare_html_renderer;
   

   We need to do the deprecation dance. Ie. we need to do BareHtmlPageRendererInterface $bare_html_renderer = NULL is the constructor and then we need if it is NULL to get the service using \Drupal::service(). This prevents the code from breaking unnecessarily.

2. +++ b/core/modules/user/src/Form/UserLoginForm.php
   @@ -237,7 +248,9 @@ public function validateFinal(array &$form, FormStateInterface $form_state) {
   +        $response = $this->bareHtmlPageRenderer->renderBarePage(['#markup' => $message], $this->t('Login Failed'), 'maintenance_page');
   

   I think it should be 'Login failed' and not 'Login Failed'... this matches

   $response = $this->bareHtmlPageRenderer->renderBarePage(['#markup' => $this->getSiteMaintenanceMessage()], $this->t('Site under maintenance'), 'maintenance_page');
   

3. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -216,6 +216,7 @@ public function assertFailedLogin($account, $flood_trigger = NULL) {
   +        $this->assertNotEmpty($this->xpath('//body[contains(@class, :class)]', [':class' => 'maintenance-page']), 'HTML body element has the maintenance-page class.');
   
   @@ -223,6 +224,7 @@ public function assertFailedLogin($account, $flood_trigger = NULL) {
   +        $this->assertNotEmpty($this->xpath('//body[contains(@class, :class)]', [':class' => 'maintenance-page']), 'HTML body element has the maintenance-page class.');
   

   This is better expressed as $this->assertSession()->elementExists('css', 'body.maintenance-page');

Working on #45

Drafted a CR for the constructor changes here https://www.drupal.org/node/3251987

Fixes #45

Reviewed the patch. All feedback in #45 has been addressed. I think this is ready now.

Committed 09190cd and pushed to 10.0.x. Thanks!
Committed 2b83cff and pushed to 9.4.x. Thanks!

I've removed the deprecation from the 10.0.x commit because there is no point in adding it.
I've updated the release to be 9.4.0 for the deprecation because we're in release candidate stage for 9.3.0 so this fix will not land there.

This is a strange change. A client is flagging this as a bug after upgrading to D9 and I have to agree. Why would it show what looks like a broken site? The change here improves its appearance slightly over the message without any HTML at all, but really we want the full themed site back so that the user can navigate around and so that it doesn't look like the site broke.

Is there any way to get the old behaviour back? The flood warning should just be a normal message shown how all other messages are shown.

The motivation for making the error page shown when flood control has kicked in as cheap as possible is that it's typically "shown" to bots that are trying to brute force the login. Such brute force attempts often take sites down if they are intensive enough.

That said, the original change in #2983395: user module's flood controls should do better logging was motivated by other factors (i.e. the lack of useful logging when flood control kicks in) and the change to theming was not deliberate (to the extent that I can speak for everyone involved).

There's nothing to stop someone proposing a change which makes it configurable to return a fully themed page or a cheap BareHtmlPage response here. I can't say whether such a change would be accepted. I personally would not advocate for the response to be changed to a fully themed page unilaterally.

Do legitimate users often get their login wrong 5 times in a row? If so, perhaps the threshold should be increased so that they have more chances before they're blocked on those particular sites (keeping in mind the security consequences of allowing brute force attacks more attempts too).

Alternatively the theming of the maintenance / BareHtmlPage could perhaps be tweaked to be more inline with the general theme.

just_my_opinion.gif :)

I have to agree that this change is not user friendly at all. We specifically themed a maintenance page for when the site is down only to find users complaining they can't login and that the site is down (when it's not). At very minimum this should have it's own theme hook to allow for it to be separate from the maintenance template...

There should definitelly be a way / switch / hook to remove behaviour of "simple error page" and replace with normal frontend theme. The website looks broken and client accidentally stumbled upon this, and I have no solution at all.

Please re-open issue, this is not fixed at all.