From c3e89a93ca507a411ee99c4515f0e39e991577e0 Mon Sep 17 00:00:00 2001 From: Nelson Date: Wed, 14 Dec 2022 18:28:42 -0500 Subject: [PATCH] Validate store sessions (#1612) * Check shop offline session is still valid when embedded * only redirect on 401 and use better naming * use guard clause instead of conditional filter * redirect to shop_login if token failed * update tests to match expected behavior * Changelog updates * raise error if not 401 * better redirect test --- CHANGELOG.md | 1 + .../concerns/shopify_app/ensure_installed.rb | 17 +++- .../redirect_for_embedded.rb | 6 +- .../concerns/ensure_installed_test.rb | 80 ++++++++++++++++++- .../concerns/require_known_shop_test.rb | 7 +- 5 files changed, 107 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6e18a12c..f0a827add 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ Unreleased ---------- +* Validates shop's offline session token is still valid when using `EnsureInstalled`[#1612](https://github.com/Shopify/shopify_app/pull/1612) 21.3.1 (Dec 12, 2022) ---------- diff --git a/app/controllers/concerns/shopify_app/ensure_installed.rb b/app/controllers/concerns/shopify_app/ensure_installed.rb index c0ce88da4..a79faf0f0 100644 --- a/app/controllers/concerns/shopify_app/ensure_installed.rb +++ b/app/controllers/concerns/shopify_app/ensure_installed.rb @@ -17,6 +17,7 @@ module EnsureInstalled before_action :check_shop_domain before_action :check_shop_known + before_action :validate_non_embedded_session end def current_shopify_domain @@ -30,6 +31,10 @@ def current_shopify_domain @shopify_domain end + def installed_shop_session + @installed_shop_session ||= SessionRepository.retrieve_shop_session_by_shopify_domain(current_shopify_domain) + end + private def check_shop_domain @@ -37,7 +42,7 @@ def check_shop_domain end def check_shop_known - @shop = SessionRepository.retrieve_shop_session_by_shopify_domain(current_shopify_domain) + @shop = installed_shop_session unless @shop if embedded_param? redirect_for_embedded @@ -58,5 +63,15 @@ def shop_login url.to_s end + + def validate_non_embedded_session + return if loaded_directly_from_admin? + + client = ShopifyAPI::Clients::Rest::Admin.new(session: installed_shop_session) + client.get(path: "shop") + rescue ShopifyAPI::Errors::HttpResponseError => error + redirect_to(shop_login) if error.code == 401 + raise error if error.code != 401 + end end end diff --git a/lib/shopify_app/controller_concerns/redirect_for_embedded.rb b/lib/shopify_app/controller_concerns/redirect_for_embedded.rb index 889e0acdf..6ba411ec9 100644 --- a/lib/shopify_app/controller_concerns/redirect_for_embedded.rb +++ b/lib/shopify_app/controller_concerns/redirect_for_embedded.rb @@ -11,7 +11,11 @@ def embedded_redirect_url? end def embedded_param? - embedded_redirect_url? && params[:embedded].present? && params[:embedded] == "1" + embedded_redirect_url? && params[:embedded].present? && loaded_directly_from_admin? + end + + def loaded_directly_from_admin? + ShopifyApp.configuration.embedded_app? && params[:embedded] == "1" end def redirect_for_embedded diff --git a/test/controllers/concerns/ensure_installed_test.rb b/test/controllers/concerns/ensure_installed_test.rb index 212e553c7..aaddd6f6a 100644 --- a/test/controllers/concerns/ensure_installed_test.rb +++ b/test/controllers/concerns/ensure_installed_test.rb @@ -52,7 +52,12 @@ def index end test "returns :ok if the shop is installed" do - ShopifyApp::SessionRepository.expects(:retrieve_shop_session_by_shopify_domain).returns(true) + session = mock + ShopifyApp::SessionRepository.stubs(:retrieve_shop_session_by_shopify_domain).returns(session) + + client = mock + ShopifyAPI::Clients::Rest::Admin.expects(:new).with(session: session).returns(client) + client.expects(:get) shopify_domain = "shop1.myshopify.com" @@ -61,6 +66,79 @@ def index assert_response :ok end + test "redirects to login_url (oauth path) to reinstall the app if the store's session token is no longer valid" do + session = mock + ShopifyApp::SessionRepository.stubs(:retrieve_shop_session_by_shopify_domain).returns(session) + + client = mock + ShopifyAPI::Clients::Rest::Admin.expects(:new).with(session: session).returns(client) + uninstalled_http_error = ShopifyAPI::Errors::HttpResponseError.new( + response: ShopifyAPI::Clients::HttpResponse.new( + code: 401, + headers: {}, + body: "Invalid API key or access token (unrecognized login or wrong password)", + ), + ) + client.expects(:get).with(path: "shop").raises(uninstalled_http_error) + + shopify_domain = "shop1.myshopify.com" + host = "https://tunnel.vision.for.webhooks.com" + + get :index, params: { shop: shopify_domain, host: host } + + url = URI(ShopifyApp.configuration.login_url) + url.query = URI.encode_www_form( + shop: shopify_domain, + host: host, + return_to: request.fullpath, + ) + assert_redirected_to url.to_s + end + + test "throws an error if the shopify error isn't a 401" do + session = mock + ShopifyApp::SessionRepository.stubs(:retrieve_shop_session_by_shopify_domain).returns(session) + + client = mock + ShopifyAPI::Clients::Rest::Admin.expects(:new).with(session: session).returns(client) + uninstalled_http_error = ShopifyAPI::Errors::HttpResponseError.new( + response: ShopifyAPI::Clients::HttpResponse.new( + code: 404, + headers: {}, + body: "Insert generic message about how we can't find your requests here.", + ), + ) + client.expects(:get).with(path: "shop").raises(uninstalled_http_error) + + shopify_domain = "shop1.myshopify.com" + + assert_raises ShopifyAPI::Errors::HttpResponseError do + get :index, params: { shop: shopify_domain } + end + end + + test "throws an error if the request session validation API check fails with an" do + session = mock + ShopifyApp::SessionRepository.stubs(:retrieve_shop_session_by_shopify_domain).returns(session) + + client = mock + ShopifyAPI::Clients::Rest::Admin.expects(:new).with(session: session).returns(client) + client.expects(:get).with(path: "shop").raises(RuntimeError) + + shopify_domain = "shop1.myshopify.com" + + assert_raises RuntimeError do + get :index, params: { shop: shopify_domain } + end + end + + test "does not perform a session validation check if coming from an embedded" do + ShopifyApp::SessionRepository.stubs(:retrieve_shop_session_by_shopify_domain) + ShopifyAPI::Clients::Rest::Admin.expects(:new).never + + get :index, params: { shop: "shop1.myshopify.com" } + end + test "detects incompatible controller concerns" do version = "22.0.0" ShopifyApp::Logger.expects(:deprecated).with(regexp_matches(/incompatible concerns/), version) diff --git a/test/controllers/concerns/require_known_shop_test.rb b/test/controllers/concerns/require_known_shop_test.rb index c85cbdf88..da1ecf3b3 100644 --- a/test/controllers/concerns/require_known_shop_test.rb +++ b/test/controllers/concerns/require_known_shop_test.rb @@ -52,7 +52,12 @@ def index end test "returns :ok if the shop is installed" do - ShopifyApp::SessionRepository.expects(:retrieve_shop_session_by_shopify_domain).returns(true) + session = mock + ShopifyApp::SessionRepository.stubs(:retrieve_shop_session_by_shopify_domain).returns(session) + + client = mock + ShopifyAPI::Clients::Rest::Admin.expects(:new).with(session: session).returns(client) + client.expects(:get) shopify_domain = "shop1.myshopify.com"