From f617469e6a24707c0f90f8317f642984ec6010ab Mon Sep 17 00:00:00 2001 From: Nelson Date: Thu, 7 Mar 2024 10:27:33 -0500 Subject: [PATCH 1/2] No longer rescue non-shopify errors during OAuth (#1807) * no longer catch non-shopify errors * No more rescues for non-shopify apps during oauth * rubocop --- CHANGELOG.md | 2 ++ .../shopify_app/callback_controller.rb | 8 +++++-- docs/Upgrading.md | 3 +++ test/controllers/callback_controller_test.rb | 24 +++++++++---------- 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e6b33833..c49720b77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ Unreleased * ⚠️ [Breaking] Bumps minimum supported Ruby version to 3.0. Bumps `shopify_api` to 14.0 [1801](https://github.com/Shopify/shopify_app/pull/1801) * ⚠️ [Breaking] Removes deprecated controller concerns that were renamed in `v21.10.0`. [1805](https://github.com/Shopify/shopify_app/pull/1805) * ⚠️ [Breaking] Removes deprecated `ScripttagManager`. We realize there was communication error in our logging where we logged future deprecation instead of our inteded removal. Since we have been logging that for 2 years we felt we'd move forward with the removal instead pushing this off until the next major release. [1806](https://github.com/Shopify/shopify_app/pull/1806) +* ⚠️ [Breaking] No longer rescues non-shopify API errors during OAuth + callback [1807](https://github.com/Shopify/shopify_app/pull/1807) * Make type param for webhooks route optional. This will fix a bug with CLI initiated webhooks.[1786](https://github.com/Shopify/shopify_app/pull/1786) * Fix redirecting to login when we catch a 401 response from Shopify, so that it can also handle cases where the app is already embedded when that happens.[1787](https://github.com/Shopify/shopify_app/pull/1787) * Always register webhooks with offline sessions.[1788](https://github.com/Shopify/shopify_app/pull/1788) diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb index 759ae5452..cedd3369c 100644 --- a/app/controllers/shopify_app/callback_controller.rb +++ b/app/controllers/shopify_app/callback_controller.rb @@ -10,8 +10,12 @@ def callback begin api_session, cookie = validated_auth_objects rescue => error - error.class.module_parent == ShopifyAPI::Errors ? callback_rescue(error) : deprecate_callback_rescue(error) - return respond_with_error + if error.class.module_parent == ShopifyAPI::Errors + callback_rescue(error) + return respond_with_error + else + raise error + end end save_session(api_session) if api_session diff --git a/docs/Upgrading.md b/docs/Upgrading.md index 18d360a06..68c63c803 100644 --- a/docs/Upgrading.md +++ b/docs/Upgrading.md @@ -59,6 +59,9 @@ Script tag usage has largely been replaced with the adoption of [theme app exten If you find yourself still using Scipt Tags and want to continue the pattern of declarative management of script tags this gem used to use, we recommend porting the logic [the manager used in prior versions](https://github.com/Shopify/shopify_app/blob/2336fabc6d0b45a4dee3f336455dace4d2d88bc4/lib/shopify_app/managers/scripttags_manager.rb#L4) and implementing it in a [post authentication job](https://github.com/Shopify/shopify_app/blob/main/docs/shopify_app/authentication.md#run-jobs-after-the-oauth-flow). This is the recommended flow to create script tags (or any other logic) for stores that install your app. +#### No longer rescue non-shopify API errors during customized OAuth flow +If you have customized authentication logic and are counting on the `CallbackController` to catch your error and redirect to login, you'll need to catch that error and redirect to `login_url_with_optional_shop`. + ## Upgrading to 21.3.0 The `Itp` controller concern has been removed from `LoginProtection` which is included by the `Authenticated`/`EnsureHasSession` controller concern. If any of your controllers are dependant on methods from `Itp` then you can include `ShopifyApp::Itp` directly. diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb index a68b5c78e..96eb53846 100644 --- a/test/controllers/callback_controller_test.rb +++ b/test/controllers/callback_controller_test.rb @@ -84,21 +84,21 @@ class CallbackControllerTest < ActionController::TestCase params: { shop: SHOP_DOMAIN, code: "code", state: "state", timestamp: "timestamp", host: "host", hmac: "hmac" } end - test "#callback rescued non-shopify errors will be deprecated" do + test "#callback rescued non-shopify errors are re-raised" do error = StandardError.new ShopifyAPI::Auth::Oauth.expects(:validate_auth_callback).raises(error) - message = <<~EOS - An error of type #{error.class} was rescued. This is not part of `ShopifyAPI::Errors`, which could indicate a - bug in your app, or a bug in the shopify_app gem. Future versions of the gem may re-raise this error rather - than rescuing it. - EOS - version = "22.0.0" - - assert_within_deprecation_schedule(version) - ShopifyApp::Logger.expects(:deprecated).with(message, version) - get :callback, - params: { shop: SHOP_DOMAIN, code: "code", state: "state", timestamp: "timestamp", host: "host", hmac: "hmac" } + assert_raise StandardError do + get :callback, + params: { + shop: SHOP_DOMAIN, + code: "code", + state: "state", + timestamp: "timestamp", + host: "host", + hmac: "hmac", + } + end end test "#callback calls ShopifyAPI::Auth::Oauth.validate_auth_callback" do From 2884e961afa04dfbad8c06a3d90addafa1f057a5 Mon Sep 17 00:00:00 2001 From: Paulo Margarido <64600052+paulomarg@users.noreply.github.com> Date: Thu, 7 Mar 2024 10:59:34 -0500 Subject: [PATCH 2/2] Improve issue template --- .github/ISSUE_TEMPLATE/bug-report.md | 41 ++++++++++++++++------------ 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug-report.md b/.github/ISSUE_TEMPLATE/bug-report.md index 1fe0a2f4f..cd3dc0bf5 100644 --- a/.github/ISSUE_TEMPLATE/bug-report.md +++ b/.github/ISSUE_TEMPLATE/bug-report.md @@ -6,36 +6,41 @@ labels: "Type: Bug 🐛" # Issue summary - -- `shopify_api` version: -- `shopify_app` version: -- Ruby version: -- Operating system: - -``` -// Paste any relevant logs here -``` - ## Expected behavior - +What do you think should happen? ## Actual behavior - +What actually happens? ## Steps to reproduce the problem 1. 1. 1. + +## Debug logs + +``` +// Paste any relevant logs here +```