Merge pull request #1773 from Shopify/liz/delete-session-with-repository

Delete session with repository
Shopify · Jan 22, 2024 · 5dfc904 · 5dfc904
2 parents 6eea15b + 65c48a5
commit 5dfc904
Show file tree

Hide file tree

Showing 12 changed files with 83 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,6 @@
 Unreleased
 ----------
+* Fix session deletion for users with customized session storage[#1773](https://github.com/Shopify/shopify_app/pull/1773)
 
 21.9.0 (January 16, 2023)
 ----------

diff --git a/docs/shopify_app/sessions.md b/docs/shopify_app/sessions.md
@@ -4,23 +4,27 @@ Sessions are used to make contextual API calls for either a shop (offline sessio
 
 #### Table of contents
 
-- [Sessions](#sessions-1)
-  - [Types of session tokens](#types-of-session-tokens) - Shop (offline) v.s. User (online)
-  - [Session token storage](#session-token-storage)
-      - [Shop (offline) token storage](#shop-offline-token-storage)
-      - [User (online) token storage](#user-online-token-storage)
-      - [In-Memory Session Storage for Testing](#in-memory-session-storage-for-testing)
-      - [Customizing Session Storage with `ShopifyApp::SessionRepository`](#customizing-session-storage-with-shopifyappsessionrepository)
-  - [Loading Sessions](#loading-sessions)
+- [Sessions](#sessions)
+      - [Table of contents](#table-of-contents)
+  - [Sessions](#sessions-1)
+      - [Types of session tokens](#types-of-session-tokens)
+      - [Session token storage](#session-token-storage)
+        - [Shop (offline) token storage](#shop-offline-token-storage)
+        - [User (online) token storage](#user-online-token-storage)
+        - [In-memory Session Storage for testing](#in-memory-session-storage-for-testing)
+        - [Customizing Session Storage with `ShopifyApp::SessionRepository`](#customizing-session-storage-with-shopifyappsessionrepository)
+        - [⚠️  Custom Session Storage Requirements](#️--custom-session-storage-requirements)
+        - [Available `ActiveSupport::Concerns` that contains implementation of the above methods](#available-activesupportconcerns-that-contains-implementation-of-the-above-methods)
+    - [Loading Sessions](#loading-sessions)
       - [Getting Sessions with Controller Concerns](#getting-sessions-with-controller-concerns)
-        - [Shop session - "EnsureInstalled" ](#shop-sessions---ensureinstalled)
-        - [User session - "EnsureHasSession" ](#user-sessions---ensurehassession)
-      - [Getting Sessions from a Shop or User model record - "with_shopify_session"](#getting-sessions-from-a-shop-or-user-model-record---with_shopify_session)
-- [Access scopes](#access-scopes)
-  - [`ShopifyApp::ShopSessionStorageWithScopes`](#shopifyappshopsessionstoragewithscopes)
-  - [``ShopifyApp::UserSessionStorageWithScopes``](#shopifyappusersessionstoragewithscopes)
-- [Migrating from shop-based to user-based token strategy](#migrating-from-shop-based-to-user-based-token-strategy)
-- [Migrating from ShopifyApi::Auth::SessionStorage to ShopifyApp::SessionStorage](#migrating-from-shopifyapiauthsessionstorage-to-shopifyappsessionstorage)
+        - [**Shop Sessions - `EnsureInstalled`**](#shop-sessions---ensureinstalled)
+        - [User Sessions - `EnsureHasSession`](#user-sessions---ensurehassession)
+      - [Getting sessions from a Shop or User model record - 'with\_shopify\_session'](#getting-sessions-from-a-shop-or-user-model-record---with_shopify_session)
+  - [Access scopes](#access-scopes)
+    - [`ShopifyApp::ShopSessionStorageWithScopes`](#shopifyappshopsessionstoragewithscopes)
+    - [`ShopifyApp::UserSessionStorageWithScopes`](#shopifyappusersessionstoragewithscopes)
+  - [Migrating from shop-based to user-based token strategy](#migrating-from-shop-based-to-user-based-token-strategy)
+  - [Migrating from `ShopifyApi::Auth::SessionStorage` to `ShopifyApp::SessionStorage`](#migrating-from-shopifyapiauthsessionstorage-to-shopifyappsessionstorage)
 
 ## Sessions
 #### Types of session tokens
@@ -103,13 +107,15 @@ The custom **Shop** repository must implement the following methods:
 | `self.store(auth_session)`                        | `auth_session` (ShopifyAPI::Auth::Session) | -                         |
 | `self.retrieve(id)`                               | `id` (String)                              | ShopifyAPI::Auth::Session |
 | `self.retrieve_by_shopify_domain(shopify_domain)` | `shopify_domain` (String)                  | ShopifyAPI::Auth::Session |
+| `self.destroy_by_shopify_domain(shopify_domain)`  | `shopify_domain` (String)                  | -                         |
 
 The custom **User** repository must implement the following methods:
 | Method                                      | Parameters                          | Return Type                  |
 |---------------------------------------------|-------------------------------------|------------------------------|
 | `self.store(auth_session, user)`            | <li>`auth_session` (ShopifyAPI::Auth::Session)<br><li>`user` (ShopifyAPI::Auth::AssociatedUser) | - |
 | `self.retrieve(id)`                         | `id` (String)                       | `ShopifyAPI::Auth::Session`  |
 | `self.retrieve_by_shopify_user_id(user_id)` | `user_id` (String)                  | `ShopifyAPI::Auth::Session`  |
+| `self.destroy_by_shopify_user_id(user_id)`  | `user_id` (String)                  | -                            |
 
 
 These methods are already implemented as a part of the `User` and `Shop` models generated from this gem's generator.

diff --git a/lib/shopify_app/session/session_repository.rb b/lib/shopify_app/session/session_repository.rb
@@ -23,6 +23,14 @@ def retrieve_user_session_by_shopify_user_id(user_id)
         user_storage.retrieve_by_shopify_user_id(user_id)
       end
 
+      def destroy_shop_session_by_domain(shopify_domain)
+        shop_storage.destroy_by_shopify_domain(shopify_domain)
+      end
+
+      def destroy_user_session_by_shopify_user_id(user_id)
+        user_storage.destroy_by_shopify_user_id(user_id)
+      end
+
       def store_shop_session(session)
         shop_storage.store(session)
       end
@@ -73,18 +81,17 @@ def load_session(id)
       def delete_session(id)
         match = id.match(/^offline_(.*)/)
 
-        record = if match
+        if match
           domain = match[1]
           ShopifyApp::Logger.debug("Destroying session by domain - domain: #{domain}")
-          Shop.find_by(shopify_domain: match[1])
+          destroy_shop_session_by_domain(domain)
+
         else
           shopify_user_id = id.split("_").last
           ShopifyApp::Logger.debug("Destroying session by user - user_id: #{shopify_user_id}")
-          User.find_by(shopify_user_id: shopify_user_id)
+          destroy_user_session_by_shopify_user_id(shopify_user_id)
         end
 
-        record.destroy
-
         true
       end
 

diff --git a/lib/shopify_app/session/shop_session_storage.rb b/lib/shopify_app/session/shop_session_storage.rb
@@ -27,6 +27,10 @@ def retrieve_by_shopify_domain(domain)
         construct_session(shop)
       end
 
+      def destroy_by_shopify_domain(domain)
+        destroy_by(shopify_domain: domain)
+      end
+
       private
 
       def construct_session(shop)

diff --git a/lib/shopify_app/session/shop_session_storage_with_scopes.rb b/lib/shopify_app/session/shop_session_storage_with_scopes.rb
@@ -29,6 +29,10 @@ def retrieve_by_shopify_domain(domain)
         construct_session(shop)
       end
 
+      def destroy_by_shopify_domain(domain)
+        destroy_by(shopify_domain: domain)
+      end
+
       private
 
       def construct_session(shop)

diff --git a/lib/shopify_app/session/user_session_storage.rb b/lib/shopify_app/session/user_session_storage.rb
@@ -28,6 +28,10 @@ def retrieve_by_shopify_user_id(user_id)
         construct_session(user)
       end
 
+      def destroy_by_shopify_user_id(user_id)
+        destroy_by(shopify_user_id: user_id)
+      end
+
       private
 
       def construct_session(user)

diff --git a/lib/shopify_app/session/user_session_storage_with_scopes.rb b/lib/shopify_app/session/user_session_storage_with_scopes.rb
@@ -30,6 +30,10 @@ def retrieve_by_shopify_user_id(user_id)
         construct_session(user)
       end
 
+      def destroy_by_shopify_user_id(user_id)
+        destroy_by(shopify_user_id: user_id)
+      end
+
       private
 
       def construct_session(user)

diff --git a/test/shopify_app/session/session_repository_test.rb b/test/shopify_app/session/session_repository_test.rb
@@ -140,21 +140,21 @@ class SessionRepositoryTest < ActiveSupport::TestCase
     end
 
     test(".delete_session destroys a shop record") do
-      shop = MockShopInstance.new(shopify_domain: "shop", shopify_token: "token")
+      SessionRepository.shop_storage = InMemoryShopSessionStore
+      mock_session_id = "offline_abra-shop"
 
-      Shop.expects(:find_by).with(shopify_domain: "shop").returns(shop)
-      shop.expects(:destroy)
+      InMemoryShopSessionStore.expects(:destroy_by_shopify_domain).with("abra-shop")
 
-      SessionRepository.delete_session("offline_shop")
+      SessionRepository.delete_session(mock_session_id)
     end
 
     test(".delete_session destroys a user record") do
-      user = MockUserInstance.new(shopify_domain: "shop", shopify_token: "token")
+      SessionRepository.user_storage = InMemoryUserSessionStore
+      mock_session_id = "test_shop.myshopify.com_1234"
 
-      User.expects(:find_by).with(shopify_user_id: "1234").returns(user)
-      user.expects(:destroy)
+      InMemoryUserSessionStore.expects(:destroy_by_shopify_user_id).with("1234")
 
-      SessionRepository.delete_session("shop_1234")
+      SessionRepository.delete_session(mock_session_id)
     end
 
     private

diff --git a/test/shopify_app/session/shop_session_storage_test.rb b/test/shopify_app/session/shop_session_storage_test.rb
@@ -40,6 +40,12 @@ class ShopSessionStorageTest < ActiveSupport::TestCase
       assert_equal expected_session.access_token, session.access_token
     end
 
+    test ".destroy_by_shopify_domain destroys shop session records by JWT" do
+      ShopMockSessionStore.expects(:destroy_by).with(shopify_domain: TEST_SHOPIFY_DOMAIN)
+
+      ShopMockSessionStore.destroy_by_shopify_domain(TEST_SHOPIFY_DOMAIN)
+    end
+
     test ".store can store shop session records" do
       mock_shop_instance = MockShopInstance.new(id: 12345)
       mock_shop_instance.stubs(:save!).returns(true)

diff --git a/test/shopify_app/session/shop_session_storage_with_scopes_test.rb b/test/shopify_app/session/shop_session_storage_with_scopes_test.rb
@@ -46,6 +46,12 @@ class ShopSessionStorageWithScopesTest < ActiveSupport::TestCase
       assert_equal expected_session.scope, session.scope
     end
 
+    test ".destroy_by_shopify_domain destroys shop session records by JWT" do
+      ShopMockSessionStoreWithScopes.expects(:destroy_by).with(shopify_domain: TEST_SHOPIFY_DOMAIN)
+
+      ShopMockSessionStoreWithScopes.destroy_by_shopify_domain(TEST_SHOPIFY_DOMAIN)
+    end
+
     test ".store can store shop session records" do
       mock_shop_instance = MockShopInstance.new(id: 12345)
       mock_shop_instance.stubs(:save!).returns(true)

diff --git a/test/shopify_app/session/user_session_storage_test.rb b/test/shopify_app/session/user_session_storage_test.rb
@@ -46,6 +46,12 @@ class UserSessionStorageTest < ActiveSupport::TestCase
       assert_equal expected_session.access_token, session.access_token
     end
 
+    test ".destroy_by_shopify_user_id destroys user session by shopify_user_id" do
+      UserMockSessionStore.expects(:destroy_by).with(shopify_user_id: TEST_SHOPIFY_USER_ID)
+
+      UserMockSessionStore.destroy_by_shopify_user_id(TEST_SHOPIFY_USER_ID)
+    end
+
     test ".store can store user session record" do
       mock_user_instance = MockUserInstance.new(shopify_user_id: 100)
       mock_user_instance.stubs(:save!).returns(true)

diff --git a/test/shopify_app/session/user_session_storage_with_scopes_test.rb b/test/shopify_app/session/user_session_storage_with_scopes_test.rb
@@ -51,6 +51,12 @@ class UserSessionStorageWithScopesTest < ActiveSupport::TestCase
       assert_equal expected_session.scope, session.scope
     end
 
+    test ".destroy_by_shopify_user_id destroys user session by shopify_user_id" do
+      UserMockSessionStoreWithScopes.expects(:destroy_by).with(shopify_user_id: TEST_SHOPIFY_USER_ID)
+
+      UserMockSessionStoreWithScopes.destroy_by_shopify_user_id(TEST_SHOPIFY_USER_ID)
+    end
+
     test ".store can store user session record" do
       mock_user_instance = MockUserInstance.new(shopify_user_id: 100)
       mock_user_instance.stubs(:save!).returns(true)