Correctly detect invalid username/password or blocked account login errors

Minor fix, please test review.

We should have a test to check if this issue still exists and not re-appear in the future.

After that, we should fix this. The MR needs a reroll, but first let's check the current situation.
Generally the fix LGTM, but has to be tested manually and automatically.

Done, please review!

Super nice, thank you @Grevil and @jlscott! Merged! :)

The issue still exists with 2.0.1.

Core version: 9.4.11

Steps to reproduce:
1. Login a user with wrong password for more than 5 times( exceed the threshold).
2. Now the user is blocked. at this point the core error message is still prevented.
3. Login as an admin user to unblock that user.
4. At this stage, if login as that user with wrong password, the login failed message from core is back.

Error message:

There have been more than 5 failed login attempts for this account. It is temporarily blocked. Try again later or request a new password.

Thanks @Mingsong, so reopening this again. Could you find the mistake in the code already?

Hi @Julian,

I debug further and here is what I found.

At the line 208 of login_security.module
https://git.drupalcode.org/project/login_security/-/blob/2.0.1/login_sec...

we check the error message of the user login form to figure out if there is a login failure.

The problem with it is that the Drupal user module (core module) won't give any error back to the user login form in that scenario mentioned in #13.

Instead, Drupal core will response as 403 without giving any error message back to the user login form.

See the following line in Drupal user module.
https://git.drupalcode.org/project/drupal/-/blob/9.4.11/core/modules/use...

Thank you very much @Mingsong!
No idea yet, how we can solve that.

Meanwhile, I found another problem in which the Banned host or User message never show up.

That problem comes from line 213,
https://git.drupalcode.org/project/login_security/-/blob/2.0.1/login_sec...

Because the block message was cleared by that line.

Instead, there is no any message show up, which is different from previous attempts. That is not good for preventing username enumeration.

New MR

The patch to test this MR.
https://git.drupalcode.org/issue/login_security-3292974/-/commit/d3a951b...

Upload the patch from MR21 for testing convenience.

Thank you @Mingsong, to be sure the issue happened before, but won't happen in the future, it would be best add a test, which implements the steps from #13 and #19 and fails. That should be run as test-only patch.
As part of the MR it should not fail. So we can be sure it works as expected now and in the future.

I understand, if you don't like writing tests, but without it, we can't be sure it works as expected and won't break again. That's worth it.

Can we create a follow-up issue for these problems? It's not recommended to reopen an issue where a fork already got merged. In this context it kinda makes sense, since the problem wasn't properly fixed, but it would still be great to create a second issue, so we can declutter the MRs.

Thanks @Grevil, you're totally right for the future!
As we've already come far here, let's keep this one.

Hi @Julian,

No problem. I will create a new test for it.

Patch with a new test for messages.

The new test only patch.

Obvious, the test will fail as this a test only patch without having the fix providing by MR21.

Test failed:

The text "The user ybATQwB8 has been blocked due to failed login attempts." was not found anywhere in the text of the current page.

Suuuper cool @Mingsong thank you for your effort on this!

@Grevil could you review and test this in the next days, please? If everything is fine, please create a MR to merge.

MR21 is still open.

https://git.drupalcode.org/project/login_security/-/merge_requests/21

I can confirm @Mingsong's problem while reconstructing the problem through #15. Setting this to major as this breaks the functionality of the module.

Great work @Mingsong! Works as expected!

Not sure what the "not mergeable" notice is about, inside the test output (as it clearly is mergeable), but LGTM!

@Anybody could you check my comment made inside the MR, before we are merging this?

@Grevil thanks! It's fine, I just left a comment :)

Any other points? Otherwise it it works, we can set this RTBC. I just retriggered the tests.

Alright, that's it then! Tests don't run, but let's see once they are merged! RTBC.

Queued a Drupal 9.5 test, to see if that works.

Nope, no idea, tests looks fine to me, let's see once it is in dev.

Probably got something todo with having multiple issue forks inside one issue. Oh well.

Fixed! Thanks! :)