From c19b9ecb74ae14038fa27264cb333e9b26e1e699 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sat, 21 Dec 2024 23:04:33 +0100 Subject: [PATCH 1/3] Chore(config): Stop respecting old configuration `Spree::Config#redirect_back_on_unauthorized` has been removed in Solidus 3.0, and we're on 4.4 now. This can all go. If users have followed the deprecation warnings, then they should be fine with using `redirect_back`. Note that we're not using redirect_back in the admin, as logging out redirects to the admin root controller, which in turn redirects back, leading to an infinite redirect. Infinite redirects are no good. --- lib/spree/auth/engine.rb | 42 ++----------------- .../spree/admin/base_controller_spec.rb | 22 ---------- .../controllers/spree/base_controller_spec.rb | 4 -- 3 files changed, 4 insertions(+), 64 deletions(-) diff --git a/lib/spree/auth/engine.rb b/lib/spree/auth/engine.rb index ca546bf0..f1d4377c 100644 --- a/lib/spree/auth/engine.rb +++ b/lib/spree/auth/engine.rb @@ -26,42 +26,16 @@ class Engine < Rails::Engine Spree::Auth::Engine.prepare_frontend if SolidusSupport.frontend_available? end - def self.redirect_back_on_unauthorized? - return false unless Spree::Config.respond_to?(:redirect_back_on_unauthorized) - - if Spree::Config.redirect_back_on_unauthorized - true - else - Spree::Deprecation.warn <<-WARN.strip_heredoc, caller - Having Spree::Config.redirect_back_on_unauthorized set - to `false` is deprecated and will not be supported in Solidus 3.0. - Please change this configuration to `true` and be sure that your - application does not break trying to redirect back when there is - an unauthorized access. - WARN - - false - end - end - def self.prepare_backend Spree::Admin::BaseController.unauthorized_redirect = -> do if spree_current_user flash[:error] = I18n.t("spree.authorization_failure") - if Spree::Auth::Engine.redirect_back_on_unauthorized? - redirect_back(fallback_location: spree.admin_unauthorized_path) - else - redirect_to spree.admin_unauthorized_path - end + redirect_to(spree.admin_unauthorized_path) else store_location - if Spree::Auth::Engine.redirect_back_on_unauthorized? - redirect_back(fallback_location: spree.admin_login_path) - else - redirect_to spree.admin_login_path - end + redirect_to(spree.admin_login_path) end end end @@ -71,19 +45,11 @@ def self.prepare_frontend if spree_current_user flash[:error] = I18n.t("spree.authorization_failure") - if Spree::Auth::Engine.redirect_back_on_unauthorized? - redirect_back(fallback_location: spree.unauthorized_path) - else - redirect_to spree.unauthorized_path - end + redirect_back(fallback_location: spree.unauthorized_path) else store_location - if Spree::Auth::Engine.redirect_back_on_unauthorized? - redirect_back(fallback_location: spree.login_path) - else - redirect_to spree.login_path - end + redirect_back(fallback_location: spree.login_path) end end end diff --git a/spec/controllers/spree/admin/base_controller_spec.rb b/spec/controllers/spree/admin/base_controller_spec.rb index 15d32a89..ef38af94 100644 --- a/spec/controllers/spree/admin/base_controller_spec.rb +++ b/spec/controllers/spree/admin/base_controller_spec.rb @@ -10,10 +10,6 @@ def index end end - before do - stub_spree_preferences(Spree::Config, redirect_back_on_unauthorized: true) - end - context "when user is logged in" do before { sign_in(create(:user)) } @@ -23,15 +19,6 @@ def index expect(response).to redirect_to(spree.admin_unauthorized_path) end end - - context "when http_referrer is present" do - before { request.env["HTTP_REFERER"] = "/redirect" } - - it "redirects back" do - get :index - expect(response).to redirect_to("/redirect") - end - end end context "when user is not logged in" do @@ -41,15 +28,6 @@ def index expect(response).to redirect_to(spree.admin_login_path) end end - - context "when http_referrer is present" do - before { request.env["HTTP_REFERER"] = "/redirect" } - - it "redirects back" do - get :index - expect(response).to redirect_to("/redirect") - end - end end end end diff --git a/spec/controllers/spree/base_controller_spec.rb b/spec/controllers/spree/base_controller_spec.rb index 3a05c261..fd43af7c 100644 --- a/spec/controllers/spree/base_controller_spec.rb +++ b/spec/controllers/spree/base_controller_spec.rb @@ -10,10 +10,6 @@ def index end end - before do - stub_spree_preferences(Spree::Config, redirect_back_on_unauthorized: true) - end - context "when user is logged in" do before { sign_in(create(:user)) } From 8279db6ad1cdb14a0f5801a5c42619ae7d1ad480 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sat, 21 Dec 2024 23:23:27 +0100 Subject: [PATCH 2/3] Extract redirection behavior from lambda to classes We have two lambdas, so we need two classes. --- .../auth/unauthorized_admin_access_handler.rb | 33 +++++++++++++++++++ .../unauthorized_customer_access_handler.rb | 33 +++++++++++++++++++ lib/spree/auth/engine.rb | 20 ++--------- 3 files changed, 68 insertions(+), 18 deletions(-) create mode 100644 app/models/spree/auth/unauthorized_admin_access_handler.rb create mode 100644 app/models/spree/auth/unauthorized_customer_access_handler.rb diff --git a/app/models/spree/auth/unauthorized_admin_access_handler.rb b/app/models/spree/auth/unauthorized_admin_access_handler.rb new file mode 100644 index 00000000..11ca58f1 --- /dev/null +++ b/app/models/spree/auth/unauthorized_admin_access_handler.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Spree + module Auth + # This service object is responsible for handling unauthorized redirects + class UnauthorizedAdminAccessHandler + # @param controller [ApplicationController] an instance of ApplicationController + # or its subclasses. + def initialize(controller) + @controller = controller + end + + # This method is responsible for handling unauthorized redirects + def call + if spree_current_user + flash[:error] = I18n.t('spree.authorization_failure') + + redirect_to(spree.admin_unauthorized_path) + else + store_location + + redirect_to(spree.admin_login_path) + end + end + + private + + attr_reader :controller + + delegate :flash, :redirect_to, :spree_current_user, :store_location, :spree, to: :controller + end + end +end diff --git a/app/models/spree/auth/unauthorized_customer_access_handler.rb b/app/models/spree/auth/unauthorized_customer_access_handler.rb new file mode 100644 index 00000000..dd510690 --- /dev/null +++ b/app/models/spree/auth/unauthorized_customer_access_handler.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Spree + module Auth + # This service object is responsible for handling unauthorized redirects + class UnauthorizedCustomerAccessHandler + # @param controller [ApplicationController] an instance of ApplicationController + # or its subclasses. + def initialize(controller) + @controller = controller + end + + # This method is responsible for handling unauthorized redirects + def call + if spree_current_user + flash[:error] = I18n.t('spree.authorization_failure') + + redirect_back(fallback_location: spree.unauthorized_path) + else + store_location + + redirect_back(fallback_location: spree.login_path) + end + end + + private + + attr_reader :controller + + delegate :flash, :redirect_back, :spree_current_user, :store_location, :spree, to: :controller + end + end +end diff --git a/lib/spree/auth/engine.rb b/lib/spree/auth/engine.rb index f1d4377c..28d4cfa9 100644 --- a/lib/spree/auth/engine.rb +++ b/lib/spree/auth/engine.rb @@ -28,29 +28,13 @@ class Engine < Rails::Engine def self.prepare_backend Spree::Admin::BaseController.unauthorized_redirect = -> do - if spree_current_user - flash[:error] = I18n.t("spree.authorization_failure") - - redirect_to(spree.admin_unauthorized_path) - else - store_location - - redirect_to(spree.admin_login_path) - end + Spree::Auth::UnauthorizedAdminAccessHandler.new(self).call end end def self.prepare_frontend Spree::BaseController.unauthorized_redirect = -> do - if spree_current_user - flash[:error] = I18n.t("spree.authorization_failure") - - redirect_back(fallback_location: spree.unauthorized_path) - else - store_location - - redirect_back(fallback_location: spree.login_path) - end + Spree::Auth::UnauthorizedCustomerAccessHandler.new(self).call end end end From 5adbc7cb1fc12f68a98c84fc30632ede4b7b5570 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sat, 21 Dec 2024 23:31:40 +0100 Subject: [PATCH 3/3] Use Solidus 4.4 Redirect extension points if available This makes the `prepare_{frontend,backend}` methods which instantiate the base controller and the admin base controller, requiring all of ActionController::Base, unnecessary to call for newer Solidus versions. Depends on https://github.com/solidusio/solidus/pull/6051 --- lib/spree/auth/engine.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/spree/auth/engine.rb b/lib/spree/auth/engine.rb index 28d4cfa9..21df8b9a 100644 --- a/lib/spree/auth/engine.rb +++ b/lib/spree/auth/engine.rb @@ -21,9 +21,20 @@ class Engine < Rails::Engine Spree::Auth::Config = Spree::AuthConfiguration.new end + if Spree::Config.respond_to?(:unauthorized_redirect_handler_class) + Spree::Config.unauthorized_redirect_handler_class = "Spree::Auth::UnauthorizedCustomerAccessHandler" + if SolidusSupport.backend_available? + Spree::Backend::Config.unauthorized_redirect_handler_class = "Spree::Auth::UnauthorizedAdminAccessHandler" + end + else + config.to_prepare do + Spree::Auth::Engine.prepare_backend if SolidusSupport.backend_available? + Spree::Auth::Engine.prepare_frontend if SolidusSupport.frontend_available? + end + end + config.to_prepare do - Spree::Auth::Engine.prepare_backend if SolidusSupport.backend_available? - Spree::Auth::Engine.prepare_frontend if SolidusSupport.frontend_available? + ApplicationController.include Spree::AuthenticationHelpers end def self.prepare_backend