Closed devdicated closed 1 year ago
That's interesting, I guess we never considered recall being used like that.
There was someone else who stumbled upon this change due to another usage with redirect here: https://github.com/heartcombo/devise/issues/5561, they were actually redirecting from the controller (rather than explicitly recalling like you did) and expecting recall to honor it. I guess we could possibly set the response to one or the other configured status, depending on what the recall app gives us back. (e.g. if it's a 3xx we use the configured redirect_status
, otherwise error_status
.)
Is the Devise::Multifactor
goal only to call that redirect? (i.e. it's a separate custom app to do it?) Is there more custom code to it, like do you need a flash message or something to show up? Because if not, I wonder if you could use the redirect!
code from Warden directly. I have never used it myself, but in theory if you call it and then throw :warden
, it should work if all you need is a redirect from within the custom strategy.
If the response code is a redirect, don't overwrite the status or use Devise.responder.redirect_status.
Yeah this is inline with what I mentioned above, I could see a change like this happening possibly.
it seems to me that a redirect status from the recall app should never be overwritten with a client error status.
It needs to, in order to be fully compatible with Turbo (ensuring a compatible response, I mean). I tried explaining a little bit about the reasoning in that other issue: https://github.com/heartcombo/devise/issues/5561#issuecomment-1436891271, but the overall idea of recalling was to simulate re-rendering the form with the authentication failure, thus responding with an error status like any other Rails controller would do.
We use fail! :multifactor_required
to trigger a flash message with the redirect. I've looked into the warden redirect when I was implementing is but I couldn't quickly figure out how to use the named routes from warden so recall
was the easiest option.
I could set up a PR which uses either redirect_status
or error_status
depending on the recall status if you think that's the way to go.
@devdicated I see, yeah I'm not sure off the top of my head how to make flash + warden redirect work, there's probably some incantation, I'll give it some thought, but what you explained makes sense.
I could set up a PR which uses either redirect_status or error_status depending on the recall status if you think that's the way to go.
Sorry I missed your message previously, I was working on making that change here: https://github.com/heartcombo/devise/pull/5573. Hopefully that's all we need to have redirects when recalling the app / action working, making this less of a "breaking" change. Let me know what you think. (or if you have an opportunity to test it) Thanks!
we're also facing to the same problem. we do redirect in failure app.
@nov thanks for reporting. Please feel free to give #5573 a try and see if that works for you.
Yeah, your fix works well.
Thanks for confirming. I'll get that merged & release a new version soon.
Hi, I use the devise FailureApp to redirect to a multifactor authentication page if the user has set up their account with MFA. The following code works as expected in 4.8:
In the strategy
In Devise::MultifactorController
Environment
Current behavior
The 4.9.0 version of the failure app overwrites the response code from the recall with
Devise.responder.error_status
:Expected behavior
If the response code is a redirect, don't overwrite the status or use
Devise.responder.redirect_status
. It could be that I am misunderstanding or misusing the Failure app, but it seems to me that a redirect status from the recall app should never be overwritten with a client error status.