After #3198010: User login page broken when there are more than 5 failed login attempts for an account if a user fails login attempts, they are presented with the maintenance template. Although I agree with the idea of this lightware template for the purpose of Bot/Brute force, this is not very user-friendly if a maintenance page is themed separately or has messaging outside of config about site outages.
- Fail 5 logins
- Get presented with maintenance twig template
- Provide a separate theme suggestion for flood triggered maintenance pages.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | test-template.png | 737.2 KB | kolesnikoff |
| #11 | login-failed.png | 739.58 KB | kolesnikoff |
| #11 | incorrect-password.png | 724.93 KB | kolesnikoff |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #4
robbdavis commentedJust had a client stumble upon this and the custom maintenance page and text made no sense in the context of failed login. They asked for a separate message / theme for failed login.
A separate template option would be great.
As a hack, I used a path-name class in the the body and hid the maintenance stuff and displayed blocked login stuff based on whether the path was `path-login`.
Comment #6
b_sharpe commentedDone. I did not create templates for themes like Claro/Olivero here as I believe those are opinionated and should be their own issues. I also didn't create tests here as no messaging has changed.
What might be needed is a change record, as pages previously utilizing the maintenance page for this will now get a different template. Marking for review for now.
Comment #7
b_sharpe commentedIt appears there were tests around this already. Updated and added stable9 template as was expected as well.
Comment #8
smustgrave commentedNeat. Will definitely need a CR for the template addition.
Comment #9
carolpettirossi commentedI'd like to contribute with the Change Record. I've started it here: https://www.drupal.org/node/3458540
Can someone please review it?
Comment #10
b_sharpe commentedLooks great! Thanks @carolpettirossi. Leaving for someone else to RTBC as I was original committer.
Comment #11
kolesnikoff commentedI reviewed the code, looks good, no issues found.
Also tested the branch locally:
1. Created user "bruteforce".
2. Tried to log in using incorrect password. Received standard error message.
3. After 5 failed attempts received new error message
4. Created new theme "Test" based on Olivero. Copied the template failed-login-page.html.twig and changed it.
5. Cleared cache, tried to log in again. Received error page that used new template.
6. Test passed.
Comment #12
catchThe new template has identical markup to the default maintenance page template. If the intention is to allow themes to theme them differently, then we ought to be able to use template suggestions for this instead of an entirely new theme hook.
See #1804614: [meta] Consolidate similar twig templates and properly use theme suggestions in core.
This would also provide a smoother upgrade path for themes that provide a custom maintenance template but want it to be the same for the failed login page.
Comment #16
b_sharpe commented@catch good call, I've moved it to just an extension of the maintenance page which will add the theme suggestion.
Comment #17
smustgrave commentedCan IS be updated with latest solution. Not 100% sure if a change record is still needed? If so existing one will need updating.
Comment #18
b_sharpe commentedComment #19
b_sharpe commentedIS updated. I don't think a change record is needed anymore as there is no longer any changes to core implementations here, just an additional theme suggestion for those who want it.
Comment #20
smustgrave commentedGoing to agree, this seems like a much simpler solution.
Comment #21
catchWhy
floodand notloginfor the theme suggestion? Otherwise looks good though.Comment #22
b_sharpe commented@catch because I realized the template is used for two different cases, both ip flood and login failure flood, seemed to be the common name
Comment #23
smustgrave commented@catch maybe the issue summary could be updated some? Or not sure if needed
Comment #24
quietone commentedNow this is a bit interesting about the change record. There is one here but #19 says it is not needed. How will the committer (or me who follows up making sure change records are published) are to know to not publish the change record.
I think that needs to be made clear in the issue summary. Also, changing the CR as well would help. There is no set practice for this and I have seen different approaches in the change records themselves. Some people have changed the title, other have added a new first sentence is added like 'do not publish', and others delete the contents as well.
To avoid setting this back to NW I have updated the CR body to indicate that it should not be published.
Comment #25
kristiaanvandeneyndeI love the way this issue evolved and the end result is really simple and elegant. Thanks to everyone involved!
Also +1 for @quietone's concerns regarding change records, it would be nice if we could flag them for removal if throughout the lifetime of an issue it becomes apparent they are no longer needed. I couldn't find an issue for this in the issue queue, but as I was searching through it I found your comment here (on the same day as your last comment in this issue): #3384938: Publish change records when issues are fixed So I guess that issue can be used for that discussion.
Anyways, big +1 from me.
Comment #28
catchI went ahead and deleted the change record here.
Committed/pushed to 11.x and cherry-picked to 10.4.x, thanks!