Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No longer rescue non-shopify errors during OAuth #1807

Merged
merged 3 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 6 additions & 2 deletions app/controllers/shopify_app/callback_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions docs/Upgrading.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
24 changes: 12 additions & 12 deletions test/controllers/callback_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading