Merge pull request #1823 from Shopify/handle-invalid-session-token

Handle invalid session token
Shopify · Apr 19, 2024 · ea884a6 · ea884a6
2 parents 94c3ac3 + 9712272
commit ea884a6
Show file tree

Hide file tree

Showing 10 changed files with 218 additions and 28 deletions.
diff --git a/app/controllers/concerns/shopify_app/ensure_installed.rb b/app/controllers/concerns/shopify_app/ensure_installed.rb
@@ -17,9 +17,11 @@ module EnsureInstalled
 
       before_action :check_shop_domain
 
-      unless ShopifyApp.configuration.use_new_embedded_auth_strategy?
-        # TODO: Add support to use new embedded auth strategy here when invalid
-        # session token can be handled by AppBridge app reload
+      if ShopifyApp.configuration.use_new_embedded_auth_strategy?
+        include ShopifyApp::TokenExchange
+        include ShopifyApp::EmbeddedApp
+        around_action :activate_shopify_session
+      else
         before_action :check_shop_known
         before_action :validate_non_embedded_session
       end

diff --git a/app/controllers/shopify_app/sessions_controller.rb b/app/controllers/shopify_app/sessions_controller.rb
@@ -7,7 +7,7 @@ class SessionsController < ActionController::Base
 
     layout false, only: :new
 
-    after_action only: [:new, :create] do |controller|
+    after_action only: [:new, :create, :patch_shopify_id_token] do |controller|
       controller.response.headers.except!("X-Frame-Options")
     end
 
@@ -19,6 +19,10 @@ def create
       authenticate
     end
 
+    def patch_shopify_id_token
+      render(layout: "shopify_app/layouts/app_bridge")
+    end
+
     def top_level_interaction
       @url = login_url_with_optional_shop(top_level: true)
       validate_shop_presence

diff --git a/app/views/shopify_app/layouts/app_bridge.html.erb b/app/views/shopify_app/layouts/app_bridge.html.erb
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="utf-8" />
+    <title><%= ShopifyApp.configuration.application_name %></title>
+    <%= yield :head %>
+    <script
+      data-api-key="<%= ShopifyApp.configuration.api_key %>"
+      src="https://cdn.shopify.com/shopifycloud/app-bridge.js">
+    </script>
+    <%= csrf_meta_tags %>
+  </head>
+
+  <body>
+    <%= yield %>
+  </body>
+</html>
diff --git a/app/views/shopify_app/sessions/patch_shopify_id_token.html.erb b/app/views/shopify_app/sessions/patch_shopify_id_token.html.erb
diff --git a/config/routes.rb b/config/routes.rb
@@ -8,6 +8,7 @@
     get login_url => :new, :as => :login
     post login_url => :create, :as => :authenticate
     get "logout" => :destroy, :as => :logout
+    get "patch_shopify_id_token" => :patch_shopify_id_token
 
     # Kept to prevent apps relying on these routes from breaking
     if login_url.gsub(%r{^/}, "") != "login"

diff --git a/lib/shopify_app/controller_concerns/token_exchange.rb b/lib/shopify_app/controller_concerns/token_exchange.rb
@@ -5,17 +5,23 @@ module TokenExchange
     extend ActiveSupport::Concern
     include ShopifyApp::AdminAPI::WithTokenRefetch
 
+    INVALID_SHOPIFY_ID_TOKEN_ERRORS = [
+      ShopifyAPI::Errors::CookieNotFoundError,
+      ShopifyAPI::Errors::InvalidJwtTokenError,
+    ].freeze
+
     def activate_shopify_session(&block)
       retrieve_session_from_token_exchange if current_shopify_session.blank? || should_exchange_expired_token?
 
-      begin
-        ShopifyApp::Logger.debug("Activating Shopify session")
-        ShopifyAPI::Context.activate_session(current_shopify_session)
-        with_token_refetch(current_shopify_session, shopify_id_token, &block)
-      ensure
-        ShopifyApp::Logger.debug("Deactivating session")
-        ShopifyAPI::Context.deactivate_session
-      end
+      ShopifyApp::Logger.debug("Activating Shopify session")
+      ShopifyAPI::Context.activate_session(current_shopify_session)
+      with_token_refetch(current_shopify_session, shopify_id_token, &block)
+    rescue *INVALID_SHOPIFY_ID_TOKEN_ERRORS => e
+      ShopifyApp::Logger.debug("Responding to invalid Shopify ID token: #{e.message}")
+      respond_to_invalid_shopify_id_token unless performed?
+    ensure
+      ShopifyApp::Logger.debug("Deactivating session")
+      ShopifyAPI::Context.deactivate_session
     end
 
     def should_exchange_expired_token?
@@ -47,9 +53,6 @@ def current_shopify_domain
     def retrieve_session_from_token_exchange
       @current_shopify_session = nil
       ShopifyApp::Auth::TokenExchange.perform(shopify_id_token)
-      # TODO: Rescue JWT validation errors when bounce page is ready
-      # rescue ShopifyAPI::Errors::InvalidJwtTokenError
-      #   respond_to_invalid_shopify_id_token
     end
 
     def shopify_id_token
@@ -61,23 +64,28 @@ def id_token_header
     end
 
     def respond_to_invalid_shopify_id_token
-      # TODO: Implement this method to handle invalid session tokens
+      return redirect_to_bounce_page if request.headers["HTTP_AUTHORIZATION"].blank?
+
+      ShopifyApp::Logger.debug("Responding to invalid Shopify ID token with unauthorized response")
+      response.set_header("X-Shopify-Retry-Invalid-Session-Request", 1)
+      unauthorized_response = { message: :unauthorized }
+      render(json: { errors: [unauthorized_response] }, status: :unauthorized)
+    end
 
-      # if request.xhr?
-      # response.set_header("X-Shopify-Retry-Invalid-Session-Request", 1)
-      # unauthorized_response = { message: :unauthorized }
-      # render(json: { errors: [unauthorized_response] }, status: :unauthorized)
-      # else
-      # patch_session_token_url = "#{ShopifyAPI::Context.host}/patch_session_token"
-      # patch_session_token_params = request.query_parameters.except(:id_token)
+    def redirect_to_bounce_page
+      ShopifyApp::Logger.debug("Redirecting to bounce page for patching Shopify ID token")
+      patch_shopify_id_token_url = "#{ShopifyApp.configuration.root_url}/patch_shopify_id_token"
+      patch_shopify_id_token_params = request.query_parameters.except(:id_token)
 
-      # bounce_url = "#{ShopifyAPI::Context.host}#{request.path}?#{patch_session_token_params.to_query}"
+      bounce_url = "#{request.path}?#{patch_shopify_id_token_params.to_query}"
 
-      # # App Bridge will trigger a fetch to the URL in shopify-reload, with a new session token in headers
-      # patch_session_token_params["shopify-reload"] = bounce_url
+      # App Bridge will trigger a fetch to the URL in shopify-reload, with a new session token in headers
+      patch_shopify_id_token_params["shopify-reload"] = bounce_url
 
-      # redirect_to("#{patch_session_token_url}?#{patch_session_token_params.to_query}", allow_other_host: true)
-      # end
+      redirect_to(
+        "#{patch_shopify_id_token_url}?#{patch_shopify_id_token_params.to_query}",
+        allow_other_host: true,
+      )
     end
 
     def online_token_configured?

diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb
@@ -402,6 +402,11 @@ class SessionsControllerTest < ActionController::TestCase
       end
     end
 
+    test "#patch_shopify_id_token renders the app bridge layout" do
+      get :patch_shopify_id_token, params: { shop: "my-shop" }
+      assert_template "shopify_app/layouts/app_bridge"
+    end
+
     private
 
     def assert_redirected_to_top_level(shop_domain, expected_url = nil)

diff --git a/test/dummy/config/initializers/shopify_app.rb b/test/dummy/config/initializers/shopify_app.rb
@@ -17,8 +17,13 @@ def self.call
       config.embedded_redirect_url = nil
 
       config.shop_session_repository = ShopifyApp::InMemorySessionStore
+      config.user_session_repository = nil
       config.after_authenticate_job = false
       config.reauth_on_access_scope_changes = true
+      config.root_url = "/"
+      config.wip_new_embedded_auth_strategy = false
+      config.check_session_expiry_date = false
+      config.custom_post_authenticate_tasks = nil
     end
 
     setup_context

diff --git a/test/routes/sessions_routes_test.rb b/test/routes/sessions_routes_test.rb
@@ -25,6 +25,10 @@ class SessionsRoutesTest < ActionController::TestCase
     assert_routing "/logout", { controller: "shopify_app/sessions", action: "destroy" }
   end
 
+  test "patch_shopify_id_token routes to sessions#patch_shopify_id_token" do
+    assert_routing "/patch_shopify_id_token", { controller: "shopify_app/sessions", action: "patch_shopify_id_token" }
+  end
+
   test "login route doesn't change with custom root URL because it is in an engine" do
     ShopifyApp.configuration.root_url = "/new-root"
     Rails.application.reload_routes!

diff --git a/test/shopify_app/controller_concerns/token_exchange_test.rb b/test/shopify_app/controller_concerns/token_exchange_test.rb
@@ -3,6 +3,7 @@
 require "test_helper"
 require "action_controller"
 require "action_controller/base"
+require "json"
 
 class ApiClass
   def self.perform; end
@@ -17,10 +18,19 @@ def index
     render(plain: "OK")
   end
 
+  def reloaded_path
+    render(plain: "OK")
+  end
+
   def make_api_call
     ApiClass.perform
     render(plain: "OK")
   end
+
+  def ensure_render
+  ensure
+    render(plain: "OK")
+  end
 end
 
 class MockPostAuthenticateTasks
@@ -178,13 +188,147 @@ class TokenExchangeControllerTest < ActionController::TestCase
     end
   end
 
+  [
+    ShopifyAPI::Errors::InvalidJwtTokenError,
+    ShopifyAPI::Errors::CookieNotFoundError,
+  ].each do |invalid_shopify_id_token_error|
+    test "Redirects to bounce page if Shopify ID token is invalid with #{invalid_shopify_id_token_error}" do
+      ShopifyApp.configuration.root_url = "/my-root"
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).raises(invalid_shopify_id_token_error)
+      request.headers["HTTP_AUTHORIZATION"] = nil
+
+      params = { shop: @shop, my_param: "for-keeps", id_token: "dont-include-this-id-token" }
+      reload_url = CGI.escape("/reloaded_path?my_param=for-keeps&shop=#{@shop}")
+      expected_redirect_url = "/my-root/patch_shopify_id_token"\
+        "?my_param=for-keeps&shop=#{@shop}"\
+        "&shopify-reload=#{reload_url}"
+
+      with_application_test_routes do
+        get :reloaded_path, params: params
+        assert_redirected_to expected_redirect_url
+      end
+    end
+
+    test "Responds with unauthorized if Shopify Id token is invalid with #{invalid_shopify_id_token_error} and authorization header exists" do
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).raises(invalid_shopify_id_token_error)
+      request.headers["HTTP_AUTHORIZATION"] = @id_token_in_header
+      expected_response = { errors: [{ message: :unauthorized }] }
+
+      with_application_test_routes do
+        get :make_api_call, params: { shop: @shop }
+
+        assert_response :unauthorized
+        assert_equal expected_response.to_json, response.body
+        assert_equal 1, response.headers["X-Shopify-Retry-Invalid-Session-Request"]
+      end
+    end
+
+    test "Redirects to bounce page from Token Exchange if Shopify ID token is invalid with #{invalid_shopify_id_token_error}" do
+      ShopifyApp.configuration.root_url = "/my-root"
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(nil, @offline_session_id)
+      ShopifyApp::Auth::TokenExchange.expects(:perform).raises(invalid_shopify_id_token_error)
+      request.headers["HTTP_AUTHORIZATION"] = nil
+
+      params = { shop: @shop, my_param: "for-keeps", id_token: "dont-include-this-id-token" }
+      reload_url = CGI.escape("/reloaded_path?my_param=for-keeps&shop=#{@shop}")
+      expected_redirect_url = "/my-root/patch_shopify_id_token"\
+        "?my_param=for-keeps&shop=#{@shop}"\
+        "&shopify-reload=#{reload_url}"
+
+      with_application_test_routes do
+        get :reloaded_path, params: params
+        assert_redirected_to expected_redirect_url
+      end
+    end
+
+    test "Responds with unauthorized from Token Exchange if Shopify Id token is invalid with #{invalid_shopify_id_token_error} and authorization header exists" do
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(nil, @offline_session_id)
+      ShopifyApp::Auth::TokenExchange.expects(:perform).raises(invalid_shopify_id_token_error)
+
+      expected_response = { errors: [{ message: :unauthorized }] }
+
+      with_application_test_routes do
+        get :make_api_call, params: { shop: @shop }
+
+        assert_response :unauthorized
+        assert_equal expected_response.to_json, response.body
+        assert_equal 1, response.headers["X-Shopify-Retry-Invalid-Session-Request"]
+      end
+    end
+
+    test "Redirects to bounce page from with_token_refetch if Shopify ID token is invalid with #{invalid_shopify_id_token_error}" do
+      ShopifyApp.configuration.root_url = "/my-root"
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id)
+      ShopifyApp::Auth::TokenExchange.stubs(:perform)
+      request.headers["HTTP_AUTHORIZATION"] = nil
+
+      @controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error)
+
+      params = { shop: @shop, my_param: "for-keeps", id_token: "dont-include-this-id-token" }
+      reload_url = CGI.escape("/reloaded_path?my_param=for-keeps&shop=#{@shop}")
+      expected_redirect_url = "/my-root/patch_shopify_id_token"\
+        "?my_param=for-keeps&shop=#{@shop}"\
+        "&shopify-reload=#{reload_url}"
+
+      with_application_test_routes do
+        get :reloaded_path, params: params
+        assert_redirected_to expected_redirect_url
+      end
+    end
+
+    test "Responds with unauthorized from with_token_refetch if Shopify Id token is invalid with #{invalid_shopify_id_token_error} and authorization header exists" do
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id)
+      ShopifyApp::Auth::TokenExchange.stubs(:perform)
+      expected_response = { errors: [{ message: :unauthorized }] }
+
+      @controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error)
+
+      with_application_test_routes do
+        get :make_api_call, params: { shop: @shop }
+
+        assert_response :unauthorized
+        assert_equal expected_response.to_json, response.body
+        assert_equal 1, response.headers["X-Shopify-Retry-Invalid-Session-Request"]
+      end
+    end
+
+    test "Does not redirect to bounce page if redirect/response has been performed already - #{invalid_shopify_id_token_error}" do
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id)
+      ShopifyApp::Auth::TokenExchange.stubs(:perform)
+      request.headers["HTTP_AUTHORIZATION"] = nil
+
+      @controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error)
+      @controller.stubs(:performed?).returns(true)
+
+      with_application_test_routes do
+        get :ensure_render, params: { shop: @shop }
+        assert_response :ok
+      end
+    end
+
+    test "Does not respond 401 if redirect/response has been performed already - #{invalid_shopify_id_token_error}" do
+      ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id)
+      ShopifyApp::Auth::TokenExchange.stubs(:perform)
+
+      @controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error)
+      @controller.stubs(:performed?).returns(true)
+
+      with_application_test_routes do
+        get :ensure_render, params: { shop: @shop }
+        assert_response :ok
+      end
+    end
+  end
+
   private
 
   def with_application_test_routes
     with_routing do |set|
       set.draw do
         get "/" => "token_exchange#index"
         get "/make_api_call" => "token_exchange#make_api_call"
+        get "/reloaded_path" => "token_exchange#reloaded_path"
+        get "/ensure_render" => "token_exchange#ensure_render"
       end
       yield
     end