Move ownership for session persistence from library to this gem (#1563)

* session stored in rails instead of library * move responsiblity for session persistence * move load_current_session to login_protection * FIXME notes * Move session concerns back to session utils * Add `embedded` param to `splash_page` (#1549) * use online tokens appropriately if there is a user session storage available * move load_current_session to login_protection * Move session concerns back to session utils * refactor login protection and tests with sesison ownership change * rubocop'd * no more session_storage in generated example * add x86_65-linux platform for CI * no more local pointer to api lib * more gemlock Co-authored-by: rdillensnyder <[email protected]> Co-authored-by: Teddy Hwang <[email protected]>
Shopify · Dec 7, 2022 · a4d5c84 · a4d5c84
1 parent d746c7f
commit a4d5c84
Show file tree

Hide file tree

Showing 9 changed files with 183 additions and 164 deletions.
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -121,7 +121,6 @@ GEM
       mime-types-data (~> 3.2015)
     mime-types-data (3.2022.0105)
     mini_mime (1.1.2)
-    mini_portile2 (2.8.0)
     minitest (5.16.3)
     mocha (2.0.2)
       ruby2_keywords (>= 0.0.5)
@@ -135,8 +134,11 @@ GEM
     net-smtp (0.3.3)
       net-protocol
     nio4r (2.5.8)
-    nokogiri (1.13.9)
-      mini_portile2 (~> 2.8.0)
+    nokogiri (1.13.9-arm64-darwin)
+      racc (~> 1.4)
+    nokogiri (1.13.9-x86_64-darwin)
+      racc (~> 1.4)
+    nokogiri (1.13.9-x86_64-linux)
       racc (~> 1.4)
     oj (3.13.23)
     openssl (3.0.1)
@@ -234,8 +236,9 @@ GEM
       actionpack (>= 5.2)
       activesupport (>= 5.2)
       sprockets (>= 3.0.0)
-    sqlite3 (1.5.4)
-      mini_portile2 (~> 2.8.0)
+    sqlite3 (1.5.4-arm64-darwin)
+    sqlite3 (1.5.4-x86_64-darwin)
+    sqlite3 (1.5.4-x86_64-linux)
     syntax_tree (4.3.0)
       prettier_print (>= 1.0.2)
     thor (1.2.1)
@@ -253,7 +256,9 @@ GEM
     zeitwerk (2.6.4)
 
 PLATFORMS
-  ruby
+  arm64-darwin-21
+  x86_64-darwin-19
+  x86_64-linux
 
 DEPENDENCIES
   byebug
@@ -273,4 +278,4 @@ DEPENDENCIES
   webmock
 
 BUNDLED WITH
-   2.3.22
+   2.3.4
diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb
@@ -8,50 +8,65 @@ class CallbackController < ActionController::Base
 
     def callback
       begin
-        filtered_params = request.parameters.symbolize_keys.slice(:code, :shop, :timestamp, :state, :host, :hmac)
-
-        auth_result = ShopifyAPI::Auth::Oauth.validate_auth_callback(
-          cookies: {
-            ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME =>
-              cookies.encrypted[ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME],
-          },
-          auth_query: ShopifyAPI::Auth::Oauth::AuthQuery.new(**filtered_params),
-        )
-      rescue => e
-        if e.class.module_parent != ShopifyAPI::Errors
-          message = <<~EOS
-            An error of type #{e.class} was rescued. This is not part of `ShopifyAPI::Errors`, which could indicate a
-            bug in your app, or a bug in the shopify_app gem. Future versions of the gem may re-raise this error rather
-            than rescuing it.
-          EOS
-          ShopifyApp::Logger.deprecated(message, "22.0.0")
-        end
+        api_session, cookie = validated_auth_objects
+      rescue => error
+        deprecate_callback_rescue(error) unless error.class.module_parent == ShopifyAPI::Errors
         return respond_with_error
       end
 
-      cookies.encrypted[auth_result[:cookie].name] = {
-        expires: auth_result[:cookie].expires,
-        secure: true,
-        http_only: true,
-        value: auth_result[:cookie].value,
-      }
+      save_session(api_session) if api_session
+      update_rails_cookie(api_session, cookie)
 
-      if auth_result[:session].online?
-        session[:shopify_user_id] = auth_result[:session].associated_user.id
-        ShopifyApp::Logger.debug("Saving Shopify user ID to cookie")
-      end
+      return respond_with_user_token_flow if start_user_token_flow?(api_session)
 
-      if start_user_token_flow?(auth_result[:session])
-        return respond_with_user_token_flow
-      end
+      perform_post_authenticate_jobs(api_session)
+      respond_successfully if check_billing(api_session)
+    end
+
+    private
 
-      perform_post_authenticate_jobs(auth_result[:session])
-      has_payment = check_billing(auth_result[:session])
+    def deprecate_callback_rescue(error)
+      message = <<~EOS
+        An error of type #{error.class} was rescued. This is not part of `ShopifyAPI::Errors`, which could indicate a
+        bug in your app, or a bug in the shopify_app gem. Future versions of the gem may re-raise this error rather
+        than rescuing it.
+      EOS
+      ShopifyApp::Logger.deprecated(message, "22.0.0")
+    end
 
-      respond_successfully if has_payment
+    def save_session(api_session)
+      ShopifyApp::SessionRepository.store_session(api_session)
     end
 
-    private
+    def validated_auth_objects
+      filtered_params = request.parameters.symbolize_keys.slice(:code, :shop, :timestamp, :state, :host, :hmac)
+
+      oauth_payload = ShopifyAPI::Auth::Oauth.validate_auth_callback(
+        cookies: {
+          ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME =>
+            cookies.encrypted[ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME],
+        },
+        auth_query: ShopifyAPI::Auth::Oauth::AuthQuery.new(**filtered_params),
+      )
+      api_session = oauth_payload.dig(:session)
+      cookie = oauth_payload.dig(:cookie)
+
+      [api_session, cookie]
+    end
+
+    def update_rails_cookie(api_session, cookie)
+      if cookie.value.present?
+        cookies.encrypted[cookie.name] = {
+          expires: cookie.expires,
+          secure: true,
+          http_only: true,
+          value: cookie.value,
+        }
+      end
+
+      session[:shopify_user_id] = api_session.associated_user.id if api_session.online?
+      ShopifyApp::Logger.debug("Saving Shopify user ID to cookie")
+    end
 
     def respond_successfully
       if ShopifyAPI::Context.embedded?

diff --git a/lib/generators/shopify_app/install/templates/shopify_app.rb.tt b/lib/generators/shopify_app/install/templates/shopify_app.rb.tt
@@ -48,7 +48,6 @@ Rails.application.config.after_initialize do
       scope: ShopifyApp.configuration.scope,
       is_private: !ENV.fetch('SHOPIFY_APP_PRIVATE_SHOP', '').empty?,
       is_embedded: ShopifyApp.configuration.embedded_app,
-      session_storage: ShopifyApp::SessionRepository,
       log_level: :info,
       logger: Rails.logger,
       private_shop: ENV.fetch('SHOPIFY_APP_PRIVATE_SHOP', nil),

diff --git a/lib/shopify_app/controller_concerns/login_protection.rb b/lib/shopify_app/controller_concerns/login_protection.rb
@@ -48,7 +48,7 @@ def activate_shopify_session
     def current_shopify_session
       @current_shopify_session ||= begin
         cookie_name = ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME
-        ShopifyAPI::Utils::SessionUtils.load_current_session(
+        load_current_session(
           auth_header: request.headers["HTTP_AUTHORIZATION"],
           cookies: { cookie_name => cookies.encrypted[cookie_name] },
           is_online: online_token_configured?,
@@ -265,6 +265,15 @@ def user_session_expected?
       online_token_configured?
     end
 
+    def load_current_session(auth_header: nil, cookies: nil, is_online: false)
+      return ShopifyAPI::Context.load_private_session if ShopifyAPI::Context.private?
+
+      session_id = ShopifyAPI::Utils::SessionUtils.current_session_id(auth_header, cookies, is_online)
+      return nil unless session_id
+
+      ShopifyApp::SessionRepository.load_session(session_id)
+    end
+
     def requested_by_javascript?
       request.xhr? ||
         request.content_type == "text/javascript" ||

diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb
@@ -28,23 +28,22 @@ class CallbackControllerTest < ActionController::TestCase
       ShopifyApp::SessionRepository.user_storage = nil
       ShopifyAppConfigurer.setup_context
       I18n.locale = :en
-
+      @stubbed_session = ShopifyAPI::Auth::Session.new(shop: "shop", access_token: "token")
+      @stubbed_cookie = ShopifyAPI::Auth::Oauth::SessionCookie.new(value: "", expires: Time.now)
+      host = Base64.strict_encode64("#{ShopifyAPI::Context.host_name}/admin")
+      @callback_params = { shop: "shop", code: "code", state: "state", timestamp: "timestamp", host: host,
+                           hmac: "hmac", }
+      @auth_query = ShopifyAPI::Auth::Oauth::AuthQuery.new(**@callback_params)
       request.env["HTTP_USER_AGENT"] = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6)"\
         "AppleWebKit/537.36 (KHTML, like Gecko)"\
         "Chrome/69.0.3497.100 Safari/537.36"
-    end
-
-    test "#callback flashes error when omniauth is not present" do
-      get :callback,
-        params: { shop: "shop", code: "code", state: "state", timestamp: "timestamp", host: "host", hmac: "hmac" }
-      assert_equal flash[:error], "Could not log in to Shopify store"
+      ShopifyApp::SessionRepository.stubs(:store_session)
     end
 
     test "#callback flashes error in Spanish" do
-      I18n.locale = :es
+      I18n.expects(:t).with("could_not_log_in")
       get :callback,
         params: { shop: "shop", code: "code", state: "state", timestamp: "timestamp", host: "host", hmac: "hmac" }
-      assert_match "sesión", flash[:error]
     end
 
     test "#callback rescued errors of ShopifyAPI::Error will not emit a deprecation notice" do
@@ -89,6 +88,53 @@ class CallbackControllerTest < ActionController::TestCase
       get :callback, params: @callback_params
     end
 
+    test "#callback saves the session when validated by API library" do
+      mock_oauth
+      ShopifyApp::SessionRepository.expects(:store_session).with(@stubbed_session)
+
+      get :callback, params: @callback_params
+    end
+
+    test "#callback sets the shopify_user_id in the Rails session when session is online" do
+      associated_user = ShopifyAPI::Auth::AssociatedUser.new(
+        id: 42,
+        first_name: "LeeeEEeeeeee3roy",
+        last_name: "Jenkins",
+        email: "[email protected]",
+        email_verified: true,
+        locale: "en",
+        collaborator: true,
+        account_owner: true,
+      )
+      mock_session = ShopifyAPI::Auth::Session.new(shop: "shop", access_token: "token", is_online: true,
+        associated_user: associated_user)
+      mock_oauth(session: mock_session)
+      get :callback, params: @callback_params
+      assert_equal session[:shopify_user_id], associated_user.id
+    end
+
+    test "#callback DOES NOT set the shopify_user_id in the Rails session when session is offline" do
+      mock_session = ShopifyAPI::Auth::Session.new(shop: "shop", access_token: "token", is_online: false)
+      mock_oauth(session: mock_session)
+      get :callback, params: @callback_params
+      assert_nil session[:shopify_user_id]
+    end
+
+    test "#callback sets encrypted cookie if API library returns cookie object" do
+      cookie = ShopifyAPI::Auth::Oauth::SessionCookie.new(value: "snickerdoodle", expires: Time.now + 1.day)
+      mock_oauth(cookie: cookie)
+
+      get :callback, params: @callback_params
+      assert_equal cookies.encrypted[cookie.name], cookie.value
+    end
+
+    test "#callback does not set encrypted cookie if API library returns empty cookie" do
+      mock_oauth
+
+      get :callback, params: @callback_params
+      refute_equal cookies.encrypted[@stubbed_cookie.name], @stubbed_cookie.value
+    end
+
     test "#callback starts the WebhooksManager if webhooks are configured" do
       ShopifyApp.configure do |config|
         config.webhooks = [{ topic: "carts/update", address: "example-app.com/webhooks" }]
@@ -238,11 +284,7 @@ class CallbackControllerTest < ActionController::TestCase
 
     private
 
-    def mock_oauth
-      host = Base64.strict_encode64("#{ShopifyAPI::Context.host_name}/admin")
-      @callback_params = { shop: "shop", code: "code", state: "state", timestamp: "timestamp", host: host,
-                           hmac: "hmac", }
-      @auth_query = ShopifyAPI::Auth::Oauth::AuthQuery.new(**@callback_params)
+    def mock_oauth(cookie: @stubbed_cookie, session: @stubbed_session)
       ShopifyAPI::Auth::Oauth::AuthQuery.stubs(:new).with(**@callback_params).returns(@auth_query)
 
       cookies.encrypted[ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME] = "nonce"
@@ -253,8 +295,8 @@ def mock_oauth
             cookies.encrypted[ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME],
         }, auth_query: @auth_query)
         .returns({
-          cookie: ShopifyAPI::Auth::Oauth::SessionCookie.new(value: "", expires: Time.now),
-          session: ShopifyAPI::Auth::Session.new(shop: "shop", access_token: "token"),
+          cookie: cookie,
+          session: session,
         })
     end
   end

diff --git a/test/controllers/concerns/ensure_billing_test.rb b/test/controllers/concerns/ensure_billing_test.rb
@@ -24,16 +24,13 @@ def index
       get "/billing", to: "ensure_billing_test/billing_test#index"
     end
 
-    ShopifyApp::SessionRepository.shop_storage = ShopifyApp::InMemoryShopSessionStore
-    ShopifyApp::SessionRepository.user_storage = ShopifyApp::InMemoryUserSessionStore
-
     @session = ShopifyAPI::Auth::Session.new(
       id: "1234",
       shop: SHOP,
       access_token: "access-token",
       scope: ["read_products"],
     )
-    ShopifyAPI::Utils::SessionUtils.stubs(:load_current_session).returns(@session)
+    @controller.stubs(:current_shopify_session).returns(@session)
 
     @api_version = ShopifyAPI::LATEST_SUPPORTED_ADMIN_VERSION
     ShopifyApp.configuration.api_version = @api_version

diff --git a/test/generators/install_generator_test.rb b/test/generators/install_generator_test.rb
@@ -36,7 +36,6 @@ class InstallGeneratorTest < Rails::Generators::TestCase
               scope: ShopifyApp.configuration.scope,
               is_private: !ENV.fetch('SHOPIFY_APP_PRIVATE_SHOP', '').empty?,
               is_embedded: ShopifyApp.configuration.embedded_app,
-              session_storage: ShopifyApp::SessionRepository,
               log_level: :info,
               logger: Rails.logger,
               private_shop: ENV.fetch('SHOPIFY_APP_PRIVATE_SHOP', nil),