EnsureBilling and EnsureAuthenticatedLinks works with token exchange #1833

zzooeeyy · 2024-04-25T16:05:41Z

Fixes: https://github.com/Shopify/develop-app-access/issues/295

What this PR does

Making sure EnsureBilling and EnsureAuthenticatedLinks can call methods defined in token exchange, by extracting some common helper methods for re-use
Fixed an infinite redirect loop caused by handling billing API error -
- Steps to reprduce:
  1. Open app
  2. EnsureHasSession checks if billing is valid
  3. Billing API responds with billing error
  4. EnsureBilling handles billing error by redirecting to shop /login with shop and host params
  5. /login starts auth again
  6. Auth CallbackController handles the callback, but it also checks billing, which could still return error - thus redirecting back to /login again. ↺
- Fix was to only redirect to shop /login withOUT shop and host params so that /login does not re-start auth.
TokenExchangeConcern -
- If id_session is invalid, redirect to embed the app first before redirecting to bounce page.

Before - Billing API error sent app into infinite redirect loop

infinite-loop-billing-error.mp4

After - Billing API error does not send app into infinite redirect loop

redirect-to-login.mp4

Token exchange works with EnsureBIlling and can load app after confirming charge

token-exchange-confirm-charge.mp4

Checklist

Before submitting the PR, please consider if any of the following are needed:

Update CHANGELOG.md if the changes would impact users
Update README.md, if appropriate.
Update any relevant pages in /docs, if necessary
For security fixes, the Disclosure Policy must be followed.

…param

zzooeeyy · 2024-04-25T17:50:21Z

lib/shopify_app/controller_concerns/ensure_billing.rb

@@ -45,8 +45,16 @@ def billing_required?
    end

    def handle_billing_error(error)
-      logger.info("#{error.message}: #{error.errors}")
-      redirect_to_login


This was causing an infinite redirect loop in auth code flow because the shop param gets added to the login path, and CallbackController would re-check billing on auto login with the shop param..

Changing the logic to redirect to login page without shop param.

zzooeeyy · 2024-04-25T17:51:52Z

lib/shopify_app/controller_concerns/login_protection.rb

-    end
-
-    def add_top_level_redirection_headers(url: nil, ignore_response_code: false)
+    def add_top_level_redirection_headers(ignore_response_code: false)


url argument is not used anywhere anymore, removing this.
Extracted helper RedirectForEmbedded.add_app_bridge_redirect_url_header(url, response) to be reused elsewhere

I don't think this is something apps can use, right? Otherwise we might be removing functionality they need.

Although I guess in that case they could just use the other concern.

hmmm yea, although this method added more checks around when to add the headers. I think I'll just add the existing behaviour back to avoid breaking behaviour for apps

zzooeeyy · 2024-04-25T17:53:15Z

lib/shopify_app/controller_concerns/login_protection.rb

-    def jwt_expire_at
-      expire_at = request.env["jwt.expire_at"]
-      return unless expire_at
-
-      expire_at - 5.seconds # 5s gap to start fetching new token in advance
-    end


Extracted these 'jwt' helpers to be inside WithShopifyIdToken concern for reusability

zzooeeyy · 2024-04-25T17:53:35Z

lib/shopify_app/controller_concerns/login_protection.rb

-    def jwt_shopify_domain
-      request.env["jwt.shopify_domain"]
-    end
-
-    def jwt_shopify_user_id
-      request.env["jwt.shopify_user_id"]
-    end


Extracted these 'jwt' helpers to be inside WithShopifyIdToken concern for reusability

paulomarg · 2024-04-29T13:40:28Z

lib/shopify_app/controller_concerns/login_protection.rb

-    end
-
-    def add_top_level_redirection_headers(url: nil, ignore_response_code: false)
+    def add_top_level_redirection_headers(ignore_response_code: false)


I don't think this is something apps can use, right? Otherwise we might be removing functionality they need.

Although I guess in that case they could just use the other concern.

paulomarg · 2024-04-29T14:48:31Z

lib/shopify_app/controller_concerns/token_exchange.rb

+      !params[:embedded].present? || params[:embedded] != "1"
+    end
+
+    def redirect_to_embed_app


Just out of curiosity - won't we have other places in the code that need to do this as well? Seems to me like this might also be extractable if that's the case.

paulomarg · 2024-04-29T14:49:35Z

lib/shopify_app/controller_concerns/token_exchange.rb

+      host = if params[:host]
+        params[:host]
+      elsif params[:shop]
+        Base64.encode64("#{sanitized_shop_name}/admin")


This is a little bit of an assumption - I think there are some cases where we might not want this (requests aren't coming from the admin), but we can deal with those if / when they happen.

I'm not quite sure if I understand? If they're using the EnsureHasSession concern, they want to validate requests that are coming from the admin right?

There are cases where host isn't necessarily shop/admin, so we're basically assuming that will be the case whenever we hit this path. Which I think should be fine and in the unlikely event that this fails on any specific interaction, we can look into it later.

paulomarg · 2024-04-29T14:53:04Z

test/shopify_app/controller_concerns/with_shopify_id_token_test.rb

+  end
+
+  test "#jwt_expire_at returns jwt.expire_at - 5 seconds from request env" do
+    expected_expire_at = Time.now.to_i


Are we locking down the time here? I wonder if this could lead to flaky tests if the second happens to shift during the run.

paulomarg

Nothing blocking.

EnsureBilling and EnsureAuthenticatedLinks works with token exchange

470164a

zzooeeyy force-pushed the ensure-other-concerns-work branch 2 times, most recently from 25026be to 4efdfb2 Compare April 25, 2024 17:46

EnsureBilling handles error by redirecting to loginpage without shop …

841857f

…param

zzooeeyy force-pushed the ensure-other-concerns-work branch from 4efdfb2 to 841857f Compare April 25, 2024 17:48

zzooeeyy commented Apr 25, 2024

View reviewed changes

Redirect to embed app before trying to redirect to patch session token

bdf4b23

zzooeeyy force-pushed the ensure-other-concerns-work branch from 7eddd95 to bdf4b23 Compare April 25, 2024 22:33

Update changelog

7b45c70

zzooeeyy marked this pull request as ready for review April 26, 2024 14:35

zzooeeyy requested a review from a team as a code owner April 26, 2024 14:36

zzooeeyy requested a review from rachel-carvalho April 26, 2024 14:36

zzooeeyy added the #gsd:35360 label Apr 26, 2024

paulomarg reviewed Apr 29, 2024

View reviewed changes

paulomarg approved these changes Apr 29, 2024

View reviewed changes

zzooeeyy added 4 commits April 29, 2024 10:27

add url back to LoginProtection.add_top_level_redirection_headers

3b912ac

Extracted EmbeddedApp::redirect_to_embed_app_in_admin

53b9c8b

Freeze time in test

cf5f209

Move EmbeddedApp concern inclusion to TokenExchangeConcern

0aef12b

paulomarg approved these changes Apr 29, 2024

View reviewed changes

Fix test

9d0123c

zzooeeyy merged commit 9e53256 into main Apr 29, 2024
7 checks passed

zzooeeyy deleted the ensure-other-concerns-work branch April 29, 2024 22:46

shopify-shipit bot temporarily deployed to rubygems May 2, 2024 20:10 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EnsureBilling and EnsureAuthenticatedLinks works with token exchange #1833

EnsureBilling and EnsureAuthenticatedLinks works with token exchange #1833

zzooeeyy commented Apr 25, 2024 •

edited

zzooeeyy Apr 25, 2024 •

edited

zzooeeyy Apr 25, 2024

paulomarg Apr 29, 2024

zzooeeyy Apr 29, 2024

zzooeeyy Apr 25, 2024

zzooeeyy Apr 25, 2024

paulomarg Apr 29, 2024

paulomarg Apr 29, 2024

paulomarg Apr 29, 2024

zzooeeyy Apr 29, 2024

paulomarg Apr 29, 2024 •

edited

paulomarg Apr 29, 2024

paulomarg left a comment

EnsureBilling and EnsureAuthenticatedLinks works with token exchange #1833

EnsureBilling and EnsureAuthenticatedLinks works with token exchange #1833

Conversation

zzooeeyy commented Apr 25, 2024 • edited

What this PR does

Before - Billing API error sent app into infinite redirect loop

After - Billing API error does not send app into infinite redirect loop

Token exchange works with EnsureBIlling and can load app after confirming charge

Checklist

zzooeeyy Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulomarg Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulomarg left a comment

Choose a reason for hiding this comment

zzooeeyy commented Apr 25, 2024 •

edited

zzooeeyy Apr 25, 2024 •

edited

paulomarg Apr 29, 2024 •

edited