FailureApp overwrites redirect status with Devise.responder.error_status

[devdicated]

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

fail! :multifactor_required
throw :warden, recall: 'Devise::Multifactor#redirect'

In Devise::MultifactorController

def redirect
  redirect_to multifactor_path(resource_name)
end

Environment

• Ruby 3.0
• Rails 7.0
• Devise 4.9.0

Current behavior

The 4.9.0 version of the failure app overwrites the response code from the recall with Devise.responder.error_status:

self.response = recall_app(warden_options[:recall]).call(request.env).tap { |response|
  response[0] = Rack::Utils.status_code(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.

[carlosantoniodasilva]

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: #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: #5561 (comment), 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.

[devdicated]

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.

[carlosantoniodasilva]

@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: #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!

[nov]

we're also facing to the same problem.
we do redirect in failure app.

[carlosantoniodasilva]

@nov thanks for reporting. Please feel free to give #5573 a try and see if that works for you.

[nov]

Yeah, your fix works well.

[carlosantoniodasilva]

Thanks for confirming. I'll get that merged & release a new version soon.