Merge pull request #4790 from Shopify/401-errors

Refresh session when 401 errors happens

.changeset/dull-shoes-clean.md

@@ -0,0 +1,6 @@
+---
+'@shopify/theme': patch
+---
+
+Prevent the `shopify theme dev` command from terminating by refreshing the session
+

packages/cli-kit/src/public/node/themes/api.test.ts

@@ -22,7 +22,7 @@ import {AbortError} from '@shopify/cli-kit/node/error'
 vi.mock('@shopify/cli-kit/node/api/admin')
 vi.mock('@shopify/cli-kit/node/system')
 
-const session = {token: 'token', storeFqdn: 'my-shop.myshopify.com'}
+const session = {token: 'token', storeFqdn: 'my-shop.myshopify.com', refresh: async () => {}}
 const themeAccessSession = {...session, token: 'shptka_token'}
 const sessions = {CLI: session, 'Theme Access': themeAccessSession}
 
@@ -370,6 +370,31 @@ describe('request errors', () => {
       // Then
     }).rejects.toThrowError(AbortError)
   })
+
+  test(`refresh the session when 401 errors happen`, async () => {
+    // Given
+    const id = 123
+    const assets: AssetParams[] = []
+
+    vi.spyOn(session, 'refresh').mockImplementation(vi.fn())
+    vi.mocked(restRequest)
+      .mockResolvedValueOnce({
+        json: {},
+        status: 401,
+        headers: {},
+      })
+      .mockResolvedValueOnce({
+        json: {},
+        status: 207,
+        headers: {},
+      })
+
+    // When
+    await bulkUploadThemeAssets(id, assets, session)
+
+    // Then
+    expect(session.refresh).toHaveBeenCalledOnce()
+  })
 })
 
 describe('bulkUploadThemeAssets', async () => {

packages/cli-kit/src/public/node/themes/api.ts

@@ -239,6 +239,18 @@ async function request<T>(
     case status === 403:
       return handleForbiddenError(response, session)
     case status === 401:
+      /**
+       * We need to resolve the call to the refresh function at runtime to
+       * avoid a circular reference.
+       *
+       * This won't be necessary when https://github.com/Shopify/cli/issues/4769
+       * gets resolved, and this condition must be removed then.
+       */
+      if ('refresh' in session) {
+        const refresh = session.refresh as () => Promise<void>
+        await refresh()
+      }
+
       // Retry 401 errors to be resilient to authentication errors.
       return handleRetriableError({
         path,

packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts

@@ -1,30 +1,33 @@
-import {abortOnMissingRequiredFile, getStorefrontSessionCookiesWithVerification} from './dev-server-session.js'
+import {
+  abortOnMissingRequiredFile,
+  getStorefrontSessionCookiesWithVerification,
+  initializeDevServerSession,
+} from './dev-server-session.js'
 import {getStorefrontSessionCookies, ShopifyEssentialError} from './storefront-session.js'
-import {AdminSession} from '@shopify/cli-kit/node/session'
+import {ensureAuthenticatedStorefront, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {fetchThemeAssets, themeDelete} from '@shopify/cli-kit/node/themes/api'
 import {describe, expect, test, vi, beforeEach} from 'vitest'
-import {ThemeAsset} from '@shopify/cli-kit/node/themes/types'
 import {AbortError} from '@shopify/cli-kit/node/error'
 import {outputContent, outputToken} from '@shopify/cli-kit/node/output'
 
-vi.mock('@shopify/cli-kit/node/themes/api')
 vi.mock('@shopify/cli-kit/node/session')
+vi.mock('@shopify/cli-kit/node/themes/api')
 vi.mock('./storefront-session.js')
 
-const mockAdminSession: AdminSession = {token: 'token', storeFqdn: 'store.myshopify.com'}
-const themeId = '1234'
-
-const mockLayoutAsset: ThemeAsset = {
+const storeFqdn = 'my-shop.myshopify.com'
+const themeId = '123'
+const adminSession = {
+  token: 'token',
+  storeFqdn,
+}
+const mockLayoutAsset = {
   key: 'layout/theme.liquid',
   value: 'content',
-  attachment: undefined,
   checksum: 'asdf',
 }
-
-const mockConfigAsset: ThemeAsset = {
+const mockConfigAsset = {
   key: 'config/settings_schema.json',
   value: '[]',
-  attachment: undefined,
   checksum: 'fdsa',
 }
 
@@ -34,16 +37,17 @@ describe('getStorefrontSessionCookiesWithVerification', () => {
     vi.mocked(getStorefrontSessionCookies).mockRejectedValue(new ShopifyEssentialError('Test error'))
     vi.mocked(fetchThemeAssets).mockResolvedValue([mockLayoutAsset])
 
-    // When/Then
-    await expect(
-      getStorefrontSessionCookiesWithVerification(
-        mockAdminSession.storeFqdn,
-        themeId,
-        mockAdminSession,
-        'storefront-token',
-        'storefront-password ',
-      ),
-    ).rejects.toThrow(
+    // When
+    const cookiesWithVerification = getStorefrontSessionCookiesWithVerification(
+      storeFqdn,
+      themeId,
+      adminSession,
+      'storefront-token',
+      'storefront-password ',
+    )
+
+    // Then
+    await expect(cookiesWithVerification).rejects.toThrow(
       new AbortError(
         outputContent`Theme ${outputToken.cyan(themeId)} is missing required files. Run ${outputToken.cyan(
           `shopify theme delete -t ${themeId}`,
@@ -63,17 +67,88 @@ describe('verifyRequiredFilesExist', () => {
     vi.mocked(fetchThemeAssets).mockResolvedValue([mockLayoutAsset, mockConfigAsset])
 
     // When
+    const abortOnMissingFiles = abortOnMissingRequiredFile(themeId, adminSession)
+
     // Then
-    await expect(abortOnMissingRequiredFile(themeId, mockAdminSession)).resolves.not.toThrow()
+    await expect(abortOnMissingFiles).resolves.not.toThrow()
   })
 
-  //   throws an AbortError if any file is missing
   test('throws an AbortError if any file is missing', async () => {
     // Given
     vi.mocked(fetchThemeAssets).mockResolvedValue([mockLayoutAsset])
 
     // When
+    const abortOnMissingFiles = abortOnMissingRequiredFile(themeId, adminSession)
+
     // Then
-    await expect(abortOnMissingRequiredFile(themeId, mockAdminSession)).rejects.toThrow()
+    await expect(abortOnMissingFiles).rejects.toThrow()
+  })
+})
+
+describe('dev server session', async () => {
+  describe('initializeDevServerSession', async () => {
+    test('returns a session', async () => {
+      // Given
+      vi.mocked(ensureAuthenticatedStorefront).mockResolvedValue('storefront_token')
+      vi.mocked(getStorefrontSessionCookies).mockResolvedValue({
+        _shopify_essential: ':AABBCCDDEEFFGGHH==123:',
+        storefront_digest: 'digest_value',
+      })
+      vi.mocked(ensureAuthenticatedThemes).mockResolvedValue({
+        token: 'token_1',
+        storeFqdn,
+      })
+
+      // When
+      const session = await initializeDevServerSession(themeId, adminSession)
+
+      // Then
+      expect(session).toEqual(
+        expect.objectContaining({
+          refresh: expect.any(Function),
+          sessionCookies: {
+            _shopify_essential: ':AABBCCDDEEFFGGHH==123:',
+            storefront_digest: 'digest_value',
+          },
+          storeFqdn: 'my-shop.myshopify.com',
+          storefrontToken: 'storefront_token',
+          token: 'token_1',
+        }),
+      )
+    })
+
+    test('returns a refreshable session', async () => {
+      // Given
+      for (const index of [1, 2, 3]) {
+        vi.mocked(ensureAuthenticatedStorefront).mockResolvedValueOnce(`storefront_token_${index}`)
+        vi.mocked(getStorefrontSessionCookies).mockResolvedValueOnce({
+          _shopify_essential: `:AABBCCDDEEFFGGHH==${index}:`,
+          storefront_digest: `digest_value_${index}`,
+        })
+        vi.mocked(ensureAuthenticatedThemes).mockResolvedValueOnce({
+          token: `token_${index}`,
+          storeFqdn,
+        })
+      }
+
+      // When
+      const session = await initializeDevServerSession(themeId, adminSession)
+      await session.refresh?.()
+      await session.refresh?.()
+
+      // Then
+      expect(session).toEqual(
+        expect.objectContaining({
+          refresh: expect.any(Function),
+          sessionCookies: {
+            _shopify_essential: ':AABBCCDDEEFFGGHH==3:',
+            storefront_digest: 'digest_value_3',
+          },
+          storeFqdn: 'my-shop.myshopify.com',
+          storefrontToken: 'storefront_token_3',
+          token: 'token_3',
+        }),
+      )
+    })
   })
 })

packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts

@@ -29,15 +29,19 @@ export async function initializeDevServerSession(
 ) {
   const session = await fetchDevServerSession(themeId, adminSession, adminPassword, storefrontPassword)
 
+  session.refresh = async () => {
+    outputDebug('Refreshing theme session...')
+    const newSession = await fetchDevServerSession(themeId, adminSession, adminPassword, storefrontPassword)
+    Object.assign(session, newSession)
+  }
+
   setInterval(() => {
-    fetchDevServerSession(themeId, adminSession, adminPassword, storefrontPassword)
-      .then((newSession) => {
-        outputDebug('Refreshing theme session...')
-        Object.assign(session, newSession)
-      })
-      .catch(() => {
-        outputDebug('Session could not be refreshed.')
-      })
+    if (!session.refresh) return
+
+    session
+      .refresh()
+      .then(() => outputDebug('Refreshed theme session via auto-refresher...'))
+      .catch(() => outputDebug('Session could not be refreshed.'))
   }, SESSION_TIMEOUT_IN_MS)
 
   return session

packages/theme/src/cli/utilities/theme-environment/types.ts

@@ -36,6 +36,12 @@ export interface DevServerSession extends AdminSession {
    *                         rendering.
    */
   sessionCookies: {[key: string]: string}
+
+  /**
+   * Refreshes the current session, ensuring any tokens and session cookies
+   * are up-to-date.
+   */
+  refresh?: () => Promise<void>
 }
 
 /**