Skip to content

Commit

Permalink
Uses the responder redirect_status when recall returns a redirect
Browse files Browse the repository at this point in the history
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.)
  • Loading branch information
carlosantoniodasilva committed Mar 20, 2023
1 parent eed5117 commit 73e2a61
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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
Expand Down
4 changes: 3 additions & 1 deletion lib/devise/failure_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ def recall

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

Expand Down
24 changes: 24 additions & 0 deletions test/failure_app_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,19 @@ def call_failure(env_params = {})
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 = {
Expand All @@ -399,6 +412,17 @@ def call_failure(env_params = {})
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

Expand Down

0 comments on commit 73e2a61

Please sign in to comment.