Token exchange prototype #448

rachel-carvalho · 2023-09-20T15:12:18Z

WHY are these changes introduced?

Fixes #0000

WHAT is this pull request doing?

Type of change

Patch: Bug (non-breaking change which fixes an issue)
Minor: New feature (non-breaking change which adds functionality)
Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
I have added/updated tests for this change
I have documented new APIs/updated the documentation for modified APIs (for public APIs)

rezaansyed · 2023-09-20T20:01:02Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+
+    logger.debug('Requesting token exchange');
+
+    const {session} = await api.auth.tokenExchange({


If an access token is expired or does not exist, we will perform token exchange to get a new access token.

rezaansyed · 2023-09-20T20:02:28Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+        if (persistedSession.isScopeChanged(config.scopes)) {
+          config.sessionStorage.deleteSession(persistedSession.id);
+
+          this.redirectToInstall(request, shop);


This is an edge case that can happen when the app changes scopes, while a merchant has an active app browser session. Please see the discussion for more details.

rezaansyed · 2023-09-20T20:04:03Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+    const {api, logger, config} = this;
+
+    const isXhrRequest = request.headers.get('authorization');
+    if (!isXhrRequest) {


If the request is a document load with an invalid or non-existent session token, we will redirect to the bounce page to render App Bridge and redirect to the original page with the session token passed in the authorization header

rezaansyed · 2023-09-20T20:04:42Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+      return redirectToBouncePage(new URL(request.url), {api, logger, config});
+    }
+
+    return new Response(undefined, {


For an XHR request made via App Bridge's augmented fetch, return a 401 with the X-Shopify-Invalid-Session header.

rezaansyed · 2023-09-20T20:07:19Z

...ages/shopify-app-remix/src/server/authenticate/admin/helpers/handle-embedded-client-error.ts

+
+    console.log('error.response', JSON.stringify(error.response));
+
+    if (error.response.code === 401) {


When making Admin API requests, a 401 signifies that the access token is invalid.

rezaansyed · 2023-09-20T20:08:01Z

...ages/shopify-app-remix/src/server/authenticate/admin/helpers/handle-embedded-client-error.ts

+      params.config.sessionStorage.deleteSession(session.id);
+
+      const isXhrRequest = request.headers.get('authorization');
+      if (isXhrRequest) {


In this scenario, we want to perform token exchange. So we delete the access token and return a 401 with X-Shopify-Invalid-Session so that the augmented fetch can retry the request, consequently triggering token exchange.

@rachel-carvalho I'm starting to think we shouldn't retry ever since it basically retries the entire request to the remix server:

loader() { const {admin} = authenticate.admin(request) add_some_records_to_local_db(); non_shopify_api_calls(); admin.createProducts() <- this is what we want to retry but would end up re-running all the previous calls. }

might be worth simply redirecting to /session-token?url={request.url} to allow the app to naturally perform token exchange without ever retrying the request.

rezaansyed · 2023-09-20T20:08:43Z

...ages/shopify-app-remix/src/server/authenticate/admin/helpers/handle-embedded-client-error.ts

+          },
+        });
+      } else {
+        throw redirect(request.url);


If an Admin API request is happening on a document load, we want to delete the access token and refresh the page in order to trigger token exchange.

byrichardpowell · 2023-09-21T19:38:24Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+    } else if (isAuthRequest) {
+      const shop = ensureValidShopParam(request, {api, logger, config});
+      const installUrl = `https://${shop}/admin/oauth/install?client_id=${config.apiKey}`;
+      throw redirect(installUrl);
+    }


Why do we need to handle auth requests in token exchange?

~~This is related to Option 1 of handling access scopes changes during a merchant's app browser session.~~

We're still handling the /auth route so that the login page where the developer can type their store url to initiate install could still work, since that's a nice tool during app development.

Then instead of initiating auth code flow like the original authenticate strategy does, it just redirects the user to the shopify managed install page.

Thanks for the context 👍

byrichardpowell · 2023-09-21T19:41:01Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+        const sessionToken = await validateSessionTokenUncaught(
+          {api, logger, config},
+          sessionTokenString,
+        );


How come we need the Uncaught version?

We catch the invalid JWT error and either return response header X-Shopify-Invalid-Session for XHR requests or redirect to bounce page. We could probably do this better but keeping this rough because we're in prototype phase.

byrichardpowell · 2023-09-21T19:51:50Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+    logger.info('Authenticating admin request');
+
+    if (sessionTokenParam) {
+      await validateUrlParams(request, {api, logger, config});


Is validing the URL params overkill given?

I think we can get all the same data from the session token

The session token can only be constructed using the API secret.

The URL params also use the API secret.

Worth checking with app sec? If so, we get to simplify our token exchange implementation.

There is the ability on the browser to simply refresh the iFrame instead of the entire page. So after a minute if the merchant only reloads the iFrame, it'll use an expired token. We should avoid that as that same session token might be used to make a token exchange request (which fails).

Yeah, good catch.

But why does that mean we need to validate the URL params and the session token? Why not just the session token, the URL params expire in roughly the same timeframe?

byrichardpowell · 2023-09-21T20:14:40Z

packages/shopify-app-remix/src/server/authenticate/admin/embedded-authenticate.ts

+
+    if (sessionTokenParam) {
+      await validateUrlParams(request, {api, logger, config});
+      await ensureAppIsEmbeddedIfRequired(request, {api, logger, config});


This is interesting.

In auth code flow ensureAppIsEmbeddedIfRequired makes sense. In token exchange IfRequired makes less sense, because it's always required?

When we product-ionize this code these are the kind of helpers that I'd want to be careful about. I'd prefer to duplicate the helper and reduce coupling between authe code flow and token exchange

Completely agree, we want the new embedded strategy to be independent from the existing auth code flow.

We wanted to only include net new token exchange code in embedded-authenticate so that the diff here in the draft PR was easier to review, and we didn't want to go too much into refactoring before build.

Makes complete sense, thanks Rachel.

byrichardpowell · 2023-09-21T20:15:59Z

packages/shopify-app-remix/src/server/shopify-app.ts

-      admin: oauth.authenticateAdmin.bind(oauth),
+      admin: config.isEmbeddedApp
+        ? tokenExchange.authenticateAdmin.bind(tokenExchange)
+        : oauth.authenticateAdmin.bind(oauth),


Love this ❤️

…en when expired

…valid on an XHR request

rezaansyed reviewed Sep 20, 2023

View reviewed changes

byrichardpowell reviewed Sep 21, 2023

View reviewed changes

rachel-carvalho force-pushed the token-exchange-prototype branch from 5bdfbe3 to 04439db Compare October 19, 2023 15:51

rezaansyed and others added 17 commits November 7, 2023 11:19

Add EmbeddedAuthStrategy class

da35ae3

Update package.json files

5daf9cc

Make EmbeddedAuthStrategy the default

3600727

Redirect to bounce page when id token in URL param is invalid

0b3e469

[temporary] add local-build.sh file

0fcb5ed

Redirect to bounce page if token exchange fails on initial load

c560cb7

delete access token & send header back when core returns 401

1559b05

reload page when access token is invalid and is not xhr request

e3e3d2c

redirect to oauth/install page when scopes changed, re-exchanging tok…

46f7bd6

…en when expired

support login page

747ab0c

use different auth strategy based on isEmbedded

e305d65

Add auth-helpers for common functions across strategies

20583fb

move remaining helper methods

d7a726e

yarn lock

eca3f10

some cleanup

5d18064

delete id token when redirecting to bounce page

be1d84f

return 401 status and invalid session header when session token is in…

1c9b3c1

…valid on an XHR request

rachel-carvalho and others added 4 commits November 7, 2023 11:22

comment out exit iframe to redirect to install when scopes do not match

8a10227

Override to use local app bridge

2eead05

fix build after refactors from main

a959711

Merge changes

0a18507

rezaansyed force-pushed the token-exchange-prototype branch from 04439db to 333d14b Compare November 8, 2023 14:40

Add future flag support

2398cbf

rezaansyed force-pushed the token-exchange-prototype branch from 333d14b to 2398cbf Compare November 8, 2023 14:42

rezaansyed closed this Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Token exchange prototype #448

Token exchange prototype #448

rachel-carvalho commented Sep 20, 2023

rezaansyed Sep 20, 2023 •

edited

Loading

rezaansyed Sep 20, 2023

rezaansyed Sep 20, 2023

rezaansyed Sep 20, 2023

rezaansyed Sep 20, 2023 •

edited

Loading

rezaansyed Sep 20, 2023

rezaansyed Dec 8, 2023

rezaansyed Dec 8, 2023

rezaansyed Sep 20, 2023

byrichardpowell Sep 21, 2023

rezaansyed Sep 21, 2023 •

edited

Loading

rachel-carvalho Sep 21, 2023

byrichardpowell Sep 22, 2023

byrichardpowell Sep 21, 2023

rezaansyed Sep 21, 2023 •

edited

Loading

byrichardpowell Sep 21, 2023

rezaansyed Sep 21, 2023

byrichardpowell Sep 21, 2023

byrichardpowell Sep 21, 2023 •

edited

Loading

rachel-carvalho Sep 21, 2023

byrichardpowell Sep 22, 2023

byrichardpowell Sep 21, 2023


		logger.debug('Requesting token exchange');

		const {session} = await api.auth.tokenExchange({


		console.log('error.response', JSON.stringify(error.response));

		if (error.response.code === 401) {

Token exchange prototype #448

Token exchange prototype #448

Conversation

rachel-carvalho commented Sep 20, 2023

WHY are these changes introduced?

WHAT is this pull request doing?

Type of change

Checklist

rezaansyed Sep 20, 2023 • edited Loading

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

rezaansyed Sep 20, 2023 • edited Loading

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

rezaansyed Sep 21, 2023 • edited Loading

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

rezaansyed Sep 21, 2023 • edited Loading

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

byrichardpowell Sep 21, 2023 • edited Loading

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

rezaansyed Sep 20, 2023 •

edited

Loading

rezaansyed Sep 20, 2023 •

edited

Loading

rezaansyed Sep 21, 2023 •

edited

Loading

rezaansyed Sep 21, 2023 •

edited

Loading

byrichardpowell Sep 21, 2023 •

edited

Loading