From 913dc5c620ea71a2eb33cdb6fdb52fda7506e361 Mon Sep 17 00:00:00 2001 From: Connor Ferguson <68167430+cpfergus1@users.noreply.github.com> Date: Mon, 12 Sep 2022 08:16:16 -0600 Subject: [PATCH 1/6] Remove #redirect_back_or_default With the deprecation of #redirect_back_or_default in solidus 4.0, we can utilize Devise helpers store_location_for and stored_location_for to provide the same functionality. Co-Authored-By: Elia Schito --- .../spree/admin/user_sessions_controller.rb | 7 +----- .../spree/user_sessions_controller.rb | 7 +----- .../frontend/spree/users_controller.rb | 2 +- lib/spree/authentication_helpers.rb | 25 +++++++++++++++++++ 4 files changed, 28 insertions(+), 13 deletions(-) diff --git a/lib/controllers/backend/spree/admin/user_sessions_controller.rb b/lib/controllers/backend/spree/admin/user_sessions_controller.rb index 7fcdbb75..3478659f 100644 --- a/lib/controllers/backend/spree/admin/user_sessions_controller.rb +++ b/lib/controllers/backend/spree/admin/user_sessions_controller.rb @@ -17,7 +17,7 @@ def create respond_to do |format| format.html { flash[:success] = I18n.t('spree.logged_in_succesfully') - redirect_back_or_default(after_sign_in_path_for(spree_current_user)) + redirect_to stored_spree_user_location_or(after_sign_in_path_for(spree_current_user)) } format.js { user = resource.record @@ -47,9 +47,4 @@ def set_user_language_locale_key def accurate_title I18n.t('spree.login') end - - def redirect_back_or_default(default) - redirect_to(session["spree_user_return_to"] || default) - session["spree_user_return_to"] = nil - end end diff --git a/lib/controllers/frontend/spree/user_sessions_controller.rb b/lib/controllers/frontend/spree/user_sessions_controller.rb index dfa67d99..9927b884 100644 --- a/lib/controllers/frontend/spree/user_sessions_controller.rb +++ b/lib/controllers/frontend/spree/user_sessions_controller.rb @@ -19,7 +19,7 @@ def create respond_to do |format| format.html do flash[:success] = I18n.t('spree.logged_in_succesfully') - redirect_back_or_default(after_sign_in_path_for(spree_current_user)) + redirect_to stored_spree_user_location_or(after_sign_in_path_for(spree_current_user)) end format.js { render success_json } end @@ -49,11 +49,6 @@ def accurate_title I18n.t('spree.login') end - def redirect_back_or_default(default) - redirect_to(session["spree_user_return_to"] || default) - session["spree_user_return_to"] = nil - end - def success_json { json: { diff --git a/lib/controllers/frontend/spree/users_controller.rb b/lib/controllers/frontend/spree/users_controller.rb index b6f31b45..8eda3578 100644 --- a/lib/controllers/frontend/spree/users_controller.rb +++ b/lib/controllers/frontend/spree/users_controller.rb @@ -17,7 +17,7 @@ def create session[:guest_token] = nil end - redirect_back_or_default(root_url) + redirect_to stored_spree_user_location_or(root_url) else render :new end diff --git a/lib/spree/authentication_helpers.rb b/lib/spree/authentication_helpers.rb index d62a20c4..93b6051e 100644 --- a/lib/spree/authentication_helpers.rb +++ b/lib/spree/authentication_helpers.rb @@ -23,5 +23,30 @@ def spree_current_user to: :spree, prefix: :spree end + + private + + def authenticate_spree_user! + store_spree_user_location! if storable_spree_user_location? + + super + end + + # It's important that the location is NOT stored if: + # - The request method is not GET (non idempotent) + # - The request is handled by a Devise controller such as Devise::SessionsController as that could cause an + # infinite redirect loop. + # - The request is an Ajax request as this can lead to very unexpected behaviour. + def storable_spree_user_location? + request.get? && is_navigational_format? && !devise_controller? && !request.xhr? + end + + def store_spree_user_location! + store_location_for(:spree_current_user, request.fullpath) + end + + def stored_spree_user_location_or(fallback_location) + stored_location_for(:spree_current_user) || fallback_location + end end end From dc7bca00e37622a1a1d4d3193a00cffc729956c9 Mon Sep 17 00:00:00 2001 From: Connor Ferguson <68167430+cpfergus1@users.noreply.github.com> Date: Tue, 13 Sep 2022 10:57:10 -0600 Subject: [PATCH 2/6] Remove extra space --- lib/spree/auth/engine.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/spree/auth/engine.rb b/lib/spree/auth/engine.rb index 7c60ccf8..8d249c2a 100644 --- a/lib/spree/auth/engine.rb +++ b/lib/spree/auth/engine.rb @@ -70,7 +70,6 @@ def self.prepare_backend end end - def self.prepare_frontend Spree::BaseController.unauthorized_redirect = -> do if spree_current_user From 13230c5bb8e02d985c25555bb3d4cb20e0c71142 Mon Sep 17 00:00:00 2001 From: Connor Ferguson <68167430+cpfergus1@users.noreply.github.com> Date: Wed, 14 Sep 2022 10:04:25 -0600 Subject: [PATCH 3/6] Replace Store Location with store_spree_user_location This method was utilized for redirect_back_or_default. It is no longer required but we still need to store a location if we get to this point --- .../controllers/spree/checkout_controller_decorator.rb | 2 +- lib/spree/auth/engine.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/decorators/frontend/controllers/spree/checkout_controller_decorator.rb b/lib/decorators/frontend/controllers/spree/checkout_controller_decorator.rb index a0f55a19..96e3dba8 100644 --- a/lib/decorators/frontend/controllers/spree/checkout_controller_decorator.rb +++ b/lib/decorators/frontend/controllers/spree/checkout_controller_decorator.rb @@ -45,7 +45,7 @@ def check_authorization def check_registration return unless registration_required? - store_location + store_spree_user_location! redirect_to spree.checkout_registration_path end diff --git a/lib/spree/auth/engine.rb b/lib/spree/auth/engine.rb index 8d249c2a..694c6a44 100644 --- a/lib/spree/auth/engine.rb +++ b/lib/spree/auth/engine.rb @@ -59,7 +59,7 @@ def self.prepare_backend redirect_to spree.admin_unauthorized_path end else - store_location + store_spree_user_location! if Spree::Auth::Engine.redirect_back_on_unauthorized? redirect_back(fallback_location: spree.admin_login_path) @@ -81,7 +81,7 @@ def self.prepare_frontend redirect_to spree.unauthorized_path end else - store_location + store_spree_user_location! if Spree::Auth::Engine.redirect_back_on_unauthorized? redirect_back(fallback_location: spree.login_path) From badc6fc6b1380a63ae56833576a37bebdac4d28a Mon Sep 17 00:00:00 2001 From: Connor Ferguson <68167430+cpfergus1@users.noreply.github.com> Date: Thu, 22 Sep 2022 10:38:37 -0600 Subject: [PATCH 4/6] Authenticate User before action Previously we would fall back on authorization to determine if a user should be able to access a certain endpoint. We should be authenticating the user first prior to checking if the specific user is authorized to access a certain endpoint Co-Authored-By: Elia Schito --- .../spree/admin/base_controller_decorator.rb | 6 ++++++ .../controllers/spree/users_controller_decorator.rb | 13 +++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 lib/decorators/frontend/controllers/spree/users_controller_decorator.rb diff --git a/lib/decorators/backend/controllers/spree/admin/base_controller_decorator.rb b/lib/decorators/backend/controllers/spree/admin/base_controller_decorator.rb index f0017543..58eee5a1 100644 --- a/lib/decorators/backend/controllers/spree/admin/base_controller_decorator.rb +++ b/lib/decorators/backend/controllers/spree/admin/base_controller_decorator.rb @@ -3,6 +3,12 @@ module Spree module Admin module BaseControllerDecorator + def self.prepended(base) + base.class_eval do + before_action :authenticate_spree_user! + end + end + protected def model_class diff --git a/lib/decorators/frontend/controllers/spree/users_controller_decorator.rb b/lib/decorators/frontend/controllers/spree/users_controller_decorator.rb new file mode 100644 index 00000000..97c50ffc --- /dev/null +++ b/lib/decorators/frontend/controllers/spree/users_controller_decorator.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Spree + module UsersControllerDecorator + def self.prepended(base) + base.class_eval do + before_action :authenticate_spree_user!, except: [:new, :create] + end + end + + ::Spree::UsersController.prepend self + end +end From b0158b84dfd5a0e7f683eff5248abda2b0f7b941 Mon Sep 17 00:00:00 2001 From: Connor Ferguson <68167430+cpfergus1@users.noreply.github.com> Date: Thu, 29 Sep 2022 11:19:22 -0600 Subject: [PATCH 5/6] Update session routes When failing authentication, the devise route would send the user to "new_spree_user_session_path", however we want our users to be redirected to "/login." This commit deprecates the route and sends users to "/login." --- config/routes.rb | 19 ++++++++++++++++--- .../spree/user_passwords_controller_spec.rb | 2 +- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 39613b7d..84a99d49 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -10,7 +10,7 @@ passwords: 'spree/user_passwords', confirmations: 'spree/user_confirmations' }, - skip: [:unlocks, :omniauth_callbacks], + skip: [:unlocks, :omniauth_callbacks, :sessions], path_names: { sign_out: 'logout' }, path_prefix: :user, router_name: :spree @@ -19,8 +19,21 @@ resources :users, only: [:edit, :update] devise_scope :spree_user do - get '/login', to: 'user_sessions#new', as: :login - post '/login', to: 'user_sessions#create', as: :create_new_session + # Legacy devise generated paths + # These are deprecated but we still want to support the incoming routes, + # in order to give existing stores an upgrade path. + get 'user/spree_user/sign_in', to: 'user_sessions#new', as: nil + post 'user/spree_user/sign_in', to: 'user_sessions#create', as: nil + + # Custom Devise routes + [:new_spree_user_session, :login].each do |helper| + get '/login', to: 'user_sessions#new', as: helper + end + + [:spree_user_session, :create_new_session].each do |helper| + post '/login', to: 'user_sessions#create', as: helper + end + match '/logout', to: 'user_sessions#destroy', as: :logout, via: Devise.sign_out_via get '/signup', to: 'user_registrations#new', as: :signup post '/signup', to: 'user_registrations#create', as: :registration diff --git a/spec/controllers/spree/user_passwords_controller_spec.rb b/spec/controllers/spree/user_passwords_controller_spec.rb index 2165a22c..056727e9 100644 --- a/spec/controllers/spree/user_passwords_controller_spec.rb +++ b/spec/controllers/spree/user_passwords_controller_spec.rb @@ -10,7 +10,7 @@ it 'redirects to the new session path' do get :edit expect(response).to redirect_to( - 'http://test.host/user/spree_user/sign_in' + 'http://test.host/login' ) end From fcd8efff7eeb814cc776201c2ea8ee8d195f5397 Mon Sep 17 00:00:00 2001 From: Connor Ferguson <68167430+cpfergus1@users.noreply.github.com> Date: Fri, 30 Sep 2022 06:00:31 -0600 Subject: [PATCH 6/6] Use spree_user instead of spree_current_user Devise utilizes spree_user scope when executing after_sign_in_path. This is an important distinction when storing the user location as the user would not be redirected as expected if we use spree_current_user. Co-Authored-By: Elia Schito --- lib/spree/authentication_helpers.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/spree/authentication_helpers.rb b/lib/spree/authentication_helpers.rb index 93b6051e..cda58643 100644 --- a/lib/spree/authentication_helpers.rb +++ b/lib/spree/authentication_helpers.rb @@ -42,11 +42,11 @@ def storable_spree_user_location? end def store_spree_user_location! - store_location_for(:spree_current_user, request.fullpath) + store_location_for(:spree_user, request.fullpath) end def stored_spree_user_location_or(fallback_location) - stored_location_for(:spree_current_user) || fallback_location + stored_location_for(:spree_user) || fallback_location end end end