Handle invalid session token #1823

zzooeeyy · 2024-04-03T21:10:29Z

closes https://github.com/Shopify/develop-app-access/issues/145

What this PR does

Handle invalid session token errors
Redirect to app-bridge-next bounce page if session token is invalid & Auth header doesn't exist
Respond with 401 if auth header exists and session token is invalid. If the app's front-end is using CDN version of app bridge next, it'll automatically be retried. otherwise, the 401 error will be exposed.

Tophatting-

📹 Redirecting to bounce page on initial app render (we are not parsing id_token from URL param yet, so session token will be invalid initially, this redirect shouldn't be hit as often once we are using id_token)
For testing - I created a temp global variable count that raises an "invalid session token" error on every other try.
- 📹 App configured with app bridge from CDN - The initial create failed from invalid session token, since X-Shopify-Retry-Invalid-Session-Request is set from the response, app bridge will retry create with new session token, thus passing the second time
- 📹 App configured with old version of app bridge NOT using the CDN - Response for create returns 401 in every other request from my test setup..

Checklist

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

[ will update once feature is released ] Update CHANGELOG.md if the changes would impact users
[will update once feature is released ] Update README.md, if appropriate.
[will update once feature is released ] Update any relevant pages in /docs, if necessary

rachel-carvalho · 2024-04-18T13:59:29Z

app/controllers/concerns/shopify_app/ensure_installed.rb

+      if ShopifyApp.configuration.use_new_embedded_auth_strategy?
+        include ShopifyApp::TokenExchange
+        include ShopifyApp::EmbeddedApp
+        around_action :activate_shopify_session


I noticed that the docs mention EnsureInstalled provides the installed_shop_session method and EnsureHasSession provides current_shopify_session. I think TokenExchange#activate_shopify_session provides current_shopify_session, right? Should it be different for EnsureInstalled? Or maybe provide both methods?

Yess, so EnsureInstalled concern itself implements the installed_shop_session method, so it'll still be able to be used. Having TokenExchange enabled, they'll just get both 'current_shopify_sessionandinstalled_shop_session` !

rachel-carvalho · 2024-04-18T14:02:04Z

lib/shopify_app/controller_concerns/token_exchange.rb

-      retrieve_session_from_token_exchange if current_shopify_session.blank? || should_exchange_expired_token?
+      begin
+        retrieve_session_from_token_exchange if current_shopify_session.blank? || should_exchange_expired_token?
+      rescue *INVALID_SHOPIFY_ID_TOKEN_ERRORS => e


Could we rescue these errors for the entire activate_shopify_session method? with_token_refetch might also end up raising this error, since it performs token exchange.

I was pairing with Mike and we thought that's not worth it since we already check for validity before we call token exchange, it'd just be adding extra work for the slightest chance that it might expire within that timeframe... There are leeways in-place to ensure we don't encounter these problems..?

Oh but what if we don't perform token exchange there because current_shopify_session exists and then do during with_token_refetch? In that case it wouldn't be double validation/rescue, right?

hmmm I think this will cause a "Double render/redirect" error since this is around the controller action.. so if we handle the error and respond with retry or redirect to bounce page and the controller action also renders... 🤔

If the action raises a token exchange error it will likely not have rendered yet, but you're right that it could happen if the app doesn't follow the pattern of rendering/redirecting as the last thing in the action 🤔

I looked up and saw there's a performed? method we can call in the controller to avoid a double render/redirect. What if we bounce unless performed?

test/routes/sessions_routes_test.rb

rachel-carvalho · 2024-04-18T14:11:59Z

test/shopify_app/controller_concerns/token_exchange_test.rb

@@ -178,13 +183,48 @@ class TokenExchangeControllerTest < ActionController::TestCase
    end
  end

+  [
+    ShopifyAPI::Errors::InvalidJwtTokenError,
+    ShopifyAPI::Errors::CookieNotFoundError,


Maybe we shouldn't respond to invalid cookies with a bounce page now that we'll be using the method that exclusively checks shopify_id_token.

Yea once I use the new utility method, we won't be handling CookieNotFoundError error anymore, but it'll be checking for MissingJwtTokenError instead

rachel-carvalho · 2024-04-18T14:18:34Z

test/shopify_app/controller_concerns/token_exchange_test.rb

+      expected_redirect_url = "/my-root/patch_shopify_id_token?my_param=for-keeps&shop=my-shop.myshopify.com"
+      expected_redirect_url += "&shopify-reload=%2Freloaded_path%3Fmy_param%3Dfor-keeps%26shop%3Dmy-shop.myshopify.com"


Nit: maybe we could URL encode the param for better readability of the test?

Suggested change

expected_redirect_url = "/my-root/patch_shopify_id_token?my_param=for-keeps&shop=my-shop.myshopify.com"

expected_redirect_url += "&shopify-reload=%2Freloaded_path%3Fmy_param%3Dfor-keeps%26shop%3Dmy-shop.myshopify.com"

reload_url = CGI.escape("/reloaded_path?my_param=for-keeps&shop=#{@shop}")

expected_redirect_url = "/my-root/patch_shopify_id_token?my_param=for-keeps&shop=my-shop.myshopify.com&shopify-reload=#{reload_url}"

rachel-carvalho · 2024-04-18T14:38:05Z

lib/shopify_app/controller_concerns/token_exchange.rb

-      # redirect_to("#{patch_session_token_url}?#{patch_session_token_params.to_query}", allow_other_host: true)
-      # end
+      redirect_to(
+        "#{patch_shopify_id_token_url}?#{patch_shopify_id_token_params.to_query}",


I wish there was a way to use the Rails URL helper method patch_shopify_id_token_path here which already does most of the URL assembly work for us, but the way our tests are setup using with_routing it's not including engine's routes, so I think it would be a bit too much work for it to be worth it.

paulomarg · 2024-04-18T14:39:05Z

lib/shopify_app/controller_concerns/token_exchange.rb

@@ -5,8 +5,18 @@ module TokenExchange
    extend ActiveSupport::Concern
    include ShopifyApp::AdminAPI::WithTokenRefetch

+    INVALID_SHOPIFY_ID_TOKEN_ERRORS = [
+      ShopifyAPI::Errors::CookieNotFoundError,


Would we ever have a cookie present for token exchange? If it's only meant to run for embedded apps there should never be any cookies involved, right?

Yea cookies are never involved.. but the way ShopifyAPI::current_session_id is setup, it'll throw a "CookieNotFoundError" if we pass in a nil auth header (which will be the case for the initial server load when we're not using id_token from URL params), so once I change this to use the new util method, we won't be catching this CookieNotFoundError but MissingJwtTokenError instead.

Hmm that's fair. I guess we can't change this without it potentially breaking apps out there, right?

Yea exactly :(

…_shopify_session

rachel-carvalho

Looks good to me, thanks Zoey!

zzooeeyy assigned rachel-carvalho Apr 3, 2024

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

zzooeeyy force-pushed the handle-invalid-session-token branch 2 times, most recently from e3e43cf to eeb490b Compare April 10, 2024 15:49

zzooeeyy added 4 commits April 16, 2024 16:54

Add new route to patch session token from app bridge next

d3a0aa1

Introduce token exchange to ensureInstalled concern

ed222c9

Rescuing invalidJWT token errors and redirecting to bounce page

ce9e370

Rename session token to shopify_id token

231b1a7

zzooeeyy force-pushed the handle-invalid-session-token branch from fadc394 to 815f60a Compare April 16, 2024 22:54

Add tests in token exchange concern for responding to invalid shopify id

3bcaff0

zzooeeyy force-pushed the handle-invalid-session-token branch from 815f60a to 3bcaff0 Compare April 16, 2024 22:58

Add test for patching shopify id token bounce page

a233974

zzooeeyy force-pushed the handle-invalid-session-token branch from 659f8a3 to 6ff99df Compare April 16, 2024 23:29

Fix fragile tests?

b65aac4

zzooeeyy force-pushed the handle-invalid-session-token branch from 6ff99df to b65aac4 Compare April 16, 2024 23:33

Remove todo

f804df3

zzooeeyy marked this pull request as ready for review April 17, 2024 19:26

zzooeeyy requested a review from a team as a code owner April 17, 2024 19:26

zzooeeyy unassigned rachel-carvalho Apr 17, 2024

zzooeeyy requested a review from rachel-carvalho April 17, 2024 19:27

rachel-carvalho reviewed Apr 18, 2024

View reviewed changes

paulomarg approved these changes Apr 18, 2024

View reviewed changes

zzooeeyy added 2 commits April 18, 2024 10:00

Format URL string in token exchange test

d925d90

handles invalid session token error in all of TokenExchange::activate…

9712272

…_shopify_session

rachel-carvalho approved these changes Apr 19, 2024

View reviewed changes

zzooeeyy merged commit ea884a6 into main Apr 19, 2024
10 checks passed

zzooeeyy deleted the handle-invalid-session-token branch April 19, 2024 18:16

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

Handle invalid session token #1823

Handle invalid session token #1823

zzooeeyy commented Apr 3, 2024 •

edited

Loading

rachel-carvalho Apr 18, 2024

zzooeeyy Apr 18, 2024

rachel-carvalho Apr 18, 2024

zzooeeyy Apr 18, 2024

rachel-carvalho Apr 18, 2024

zzooeeyy Apr 18, 2024

rachel-carvalho Apr 19, 2024

rachel-carvalho Apr 18, 2024

zzooeeyy Apr 18, 2024

rachel-carvalho Apr 18, 2024

zzooeeyy Apr 18, 2024

rachel-carvalho Apr 18, 2024

paulomarg Apr 18, 2024

zzooeeyy Apr 18, 2024

paulomarg Apr 18, 2024

zzooeeyy Apr 18, 2024

rachel-carvalho left a comment

		expected_redirect_url = "/my-root/patch_shopify_id_token?my_param=for-keeps&shop=my-shop.myshopify.com"
		expected_redirect_url += "&shopify-reload=%2Freloaded_path%3Fmy_param%3Dfor-keeps%26shop%3Dmy-shop.myshopify.com"

Handle invalid session token #1823

Handle invalid session token #1823

Conversation

zzooeeyy commented Apr 3, 2024 • edited Loading

What this PR does

Tophatting-

Checklist

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

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

rachel-carvalho left a comment

Choose a reason for hiding this comment

zzooeeyy commented Apr 3, 2024 •

edited

Loading