From 19497b03ac5e6ecc5d157820f8a1a63f9ce822ba Mon Sep 17 00:00:00 2001 From: Elizabeth Kenyon Date: Tue, 9 Jan 2024 16:18:56 -0600 Subject: [PATCH] Delete session with repository users can customize their session storage so we cant rely on models like shop --- CHANGELOG.md | 1 + docs/shopify_app/sessions.md | 38 +++++++++++-------- lib/shopify_app/session/session_repository.rb | 17 ++++++--- .../session/shop_session_storage.rb | 4 ++ .../shop_session_storage_with_scopes.rb | 4 ++ .../session/user_session_storage.rb | 4 ++ .../user_session_storage_with_scopes.rb | 4 ++ .../session/session_repository_test.rb | 16 ++++---- .../session/shop_session_storage_test.rb | 6 +++ .../shop_session_storage_with_scopes_test.rb | 6 +++ .../session/user_session_storage_test.rb | 6 +++ .../user_session_storage_with_scopes_test.rb | 6 +++ 12 files changed, 83 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5d96c4e0..897465083 100644 --- 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) * Fix `add_webhook` generator to create the webhook jobs under the correct directory[#1748](https://github.com/Shopify/shopify_app/pull/1748) * Add support for metafield_namespaces in webhook registration [#1745](https://github.com/Shopify/shopify_app/pull/1745) diff --git a/docs/shopify_app/sessions.md b/docs/shopify_app/sessions.md index b5af10e16..140e4efa1 100644 --- 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,6 +107,7 @@ 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 | @@ -110,6 +115,7 @@ The custom **User** repository must implement the following methods: | `self.store(auth_session, user)` |
  • `auth_session` (ShopifyAPI::Auth::Session)
  • `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 index e02ff4771..d1652e7b1 100644 --- 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 index a1cc27ef7..f3cc08171 100644 --- 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 index 5312608e5..24458e421 100644 --- 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 index 78a77d1af..1cba9b2bf 100644 --- 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 index 440226862..446ef5544 100644 --- 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 index 1c18bce48..2ed23a37f 100644 --- 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 index cecad0696..02bfc6524 100644 --- 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 index 87f1e08a2..663b1a9aa 100644 --- 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 index 7f399f361..6f1037209 100644 --- 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 index 1bb47188b..f1e5457e3 100644 --- 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)