Add token exchange concern #1817

zzooeeyy · 2024-03-25T13:36:04Z

What this PR does

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

Tophat:
📹 https://videobin.shopify.io/v/J4j8l2

Happy path for token exchange

Add token exchange concern to retrieve current session. This replaces "LoginProtection" logic when "use_new_embedded_auth_strategy" is turned on.
"EnsureHasSession" concern can be used with token exchange.

Note on what else needs to be implemented in other tasks:

Handle invalid or missing session tokens
Handle invalid access tokens
Handle post auth tasks (webhooks, jobs, etc)
Add support to use id_token from URL params.
Ensure billing works with token exchange

Impact on existing apps:

None - All the code is gated behind the configuration flag: config.wip_new_embedded_auth_strategy

Checklist

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

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

paulomarg

Had some general questions, not necessarily things we need / want to apply.

paulomarg · 2024-03-25T13:40:31Z

app/controllers/concerns/shopify_app/ensure_has_session.rb

+
+      if ShopifyApp.configuration.use_new_embedded_auth_strategy?
+        include ShopifyApp::RetrieveSessionFromTokenExchange
+        around_action :activate_shopify_session


Just wondering - is this repeated in both paths because we need the after_action to kick in after this in the else branch?

Not that I mind the repetition, just curious.

I repeated it so that it's explicit in which action is called, if we leave this as a shared/common action outside of this condition, then we're implicitly forcing the method name to be the same between the 2 concerns. Which would also work, but I feel like less implicit dependency the better? 🤔

Yeah I agree - less magic, more good.

paulomarg · 2024-03-25T13:42:11Z

app/controllers/concerns/shopify_app/ensure_has_session.rb

@@ -6,14 +6,20 @@ module EnsureHasSession

    included do
      include ShopifyApp::Localization
-      include ShopifyApp::LoginProtection
+
+      if ShopifyApp.configuration.use_new_embedded_auth_strategy?


I believe these conditions should only apply if the app is embedded, right? We'd still want to use LoginProtection for non-embedded?

Yes that's right, use_new_embedded_auth_strategy is only true if app is embedded AND has wip_new_embedded_auth_strategy enabled.

Oh perfect, I missed that part :)

paulomarg · 2024-03-25T14:15:00Z

lib/shopify_app/controller_concerns/retrieve_session_from_token_exchange.rb

+# frozen_string_literal: true
+
+module ShopifyApp
+  module RetrieveSessionFromTokenExchange


Could we call this just TokenExchange? Would that be confusing?

paulomarg · 2024-03-25T14:15:38Z

lib/shopify_app/controller_concerns/retrieve_session_from_token_exchange.rb

+  module RetrieveSessionFromTokenExchange
+    extend ActiveSupport::Concern
+
+    def activate_shopify_session


A couple of these methods seem to be the same for both cases, right? Should we create one or more helper concerns for them?

yes and no --?

Some of the functions might change as we add more error handling (e.g. current_shopify_session might raise and rescue error so we can redirect to bounce page to get session token)

Some methods are only extracted to call 1 single method from the ShopifyAPI, which I think we should keep as is to reduce even more magic from more concerns..

I think the only method that makes sense is online_token_configured? this doesn't seem like it belongs in the concerns but in the configuration itself.. I've extracted this one so we don't check the business logic from the concerns - bcc2e1e

That's fair - I think if we expect the code to change then it won't make sense to extract them. We can live with some minor duplications here, as long as we're aware we'll need to consider both cases when making changes.

paulomarg · 2024-03-25T14:16:19Z

lib/shopify_app/controller_concerns/retrieve_session_from_token_exchange.rb

+      # TODO: Right now JWT Middleware only updates env['jwt.shopify_domain'] from request headers tokens,
+      # which won't work for new installs.
+      # we need to update the middleware to also update the env['jwt.shopify_domain'] from the query params


This should happen as part of this PR, right?

Hmmm not necessarily. We won't always have session token in the query params (beta flag can still be turned off), so our feature shouldn't rely on having session tokens from the params. Thus I think that could be handled as a part of another ticket to add support for using id_token from param.

But won't that mean apps won't be able to do the first install flow with the setting turned on?

Yess, so the EnsureInstalled concern doesn't work yet. We'll have to handle reloading from AppBridge with session token in request headers (part of this ticket: https://github.com/Shopify/develop-app-access/issues/145) to make the first install flow work. That's why EnsureInstalled doesn't work with token exchange yet

That makes sense

paulomarg · 2024-03-25T14:17:26Z

lib/shopify_app/controller_concerns/retrieve_session_from_token_exchange.rb

+        # respond_to_invalid_session_token
+        return
+      rescue ShopifyAPI::Errors::HttpResponseError => error
+        ShopifyApp::Logger.info(


Should we log these error messages as info?

Good catch, I'll change them to :error!

paulomarg · 2024-03-25T16:07:25Z

lib/shopify_app/controller_concerns/retrieve_session_from_token_exchange.rb

+      # TODO: Right now JWT Middleware only updates env['jwt.shopify_domain'] from request headers tokens,
+      # which won't work for new installs.
+      # we need to update the middleware to also update the env['jwt.shopify_domain'] from the query params


That makes sense

paulomarg · 2024-03-25T16:08:12Z

lib/shopify_app/controller_concerns/retrieve_session_from_token_exchange.rb

+  module RetrieveSessionFromTokenExchange
+    extend ActiveSupport::Concern
+
+    def activate_shopify_session


That's fair - I think if we expect the code to change then it won't make sense to extract them. We can live with some minor duplications here, as long as we're aware we'll need to consider both cases when making changes.

paulomarg · 2024-03-25T16:10:37Z

I think this makes sense, assuming the TODOs will be resolved before we release :)

ragalie · 2024-03-26T16:36:36Z

This looks good to me! One question: should we have unit tests around the TokenExchange class?

paulomarg · 2024-03-26T17:33:13Z

Great callout, we should definitely add some tests for the new paths.

zzooeeyy · 2024-03-26T17:59:33Z

This looks good to me! One question: should we have unit tests around the TokenExchange class?

Yes we totally should, I was going to tackle that after we handle all the special invalid token cases. That way we can ensure we have all the test cases and ways to recover in 1 go... I've added that another task so we don't forget about it:

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

ragalie · 2024-03-26T18:08:50Z

I was going to tackle that after we handle all the special invalid token cases

I'd encourage you to test the happy path in the happy path PR, and the sad path in the sad path PR! Doing all the tests at once can make it harder to discover what caused errors, whereas doing them more iteratively makes it more obvious when a change is breaking an assumption.

zzooeeyy · 2024-03-27T15:09:01Z

Thanks @ragalie & @paulomarg -- I've updated this PR with tests!

ragalie · 2024-03-27T17:23:29Z

test/shopify_app/controller_concerns/token_exchange_test.rb

+
+  test "Exchanges offline token when session doesn't exist" do
+    with_application_test_routes do
+      ShopifyAPI::Utils::SessionUtils.expects(:current_session_id).twice.with(


I'd maybe go with a stubs here instead of an expectation. It doesn't seem essential to the test outcome that this gets called twice; that's an implementation detail. The thing we care about is that the exchange_token happens once, which you're expecting below. What do you think?

Sure I don't disagree with that!

ragalie

Left a comment on test tactics but otherwise LGTM!

zzooeeyy requested a review from a team as a code owner March 25, 2024 13:36

paulomarg reviewed Mar 25, 2024

View reviewed changes

zzooeeyy force-pushed the token-exchange-happy-path branch from 5e741e5 to 4fbe18c Compare March 25, 2024 16:08

zzooeeyy added 3 commits March 25, 2024 10:09

Add token exchange concern

8c72def

Extract to configuration

0efe87f

Rename concern to TokenExchange

b53fe3d

zzooeeyy force-pushed the token-exchange-happy-path branch from 4fbe18c to b53fe3d Compare March 25, 2024 16:09

paulomarg approved these changes Mar 25, 2024

View reviewed changes

Move common action 'check_shop_domain'

fcef5f2

zzooeeyy added the #gsd:35360 label Mar 25, 2024

zzooeeyy mentioned this pull request Mar 25, 2024

Redirect to shopify managed install path during login #1818

Merged

zzooeeyy added 3 commits March 26, 2024 18:54

Add test case to EnsureHasSessionTest

c3c6fb5

Add tests for token exchange concern

525aeea

Add more tests for token exchange

6079cf4

ragalie reviewed Mar 27, 2024

View reviewed changes

ragalie approved these changes Mar 27, 2024

View reviewed changes

Replace expects with stubs

5448d55

zzooeeyy merged commit 932f5f3 into main Mar 27, 2024
6 of 7 checks passed

zzooeeyy deleted the token-exchange-happy-path branch March 27, 2024 17:47

shopify-shipit bot temporarily deployed to rubygems April 9, 2024 14:45 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add token exchange concern #1817

Add token exchange concern #1817

zzooeeyy commented Mar 25, 2024 •

edited

paulomarg left a comment

paulomarg Mar 25, 2024

zzooeeyy Mar 25, 2024

paulomarg Mar 25, 2024

paulomarg Mar 25, 2024

zzooeeyy Mar 25, 2024

paulomarg Mar 25, 2024

paulomarg Mar 25, 2024

paulomarg Mar 25, 2024

zzooeeyy Mar 25, 2024

paulomarg Mar 25, 2024

paulomarg Mar 25, 2024

zzooeeyy Mar 25, 2024

paulomarg Mar 25, 2024

zzooeeyy Mar 25, 2024

paulomarg Mar 25, 2024

paulomarg Mar 25, 2024

zzooeeyy Mar 25, 2024

paulomarg Mar 25, 2024

paulomarg Mar 25, 2024

paulomarg commented Mar 25, 2024

ragalie commented Mar 26, 2024

paulomarg commented Mar 26, 2024

zzooeeyy commented Mar 26, 2024

ragalie commented Mar 26, 2024

zzooeeyy commented Mar 27, 2024

ragalie Mar 27, 2024

zzooeeyy Mar 27, 2024

ragalie left a comment

Add token exchange concern #1817

Add token exchange concern #1817

Conversation

zzooeeyy commented Mar 25, 2024 • edited

What this PR does

Impact on existing apps:

Checklist

paulomarg left a 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

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 commented Mar 25, 2024

ragalie commented Mar 26, 2024

paulomarg commented Mar 26, 2024

zzooeeyy commented Mar 26, 2024

ragalie commented Mar 26, 2024

zzooeeyy commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragalie left a comment

Choose a reason for hiding this comment

zzooeeyy commented Mar 25, 2024 •

edited