From 9287e15de9a21c8d4c5485058d8c822fe49d49f8 Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Tue, 13 Dec 2022 12:27:31 -0500
Subject: [PATCH 1/8] Check shop offline session is still valid when embedded

---
 .../concerns/shopify_app/ensure_installed.rb  | 12 ++++++++++
 .../concerns/ensure_installed_test.rb         | 22 +++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/app/controllers/concerns/shopify_app/ensure_installed.rb b/app/controllers/concerns/shopify_app/ensure_installed.rb
index c0ce88da4..42d761cb9 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_emebedded_session_is_active, if: :embedded_param?
     end
 
     def current_shopify_domain
@@ -30,6 +31,10 @@ def current_shopify_domain
       @shopify_domain
     end
 
+    def installed_shop_session
+      @installed_shop_session ||= @shop
+    end
+
     private
 
     def check_shop_domain
@@ -58,5 +63,12 @@ def shop_login
 
       url.to_s
     end
+
+    def validate_emebedded_session_is_active
+      client = ShopifyAPI::Clients::Rest::Admin.new(session: installed_shop_session)
+      client.get(path: "shop")
+    rescue ShopifyAPI::Errors::HttpResponseError
+      redirect_for_embedded
+    end
   end
 end
diff --git a/test/controllers/concerns/ensure_installed_test.rb b/test/controllers/concerns/ensure_installed_test.rb
index 212e553c7..4d3ed776f 100644
--- a/test/controllers/concerns/ensure_installed_test.rb
+++ b/test/controllers/concerns/ensure_installed_test.rb
@@ -87,4 +87,26 @@ def index
 
     assert_within_deprecation_schedule(version)
   end
+
+  test "redirects to redirect_for_embedded if app is no longer installed according to the API but not the session repository" 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: "Invalid API key or access token (unrecognized login or wrong password)",
+      ),
+    )
+    client.expects(:get).with(path: "shop").raises(uninstalled_http_error)
+
+    embedded_redirect_url = "reverse_card_on_reverse_card.embedded.joke"
+    ShopifyApp.configuration.stubs(:embedded_redirect_url).returns(embedded_redirect_url)
+
+    @controller.expects(:redirect_for_embedded)
+    get :index, params: { shop: "shop1.myshopify.com", embedded: "1" }
+  end
 end

From 04a2f53c2b8e50483e11f1a1d5822602a5b14e07 Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Tue, 13 Dec 2022 14:42:32 -0500
Subject: [PATCH 2/8] only redirect on 401 and use better naming

---
 .../concerns/shopify_app/ensure_installed.rb           | 10 +++++-----
 .../controller_concerns/redirect_for_embedded.rb       |  6 +++++-
 test/controllers/concerns/ensure_installed_test.rb     |  2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/app/controllers/concerns/shopify_app/ensure_installed.rb b/app/controllers/concerns/shopify_app/ensure_installed.rb
index 42d761cb9..e11c0a0bd 100644
--- a/app/controllers/concerns/shopify_app/ensure_installed.rb
+++ b/app/controllers/concerns/shopify_app/ensure_installed.rb
@@ -17,7 +17,7 @@ module EnsureInstalled
 
       before_action :check_shop_domain
       before_action :check_shop_known
-      before_action :validate_emebedded_session_is_active, if: :embedded_param?
+      before_action :validate_emebedded_session_is_active, if: :loaded_directly_from_admin?
     end
 
     def current_shopify_domain
@@ -32,7 +32,7 @@ def current_shopify_domain
     end
 
     def installed_shop_session
-      @installed_shop_session ||= @shop
+      @installed_shop_session ||= SessionRepository.retrieve_shop_session_by_shopify_domain(current_shopify_domain)
     end
 
     private
@@ -42,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
@@ -67,8 +67,8 @@ def shop_login
     def validate_emebedded_session_is_active
       client = ShopifyAPI::Clients::Rest::Admin.new(session: installed_shop_session)
       client.get(path: "shop")
-    rescue ShopifyAPI::Errors::HttpResponseError
-      redirect_for_embedded
+    rescue ShopifyAPI::Errors::HttpResponseError => error
+      redirect_for_embedded if error.code == 401 # unauthorized due to uninstall
     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..cbb7476f9 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?
+      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 4d3ed776f..745faa960 100644
--- a/test/controllers/concerns/ensure_installed_test.rb
+++ b/test/controllers/concerns/ensure_installed_test.rb
@@ -96,7 +96,7 @@ def index
     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,
+        code: 401,
         headers: {},
         body: "Invalid API key or access token (unrecognized login or wrong password)",
       ),

From a7e2677140a2849ffc53b12bfd0c989e327a9aec Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Tue, 13 Dec 2022 14:54:41 -0500
Subject: [PATCH 3/8] use guard clause instead of conditional filter

---
 app/controllers/concerns/shopify_app/ensure_installed.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/controllers/concerns/shopify_app/ensure_installed.rb b/app/controllers/concerns/shopify_app/ensure_installed.rb
index e11c0a0bd..214e01940 100644
--- a/app/controllers/concerns/shopify_app/ensure_installed.rb
+++ b/app/controllers/concerns/shopify_app/ensure_installed.rb
@@ -17,7 +17,7 @@ module EnsureInstalled
 
       before_action :check_shop_domain
       before_action :check_shop_known
-      before_action :validate_emebedded_session_is_active, if: :loaded_directly_from_admin?
+      before_action :validate_emebedded_session_is_active
     end
 
     def current_shopify_domain
@@ -65,6 +65,8 @@ def shop_login
     end
 
     def validate_emebedded_session_is_active
+      return unless loaded_directly_from_admin?
+
       client = ShopifyAPI::Clients::Rest::Admin.new(session: installed_shop_session)
       client.get(path: "shop")
     rescue ShopifyAPI::Errors::HttpResponseError => error

From 7fa7cabc98ee1a35d41e9fa585bc707d82e3309b Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Wed, 14 Dec 2022 15:36:42 -0500
Subject: [PATCH 4/8] redirect to shop_login if token failed

---
 app/controllers/concerns/shopify_app/ensure_installed.rb     | 4 ++--
 lib/shopify_app/controller_concerns/redirect_for_embedded.rb | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/concerns/shopify_app/ensure_installed.rb b/app/controllers/concerns/shopify_app/ensure_installed.rb
index 214e01940..1214fde4e 100644
--- a/app/controllers/concerns/shopify_app/ensure_installed.rb
+++ b/app/controllers/concerns/shopify_app/ensure_installed.rb
@@ -65,12 +65,12 @@ def shop_login
     end
 
     def validate_emebedded_session_is_active
-      return unless loaded_directly_from_admin?
+      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_for_embedded if error.code == 401 # unauthorized due to uninstall
+      redirect_to(shop_login) 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 cbb7476f9..6ba411ec9 100644
--- a/lib/shopify_app/controller_concerns/redirect_for_embedded.rb
+++ b/lib/shopify_app/controller_concerns/redirect_for_embedded.rb
@@ -15,7 +15,7 @@ def embedded_param?
     end
 
     def loaded_directly_from_admin?
-      params[:embedded] == "1"
+      ShopifyApp.configuration.embedded_app? && params[:embedded] == "1"
     end
 
     def redirect_for_embedded

From e13f5a43de6b59047c5b735cce7cb4f572afe0f7 Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Wed, 14 Dec 2022 16:02:01 -0500
Subject: [PATCH 5/8] update tests to match expected behavior

---
 .../concerns/ensure_installed_test.rb         | 59 +++++++++++--------
 .../concerns/require_known_shop_test.rb       |  7 ++-
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/test/controllers/concerns/ensure_installed_test.rb b/test/controllers/concerns/ensure_installed_test.rb
index 745faa960..1125acd7a 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,36 @@ 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
+    ShopifyApp.configuration.stubs(:embedded_app?).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)
+    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"
+    get :index, params: { shop: shopify_domain }
+
+    assert_response :redirect
+  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)
@@ -87,26 +122,4 @@ def index
 
     assert_within_deprecation_schedule(version)
   end
-
-  test "redirects to redirect_for_embedded if app is no longer installed according to the API but not the session repository" 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)
-
-    embedded_redirect_url = "reverse_card_on_reverse_card.embedded.joke"
-    ShopifyApp.configuration.stubs(:embedded_redirect_url).returns(embedded_redirect_url)
-
-    @controller.expects(:redirect_for_embedded)
-    get :index, params: { shop: "shop1.myshopify.com", embedded: "1" }
-  end
 end
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"
 

From debe184611b6cd9f3fe1ad4f41117230f81126c8 Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Wed, 14 Dec 2022 16:27:01 -0500
Subject: [PATCH 6/8] Changelog updates

---
 CHANGELOG.md                                             | 1 +
 app/controllers/concerns/shopify_app/ensure_installed.rb | 4 ++--
 2 files changed, 3 insertions(+), 2 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 1214fde4e..b16829348 100644
--- a/app/controllers/concerns/shopify_app/ensure_installed.rb
+++ b/app/controllers/concerns/shopify_app/ensure_installed.rb
@@ -17,7 +17,7 @@ module EnsureInstalled
 
       before_action :check_shop_domain
       before_action :check_shop_known
-      before_action :validate_emebedded_session_is_active
+      before_action :validate_non_embedded_session
     end
 
     def current_shopify_domain
@@ -64,7 +64,7 @@ def shop_login
       url.to_s
     end
 
-    def validate_emebedded_session_is_active
+    def validate_non_embedded_session
       return if loaded_directly_from_admin?
 
       client = ShopifyAPI::Clients::Rest::Admin.new(session: installed_shop_session)

From 29199b1a668d713271c2268d15181da05f7efc17 Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Wed, 14 Dec 2022 17:06:32 -0500
Subject: [PATCH 7/8] raise error if not 401

---
 .../concerns/shopify_app/ensure_installed.rb  |  1 +
 .../concerns/ensure_installed_test.rb         | 39 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/app/controllers/concerns/shopify_app/ensure_installed.rb b/app/controllers/concerns/shopify_app/ensure_installed.rb
index b16829348..a79faf0f0 100644
--- a/app/controllers/concerns/shopify_app/ensure_installed.rb
+++ b/app/controllers/concerns/shopify_app/ensure_installed.rb
@@ -71,6 +71,7 @@ def validate_non_embedded_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/test/controllers/concerns/ensure_installed_test.rb b/test/controllers/concerns/ensure_installed_test.rb
index 1125acd7a..82c92d303 100644
--- a/test/controllers/concerns/ensure_installed_test.rb
+++ b/test/controllers/concerns/ensure_installed_test.rb
@@ -67,8 +67,6 @@ def index
   end
 
   test "redirects to login_url (oauth path) to reinstall the app if the store's session token is no longer valid" do
-    ShopifyApp.configuration.stubs(:embedded_app?).returns(true)
-
     session = mock
     ShopifyApp::SessionRepository.stubs(:retrieve_shop_session_by_shopify_domain).returns(session)
 
@@ -89,6 +87,43 @@ def index
     assert_response :redirect
   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

From 7f88b9f7c996bb58da9bce028f22f02500a85c0f Mon Sep 17 00:00:00 2001
From: Nelson Wittwer <nelson.wittwer@shopify.com>
Date: Wed, 14 Dec 2022 17:14:40 -0500
Subject: [PATCH 8/8] better redirect test

---
 .../concerns/ensure_installed_test.rb            | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/test/controllers/concerns/ensure_installed_test.rb b/test/controllers/concerns/ensure_installed_test.rb
index 82c92d303..aaddd6f6a 100644
--- a/test/controllers/concerns/ensure_installed_test.rb
+++ b/test/controllers/concerns/ensure_installed_test.rb
@@ -82,9 +82,17 @@ def index
     client.expects(:get).with(path: "shop").raises(uninstalled_http_error)
 
     shopify_domain = "shop1.myshopify.com"
-    get :index, params: { shop: shopify_domain }
+    host = "https://tunnel.vision.for.webhooks.com"
+
+    get :index, params: { shop: shopify_domain, host: host }
 
-    assert_response :redirect
+    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
@@ -104,7 +112,7 @@ def index
 
     shopify_domain = "shop1.myshopify.com"
 
-   assert_raises ShopifyAPI::Errors::HttpResponseError do
+    assert_raises ShopifyAPI::Errors::HttpResponseError do
       get :index, params: { shop: shopify_domain }
     end
   end
@@ -119,7 +127,7 @@ def index
 
     shopify_domain = "shop1.myshopify.com"
 
-   assert_raises RuntimeError do
+    assert_raises RuntimeError do
       get :index, params: { shop: shopify_domain }
     end
   end