Fix Location Storage

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

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

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: {

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

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

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
 

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

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)
@@ -70,7 +70,6 @@ def self.prepare_backend
         end
       end
 
-
       def self.prepare_frontend
         Spree::BaseController.unauthorized_redirect = -> do
           if spree_current_user
@@ -82,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)

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_user, request.fullpath)
+    end
+
+    def stored_spree_user_location_or(fallback_location)
+      stored_location_for(:spree_user) || fallback_location
+    end
   end
 end

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