Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows multiple messages #5764

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zedtux
Copy link
Contributor

@zedtux zedtux commented Feb 10, 2025

So far Devise models are using method overloading, or monkey-patching in order to provide the right flash message to be shown to the user informing about the authentication failure reason.

While this work fine, it's hard to understand and to debug (like in which order are executed the functions?), plus it prevent one little case to work : when you're reaching the lockable last_attempt, before your account is locked, you only see one flash message while there should be actually two.

All that was fine and nice until we added our own model (with a hook) to add exponential delay between login failures (quite common security request) showing a flash message with the delay to be waited before retrying a login attempt and being at the lockable last_attempt step : a flash message was missing.

This PR update Devise so that:

  • it uses the future feature of warden to manage multiple messages
  • it adapts Models to push messages to the Models.devise_messages array
  • it passes the Models.devise_messages the warden fail! method
  • it adapts test suite to expect messages instead of message
  • it propose a Earthfile to easily run test suite on any platform

To be done

  • open discussion about this change (Devise team feedback about this change, earthly, would you like some deprecation warnings,..?)
  • discuss Earthly usage locally and/or from Github actions
  • adapt the inactive_message case in order to push message to Models.devise_messages instead
  • review the lockable test which still assert on the unauthenticated_message method
  • increase Earthfile VERSION

@zedtux zedtux force-pushed the features/allow-multiple-error-messages branch from 7fcd763 to fe5c48d Compare February 11, 2025 08:48
This commit adapts Devise to support multiple flash messages from the Models, utilizing the Warden multi-messages feature.
@zedtux zedtux force-pushed the features/allow-multiple-error-messages branch from fe5c48d to 6ecf1b4 Compare February 11, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant