From 70c5f4bfaf8eb8f5d9dbdcf2a3bf83ad2250ecb4 Mon Sep 17 00:00:00 2001 From: Jan Bussieck Date: Sun, 24 Feb 2019 11:55:51 +0100 Subject: [PATCH 1/2] =?UTF-8?q?Use=20warden=E2=80=99s=20`authenticated=3F`?= =?UTF-8?q?=20for=20query=20methods=20to=20avoid=20side=20effects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously checks whether a certain scope is signed in were performed using warden’s `authenticate?` or `authenticate` methods which would run the strategies and sign in the scope if valid params were given. We want to remove this side effect from query methods. --- lib/devise/controllers/helpers.rb | 4 ++-- lib/devise/controllers/sign_in_out.rb | 2 +- test/controllers/helpers_test.rb | 16 ++++++++-------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/devise/controllers/helpers.rb b/lib/devise/controllers/helpers.rb index 7ef8507f..04e0c457 100644 --- a/lib/devise/controllers/helpers.rb +++ b/lib/devise/controllers/helpers.rb @@ -53,7 +53,7 @@ module Devise def #{group_name}_signed_in? #{mappings}.any? do |mapping| - warden.authenticate?(scope: mapping) + warden.authenticated?(scope: mapping) end end @@ -119,7 +119,7 @@ module Devise end def #{mapping}_signed_in? - !!current_#{mapping} + !!(@current_#{mapping} || warden.authenticated?(scope: :#{mapping})) end def current_#{mapping} diff --git a/lib/devise/controllers/sign_in_out.rb b/lib/devise/controllers/sign_in_out.rb index 0e699b41..09ff415c 100644 --- a/lib/devise/controllers/sign_in_out.rb +++ b/lib/devise/controllers/sign_in_out.rb @@ -12,7 +12,7 @@ module Devise # authentication hooks, you can directly call `warden.authenticated?(scope: scope)` def signed_in?(scope=nil) [scope || Devise.mappings.keys].flatten.any? do |_scope| - warden.authenticate?(scope: _scope) + warden.authenticated?(scope: _scope) end end diff --git a/test/controllers/helpers_test.rb b/test/controllers/helpers_test.rb index b4850264..406226f9 100644 --- a/test/controllers/helpers_test.rb +++ b/test/controllers/helpers_test.rb @@ -16,20 +16,20 @@ class ControllerAuthenticatableTest < Devise::ControllerTestCase end test 'proxy signed_in?(scope) to authenticate?' do - @mock_warden.expects(:authenticate?).with(scope: :my_scope) + @mock_warden.expects(:authenticated?).with(scope: :my_scope) @controller.signed_in?(:my_scope) end test 'proxy signed_in?(nil) to authenticate?' do Devise.mappings.keys.each do |scope| # :user, :admin, :manager - @mock_warden.expects(:authenticate?).with(scope: scope) + @mock_warden.expects(:authenticated?).with(scope: scope) end @controller.signed_in? end test 'proxy [group]_signed_in? to authenticate? with each scope' do [:user, :admin].each do |scope| - @mock_warden.expects(:authenticate?).with(scope: scope).returns(false) + @mock_warden.expects(:authenticated?).with(scope: scope).returns(false) end @controller.commenter_signed_in? end @@ -81,7 +81,7 @@ class ControllerAuthenticatableTest < Devise::ControllerTestCase 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).returns(false) + @mock_warden.expects(:authenticated?).with(scope: scope).returns(false) end @controller.authenticate_commenter! end @@ -92,17 +92,17 @@ class ControllerAuthenticatableTest < Devise::ControllerTestCase end test 'proxy user_signed_in? to authenticate with user scope' do - @mock_warden.expects(:authenticate).with(scope: :user).returns("user") + @mock_warden.expects(:authenticated?).with(scope: :user).returns("user") assert @controller.user_signed_in? end - test 'proxy admin_signed_in? to authenticatewith admin scope' do - @mock_warden.expects(:authenticate).with(scope: :admin) + test 'proxy admin_signed_in? to authenticate with admin scope' do + @mock_warden.expects(:authenticated?).with(scope: :admin) refute @controller.admin_signed_in? end test 'proxy publisher_account_signed_in? to authenticate with namespaced publisher account scope' do - @mock_warden.expects(:authenticate).with(scope: :publisher_account) + @mock_warden.expects(:authenticated?).with(scope: :publisher_account) @controller.publisher_account_signed_in? end From 2fde07b9be18a8f14ec1b8e0d0aaa1d05433097b Mon Sep 17 00:00:00 2001 From: Jan Bussieck Date: Thu, 19 Sep 2019 14:42:51 +0200 Subject: [PATCH 2/2] Fix test case descriptions, since we are testing proxying to 'authenticated?' --- test/controllers/helpers_test.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/controllers/helpers_test.rb b/test/controllers/helpers_test.rb index 406226f9..9dd9498a 100644 --- a/test/controllers/helpers_test.rb +++ b/test/controllers/helpers_test.rb @@ -15,19 +15,19 @@ class ControllerAuthenticatableTest < Devise::ControllerTestCase assert_equal @mock_warden, @controller.warden end - test 'proxy signed_in?(scope) to authenticate?' do + test 'proxy signed_in?(scope) to authenticated?' do @mock_warden.expects(:authenticated?).with(scope: :my_scope) @controller.signed_in?(:my_scope) end - test 'proxy signed_in?(nil) to authenticate?' do + test 'proxy signed_in?(nil) to authenticated?' do Devise.mappings.keys.each do |scope| # :user, :admin, :manager @mock_warden.expects(:authenticated?).with(scope: scope) end @controller.signed_in? end - test 'proxy [group]_signed_in? to authenticate? with each scope' do + test 'proxy [group]_signed_in? to authenticated? with each scope' do [:user, :admin].each do |scope| @mock_warden.expects(:authenticated?).with(scope: scope).returns(false) end @@ -91,17 +91,17 @@ class ControllerAuthenticatableTest < Devise::ControllerTestCase @controller.authenticate_publisher_account! end - test 'proxy user_signed_in? to authenticate with user scope' do + test 'proxy user_signed_in? to authenticated? with user scope' do @mock_warden.expects(:authenticated?).with(scope: :user).returns("user") assert @controller.user_signed_in? end - test 'proxy admin_signed_in? to authenticate with admin scope' do + test 'proxy admin_signed_in? to authenticated? with admin scope' do @mock_warden.expects(:authenticated?).with(scope: :admin) refute @controller.admin_signed_in? end - test 'proxy publisher_account_signed_in? to authenticate with namespaced publisher account scope' do + test 'proxy publisher_account_signed_in? to authenticated? with namespaced publisher account scope' do @mock_warden.expects(:authenticated?).with(scope: :publisher_account) @controller.publisher_account_signed_in? end