Closed lcmen closed 1 year ago
It seems extremely uncommon (to my knowledge, at least) to have the new
action redirect there... that's generally used to present a form to sign in, which is why the recall
logic calls it again... it's essentially doing a render :new
call in that context. Doesn't your app have a view being displayed in the new
action?
The controller was never meant to handle the response status of authentication failures. Ideally to redirect after authentication failures, you'd have to create a custom failure app and override recall to simply redirect. In other words, a method like this in a custom failure app:
def recall
redirect
end
or alternatively/more broadly
def respond
redirect
end
This would ensure recall does a redirect... you do lose some things like the flash message set by Devise though, but you could potentially add that yourself too.
Version 4.8.1 FailureApp#recall relied on response statuses from controllers.
This was a bit of an unintended side-effect of how it was implemented, but not necessarily meant to be like that, since the failure app should be the one in control of handling the authentication failure.
response statues should be changed in controllers to make them Turbo compatible.
You shouldn't have to change the response status in the controller new
action for a failure to submit the session/login form... for example, in a "normal" Rails create
action, you'd have a simple if..else
controlling the responses, e.g.:
if something_successful
redirect_to some_path
else
render :new, status: :unprocessable_entity
end
This is essentially what Devise is trying to reproduce, but via Warden and a failure app there. So the status is controlled via the failure app. Redirecting in the else
block does work for Turbo as well, but you might lose some context in the process.
redirect_to 'some url' # this should redirect with normal 302 status, instead of being rewritten to 200
It's also not required to redirect
to make this work with Turbo, you can respond with a 422 to have Turbo re-render the HTML that's being sent back... this is what Devise + responders are doing, if you are setting up the statuses as outlined in the changelog. Please note that the status is a change not only for authenticating, but a few other actions too that need to respond similarly for Turbo.
I would be willing to consider not overriding the response status if we get back a redirect on recall, but it does seem like a very unexpected / unique use case. Would using the more standard config responding with 422 instead of redirecting work for you?
@carlosantoniodasilva thank you for detailed reply.
Actually I re-thought my design and I was able to find a workaround. I decided to use Devise.responder.error_status
to set redirect status (:found
) globally and in a single controller that need to behave differently, I defined custom responder. It's simpler than overriding FailureApp
.
@lcmen okay. Just keep in mind that this will be the response for places like a failed reset password or confirmation form submission as well, but may not contain a redirect location, not sure if it could have other unintended side-effects for your use case. Thanks!
@carlosantoniodasilva yes, I'm aware of that.
Since we have multi-steps login form in our app, redirects suit us better. For a password reset, I already have custom controller (for small customization) so I just defined different responder.
@carlosantoniodasilva thanks once again for your help! I really appreciate it!
Hey @lcmen , just a heads up that in the end I'm making a change to respect redirect statuses (based on the configured option too) here: https://github.com/heartcombo/devise/pull/5573. There was another completely different use case but that also relied on the redirect status being set from the recall flow, so I thought this change would make sense and be less of a "breaking" one.
Hey @carlosantoniodasilva , thank you for getting back to me with the update! It's good too see this change being implemented, it will definitely help me with my usecase (I had to make a few more changes than I initially thought).
Environment
Current behavior
Version 4.8.1
FailureApp#recall
relied on response statuses from controllers. Version 4.9.0 overrides response status withDevise.responder.error_status
. This leads to redirects with 200 code when controller's action responds withredirect
afterauthenticate
call fails.Sudo code:
Expected behavior
Devise::FailureApp#recall
should not override response's statuses asrecall_app
executes normal controller's action so response statues should be changed in controllers to make them Turbo compatible.