Allow failed logins in maintenance mode to be themed differently to other maintenance pages

Created on 25 August 2022, over 2 years ago
Updated 9 September 2024, 4 months ago

Problem/Motivation

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.

Steps to reproduce

- Fail 5 logins
- Get presented with maintenance twig template

Proposed resolution

- Provide a separate theme suggestion for flood triggered maintenance pages.

✨ Feature request
Status

Fixed

Version

10.4 ✨

Component
User systemΒ  β†’

Last updated 3 days ago

Created by

πŸ‡¨πŸ‡¦Canada b_sharpe

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¨πŸ‡¦Canada robbdavis

    Just 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`.

  • Status changed to Needs review 7 months ago
  • πŸ‡¨πŸ‡¦Canada b_sharpe

    Done. 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.

  • Pipeline finished with Failed
    7 months ago
    Total: 1489s
    #203044
  • πŸ‡¨πŸ‡¦Canada b_sharpe

    It appears there were tests around this already. Updated and added stable9 template as was expected as well.

  • Pipeline finished with Success
    7 months ago
    Total: 887s
    #203070
  • Status changed to Needs work 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Neat. Will definitely need a CR for the template addition.

  • Pipeline finished with Success
    7 months ago
    Total: 512s
    #210154
  • Status changed to Needs review 7 months ago
  • πŸ‡§πŸ‡·Brazil carolpettirossi Campinas - SP

    I'd like to contribute with the Change Record. I've started it here: https://www.drupal.org/node/3458540 β†’

    Can someone please review it?

  • πŸ‡¨πŸ‡¦Canada b_sharpe

    Looks great! Thanks @carolpettirossi. Leaving for someone else to RTBC as I was original committer.

  • Status changed to RTBC 7 months ago
  • πŸ‡ΊπŸ‡¦Ukraine kolesnikoff

    I 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.

  • Status changed to Needs work 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    The 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.

  • πŸ‡¨πŸ‡¦Canada b_sharpe

    b_sharpe β†’ changed the visibility of the branch 3306107-create-a-failed to hidden.

  • πŸ‡¨πŸ‡¦Canada b_sharpe

    b_sharpe β†’ changed the visibility of the branch 11.x to hidden.

  • Pipeline finished with Success
    6 months ago
    Total: 549s
    #234269
  • Status changed to Needs review 6 months ago
  • πŸ‡¨πŸ‡¦Canada b_sharpe

    @catch good call, I've moved it to just an extension of the maintenance page which will add the theme suggestion.

  • Status changed to Needs work 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Can IS be updated with latest solution. Not 100% sure if a change record is still needed? If so existing one will need updating.

  • Status changed to Needs review 6 months ago
  • πŸ‡¨πŸ‡¦Canada b_sharpe

    IS 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.

  • Status changed to RTBC 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Going to agree, this seems like a much simpler solution.

  • Status changed to Needs work 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Why flood and not login for the theme suggestion? Otherwise looks good though.

  • Status changed to Needs review 6 months ago
  • πŸ‡¨πŸ‡¦Canada b_sharpe

    @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

  • Status changed to RTBC 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    @catch maybe the issue summary could be updated some? Or not sure if needed

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Now 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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    I 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): ✨ Publish change records when issues are fixed Postponed So I guess that issue can be used for that discussion.

    Anyways, big +1 from me.

    • catch β†’ committed fdee3395 on 10.4.x
      Issue #3306107 by b_sharpe, catch, carolpettirossi, quietone: Allow...
    • catch β†’ committed b9161b4f on 11.x
      Issue #3306107 by b_sharpe, catch, carolpettirossi, quietone: Allow...
  • Status changed to Fixed 4 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    I went ahead and deleted the change record here.

    Committed/pushed to 11.x and cherry-picked to 10.4.x, thanks!

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024