Skip to content

Commit

Permalink
Return a 422 from FailureApp when login fails
Browse files Browse the repository at this point in the history
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo.

By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
  • Loading branch information
ghiculescu committed Feb 3, 2021
1 parent 743b693 commit dc8cea8
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 7 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
@@ -1,6 +1,8 @@
### unreleased

* enhancements
* Devise now treats `:turbo_stream` as a navigational format. For Turbo Drive users this ensures that Devise will return a redirect, rather than a `401`, where appropriate (this may be a **breaking change**). This has no impact if you are not using [turbo-rails](https://github.com/hotwired/turbo-rails) or declaring an equivalent MIME type.
* Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with https://github.com/heartcombo/responders/pull/223, enables Devise to work correctly with the [new Rails defaults](https://github.com/rails/rails/pull/41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48). But you should check if it is required in other flows too - this may be a **breaking change**.
* Devise now enables the upgrade of OmniAuth 2+. Previously Devise would raise an error if you'd try to upgrade. Please note that OmniAuth 2 is considered a security upgrade and recommended to everyone. You can read more about the details (and possible necessary changes to your app as part of the upgrade) in [their release notes](https://github.com/omniauth/omniauth/releases/tag/v2.0.0). [Devise's OmniAuth Overview wiki](https://github.com/heartcombo/devise/wiki/OmniAuth:-Overview) was also updated to cover OmniAuth 2.0 requirements.
- Note that the upgrade required Devise shared links that initiate the OmniAuth flow to be changed to `method: :post`, which is now a requirement for OmniAuth, part of the security improvement. If you have copied and customized the Devise shared links partial to your app, or if you have other links in your app that initiate the OmniAuth flow, they will have to be updated to use `method: :post`, or changed to use buttons (e.g. `button_to`) to work with OmniAuth 2. (if you're using links with `method: :post`, make sure your app has `rails-ujs` or `jquery-ujs` included in order for these links to work properly.)
- As part of the OmniAuth 2.0 upgrade you might also need to add the [`omniauth-rails_csrf_protection`](https://github.com/cookpad/omniauth-rails_csrf_protection) gem to your app if you don't have it already. (and you don't want to roll your own code to verify requests.) Check the OmniAuth v2 release notes for more info.
Expand Down
2 changes: 1 addition & 1 deletion lib/devise.rb
Expand Up @@ -217,7 +217,7 @@ module Test

# Which formats should be treated as navigational.
mattr_accessor :navigational_formats
@@navigational_formats = ["*/*", :html]
@@navigational_formats = ["*/*", :html, :turbo_stream]

# When set to true, signing out a user signs out all other scopes.
mattr_accessor :sign_out_all_scopes
Expand Down
4 changes: 3 additions & 1 deletion lib/devise/failure_app.rb
Expand Up @@ -71,7 +71,9 @@ def recall
end

flash.now[:alert] = i18n_message(:invalid) if is_flashing_format?
self.response = recall_app(warden_options[:recall]).call(request.env)
response_from_app = recall_app(warden_options[:recall]).call(request.env)
response_from_app[0] = 422
self.response = response_from_app
end

def redirect
Expand Down
4 changes: 2 additions & 2 deletions lib/generators/templates/devise.rb
Expand Up @@ -256,14 +256,14 @@

# ==> Navigation configuration
# Lists the formats that should be treated as navigational. Formats like
# :html, should redirect to the sign in page when the user does not have
# :html should redirect to the sign in page when the user does not have
# access, but formats like :xml or :json, should return 401.
#
# If you have any extra navigational formats, like :iphone or :mobile, you
# should add them to the navigational formats lists.
#
# The "*/*" below is required to match Internet Explorer requests.
# config.navigational_formats = ['*/*', :html]
# config.navigational_formats = ['*/*', :html, :turbo_stream]

# The default HTTP method used to sign out a resource. Default is :delete.
config.sign_out_via = :delete
Expand Down
6 changes: 3 additions & 3 deletions test/controllers/sessions_controller_test.rb
Expand Up @@ -19,7 +19,7 @@ class SessionsControllerTest < Devise::ControllerTestCase
password: "wrongpassword"
}
}
assert_equal 200, @response.status
assert_equal 422, @response.status
ensure
ActiveSupport::Notifications.unsubscribe(subscriber)
end
Expand All @@ -29,7 +29,7 @@ class SessionsControllerTest < Devise::ControllerTestCase
swap Devise, scoped_views: true do
request.env["devise.mapping"] = Devise.mappings[:user]
post :create
assert_equal 200, @response.status
assert_equal 422, @response.status
assert_template "users/sessions/new"
end
end
Expand Down Expand Up @@ -70,7 +70,7 @@ class SessionsControllerTest < Devise::ControllerTestCase
password: "wevdude"
}
}
assert_equal 200, @response.status
assert_equal 422, @response.status
assert_template "devise/sessions/new"
end

Expand Down
5 changes: 5 additions & 0 deletions test/failure_app_test.rb
Expand Up @@ -326,6 +326,7 @@ def call_failure(env_params = {})
"warden" => stub_everything
}
call_failure(env)
assert_equal 422, @response.first
assert_includes @response.third.body, '<h2>Log in</h2>'
assert_includes @response.third.body, 'Invalid Email or password.'
end
Expand All @@ -337,6 +338,7 @@ def call_failure(env_params = {})
"warden" => stub_everything
}
call_failure(env)
assert_equal 422, @response.first
assert_includes @response.third.body, '<h2>Log in</h2>'
assert_includes @response.third.body, 'You have to confirm your email address before continuing.'
end
Expand All @@ -348,6 +350,7 @@ def call_failure(env_params = {})
"warden" => stub_everything
}
call_failure(env)
assert_equal 422, @response.first
assert_includes @response.third.body, '<h2>Log in</h2>'
assert_includes @response.third.body, 'Your account is not activated yet.'
end
Expand All @@ -361,6 +364,7 @@ def call_failure(env_params = {})
"warden" => stub_everything
}
call_failure(env)
assert_equal 422, @response.first
assert_includes @response.third.body, '<h2>Log in</h2>'
assert_includes @response.third.body, 'Invalid Email or password.'
assert_equal '/sample', @request.env["SCRIPT_NAME"]
Expand All @@ -375,6 +379,7 @@ def call_failure(env_params = {})
assert_equal "yes it does", Devise::FailureApp.new.lazy_loading_works?
end
end

context "Without Flash Support" do
test "returns to the default redirect location without a flash message" do
call_failure request_klass: RequestWithoutFlashSupport
Expand Down

0 comments on commit dc8cea8

Please sign in to comment.