Merge pull request #1079 from Shopify/fix-inconsistent-callback-flow

Ensure post authenticate jobs run after for jwt based callback requests
Shopify · Sep 23, 2020 · d76175c · d76175c
2 parents 8eee69b + ebcdf65
commit d76175c
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 21 deletions.
diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb
@@ -6,50 +6,77 @@ class CallbackController < ActionController::Base
     include ShopifyApp::LoginProtection
 
     def callback
-      unless auth_hash
-        return respond_with_error
-      end
+      return respond_with_error if invalid_request?
 
-      if jwt_request? && !valid_jwt_auth?
-        Rails.logger.debug("[ShopifyApp::CallbackController] Invalid JWT auth detected.")
-        return respond_with_error
+      store_access_token_and_build_session
+
+      if start_user_token_flow?
+        return respond_with_user_token_flow
       end
 
+      perform_post_authenticate_jobs
+
+      respond_successfully
+    end
+
+    private
+
+    def respond_successfully
       if jwt_request?
-        Rails.logger.debug("[ShopifyApp::CallbackController] JWT request detected. Setting shopify session...")
-        set_shopify_session
         head(:ok)
       else
+        redirect_to(return_address)
+      end
+    end
+
+    def respond_with_user_token_flow
+      Rails.logger.debug("[ShopifyApp::CallbackController] Redirecting for user token...")
+      redirect_to(login_url_with_optional_shop)
+    end
+
+    def store_access_token_and_build_session
+      if native_browser_request?
         Rails.logger.debug("[ShopifyApp::CallbackController] Not a JWT request. Resetting session options...")
         reset_session_options
-        set_shopify_session
+      else
+        Rails.logger.debug("[ShopifyApp::CallbackController] JWT request detected. Setting shopify session...")
+      end
+      set_shopify_session
+    end
 
-        if redirect_for_user_token?
-          Rails.logger.debug("[ShopifyApp::CallbackController] Redirecting for user token...")
-          return redirect_to(login_url_with_optional_shop)
-        end
+    def invalid_request?
+      return true unless auth_hash
 
-        install_webhooks
-        install_scripttags
-        perform_after_authenticate_job
+      jwt_request? && !valid_jwt_auth?
+    end
 
-        redirect_to(return_address)
-      end
+    def native_browser_request?
+      !jwt_request?
     end
 
-    private
+    def perform_post_authenticate_jobs
+      install_webhooks
+      install_scripttags
+      perform_after_authenticate_job
+    end
 
     def respond_with_error
       if jwt_request?
+        Rails.logger.debug("[ShopifyApp::CallbackController] Invalid JWT auth detected.")
         head(:unauthorized)
       else
+        Rails.logger.debug("[ShopifyApp::CallbackController] Invalid non JWT auth detected.")
         flash[:error] = I18n.t('could_not_log_in')
         redirect_to(login_url_with_optional_shop)
       end
     end
 
-    def redirect_for_user_token?
-      ShopifyApp::SessionRepository.user_storage.present? && user_session.blank?
+    def start_user_token_flow?
+      if jwt_request?
+        false
+      else
+        ShopifyApp::SessionRepository.user_storage.present? && user_session.blank?
+      end
     end
 
     def jwt_request?

diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb
@@ -322,6 +322,48 @@ class CallbackControllerTest < ActionController::TestCase
       assert_redirected_to "/?shop=#{TEST_SHOPIFY_DOMAIN}"
     end
 
+    test "#callback performs install_webhook job after JWT authentication" do
+      mock_shopify_user_omniauth
+      mock_shopify_jwt
+
+      ShopifyApp.configure do |config|
+        config.webhooks = [{ topic: 'carts/update', address: 'example-app.com/webhooks' }]
+      end
+
+      ShopifyApp::WebhooksManager.expects(:queue)
+
+      get :callback
+      assert_response :ok
+    end
+
+    test "#callback performs install_scripttags job after JWT authentication" do
+      mock_shopify_user_omniauth
+      mock_shopify_jwt
+
+      ShopifyApp.configure do |config|
+        config.scripttags = [{ topic: 'carts/update', address: 'example-app.com/webhooks' }]
+      end
+
+      ShopifyApp::ScripttagsManager.expects(:queue)
+
+      get :callback
+      assert_response :ok
+    end
+
+    test "#callback performs after_authenticate job after JWT authentication" do
+      mock_shopify_user_omniauth
+      mock_shopify_jwt
+
+      ShopifyApp.configure do |config|
+        config.after_authenticate_job = { job: Shopify::AfterAuthenticateJob, inline: true }
+      end
+
+      Shopify::AfterAuthenticateJob.expects(:perform_now)
+
+      get :callback
+      assert_response :ok
+    end
+
     private
 
     def mock_shopify_jwt