From 9f49a84e7f04639c824915ecf9495091af6ad22c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 16 Dec 2021 13:35:55 +0000 Subject: [PATCH 1/3] Clarify use of access tokens used for viewing order details as a guest user There are 4 or 5 different places in the app where we reference a :token and params[:token] for completely different purposes (they're not even vaguely the *same* token). This is an attempt to clarify the places in the app where we use params[:token] in relation to *orders*, for allowing guest users (who are not logged in) to view details of an order they have placed (like after checkout completion), and differentiate it from the various other places where params[:token] can actually be used for something entirely different! --- app/controllers/cart_controller.rb | 2 +- app/controllers/payment_gateways/paypal_controller.rb | 2 +- app/controllers/spree/orders_controller.rb | 4 ++-- spec/controllers/spree/orders_controller_spec.rb | 4 ++-- spec/requests/checkout/paypal_spec.rb | 2 +- spec/system/consumer/shopping/orders_spec.rb | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/cart_controller.rb b/app/controllers/cart_controller.rb index 755b994a4c..c8d246ee21 100644 --- a/app/controllers/cart_controller.rb +++ b/app/controllers/cart_controller.rb @@ -28,7 +28,7 @@ class CartController < BaseController end def check_authorization - session[:access_token] ||= params[:token] + session[:access_token] ||= params[:order_token] order = Spree::Order.find_by(number: params[:id]) || current_order if order diff --git a/app/controllers/payment_gateways/paypal_controller.rb b/app/controllers/payment_gateways/paypal_controller.rb index a912e0b624..deb0c1a885 100644 --- a/app/controllers/payment_gateways/paypal_controller.rb +++ b/app/controllers/payment_gateways/paypal_controller.rb @@ -192,7 +192,7 @@ module PaymentGateways end def completion_route(order) - main_app.order_path(order, token: order.token) + main_app.order_path(order, order_token: order.token) end def address_required? diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index 327c6cc3ac..a39ab94629 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -113,7 +113,7 @@ module Spree end def check_authorization - session[:access_token] ||= params[:token] + session[:access_token] ||= params[:order_token] order = Spree::Order.find_by(number: params[:id]) || current_order if order @@ -154,7 +154,7 @@ module Spree end def require_order_authentication - return if session[:access_token] || params[:token] || spree_current_user + return if session[:access_token] || params[:order_token] || spree_current_user flash[:error] = I18n.t("spree.orders.edit.login_to_view_order") redirect_to main_app.root_path(anchor: "login?after_login=#{request.env['PATH_INFO']}") diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 1f8ab6b2ef..ba74e63d3f 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -26,12 +26,12 @@ describe Spree::OrdersController, type: :controller do let(:current_user) { nil } it "loads page" do - get :show, params: { id: order.number, token: order.token } + get :show, params: { id: order.number, order_token: order.token } expect(response.status).to eq 200 end it "stores order token in session as 'access_token'" do - get :show, params: { id: order.number, token: order.token } + get :show, params: { id: order.number, order_token: order.token } expect(session[:access_token]).to eq(order.token) end end diff --git a/spec/requests/checkout/paypal_spec.rb b/spec/requests/checkout/paypal_spec.rb index 44bca06ae5..682a27e57f 100644 --- a/spec/requests/checkout/paypal_spec.rb +++ b/spec/requests/checkout/paypal_spec.rb @@ -58,7 +58,7 @@ describe "checking out an order with a paypal express payment method", type: :re get payment_gateways_confirm_paypal_path, params: params # Processing was successful, order is complete - expect(response).to redirect_to order_path(order, token: order.token) + expect(response).to redirect_to order_path(order, order_token: order.token) expect(order.reload.complete?).to be true # We have only one payment, and one transaction fee diff --git a/spec/system/consumer/shopping/orders_spec.rb b/spec/system/consumer/shopping/orders_spec.rb index 57724a81ef..c0e4489d3e 100644 --- a/spec/system/consumer/shopping/orders_spec.rb +++ b/spec/system/consumer/shopping/orders_spec.rb @@ -48,7 +48,7 @@ describe "Order Management", js: true do expect(page).to_not be_confirmed_order_page # Can load the page with token - visit order_path(order, token: order.token) + visit order_path(order, order_token: order.token) expect(page).to be_confirmed_order_page # Can load the page even without the token, after loading the page with From ec3dadfe68762f5773e5cb7c033f438ad842d1db Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 16 Dec 2021 13:41:04 +0000 Subject: [PATCH 2/3] Remove reference to params[:token] in PaymentsController There seemingly shouldn't be any case where this controller actually receives a token param. There's only one place that creates urls that direct to this controller (Stripe authorization emails), and they do not attach any kind of token to the URL. If the user is not logged in here (or doesn't have an access_token in their session), they get asked to log in. Note to future devs: see previous commit for additional context. --- app/controllers/payments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/payments_controller.rb b/app/controllers/payments_controller.rb index 47d2004dc2..e232574b63 100644 --- a/app/controllers/payments_controller.rb +++ b/app/controllers/payments_controller.rb @@ -19,7 +19,7 @@ class PaymentsController < BaseController private def require_logged_in - return if session[:access_token] || params[:token] || spree_current_user + return if session[:access_token] || spree_current_user flash[:error] = I18n.t("spree.orders.edit.login_to_view_order") redirect_to main_app.root_path(anchor: "login?after_login=#{request.env['PATH_INFO']}") From fb2c0a253bf4bea3dd0bdef7289cf12a283cd88b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 16 Dec 2021 14:02:50 +0000 Subject: [PATCH 3/3] Remove reference to params[:token] in Admin::Orders::CustomerDetailsController params[:token] and session[:access_token] are only really used in the context of guest users in the customer-facing parts of the app. Here the user should be fully authenticated already to view the page. There aren't any URL that point at this controller which append a token to the params. --- .../spree/admin/orders/customer_details_controller.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/controllers/spree/admin/orders/customer_details_controller.rb b/app/controllers/spree/admin/orders/customer_details_controller.rb index 8bdd0990c4..2a1f7a2595 100644 --- a/app/controllers/spree/admin/orders/customer_details_controller.rb +++ b/app/controllers/spree/admin/orders/customer_details_controller.rb @@ -75,14 +75,10 @@ module Spree end def check_authorization - load_order - session[:access_token] ||= params[:token] - - resource = @order action = params[:action].to_sym action = :edit if action == :show # show route renders :edit for this controller - authorize! action, resource, session[:access_token] + authorize! action, @order end def set_guest_checkout_status