From 719093afcdd163b12725e72ec463162ea23f212e Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Fri, 23 Dec 2022 11:43:49 +0100 Subject: [PATCH 1/4] Fix token refresh error handling --- packages/cli-kit/src/session.ts | 5 +++++ packages/cli-kit/src/session/exchange.ts | 13 ++++--------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/cli-kit/src/session.ts b/packages/cli-kit/src/session.ts index 9f8c98a3f0..624df5661d 100644 --- a/packages/cli-kit/src/session.ts +++ b/packages/cli-kit/src/session.ts @@ -11,6 +11,7 @@ import { ExchangeScopes, refreshAccessToken, InvalidGrantError, + InvalidRequestError, } from './session/exchange.js' import {content, token, debug} from './output.js' @@ -26,6 +27,7 @@ import {partners} from './api.js' import {RequestClientError} from './api/common.js' import {firstPartyDev, useDeviceAuth} from './environment/local.js' import {pollForDeviceAuthorization, requestDeviceAuthorization} from './session/device-authorization.js' +import {AbortError} from './public/node/error.js' import {gql} from 'graphql-request' const NoSessionError = new Bug('No session found after ensuring authenticated') @@ -215,6 +217,9 @@ ${token.json(applications)} } catch (error) { if (error instanceof InvalidGrantError) { newSession = await executeCompleteFlow(applications, fqdn) + } else if (error instanceof InvalidRequestError) { + await secureStore.remove() + throw new AbortError('\nError validating auth session', "We've cleared the current session, please try again") } else { throw error } diff --git a/packages/cli-kit/src/session/exchange.ts b/packages/cli-kit/src/session/exchange.ts index 64cfc9028a..8f441d6845 100644 --- a/packages/cli-kit/src/session/exchange.ts +++ b/packages/cli-kit/src/session/exchange.ts @@ -1,8 +1,6 @@ import {ApplicationToken, IdentityToken} from './schema.js' import {applicationId, clientId as getIdentityClientId} from './identity.js' import {CodeAuthResult} from './authorize.js' -import * as secureStore from './store.js' -import {Abort} from '../error.js' import {API} from '../network/api.js' import {identity as identityFqdn} from '../environment/fqdn.js' import {shopifyFetch} from '../http.js' @@ -10,9 +8,7 @@ import {err, ok, Result} from '../public/node/result.js' import {AbortError} from '../public/node/error.js' export class InvalidGrantError extends Error {} - -const InvalidIdentityError = () => - new Abort('\nError validating auth session', "We've cleared the current session, please try again") +export class InvalidRequestError extends Error {} export interface ExchangeScopes { admin: string[] @@ -74,7 +70,7 @@ export async function exchangeAccessForApplicationTokens( * Given an expired access token, refresh it to get a new one. */ export async function refreshAccessToken(currentToken: IdentityToken): Promise { - const clientId = await getIdentityClientId() + const clientId = getIdentityClientId() const params = { grant_type: 'refresh_token', access_token: currentToken.accessToken, @@ -167,7 +163,7 @@ interface TokenRequestResult { scope: string } -async function tokenRequestErrorHandler(error: string) { +function tokenRequestErrorHandler(error: string) { if (error === 'invalid_grant') { // There's an scenario when Identity returns "invalid_grant" when trying to refresh the token // using a valid refresh token. When that happens, we take the user through the authentication flow. @@ -176,8 +172,7 @@ async function tokenRequestErrorHandler(error: string) { if (error === 'invalid_request') { // There's an scenario when Identity returns "invalid_request" when exchanging an identity token. // This means the token is invalid. We clear the session and throw an error to let the caller know. - await secureStore.remove() - return new AbortError('\nError validating auth session', "We've cleared the current session, please try again") + return new InvalidRequestError() } return new AbortError(error) } From 08446f629dead4bca0a4ad7ba1a5450864a24003 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Fri, 23 Dec 2022 12:59:36 +0100 Subject: [PATCH 2/4] Add tests --- packages/cli-kit/src/session/exchange.test.ts | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/packages/cli-kit/src/session/exchange.test.ts b/packages/cli-kit/src/session/exchange.test.ts index 8b780f15f2..d1e6de409c 100644 --- a/packages/cli-kit/src/session/exchange.test.ts +++ b/packages/cli-kit/src/session/exchange.test.ts @@ -1,10 +1,17 @@ -import {exchangeAccessForApplicationTokens, exchangeCodeForAccessToken} from './exchange.js' +import { + exchangeAccessForApplicationTokens, + exchangeCodeForAccessToken, + InvalidGrantError, + InvalidRequestError, + refreshAccessToken, +} from './exchange.js' import {applicationId, clientId} from './identity.js' import {IdentityToken} from './schema.js' import {shopifyFetch} from '../http.js' import {identity} from '../environment/fqdn.js' import {describe, it, expect, vi, afterAll, beforeEach} from 'vitest' import {Response} from 'node-fetch' +import {AbortError} from '@shopify/cli-kit/node/error.js' const currentDate = new Date(2022, 1, 1, 10) const expiredDate = new Date(2022, 1, 1, 11) @@ -140,3 +147,44 @@ describe('exchange identity token for application tokens', () => { expect(got).toEqual(expected) }) }) + +describe('refresh access tokens', () => { + it('throws a InvalidGrantError when Identity returns invalid_grant', async () => { + // Given + const error = {error: 'invalid_grant'} + const response = new Response(JSON.stringify(error), {status: 400}) + vi.mocked(shopifyFetch).mockResolvedValue(response) + + // When + const got = () => refreshAccessToken(identityToken) + + // Then + return expect(got).rejects.toThrowError(InvalidGrantError) + }) + + it('throws a InvalidRequestError when Identity returns invalid_request', async () => { + // Given + const error = {error: 'invalid_request'} + const response = new Response(JSON.stringify(error), {status: 400}) + vi.mocked(shopifyFetch).mockResolvedValue(response) + + // When + const got = () => refreshAccessToken(identityToken) + + // Then + return expect(got).rejects.toThrowError(InvalidRequestError) + }) + + it('throws an AbortError when Identity returns another error', async () => { + // Given + const error = {error: 'another'} + const response = new Response(JSON.stringify(error), {status: 400}) + vi.mocked(shopifyFetch).mockResolvedValue(response) + + // When + const got = () => refreshAccessToken(identityToken) + + // Then + return expect(got).rejects.toThrowError(AbortError) + }) +}) From 2ac83ce6e2c660ad02e3fe0147788a2a7bd7334c Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Fri, 23 Dec 2022 13:00:56 +0100 Subject: [PATCH 3/4] Add changeset --- .changeset/thick-papayas-tan.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/thick-papayas-tan.md diff --git a/.changeset/thick-papayas-tan.md b/.changeset/thick-papayas-tan.md new file mode 100644 index 0000000000..8815a586dd --- /dev/null +++ b/.changeset/thick-papayas-tan.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': patch +--- + +Fix error handling on session refresh From 530da146f82b5f8fd4f660cd65a9d609d011173d Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Fri, 23 Dec 2022 13:12:14 +0100 Subject: [PATCH 4/4] Extend from ExtendableError instead of Error --- packages/cli-kit/src/session/exchange.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/session/exchange.ts b/packages/cli-kit/src/session/exchange.ts index 8f441d6845..c2b60b6fcf 100644 --- a/packages/cli-kit/src/session/exchange.ts +++ b/packages/cli-kit/src/session/exchange.ts @@ -6,9 +6,10 @@ import {identity as identityFqdn} from '../environment/fqdn.js' import {shopifyFetch} from '../http.js' import {err, ok, Result} from '../public/node/result.js' import {AbortError} from '../public/node/error.js' +import {ExtendableError} from '../error.js' -export class InvalidGrantError extends Error {} -export class InvalidRequestError extends Error {} +export class InvalidGrantError extends ExtendableError {} +export class InvalidRequestError extends ExtendableError {} export interface ExchangeScopes { admin: string[]