Skip to content

Commit

Permalink
Respect locale set by controller in the failure app
Browse files Browse the repository at this point in the history
A common usage of I18n with different locales is to create some around
callbcak in the application controller that sets the locale for the
entire action, via params/url/user/etc., which ensure the locale is
respected for the duration of that action, and resets at the end.

Devise was not respecting the locale when the authenticate failed and
triggered the failure app, because that happens in a warden middleware
right up in the change, by that time the controller around callback had
already reset the locale back to its default, and the failure app would
just translate flash messages using the default locale.

Now we are passing the current locale down to the failure app via warden
options, and wrapping it with an around callback, which makes the
failure app respect the set I18n locale by the controller at the time
the authentication failure is triggered, working as expected. (much more
like a normal controller would.)

I chose to introduce a callback in the failure app so we could wrap the
whole `respond` action processing rather than adding individual `locale`
options to the `I18n.t` calls, because that should ensure other possible
`I18n.t` calls from overridden failure apps would respect the set locale
as well, and makes it more like one would implement in a controller. I
don't recommend people using callbacks in their own failure apps though,
as this is not going to be documented as a "feature" of failures apps,
it's considered "internal" and could be refactored at any point.

It is possible to override the locale with the new `i18n_locale` method,
which simply defaults to the passed locale from the controller.

Closes #5247
Closes #5246

Related to: #3052, #4823, and possible others already closed.
Related to warden: (may be closed there afterwards)
wardencommunity/warden#180
wardencommunity/warden#170
  • Loading branch information
carlosantoniodasilva committed Mar 30, 2023
1 parent 506eaf4 commit ec708c9
Show file tree
Hide file tree
Showing 9 changed files with 66 additions and 8 deletions.
2 changes: 1 addition & 1 deletion app/controllers/devise/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def serialize_options(resource)
end

def auth_options
{ scope: resource_name, recall: "#{controller_path}#new" }
{ scope: resource_name, recall: "#{controller_path}#new", locale: I18n.locale }
end

def translation_scope
Expand Down
2 changes: 2 additions & 0 deletions lib/devise/controllers/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def authenticate_#{group_name}!(favorite = nil, opts = {})
mappings.unshift mappings.delete(favorite.to_sym) if favorite
mappings.each do |mapping|
opts[:scope] = mapping
opts[:locale] = I18n.locale
warden.authenticate!(opts) if !devise_controller? || opts.delete(:force)
end
end
Expand Down Expand Up @@ -115,6 +116,7 @@ def self.define_helpers(mapping) #:nodoc:
class_eval <<-METHODS, __FILE__, __LINE__ + 1
def authenticate_#{mapping}!(opts = {})
opts[:scope] = :#{mapping}
opts[:locale] = I18n.locale
warden.authenticate!(opts) if !devise_controller? || opts.delete(:force)
end
Expand Down
11 changes: 10 additions & 1 deletion lib/devise/failure_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ class FailureApp < ActionController::Metal

delegate :flash, to: :request

include AbstractController::Callbacks
around_action do |failure_app, action|
I18n.with_locale(failure_app.i18n_locale, &action)
end

def self.call(env)
@respond ||= action(:respond)
@respond.call(env)
Expand Down Expand Up @@ -107,7 +112,7 @@ def i18n_message(default = nil)
options[:default] = [message]
auth_keys = scope_class.authentication_keys
keys = (auth_keys.respond_to?(:keys) ? auth_keys.keys : auth_keys).map { |key| scope_class.human_attribute_name(key) }
options[:authentication_keys] = keys.join(I18n.translate(:"support.array.words_connector"))
options[:authentication_keys] = keys.join(I18n.t(:"support.array.words_connector"))
options = i18n_options(options)

I18n.t(:"#{scope}.#{message}", **options)
Expand All @@ -116,6 +121,10 @@ def i18n_message(default = nil)
end
end

def i18n_locale
warden_options[:locale]
end

def redirect_url
if warden_message == :timeout
flash[:timedout] = true if is_flashing_format?
Expand Down
10 changes: 5 additions & 5 deletions test/controllers/helpers_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,30 +64,30 @@ def setup
end

test 'proxy authenticate_user! to authenticate with user scope' do
@mock_warden.expects(:authenticate!).with(scope: :user)
@mock_warden.expects(:authenticate!).with(scope: :user, locale: :en)
@controller.authenticate_user!
end

test 'proxy authenticate_user! options to authenticate with user scope' do
@mock_warden.expects(:authenticate!).with(scope: :user, recall: "foo")
@mock_warden.expects(:authenticate!).with(scope: :user, recall: "foo", locale: :en)
@controller.authenticate_user!(recall: "foo")
end

test 'proxy authenticate_admin! to authenticate with admin scope' do
@mock_warden.expects(:authenticate!).with(scope: :admin)
@mock_warden.expects(:authenticate!).with(scope: :admin, locale: :en)
@controller.authenticate_admin!
end

test 'proxy authenticate_[group]! to authenticate!? with each scope' do
[:user, :admin].each do |scope|
@mock_warden.expects(:authenticate!).with(scope: scope)
@mock_warden.expects(:authenticate!).with(scope: scope, locale: :en)
@mock_warden.expects(:authenticate?).with(scope: scope).returns(false)
end
@controller.authenticate_commenter!
end

test 'proxy authenticate_publisher_account! to authenticate with namespaced publisher account scope' do
@mock_warden.expects(:authenticate!).with(scope: :publisher_account)
@mock_warden.expects(:authenticate!).with(scope: :publisher_account, locale: :en)
@controller.authenticate_publisher_account!
end

Expand Down
26 changes: 26 additions & 0 deletions test/failure_app_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ def call_failure(env_params = {})
assert_equal 'User Steve does not exist', @request.flash[:alert]
end

test 'respects the i18n locale passed via warden options when redirecting' do
call_failure('warden' => OpenStruct.new(message: :invalid), 'warden.options' => { locale: :"pt-BR" })

assert_equal 'Email ou senha inválidos.', @request.flash[:alert]
assert_equal 'http://test.host/users/sign_in', @response.second["Location"]
end

test 'uses the proxy failure message as string' do
call_failure('warden' => OpenStruct.new(message: 'Hello world'))
assert_equal 'Hello world', @request.flash[:alert]
Expand Down Expand Up @@ -284,6 +291,12 @@ def call_failure(env_params = {})
assert_match '<error>Invalid Email or password.</error>', @response.third.body
end

test 'respects the i18n locale passed via warden options when responding to HTTP request' do
call_failure('formats' => Mime[:json], 'warden' => OpenStruct.new(message: :invalid), 'warden.options' => { locale: :"pt-BR" })

assert_equal %({"error":"Email ou senha inválidos."}), @response.third.body
end

context 'on ajax call' do
context 'when http_authenticatable_on_xhr is false' do
test 'dont return 401 with navigational formats' do
Expand Down Expand Up @@ -372,6 +385,18 @@ def call_failure(env_params = {})
end
end

test 'respects the i18n locale passed via warden options when recalling original controller' do
env = {
"warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in", locale: :"pt-BR" },
"devise.mapping" => Devise.mappings[:user],
"warden" => stub_everything
}
call_failure(env)

assert_includes @response.third.body, '<h2>Log in</h2>'
assert_includes @response.third.body, 'Email ou senha inválidos.'
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
Expand Down Expand Up @@ -431,6 +456,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
9 changes: 9 additions & 0 deletions test/integration/authenticatable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,15 @@ class AuthenticationRedirectTest < Devise::IntegrationTest
assert_contain 'You need to sign in or sign up before continuing.'
end

test 'redirect from warden respects i18n locale set at the controller' do
get admins_path(locale: "pt-BR")

assert_redirected_to new_admin_session_path
follow_redirect!

assert_contain 'Para continuar, faça login ou registre-se.'
end

test 'redirect to default url if no other was configured' do
sign_in_as_user
assert_template 'home/index'
Expand Down
7 changes: 7 additions & 0 deletions test/rails_app/app/controllers/admins_controller.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
# frozen_string_literal: true

class AdminsController < ApplicationController
around_action :set_locale
before_action :authenticate_admin!

def index
end

private

def set_locale
I18n.with_locale(params[:locale] || I18n.default_locale) { yield }
end
end
5 changes: 5 additions & 0 deletions test/support/locale/pt-BR.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pt-BR:
devise:
failure:
invalid: "%{authentication_keys} ou senha inválidos."
unauthenticated: "Para continuar, faça login ou registre-se."
2 changes: 1 addition & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
require "rails/test_help"
require "orm/#{DEVISE_ORM}"

I18n.load_path << File.expand_path("../support/locale/en.yml", __FILE__)
I18n.load_path.concat Dir["#{File.dirname(__FILE__)}/support/locale/*.yml"]

require 'mocha/minitest'
require 'timecop'
Expand Down

0 comments on commit ec708c9

Please sign in to comment.