From eed51179c7ac90d565f8a10847577cedd627d92b Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Mon, 20 Mar 2023 17:42:59 -0300 Subject: [PATCH 1/2] Add explicit test for respecting the `error_status` responder config While introducing this on turbo, looks like no specific test was added, so this at least covers that a bit. It needs some conditional checks since not all supported Rails + Responders version work with the customization, so there's one test for the hardcoded status version too, which can be removed in the future. --- test/failure_app_test.rb | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/failure_app_test.rb b/test/failure_app_test.rb index 883cf8b9..1500c36a 100644 --- a/test/failure_app_test.rb +++ b/test/failure_app_test.rb @@ -371,6 +371,35 @@ class FailureTest < ActiveSupport::TestCase end end end + + # TODO: remove conditional/else when supporting only responders 3.1+ + if ActionController::Responder.respond_to?(:error_status=) + test 'respects the configured responder `error_status` for the status code' do + swap Devise.responder, error_status: :unprocessable_entity do + env = { + "warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in" }, + "devise.mapping" => Devise.mappings[:user], + "warden" => stub_everything + } + call_failure(env) + + assert_equal 422, @response.first + assert_includes @response.third.body, 'Invalid Email or password.' + end + end + else + test 'uses default hardcoded responder `error_status` for the status code since responders version does not support configuring it' do + env = { + "warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in" }, + "devise.mapping" => Devise.mappings[:user], + "warden" => stub_everything + } + call_failure(env) + + assert_equal 200, @response.first + assert_includes @response.third.body, 'Invalid Email or password.' + end + end end context "Lazy loading" do From 89a08357d6e82ec907071f7714bf27358dbf868f Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Mon, 20 Mar 2023 17:59:06 -0300 Subject: [PATCH 2/2] Uses the responder `redirect_status` when recall returns a redirect It appears some people use the recall functionality with a redirect response, and Devise starting on version 4.9 was overriding that status code to the configured `error_status` for better Turbo support, which broke the redirect functionality / expectation. While I don't think it's really great usage of the recall functionality, or at least it was unexpected usage, it's been working like that basically forever where recalling would use the status code of the recalled action, so this at least keeps it more consistent with that behavior by respecting redirects and keeping that response as a redirect based on the configured status, which should also work with Turbo I believe, and makes this less of a breaking change. Closes #5570 Closes #5561 (it was closed previously, but related / closes with an actual change now.) --- CHANGELOG.md | 1 + lib/devise/failure_app.rb | 4 +++- test/failure_app_test.rb | 24 ++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22b1b763..a3a9b2eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * Allow resource class scopes to override the global configuration for `sign_in_after_reset_password` behaviour. [#5429](https://github.com/heartcombo/devise/pull/5429) [@mattr](https://github.com/mattr) * bug fixes + * Failure app will respond with configured `redirect_status` instead of `error_status` if the recall app returns a redirect status (300..399) [#5573](https://github.com/heartcombo/devise/pull/5573) * Fix frozen string exception in validatable. [#5563](https://github.com/heartcombo/devise/pull/5563) [#5465](https://github.com/heartcombo/devise/pull/5465) [@mameier](https://github.com/mameier) ### 4.9.0 - 2023-02-17 diff --git a/lib/devise/failure_app.rb b/lib/devise/failure_app.rb index d8042ec3..8458aef3 100644 --- a/lib/devise/failure_app.rb +++ b/lib/devise/failure_app.rb @@ -72,7 +72,9 @@ module Devise flash.now[:alert] = i18n_message(:invalid) if is_flashing_format? self.response = recall_app(warden_options[:recall]).call(request.env).tap { |response| - response[0] = Rack::Utils.status_code(Devise.responder.error_status) + response[0] = Rack::Utils.status_code( + response[0].in?(300..399) ? Devise.responder.redirect_status : Devise.responder.error_status + ) } end diff --git a/test/failure_app_test.rb b/test/failure_app_test.rb index 1500c36a..59f291e2 100644 --- a/test/failure_app_test.rb +++ b/test/failure_app_test.rb @@ -387,6 +387,19 @@ class FailureTest < ActiveSupport::TestCase assert_includes @response.third.body, 'Invalid Email or password.' end end + + test 'respects the configured responder `redirect_status` if the recall app returns a redirect status code' do + swap Devise.responder, redirect_status: :see_other do + env = { + "warden.options" => { recall: "devise/registrations#cancel", attempted_path: "/users/cancel" }, + "devise.mapping" => Devise.mappings[:user], + "warden" => stub_everything + } + call_failure(env) + + assert_equal 303, @response.first + end + end else test 'uses default hardcoded responder `error_status` for the status code since responders version does not support configuring it' do env = { @@ -399,6 +412,17 @@ class FailureTest < ActiveSupport::TestCase assert_equal 200, @response.first assert_includes @response.third.body, 'Invalid Email or password.' end + + test 'users default hardcoded responder `redirect_status` for the status code since responders version does not support configuring it' do + env = { + "warden.options" => { recall: "devise/registrations#cancel", attempted_path: "/users/cancel" }, + "devise.mapping" => Devise.mappings[:user], + "warden" => stub_everything + } + call_failure(env) + + assert_equal 302, @response.first + end end end