Merge pull request #979 from Shopify/fix-token-refresh-errors

Fix error handling on session refresh

.changeset/thick-papayas-tan.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-kit': patch
+---
+
+Fix error handling on session refresh

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
       }

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)
+  })
+})

packages/cli-kit/src/session/exchange.ts

@@ -1,18 +1,15 @@
 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'
 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 {}
-
-const InvalidIdentityError = () =>
-  new Abort('\nError validating auth session', "We've cleared the current session, please try again")
+export class InvalidGrantError extends ExtendableError {}
+export class InvalidRequestError extends ExtendableError {}
 
 export interface ExchangeScopes {
   admin: string[]
@@ -74,7 +71,7 @@ export async function exchangeAccessForApplicationTokens(
  * Given an expired access token, refresh it to get a new one.
  */
 export async function refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken> {
-  const clientId = await getIdentityClientId()
+  const clientId = getIdentityClientId()
   const params = {
     grant_type: 'refresh_token',
     access_token: currentToken.accessToken,
@@ -167,7 +164,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 +173,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)
 }