From 1dc3a79c10c1b94f517e6978f51add262959b342 Mon Sep 17 00:00:00 2001 From: Nelson Wittwer Date: Tue, 31 Oct 2023 09:10:16 -0400 Subject: [PATCH 1/3] Do not allow registration to mandatory webhooks --- lib/shopify_api/webhooks/registry.rb | 19 +++++++++++++++++++ test/webhooks/registry_test.rb | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/lib/shopify_api/webhooks/registry.rb b/lib/shopify_api/webhooks/registry.rb index b19265b5e..f6bad3cfa 100644 --- a/lib/shopify_api/webhooks/registry.rb +++ b/lib/shopify_api/webhooks/registry.rb @@ -5,6 +5,11 @@ module ShopifyAPI module Webhooks class Registry @registry = T.let({}, T::Hash[String, Registration]) + MANDATORY_TOPICS = T.let([ + "SHOP_REDACT", + "CUSTOMERS_REDACT", + "CUSTOMERS_DATA_REQUEST" + ].freeze, T::Array[String]) class << self extend T::Sig @@ -17,6 +22,8 @@ class << self metafield_namespaces: T.nilable(T::Array[String])).void end def add_registration(topic:, delivery_method:, path:, handler: nil, fields: nil, metafield_namespaces: nil) + return if trying_to_register_mandatory_topic?(topic) + @registry[topic] = case delivery_method when :pub_sub Registrations::PubSub.new(topic: topic, path: path, fields: fields, @@ -42,6 +49,11 @@ def clear @registry.clear end + sig { returns(T::Hash[String, ShopifyAPI::Webhooks::Registrations::Http]) } + def topics_in_registry + @registry + end + sig do params( topic: String, @@ -213,6 +225,13 @@ def send_register_request(client, registration, webhook_id) def registration_sucessful?(body, mutation_name) !body.dig("data", mutation_name, "webhookSubscription").nil? end + + # Mandatory webhooks are subscribed to via the partner dashboard not the API + # https://shopify.dev/docs/apps/webhooks/configuration/mandatory-webhooks + sig { params(topic: String).returns(T::Boolean) } + def trying_to_register_mandatory_topic?(topic) + MANDATORY_TOPICS.include?(topic) + end end end end diff --git a/test/webhooks/registry_test.rb b/test/webhooks/registry_test.rb index 2c966fc1b..7cebe564b 100644 --- a/test/webhooks/registry_test.rb +++ b/test/webhooks/registry_test.rb @@ -262,6 +262,24 @@ def test_get_webhook_id_with_graphql_errors assert_equal("Failed to fetch webhook from Shopify: some error", exception.message) end + def test_registrations_to_mandatory_topics_are_ignored + ShopifyAPI::Webhooks::Registry.clear + + ShopifyAPI::Webhooks::Registry::MANDATORY_TOPICS.each do |topic| + ShopifyAPI::Webhooks::Registry.add_registration( + topic: topic, + delivery_method: :http, + path: "some_path_under_the_rainbow", + handler: TestHelpers::FakeWebhookHandler.new( + lambda do |topic, shop, body| + end, + ), + ) + end + + assert_empty(ShopifyAPI::Webhooks::Registry.topics_in_registry) + end + private def do_registration_test(delivery_method, path, fields: nil, metafield_namespaces: nil) @@ -359,6 +377,7 @@ def do_registration_check_error_test(delivery_method, path) ) end end + end end end From 652b65597f7563fa5d8093fc89cd038836b0dd65 Mon Sep 17 00:00:00 2001 From: Nelson Wittwer Date: Tue, 31 Oct 2023 09:44:46 -0400 Subject: [PATCH 2/3] skip unregister calls for mandatory webhooks --- lib/shopify_api/webhooks/registry.rb | 17 +++++++---------- test/webhooks/registry_test.rb | 15 +++++++++++++-- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/lib/shopify_api/webhooks/registry.rb b/lib/shopify_api/webhooks/registry.rb index f6bad3cfa..cbbcc8cef 100644 --- a/lib/shopify_api/webhooks/registry.rb +++ b/lib/shopify_api/webhooks/registry.rb @@ -6,9 +6,9 @@ module Webhooks class Registry @registry = T.let({}, T::Hash[String, Registration]) MANDATORY_TOPICS = T.let([ - "SHOP_REDACT", - "CUSTOMERS_REDACT", - "CUSTOMERS_DATA_REQUEST" + "shop/redact", + "customers/redact", + "customers/data_request" ].freeze, T::Array[String]) class << self @@ -22,7 +22,7 @@ class << self metafield_namespaces: T.nilable(T::Array[String])).void end def add_registration(topic:, delivery_method:, path:, handler: nil, fields: nil, metafield_namespaces: nil) - return if trying_to_register_mandatory_topic?(topic) + return if mandatory_webhook_topic?(topic) @registry[topic] = case delivery_method when :pub_sub @@ -49,11 +49,6 @@ def clear @registry.clear end - sig { returns(T::Hash[String, ShopifyAPI::Webhooks::Registrations::Http]) } - def topics_in_registry - @registry - end - sig do params( topic: String, @@ -113,6 +108,8 @@ def register_all(session:) ).returns(T::Hash[String, T.untyped]) end def unregister(topic:, session:) + return {"response": nil} if mandatory_webhook_topic?(topic) + client = Clients::Graphql::Admin.new(session: session) webhook_id = get_webhook_id(topic: topic, client: client) @@ -229,7 +226,7 @@ def registration_sucessful?(body, mutation_name) # Mandatory webhooks are subscribed to via the partner dashboard not the API # https://shopify.dev/docs/apps/webhooks/configuration/mandatory-webhooks sig { params(topic: String).returns(T::Boolean) } - def trying_to_register_mandatory_topic?(topic) + def mandatory_webhook_topic?(topic) MANDATORY_TOPICS.include?(topic) end end diff --git a/test/webhooks/registry_test.rb b/test/webhooks/registry_test.rb index 7cebe564b..f4b4c812e 100644 --- a/test/webhooks/registry_test.rb +++ b/test/webhooks/registry_test.rb @@ -206,6 +206,17 @@ def test_unregister_fail_with_user_errors assert_equal("Failed to delete webhook from Shopify: some error", exception.message) end + def test_unregister_to_mandatory_topics_are_skipped + ShopifyAPI::Clients::Graphql::Admin.expects(:new).never + + ShopifyAPI::Webhooks::Registry::MANDATORY_TOPICS.each do |topic| + ShopifyAPI::Webhooks::Registry.unregister( + topic: topic, + session: @session, + ) + end + end + def test_get_webhook_id_success stub_request(:post, @url) .with(body: JSON.dump({ query: queries[:fetch_id_query], variables: nil })) @@ -265,6 +276,8 @@ def test_get_webhook_id_with_graphql_errors def test_registrations_to_mandatory_topics_are_ignored ShopifyAPI::Webhooks::Registry.clear + ShopifyAPI::Webhooks::Registrations::Http.expects(:new).never + ShopifyAPI::Webhooks::Registry::MANDATORY_TOPICS.each do |topic| ShopifyAPI::Webhooks::Registry.add_registration( topic: topic, @@ -276,8 +289,6 @@ def test_registrations_to_mandatory_topics_are_ignored ), ) end - - assert_empty(ShopifyAPI::Webhooks::Registry.topics_in_registry) end private From 13c09fe1b71df195bede5f24523384d10cb7db8a Mon Sep 17 00:00:00 2001 From: Nelson Wittwer Date: Tue, 31 Oct 2023 09:55:02 -0400 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 1 + lib/shopify_api/webhooks/registry.rb | 4 ++-- test/webhooks/registry_test.rb | 1 - 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9dc7ea31..49d402858 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ Note: For changes to the API, see https://shopify.dev/changelog?filter=api ## Unreleased +- [#1237](https://github.com/Shopify/shopify-api-ruby/pull/1237) Skip mandatory webhook topic registration/unregistrations ## 13.2.0 diff --git a/lib/shopify_api/webhooks/registry.rb b/lib/shopify_api/webhooks/registry.rb index cbbcc8cef..e52204cc9 100644 --- a/lib/shopify_api/webhooks/registry.rb +++ b/lib/shopify_api/webhooks/registry.rb @@ -8,7 +8,7 @@ class Registry MANDATORY_TOPICS = T.let([ "shop/redact", "customers/redact", - "customers/data_request" + "customers/data_request", ].freeze, T::Array[String]) class << self @@ -108,7 +108,7 @@ def register_all(session:) ).returns(T::Hash[String, T.untyped]) end def unregister(topic:, session:) - return {"response": nil} if mandatory_webhook_topic?(topic) + return { "response": nil } if mandatory_webhook_topic?(topic) client = Clients::Graphql::Admin.new(session: session) diff --git a/test/webhooks/registry_test.rb b/test/webhooks/registry_test.rb index f4b4c812e..1d0ab4783 100644 --- a/test/webhooks/registry_test.rb +++ b/test/webhooks/registry_test.rb @@ -388,7 +388,6 @@ def do_registration_check_error_test(delivery_method, path) ) end end - end end end