diff --git a/.changeset/theme-commands-recover-after-preview-store-claim.md b/.changeset/theme-commands-recover-after-preview-store-claim.md new file mode 100644 index 00000000000..663253fa086 --- /dev/null +++ b/.changeset/theme-commands-recover-after-preview-store-claim.md @@ -0,0 +1,7 @@ +--- +'@shopify/cli-kit': patch +'@shopify/theme': patch +'@shopify/store': patch +--- + +Fix theme and `store` commands failing with an unhelpful error on a preview store after it has been claimed; they now prompt to run `store auth` to re-authenticate diff --git a/packages/cli-kit/src/public/node/api/admin.test.ts b/packages/cli-kit/src/public/node/api/admin.test.ts index 1a503448dfd..a8571bb5c31 100644 --- a/packages/cli-kit/src/public/node/api/admin.test.ts +++ b/packages/cli-kit/src/public/node/api/admin.test.ts @@ -6,6 +6,7 @@ import * as http from '../http.js' import {defaultThemeKitAccessDomain} from '../../../private/node/constants.js' import {test, vi, expect, describe} from 'vitest' +import {ClientError} from 'graphql-request' vi.mock('./graphql.js') vi.mock('../../../private/node/api/headers.js') @@ -193,3 +194,36 @@ describe('admin-rest-api', () => { ) }) }) + +describe('fetchApiVersions', () => { + // The HTTP status has to survive version discovery: callers such as the stored `store auth` + // recovery flow classify on the status, and they can't recover it from the rendered message. + test.each([401, 404])('preserves HTTP %i on the thrown error while keeping the message intact', async (status) => { + // Given a store whose API version has not been discovered yet + const session: AdminSession = {token, storeFqdn: `status-${status}.myshopify.com`} + vi.mocked(graphqlRequestDoc).mockRejectedValue( + new ClientError({status, data: 'body', errors: []}, {query: 'query'}), + ) + + // When + const error = await admin.fetchApiVersions(session).catch((thrown: unknown) => thrown) + + // Then + expect(error).toBeInstanceOf(admin.AdminApiRequestError) + expect(error).toMatchObject({status}) + expect((error as Error).message).toContain(`Error connecting to your store status-${status}.myshopify.com:`) + }) + + test('leaves other statuses to the existing classification', async () => { + // Given + const session: AdminSession = {token, storeFqdn: 'forbidden.myshopify.com'} + vi.mocked(graphqlRequestDoc).mockRejectedValue(new ClientError({status: 403, errors: []}, {query: 'query'})) + + // When + const error = await admin.fetchApiVersions(session).catch((thrown: unknown) => thrown) + + // Then + expect(error).not.toBeInstanceOf(admin.AdminApiRequestError) + expect((error as Error).message).toContain("Looks like you don't have access to this dev store") + }) +}) diff --git a/packages/cli-kit/src/public/node/api/admin.ts b/packages/cli-kit/src/public/node/api/admin.ts index f496b9d533b..41eb42c372a 100644 --- a/packages/cli-kit/src/public/node/api/admin.ts +++ b/packages/cli-kit/src/public/node/api/admin.ts @@ -26,6 +26,24 @@ import {TypedDocumentNode} from '@graphql-typed-document-node/core' const LatestApiVersionByFQDN = new Map() +/** + * Thrown when the Admin API rejects a request with a status a caller may need to act on. It carries + * the HTTP status so callers can classify the failure - for instance a stored `shopify store auth` + * session that stopped being accepted once its preview store was claimed - rather than parsing the + * rendered message. It renders exactly like any other abort error. + */ +export class AdminApiRequestError extends AbortError { + /** + * HTTP status the Admin API responded with. + */ + public readonly status: number + + constructor(status: number, message: string) { + super(message) + this.status = status + } +} + /** * Executes a GraphQL query against the Admin API. * @@ -187,7 +205,11 @@ export async function fetchApiVersions( ) } if (error instanceof ClientError && (error.response.status === 401 || error.response.status === 404)) { - throw new AbortError( + // Every uncached `adminRequestDoc` runs version discovery first, so this is the dominant + // first-failure path. The status is preserved on the thrown error because higher-level + // classifiers (e.g. stored store-auth recovery) can't recover it from the message. + throw new AdminApiRequestError( + error.response.status, `Error connecting to your store ${session.storeFqdn}: ${error.message} ${error.response.status} ${error.response.data}`, ) } diff --git a/packages/cli-kit/src/public/node/store-auth-recovery.test.ts b/packages/cli-kit/src/public/node/store-auth-recovery.test.ts new file mode 100644 index 00000000000..be61ad037cf --- /dev/null +++ b/packages/cli-kit/src/public/node/store-auth-recovery.test.ts @@ -0,0 +1,223 @@ +import { + throwIfStoredStoreAuthIsInvalid, + throwMissingStoredStoreAuthError, + throwReauthenticateStoreAuthError, + throwStoredStoreAuthInvalidError, +} from './store-auth-recovery.js' +import {STORE_AUTH_APP_CLIENT_ID} from './constants.js' +import {AbortError} from './error.js' +import {inTemporaryDirectory} from './fs.js' +import {LocalStorage} from './local-storage.js' +import { + getCurrentStoredStoreAppSession, + setStoredStoreAppSession, + type StoreAuthSessionSchema, + type StoredStoreAppSession, +} from './store-auth-session.js' +import {AdminApiRequestError} from './api/admin.js' +import {describe, expect, test} from 'vitest' + +const SHOP = 'shop.myshopify.com' + +function standardSession(overrides: Partial = {}): StoredStoreAppSession { + return { + store: SHOP, + clientId: STORE_AUTH_APP_CLIENT_ID, + userId: '42', + accessToken: 'token', + scopes: ['read_products', 'write_orders'], + acquiredAt: '2026-03-27T00:00:00.000Z', + ...overrides, + } +} + +function previewSession(overrides: Partial = {}): StoredStoreAppSession { + return { + ...standardSession({ + userId: 'preview:placeholder-uuid', + // The full preapproved catalog is much larger in practice; a couple of entries are enough + // to prove the placeholder is used instead of these. + scopes: ['read_products', 'write_products', 'read_themes'], + }), + kind: 'preview', + preview: { + shopId: '123', + name: 'Lavender Candles', + createdAt: '2026-03-27T00:00:00.000Z', + }, + ...overrides, + } +} + +// The shape graphql-request's `ClientError` has: the HTTP status nested under `response`. This is +// what escapes an Admin API call once the API version has been cached. +function graphQLClientError(status: number, message = 'GraphQL Error'): Error { + const error = new Error(message) as Error & {response: {status: number; errors: {message: string}[]}} + error.response = {status, errors: [{message}]} + return error +} + +function captureThrown(run: () => void): AbortError | undefined { + try { + run() + } catch (error) { + if (!(error instanceof AbortError)) throw error + return error + } + return undefined +} + +async function withStoredSession( + session: StoredStoreAppSession, + run: (storage: LocalStorage) => void, +): Promise { + await inTemporaryDirectory((cwd) => { + const storage = new LocalStorage({cwd}) + setStoredStoreAppSession(session, storage) + + run(storage) + }) +} + +describe('throwMissingStoredStoreAuthError', () => { + test('reports no stored auth and prompts to authenticate (not re-authenticate) with a scopes placeholder', () => { + const captured = captureThrown(() => throwMissingStoredStoreAuthError(SHOP)) + + expect(captured).toMatchObject({ + message: `No stored app authentication found for ${SHOP}.`, + nextSteps: [ + ['Run', {command: `shopify store auth --store ${SHOP} --scopes `}, 'to authenticate'], + ], + }) + }) +}) + +describe('throwReauthenticateStoreAuthError', () => { + test('suggests the real scopes for a standard session', () => { + const captured = captureThrown(() => throwReauthenticateStoreAuthError('Custom message.', standardSession())) + + expect(captured).toMatchObject({ + message: 'Custom message.', + nextSteps: [ + [ + 'Run', + {command: `shopify store auth --store ${SHOP} --scopes read_products,write_orders`}, + 'to re-authenticate', + ], + ], + }) + }) + + test('suggests a scopes placeholder for a preview session instead of its preapproved catalog', () => { + const captured = captureThrown(() => throwReauthenticateStoreAuthError('Custom message.', previewSession())) + + expect(captured).toMatchObject({ + message: 'Custom message.', + nextSteps: [ + [ + 'Run', + {command: `shopify store auth --store ${SHOP} --scopes `}, + 'to re-authenticate', + ], + ], + }) + }) +}) + +describe('throwStoredStoreAuthInvalidError', () => { + test('uses the generic invalid-auth message and real scopes for a standard session', () => { + const captured = captureThrown(() => throwStoredStoreAuthInvalidError(standardSession())) + + expect(captured).toMatchObject({ + message: `Stored app authentication for ${SHOP} is no longer valid.`, + nextSteps: [ + [ + 'Run', + {command: `shopify store auth --store ${SHOP} --scopes read_products,write_orders`}, + 'to re-authenticate', + ], + ], + }) + }) + + test('flags a likely claim and suggests a scopes placeholder for a preview session', () => { + const captured = captureThrown(() => throwStoredStoreAuthInvalidError(previewSession())) + + expect(captured).toMatchObject({ + message: `The preview store ${SHOP} has likely been claimed, so its stored authentication is no longer valid.`, + nextSteps: [ + [ + 'Run', + {command: `shopify store auth --store ${SHOP} --scopes `}, + 'to re-authenticate', + ], + ], + }) + }) +}) + +describe('throwIfStoredStoreAuthIsInvalid', () => { + test.each([401, 404])('reports an invalid standard session and clears it for HTTP %i by default', async (status) => { + const session = standardSession() + + await withStoredSession(session, (storage) => { + const captured = captureThrown(() => + throwIfStoredStoreAuthIsInvalid(graphQLClientError(status), session, {storage}), + ) + + expect(captured).toBeInstanceOf(AbortError) + expect(captured).toMatchObject({message: `Stored app authentication for ${SHOP} is no longer valid.`}) + expect(getCurrentStoredStoreAppSession(SHOP, storage)).toBeUndefined() + }) + }) + + test('recognizes the status carried directly by a typed transport error', async () => { + const session = standardSession() + + await withStoredSession(session, (storage) => { + const transportError = new AdminApiRequestError(401, `Error connecting to your store ${SHOP}: Unauthorized`) + + const captured = captureThrown(() => throwIfStoredStoreAuthIsInvalid(transportError, session, {storage})) + + expect(captured).toMatchObject({message: `Stored app authentication for ${SHOP} is no longer valid.`}) + }) + }) + + // Clearing a preview session is what makes the `store auth` command this error prints runnable: + // `store auth` refuses to start while a preview session is still stored for the store. + test('clears an invalid preview session so the re-authentication it suggests can run', async () => { + const session = previewSession() + + await withStoredSession(session, (storage) => { + const captured = captureThrown(() => throwIfStoredStoreAuthIsInvalid(graphQLClientError(401), session, {storage})) + + expect(captured).toMatchObject({ + message: `The preview store ${SHOP} has likely been claimed, so its stored authentication is no longer valid.`, + }) + expect(getCurrentStoredStoreAppSession(SHOP, storage)).toBeUndefined() + }) + }) + + test('ignores a status the caller did not name, so a genuine not-found is not reported as invalid auth', async () => { + const session = standardSession() + + await withStoredSession(session, (storage) => { + expect(() => + throwIfStoredStoreAuthIsInvalid(graphQLClientError(404), session, {invalidStatuses: [401], storage}), + ).not.toThrow() + expect(getCurrentStoredStoreAppSession(SHOP, storage)).toMatchObject({accessToken: 'token'}) + }) + }) + + test.each([ + ['an unrelated HTTP status', graphQLClientError(500)], + ['an error carrying no status at all', new Error('socket hang up')], + ])('ignores %s', async (_description, error) => { + const session = standardSession() + + await withStoredSession(session, (storage) => { + expect(() => throwIfStoredStoreAuthIsInvalid(error, session, {storage})).not.toThrow() + expect(getCurrentStoredStoreAppSession(SHOP, storage)).toMatchObject({accessToken: 'token'}) + }) + }) +}) diff --git a/packages/cli-kit/src/public/node/store-auth-recovery.ts b/packages/cli-kit/src/public/node/store-auth-recovery.ts new file mode 100644 index 00000000000..9b2ddadfc74 --- /dev/null +++ b/packages/cli-kit/src/public/node/store-auth-recovery.ts @@ -0,0 +1,134 @@ +import {clearStoredStoreAppSession} from './store-auth-session.js' +import {AbortError} from './error.js' +import type {LocalStorage} from './local-storage.js' +import type {StoreAuthSessionSchema, StoredStoreAppSession} from './store-auth-session.js' + +const UNKNOWN_SCOPES_PLACEHOLDER = '' + +// HTTP statuses that mean a stored store-app session is no longer accepted, used when a caller +// doesn't name its own. 401 and 404 are the default because that is what the `store` commands have +// classified since this recovery flow was introduced. Callers whose requests can legitimately +// answer 404 for an unrelated reason - `theme push --theme ` against a theme that no longer +// exists - pass `[401]` instead, so a genuine not-found isn't misreported as an invalid session. +const DEFAULT_INVALID_STORE_AUTH_STATUSES: ReadonlyArray = [401, 404] + +function storeAuthCommandNextSteps(store: string, scopes: string, purpose: string) { + return [['Run', {command: `shopify store auth --store ${store} --scopes ${scopes}`}, purpose]] +} + +// Preview-store sessions are preapproved for a large, fixed scope catalog (often 30+ scopes). +// Suggesting the user re-request all of them encourages over-scoping, so they get the same +// placeholder as the "no stored auth" case and choose deliberately instead. +function reauthScopesFor(session: StoredStoreAppSession): string { + return session.kind === 'preview' ? UNKNOWN_SCOPES_PLACEHOLDER : session.scopes.join(',') +} + +// Two error shapes reach this module. graphql-request's `ClientError` nests the status under +// `response`, which is what escapes an Admin API call once the API version is cached, while +// cli-kit's typed transport errors (`AdminApiRequestError`, `PreviewStoreRequestError`) expose it +// directly. Both are read structurally so this module stays free of the Admin API transport's +// import graph, which every `store auth` run would otherwise pay for. +function httpStatusFromError(error: unknown): number | undefined { + if (!error || typeof error !== 'object') return undefined + + const {status, response} = error as {status?: unknown; response?: unknown} + if (typeof status === 'number') return status + if (!response || typeof response !== 'object') return undefined + + const responseStatus = (response as {status?: unknown}).status + return typeof responseStatus === 'number' ? responseStatus : undefined +} + +/** + * Throw an actionable error reporting that no store-app authentication is stored for a store. + * + * @param store - The store FQDN the caller tried to use. + * @throws AbortError pointing at `shopify store auth`. + */ +export function throwMissingStoredStoreAuthError(store: string): never { + throw new AbortError( + `No stored app authentication found for ${store}.`, + undefined, + storeAuthCommandNextSteps(store, UNKNOWN_SCOPES_PLACEHOLDER, 'to authenticate'), + ) +} + +/** + * Throw a caller-supplied message alongside the next step for re-authenticating a stored session. + * + * @param message - The message describing why the stored session can no longer be used. + * @param session - The stored store auth session that needs re-authenticating. + * @throws AbortError pointing at `shopify store auth`. + */ +export function throwReauthenticateStoreAuthError(message: string, session: StoredStoreAppSession): never { + throw new AbortError( + message, + undefined, + storeAuthCommandNextSteps(session.store, reauthScopesFor(session), 'to re-authenticate'), + ) +} + +/** + * Throw an actionable error reporting that a stored store-app session is no longer valid. + * + * A preview store's local session has no way to know it was claimed through the browser claim + * flow; a rejected request the first time the stale session is used again is the only signal. + * Surfacing that possibility is more useful than the generic "no longer valid" message a standard + * session gets, so every call site that detects an invalid stored session (regardless of which API + * it hit) should go through here instead of writing its own message. + * + * @param session - The stored store auth session that is no longer valid. + * @throws AbortError pointing at `shopify store auth`. + */ +export function throwStoredStoreAuthInvalidError(session: StoredStoreAppSession): never { + const message = + session.kind === 'preview' + ? `The preview store ${session.store} has likely been claimed, so its stored authentication is no longer valid.` + : `Stored app authentication for ${session.store} is no longer valid.` + + throwReauthenticateStoreAuthError(message, session) +} + +interface InvalidStoredStoreAuthOptions { + /** HTTP statuses that mean the stored session is invalid. Defaults to 401 and 404. */ + invalidStatuses?: ReadonlyArray + /** Storage override for tests. */ + storage?: LocalStorage +} + +/** + * Convert a failed request into an actionable re-authentication error when its HTTP status means + * the stored store-app session behind it stopped being accepted. + * + * The status is read from either shape a rejected Admin API request arrives in: carried directly on + * the error (cli-kit's typed transport errors) or nested under `response` (graphql-request's + * `ClientError`). When it matches, the stored session is cleared from local storage and an + * `AbortError` pointing at `shopify store auth` is thrown. An error carrying any other status - or + * no status at all - is left untouched for the caller to classify. + * + * @param error - The error thrown by the request made with the stored session. + * @param session - The stored store auth session the request was made with. + * @param options - Statuses to classify as invalid, and a storage override for tests. + * @throws AbortError pointing at `shopify store auth` when the stored session is invalid. + */ +export function throwIfStoredStoreAuthIsInvalid( + error: unknown, + session: StoredStoreAppSession, + options: InvalidStoredStoreAuthOptions = {}, +): void { + const invalidStatuses = options.invalidStatuses ?? DEFAULT_INVALID_STORE_AUTH_STATUSES + const status = httpStatusFromError(error) + if (status === undefined || !invalidStatuses.includes(status)) return + + // Preview sessions are cleared too, reversing the deliberate retention this flow shipped with. + // The stored record is what blocks the recovery this function points at: `throwIfPreviewStore` + // (`packages/store/src/cli/services/store/auth/index.ts`) aborts `store auth` for any store that + // still has a stored preview session, so retaining it turns the printed next step into a dead + // end. Clearing is irreversible - `store create preview` writes preview credentials once, + // straight from the creation response, with no `expiresAt` and no `refreshToken` - but that is + // also why nothing usable is discarded: a preview token never expires, so a status that lands + // here means the credentials stopped being accepted outright (claimed or revoked). + clearStoredStoreAppSession(session.store, session.userId, options.storage) + + throwStoredStoreAuthInvalidError(session) +} diff --git a/packages/cli-kit/src/public/node/store-auth-session.ts b/packages/cli-kit/src/public/node/store-auth-session.ts index 430d1fc35b7..2b1abd0e3d7 100644 --- a/packages/cli-kit/src/public/node/store-auth-session.ts +++ b/packages/cli-kit/src/public/node/store-auth-session.ts @@ -65,7 +65,11 @@ interface StoredStoreAppSessionBucket { sessionsByUserId: {[userId: string]: StoredStoreAppSession} } -interface StoreAuthSessionSchema { +/** + * Local-storage layout for store-auth sessions, keyed by {@link storeAuthSessionKey}. Exported so + * that sibling modules taking a storage override can type it the same way this module does. + */ +export interface StoreAuthSessionSchema { [key: string]: StoredStoreAppSessionBucket } diff --git a/packages/cli-kit/src/public/node/themes/api.test.ts b/packages/cli-kit/src/public/node/themes/api.test.ts index c6da96b898a..ee32baab383 100644 --- a/packages/cli-kit/src/public/node/themes/api.test.ts +++ b/packages/cli-kit/src/public/node/themes/api.test.ts @@ -26,13 +26,19 @@ import {ThemeFilesDelete} from '../../../cli/api/graphql/admin/generated/theme_f import {GetThemes} from '../../../cli/api/graphql/admin/generated/get_themes.js' import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js' import {FindDevelopmentThemeByName} from '../../../cli/api/graphql/admin/generated/find_development_theme_by_name.js' -import {adminRequestDoc, supportedApiVersions} from '../api/admin.js' +import {AdminApiRequestError, adminRequestDoc, supportedApiVersions} from '../api/admin.js' import {AbortError} from '../error.js' import {test, vi, expect, describe, beforeEach} from 'vitest' import {ClientError} from 'graphql-request' -vi.mock('../api/admin.js') +// Only the transport is mocked. `AdminApiRequestError` is a real class so that the `instanceof` +// check `fetchTheme` uses to tell authentication failures apart from not-found still holds. +vi.mock('../api/admin.js', async (importOriginal) => ({ + ...(await importOriginal()), + adminRequestDoc: vi.fn(), + supportedApiVersions: vi.fn(), +})) vi.mock('@shopify/cli-kit/node/system') vi.stubGlobal('fetch', vi.fn()) @@ -97,6 +103,30 @@ describe('fetchTheme', () => { 'The authenticated account or access token is missing `read_themes` access scope.', ) }) + + // A 401 says nothing about whether the theme exists, so it must not be flattened into the + // `undefined` that callers read as "this theme is gone". + test.each([ + ['the API version is already cached', new ClientError({status: 401, errors: []}, {query: ''})], + [ + 'the API version still has to be discovered', + new AdminApiRequestError(401, 'Error connecting to your store store.myshopify.com: Unauthorized'), + ], + ])('propagates an authentication failure when %s', async (_description, error) => { + vi.mocked(adminRequestDoc).mockRejectedValue(error) + + // Identity, not equality: the caller classifies this error by its own type and status, so the + // very error the transport threw has to come back out. + await expect(fetchTheme(123, session)).rejects.toBe(error) + }) + + // The counterpart to the 401 cases: a 404 really does mean the theme is gone, so it keeps being + // reported as `undefined` no matter which shape the rejection arrives in. + test('returns undefined when a typed transport error reports the theme was not found', async () => { + vi.mocked(adminRequestDoc).mockRejectedValue(new AdminApiRequestError(404, 'Not Found')) + + await expect(fetchTheme(123, session)).resolves.toBeUndefined() + }) }) describe('findDevelopmentThemeByName', () => { diff --git a/packages/cli-kit/src/public/node/themes/api.ts b/packages/cli-kit/src/public/node/themes/api.ts index 5bbd328b6aa..c324bbe34e7 100644 --- a/packages/cli-kit/src/public/node/themes/api.ts +++ b/packages/cli-kit/src/public/node/themes/api.ts @@ -25,7 +25,7 @@ import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js' import {FindDevelopmentThemeByName} from '../../../cli/api/graphql/admin/generated/find_development_theme_by_name.js' import {OnlineStorePasswordProtection} from '../../../cli/api/graphql/admin/generated/online_store_password_protection.js' import {RequestModeInput} from '../http.js' -import {adminRequestDoc, type AdminRequestOptions} from '../api/admin.js' +import {adminRequestDoc, AdminApiRequestError, type AdminRequestOptions} from '../api/admin.js' import {AdminSession} from '../session.js' import {AbortError} from '../error.js' import {outputDebug} from '../output.js' @@ -65,10 +65,20 @@ export async function fetchTheme(id: number, session: AdminSession): Promise( } } +// A 401 from the Admin API reaches theme callers in two shapes: an `AdminApiRequestError` raised by +// API-version discovery on the first (uncached) request, and a raw `ClientError` on every request +// after the version has been cached. +function isUnauthorizedAdminApiError(error: unknown): boolean { + if (error instanceof AdminApiRequestError) return error.status === 401 + return error instanceof ClientError && error.response.status === 401 +} + function abortIfMissingThemeAccessScope(error: unknown): void { if (!(error instanceof ClientError)) return diff --git a/packages/cli-kit/src/public/node/themes/theme-manager.test.ts b/packages/cli-kit/src/public/node/themes/theme-manager.test.ts index a8b0ef88c19..b90358822b8 100644 --- a/packages/cli-kit/src/public/node/themes/theme-manager.test.ts +++ b/packages/cli-kit/src/public/node/themes/theme-manager.test.ts @@ -2,7 +2,7 @@ import {ThemeManager} from './theme-manager.js' import {Theme} from './types.js' import {fetchTheme, findDevelopmentThemeByName, themeCreate} from './api.js' import {DEVELOPMENT_THEME_ROLE, UNPUBLISHED_THEME_ROLE} from './utils.js' -import {BugError} from '../error.js' +import {AbortError, BugError} from '../error.js' import {test, describe, expect, vi, beforeEach} from 'vitest' vi.mock('./api.js') @@ -29,6 +29,13 @@ class TestThemeManager extends ThemeManager { this.themeId = themeId } + // `setTheme` is the only writer of the stored id, so seeding through it is what a real manager + // does after a fetch or a create. Tests that assert on the stored id have to use this instead of + // `setThemeId`, which leaves the stored id undefined and makes such assertions vacuous. + storeTheme(themeId: string): void { + this.setTheme(themeId) + } + protected setTheme(themeId: string): void { this.storedThemeId = themeId this.themeId = themeId @@ -151,7 +158,7 @@ describe('ThemeManager', () => { test('removes theme when fetch returns undefined', async () => { // Given - manager.setThemeId('123') + manager.storeTheme('123') vi.mocked(fetchTheme).mockResolvedValue(undefined) // When @@ -162,6 +169,24 @@ describe('ThemeManager', () => { expect(result).toBeUndefined() expect(manager.getStoredThemeId()).toBeUndefined() }) + + // Only `undefined` means "this theme is gone". An authentication failure has to propagate with + // the cached id intact, or the next successful run would recreate a theme that still exists. + test('propagates a failed fetch and keeps the stored theme id', async () => { + // Given + manager.storeTheme('123') + const authenticationFailure = new AbortError('Stored app authentication is no longer valid.') + vi.mocked(fetchTheme).mockRejectedValue(authenticationFailure) + + // When + await expect(manager.fetch()).rejects.toBe(authenticationFailure) + + // Then the id was not forgotten, so a later run still targets the same theme + expect(manager.getStoredThemeId()).toBe('123') + vi.mocked(fetchTheme).mockResolvedValue(mockTheme) + await expect(manager.fetch()).resolves.toEqual(mockTheme) + expect(fetchTheme).toHaveBeenLastCalledWith(123, session) + }) }) describe('generateThemeName', () => { diff --git a/packages/store/src/cli/services/store/admin-errors.ts b/packages/store/src/cli/services/store/admin-errors.ts index 791029397ca..5f5de7a31f8 100644 --- a/packages/store/src/cli/services/store/admin-errors.ts +++ b/packages/store/src/cli/services/store/admin-errors.ts @@ -1,7 +1,4 @@ -import {throwStoredAuthInvalidError} from './auth/recovery.js' -import {clearStoredStoreAppSession} from '@shopify/cli-kit/node/store-auth-session' import {AbortError} from '@shopify/cli-kit/node/error' -import type {StoredStoreAppSession} from '@shopify/cli-kit/node/store-auth-session' interface GraphQLClientErrorLike { response: {status?: number; errors?: unknown} @@ -54,17 +51,3 @@ export function classifyAdminApiError(error: unknown, storeFqdn: string): AbortE return undefined } - -export function throwIfStoredStoreAuthIsInvalid(error: unknown, session: StoredStoreAppSession): void { - const status = graphQLClientErrorStatus(error) - if (status !== 401 && status !== 404) return - - // Preview-store sessions are left uncleared: `store auth` overwrites the bucket's - // `currentUserId` regardless, and clearing here would make a follow-up `store info` run - // fall through to a full interactive login instead of repeating this same actionable message. - if (session.kind !== 'preview') { - clearStoredStoreAppSession(session.store, session.userId) - } - - throwStoredAuthInvalidError(session) -} diff --git a/packages/store/src/cli/services/store/auth/preview-claim-recovery.test.ts b/packages/store/src/cli/services/store/auth/preview-claim-recovery.test.ts new file mode 100644 index 00000000000..2561700eb0c --- /dev/null +++ b/packages/store/src/cli/services/store/auth/preview-claim-recovery.test.ts @@ -0,0 +1,77 @@ +import {authenticateStoreWithApp} from './index.js' +import {STORE_AUTH_APP_CLIENT_ID} from './config.js' +import {throwIfStoredStoreAuthIsInvalid} from '@shopify/cli-kit/node/store-auth-recovery' +import { + getCurrentStoredStoreAppSession, + setStoredStoreAppSession, + type StoreAuthSessionSchema, + type StoredStoreAppSession, +} from '@shopify/cli-kit/node/store-auth-session' +import {LocalStorage} from '@shopify/cli-kit/node/local-storage' +import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs' +import {AbortError} from '@shopify/cli-kit/node/error' +import {AdminApiRequestError} from '@shopify/cli-kit/node/api/admin' +import {describe, expect, test, vi} from 'vitest' + +vi.mock('../attribution.js') + +const SHOP = 'shop.myshopify.com' + +// Stopping the run at scope resolution is deliberate: the only question this test asks is whether +// the preview-store guard lets `store auth` start at all, and letting the OAuth flow finish would +// persist a session through the module-level storage this test intentionally bypasses. +const SCOPE_RESOLUTION_REACHED = 'Scope resolution reached, so the preview-store guard did not fire.' + +function previewSession(): StoredStoreAppSession { + return { + store: SHOP, + clientId: STORE_AUTH_APP_CLIENT_ID, + userId: 'preview:placeholder-uuid', + accessToken: 'shpat_preview_token', + scopes: ['read_themes', 'write_themes'], + acquiredAt: '2026-06-08T12:00:00.000Z', + kind: 'preview', + preview: {shopId: '123', name: 'Lavender Candles', createdAt: '2026-06-08T12:00:00.000Z'}, + } +} + +function runStoreAuth(storage: LocalStorage): Promise { + return authenticateStoreWithApp( + {store: SHOP, scopes: 'read_products'}, + { + getCurrentStoredStoreAppSession: (store) => getCurrentStoredStoreAppSession(store, storage), + resolveExistingScopes: () => Promise.reject(new AbortError(SCOPE_RESOLUTION_REACHED)), + openURL: vi.fn(), + waitForStoreAuthCode: vi.fn(), + exchangeStoreAuthCodeForToken: vi.fn(), + presenter: {openingBrowser: vi.fn(), manualAuthUrl: vi.fn(), success: vi.fn()}, + }, + ) +} + +// The two halves of the claimed-preview-store recovery only work as a pair: `store auth` is +// unavailable while a preview session is stored, so the `store auth` next step the invalid-session +// error prints is reachable only because that error clears the session on its way out. +describe('recovering from a claimed preview store', () => { + test('clearing the invalid preview session unblocks the `store auth` run the error points at', async () => { + await inTemporaryDirectory(async (cwd) => { + const storage = new LocalStorage({cwd}) + const session = previewSession() + setStoredStoreAppSession(session, storage) + + // Before recovery: `store auth` refuses to start for a store with a stored preview session. + await expect(runStoreAuth(storage)).rejects.toThrow('`store auth` is unavailable for preview stores.') + + // The 401 the claimed store's stale preview token now answers with. + const claimFailure = new AdminApiRequestError(401, `Error connecting to your store ${SHOP}: Unauthorized`) + expect(() => throwIfStoredStoreAuthIsInvalid(claimFailure, session, {storage})).toThrow( + `The preview store ${SHOP} has likely been claimed, so its stored authentication is no longer valid.`, + ) + expect(getCurrentStoredStoreAppSession(SHOP, storage)).toBeUndefined() + + // After recovery: the guard no longer fires, so the suggested command gets into the flow that + // mints a fresh standard session. + await expect(runStoreAuth(storage)).rejects.toThrow(SCOPE_RESOLUTION_REACHED) + }) + }) +}) diff --git a/packages/store/src/cli/services/store/auth/recovery.test.ts b/packages/store/src/cli/services/store/auth/recovery.test.ts index add964a805e..9dc0b9f5e31 100644 --- a/packages/store/src/cli/services/store/auth/recovery.test.ts +++ b/packages/store/src/cli/services/store/auth/recovery.test.ts @@ -1,152 +1,6 @@ -import { - throwStoredStoreAuthError, - throwReauthenticateStoreAuthError, - throwStoredAuthInvalidError, - retryStoreAuthWithPermanentDomainError, -} from './recovery.js' -import {STORE_AUTH_APP_CLIENT_ID} from './config.js' +import {retryStoreAuthWithPermanentDomainError} from './recovery.js' import {AbortError} from '@shopify/cli-kit/node/error' import {describe, expect, test} from 'vitest' -import type {StoredStoreAppSession} from '@shopify/cli-kit/node/store-auth-session' - -const SHOP = 'shop.myshopify.com' - -function standardSession(overrides: Partial = {}): StoredStoreAppSession { - return { - store: SHOP, - clientId: STORE_AUTH_APP_CLIENT_ID, - userId: '42', - accessToken: 'token', - scopes: ['read_products', 'write_orders'], - acquiredAt: '2026-03-27T00:00:00.000Z', - ...overrides, - } -} - -function previewSession(overrides: Partial = {}): StoredStoreAppSession { - return { - ...standardSession({ - userId: 'preview:placeholder-uuid', - // The full preapproved catalog is much larger in practice; a couple of entries are enough - // to prove the placeholder is used instead of these. - scopes: ['read_products', 'write_products', 'read_themes'], - }), - kind: 'preview', - preview: { - shopId: '123', - name: 'Lavender Candles', - createdAt: '2026-03-27T00:00:00.000Z', - }, - ...overrides, - } -} - -describe('throwStoredStoreAuthError', () => { - test('reports no stored auth and prompts to authenticate (not re-authenticate) with a scopes placeholder', () => { - let captured: AbortError | undefined - try { - throwStoredStoreAuthError(SHOP) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - captured = error as AbortError - } - - expect(captured).toMatchObject({ - message: `No stored app authentication found for ${SHOP}.`, - nextSteps: [ - ['Run', {command: `shopify store auth --store ${SHOP} --scopes `}, 'to authenticate'], - ], - }) - }) -}) - -describe('throwReauthenticateStoreAuthError', () => { - test('suggests the real scopes for a standard session', () => { - let captured: AbortError | undefined - try { - throwReauthenticateStoreAuthError('Custom message.', standardSession()) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - captured = error as AbortError - } - - expect(captured).toMatchObject({ - message: 'Custom message.', - nextSteps: [ - [ - 'Run', - {command: `shopify store auth --store ${SHOP} --scopes read_products,write_orders`}, - 'to re-authenticate', - ], - ], - }) - }) - - test('suggests a scopes placeholder for a preview session instead of its preapproved catalog', () => { - let captured: AbortError | undefined - try { - throwReauthenticateStoreAuthError('Custom message.', previewSession()) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - captured = error as AbortError - } - - expect(captured).toMatchObject({ - message: 'Custom message.', - nextSteps: [ - [ - 'Run', - {command: `shopify store auth --store ${SHOP} --scopes `}, - 'to re-authenticate', - ], - ], - }) - }) -}) - -describe('throwStoredAuthInvalidError', () => { - test('uses the generic invalid-auth message and real scopes for a standard session', () => { - let captured: AbortError | undefined - try { - throwStoredAuthInvalidError(standardSession()) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - captured = error as AbortError - } - - expect(captured).toMatchObject({ - message: `Stored app authentication for ${SHOP} is no longer valid.`, - nextSteps: [ - [ - 'Run', - {command: `shopify store auth --store ${SHOP} --scopes read_products,write_orders`}, - 'to re-authenticate', - ], - ], - }) - }) - - test('flags a likely claim and suggests a scopes placeholder for a preview session', () => { - let captured: AbortError | undefined - try { - throwStoredAuthInvalidError(previewSession()) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - captured = error as AbortError - } - - expect(captured).toMatchObject({ - message: `The preview store ${SHOP} has likely been claimed, so its stored authentication is no longer valid.`, - nextSteps: [ - [ - 'Run', - {command: `shopify store auth --store ${SHOP} --scopes `}, - 'to re-authenticate', - ], - ], - }) - }) -}) describe('retryStoreAuthWithPermanentDomainError', () => { test('returns (rather than throws) an AbortError pointing at the permanent domain with a scopes placeholder', () => { diff --git a/packages/store/src/cli/services/store/auth/recovery.ts b/packages/store/src/cli/services/store/auth/recovery.ts index bcf2f95ae4d..d459aefa719 100644 --- a/packages/store/src/cli/services/store/auth/recovery.ts +++ b/packages/store/src/cli/services/store/auth/recovery.ts @@ -1,62 +1,12 @@ import {AbortError} from '@shopify/cli-kit/node/error' -import type {StoredStoreAppSession} from '@shopify/cli-kit/node/store-auth-session' const UNKNOWN_SCOPES_PLACEHOLDER = '' -function storeAuthCommand(store: string, scopes: string): {command: string} { - return {command: `shopify store auth --store ${store} --scopes ${scopes}`} -} - -function storeAuthCommandNextStepsWithUnknownScopes(store: string) { - return [[storeAuthCommand(store, UNKNOWN_SCOPES_PLACEHOLDER)]] -} - -function storeAuthCommandNextStepsWithPurpose(store: string, scopes: string, purpose: string) { - return [['Run', storeAuthCommand(store, scopes), purpose]] -} - -// Preview-store sessions are preapproved for a large, fixed scope catalog (often 30+ scopes). -// Suggesting the user re-request all of them encourages over-scoping, so they get the same -// placeholder as the "no stored auth" case and choose deliberately instead. -function reauthScopesFor(session: StoredStoreAppSession): string { - return session.kind === 'preview' ? UNKNOWN_SCOPES_PLACEHOLDER : session.scopes.join(',') -} - -export function throwStoredStoreAuthError(store: string): never { - throw new AbortError( - `No stored app authentication found for ${store}.`, - undefined, - storeAuthCommandNextStepsWithPurpose(store, UNKNOWN_SCOPES_PLACEHOLDER, 'to authenticate'), - ) -} - -export function throwReauthenticateStoreAuthError(message: string, session: StoredStoreAppSession): never { - throw new AbortError( - message, - undefined, - storeAuthCommandNextStepsWithPurpose(session.store, reauthScopesFor(session), 'to re-authenticate'), - ) -} - -// A preview store's local session has no way to know it was claimed through the browser claim -// flow; a 401/404 the first time the stale session is used again is the only signal. Surfacing -// that possibility is more useful than the generic "no longer valid" message a standard session -// gets, so every call site that detects an invalid stored session (regardless of which API it -// hit) should go through here instead of writing its own message. -export function throwStoredAuthInvalidError(session: StoredStoreAppSession): never { - const message = - session.kind === 'preview' - ? `The preview store ${session.store} has likely been claimed, so its stored authentication is no longer valid.` - : `Stored app authentication for ${session.store} is no longer valid.` - - throwReauthenticateStoreAuthError(message, session) -} - export function retryStoreAuthWithPermanentDomainError(returnedStore: string): AbortError { // eslint-disable-next-line @shopify/cli/no-error-factory-functions return new AbortError( 'OAuth callback store does not match the requested store.', `Shopify returned ${returnedStore} during authentication. Re-run using the permanent store domain:`, - storeAuthCommandNextStepsWithUnknownScopes(returnedStore), + [[{command: `shopify store auth --store ${returnedStore} --scopes ${UNKNOWN_SCOPES_PLACEHOLDER}`}]], ) } diff --git a/packages/store/src/cli/services/store/auth/session-lifecycle.ts b/packages/store/src/cli/services/store/auth/session-lifecycle.ts index 5cb8c7f090c..fa8cf6a5a61 100644 --- a/packages/store/src/cli/services/store/auth/session-lifecycle.ts +++ b/packages/store/src/cli/services/store/auth/session-lifecycle.ts @@ -1,6 +1,9 @@ import {maskToken} from './config.js' -import {throwStoredStoreAuthError, throwReauthenticateStoreAuthError} from './recovery.js' import {refreshStoreAccessToken} from './token-client.js' +import { + throwMissingStoredStoreAuthError, + throwReauthenticateStoreAuthError, +} from '@shopify/cli-kit/node/store-auth-recovery' import { clearStoredStoreAppSession, getCurrentStoredStoreAppSession, @@ -49,7 +52,7 @@ export async function loadStoredStoreSession(store: string): Promise { ], ], }) - expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42') + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42', undefined) }) test('also clears stored auth on a 401 ClientError-shaped rejection', async () => { @@ -97,7 +97,7 @@ describe('runAdminStoreGraphQLOperation', () => { await expect(runAdminStoreGraphQLOperation({context, request})).rejects.toMatchObject({ message: `Stored app authentication for ${store} is no longer valid.`, }) - expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42') + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42', undefined) }) test('also treats a 404 as a stored-auth-no-longer-valid signal', async () => { @@ -107,7 +107,7 @@ describe('runAdminStoreGraphQLOperation', () => { await expect(runAdminStoreGraphQLOperation({context, request})).rejects.toMatchObject({ message: `Stored app authentication for ${store} is no longer valid.`, }) - expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42') + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42', undefined) }) test('flags a likely claim and does not re-list scopes when a lingering preview session 401s', async () => { @@ -133,7 +133,9 @@ describe('runAdminStoreGraphQLOperation', () => { ], ], }) - expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + // Clearing is what makes the suggested `store auth` runnable: it refuses to start while a + // preview session is still stored for the store. + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, 'preview:placeholder-uuid', undefined) }) test('throws a GraphQL operation error when errors are returned', async () => { @@ -261,7 +263,7 @@ describe('fetchPublicApiVersions', () => { ], ], }) - expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42') + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42', undefined) }) test('also handles 404 as a stored-auth-no-longer-valid signal', async () => { @@ -270,7 +272,7 @@ describe('fetchPublicApiVersions', () => { await expect(fetchPublicApiVersions({adminSession, session})).rejects.toMatchObject({ message: `Stored app authentication for ${store} is no longer valid.`, }) - expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42') + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, '42', undefined) }) test('flags a likely claim and does not re-list scopes when a lingering preview session 401s', async () => { @@ -292,7 +294,7 @@ describe('fetchPublicApiVersions', () => { ], ], }) - expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(store, 'preview:placeholder-uuid', undefined) }) test('maps 402 Unavailable Shop to an AbortError without clearing stored auth', async () => { diff --git a/packages/store/src/cli/services/store/execute/admin-transport.ts b/packages/store/src/cli/services/store/execute/admin-transport.ts index 09aafa7932d..37cadd48868 100644 --- a/packages/store/src/cli/services/store/execute/admin-transport.ts +++ b/packages/store/src/cli/services/store/execute/admin-transport.ts @@ -1,9 +1,5 @@ -import { - classifyAdminApiError, - isGraphQLClientErrorLike, - throwIfStoredStoreAuthIsInvalid, - ABORTED_FETCH_MESSAGE_FRAGMENTS, -} from '../admin-errors.js' +import {classifyAdminApiError, isGraphQLClientErrorLike, ABORTED_FETCH_MESSAGE_FRAGMENTS} from '../admin-errors.js' +import {throwIfStoredStoreAuthIsInvalid} from '@shopify/cli-kit/node/store-auth-recovery' import {adminUrl} from '@shopify/cli-kit/node/api/admin' import {graphqlRequest} from '@shopify/cli-kit/node/api/graphql' import {AbortError} from '@shopify/cli-kit/node/error' diff --git a/packages/store/src/cli/services/store/info/index.test.ts b/packages/store/src/cli/services/store/info/index.test.ts index c91c7f2c76b..ceec547d57b 100644 --- a/packages/store/src/cli/services/store/info/index.test.ts +++ b/packages/store/src/cli/services/store/info/index.test.ts @@ -194,8 +194,10 @@ describe('getStoreInfo', () => { expect(result.adminUrl).toBeUndefined() }) + // The session has to be cleared, not kept: `store auth` refuses to run while a preview session is + // stored for the store, so keeping it would make the next step printed here unreachable. test.each([401, 404])( - 'prompts re-auth without clearing the stale preview session when the preview store lookup returns %s', + 'prompts re-auth and clears the stale preview session when the preview store lookup returns %s', async (status) => { vi.mocked(getCurrentStoredStoreAppSession).mockReturnValueOnce({ store: SHOP, @@ -229,7 +231,7 @@ describe('getStoreInfo', () => { ], ], }) - expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(SHOP, 'placeholder-uuid') }, ) @@ -559,7 +561,7 @@ The CLI is currently unable to prompt for reauthentication.`) ['Run', {command: `shopify store auth --store ${SHOP} --scopes read_products`}, 'to re-authenticate'], ], }) - expect(clearStoredStoreAppSession).toHaveBeenCalledWith(SHOP, '42') + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(SHOP, '42', undefined) }) test('flags a likely claim (not a generic invalid-auth error) for a lingering preview session that 401s against Admin', async () => { @@ -582,7 +584,7 @@ The CLI is currently unable to prompt for reauthentication.`) ], ], }) - expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(SHOP, 'placeholder-uuid', undefined) }) test('also treats Admin 404 as a stored-auth-no-longer-valid signal', async () => { @@ -592,7 +594,7 @@ The CLI is currently unable to prompt for reauthentication.`) await expect(getStoreInfo({store: SHOP})).rejects.toMatchObject({ message: `Stored app authentication for ${SHOP} is no longer valid.`, }) - expect(clearStoredStoreAppSession).toHaveBeenCalledWith(SHOP, '42') + expect(clearStoredStoreAppSession).toHaveBeenCalledWith(SHOP, '42', undefined) }) test('maps unavailable Admin stores to a user-facing AbortError', async () => { diff --git a/packages/store/src/cli/services/store/info/index.ts b/packages/store/src/cli/services/store/info/index.ts index 8edf47be6c4..3d0c6f33a05 100644 --- a/packages/store/src/cli/services/store/info/index.ts +++ b/packages/store/src/cli/services/store/info/index.ts @@ -1,13 +1,16 @@ import {StoreInfoBusinessPlatformStoreNotFoundError, fetchDestinationsContext} from './destinations.js' import {fetchOrganizationShop} from './organization-shop.js' import {mapPlanToPublicHandle} from './plan.js' -import {classifyAdminApiError, throwIfStoredStoreAuthIsInvalid} from '../admin-errors.js' +import {classifyAdminApiError} from '../admin-errors.js' import {recordStoreFqdnMetadata} from '../attribution.js' -import {throwStoredAuthInvalidError} from '../auth/recovery.js' import {loadStoredStoreSession} from '../auth/session-lifecycle.js' import {getPreviewStore, PreviewStoreRequestError} from '../create/preview/client.js' import {storeTypeHandle} from '../store-type.js' -import {getCurrentStoredStoreAppSession} from '@shopify/cli-kit/node/store-auth-session' +import { + throwIfStoredStoreAuthIsInvalid, + throwStoredStoreAuthInvalidError, +} from '@shopify/cli-kit/node/store-auth-recovery' +import {clearStoredStoreAppSession, getCurrentStoredStoreAppSession} from '@shopify/cli-kit/node/store-auth-session' import {AbortError} from '@shopify/cli-kit/node/error' import {adminUrl} from '@shopify/cli-kit/node/api/admin' import {graphqlRequest} from '@shopify/cli-kit/node/api/graphql' @@ -160,11 +163,15 @@ async function fetchPreviewStoreUrls(previewSession: PreviewStoreSession): Promi } } catch (error) { // The CLI has no local signal for when a preview store gets claimed via the browser; a - // 401/404 here is the first indication. The stored session is left uncleared on purpose: it - // isn't needed for `store auth` to take over, and keeping it means every `store info` run - // keeps producing this same actionable message instead of falling through to a full login. + // 401/404 here is the first indication. The stored session has to be cleared for the same + // reason `throwIfStoredStoreAuthIsInvalid` clears it: `throwIfPreviewStore` refuses to run + // `store auth` while a preview session is still stored, so keeping the record would make the + // `store auth` next step this error prints unreachable. Preview credentials are write-once and + // never expire, so a rejection here means they stopped being accepted outright and nothing + // usable is discarded. if (error instanceof PreviewStoreRequestError && (error.status === 401 || error.status === 404)) { - throwStoredAuthInvalidError(previewSession) + clearStoredStoreAppSession(previewSession.store, previewSession.userId) + throwStoredStoreAuthInvalidError(previewSession) } throw error diff --git a/packages/theme/src/cli/utilities/theme-command.test.ts b/packages/theme/src/cli/utilities/theme-command.test.ts index 616e8563d78..762f33b8ce9 100644 --- a/packages/theme/src/cli/utilities/theme-command.test.ts +++ b/packages/theme/src/cli/utilities/theme-command.test.ts @@ -4,8 +4,10 @@ import {describe, vi, expect, test, beforeEach} from 'vitest' import {Config, Flags} from '@oclif/core' import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session' import { + clearStoredStoreAppSession, getCurrentStoredStoreAppSession, listCurrentStoredStoreAppSessions, + type StoredStoreAppSession, } from '@shopify/cli-kit/node/store-auth-session' import {loadEnvironment} from '@shopify/cli-kit/node/environments' import {fileExistsSync} from '@shopify/cli-kit/node/fs' @@ -13,6 +15,7 @@ import {resolvePath} from '@shopify/cli-kit/node/path' import {renderConcurrent, renderConfirmationPrompt, renderError, renderWarning} from '@shopify/cli-kit/node/ui' import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' import {hashString} from '@shopify/cli-kit/node/crypto' +import {AbortError} from '@shopify/cli-kit/node/error' import type {Writable} from 'stream' @@ -112,6 +115,50 @@ class TestThemeCommandWithPath extends TestThemeCommand { static multiEnvironmentsFlags: RequiredFlags = ['store', 'path'] } +class TestFailingThemeCommand extends TestThemeCommand { + failure: Error = new Error('Not configured') + + async command( + flags: any, + session: AdminSession, + multiEnvironment = false, + args?: any, + context?: {stdout?: Writable; stderr?: Writable}, + ): Promise { + await super.command(flags, session, multiEnvironment, args, context) + throw this.failure + } +} + +// The shape a 401 has once the Admin API version is cached: graphql-request's `ClientError`, whose +// HTTP status lives under `response`. +function adminApiClientError(status: number, message = 'GraphQL Error'): Error { + const error = new Error(message) as Error & {response: {status: number; errors: {message: string}[]}} + error.response = {status, errors: [{message}]} + return error +} + +function storedStoreAuthSession(overrides: Partial = {}): StoredStoreAppSession { + return { + store: 'test-store.myshopify.com', + clientId: 'store-auth-client-id', + userId: '42', + accessToken: 'shpat_stored_token', + scopes: ['read_themes'], + acquiredAt: '2026-06-08T11:00:00.000Z', + ...overrides, + } +} + +function storedPreviewStoreAuthSession(overrides: Partial = {}): StoredStoreAppSession { + return storedStoreAuthSession({ + userId: 'preview:123', + kind: 'preview', + preview: {shopId: '123', name: 'Lavender Candles', createdAt: '2026-06-08T11:00:00.000Z'}, + ...overrides, + }) +} + class TestUnauthenticatedThemeCommand extends ThemeCommand { static flags = { environment: Flags.string({ @@ -916,11 +963,39 @@ describe('ThemeCommand', () => { // Then expect(renderError).toHaveBeenCalledWith( expect.objectContaining({ - body: ['Environment command-error failed: \n\nMocking a command error'], + headline: 'Environment command-error failed:', + body: 'Mocking a command error', }), ) }) + // `renderError` renders a token array as one space-joined paragraph, so the `tryMessage` has to + // carry the paragraph break itself to stay the distinct block `renderFatalError` gets for free. + test("keeps a failure's try message a separate paragraph from its message", async () => { + vi.mocked(loadEnvironment).mockResolvedValue({store: 'store.myshopify.com'}) + vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) + vi.mocked(renderConcurrent).mockImplementation(async ({processes}) => { + for (const process of processes) { + // eslint-disable-next-line no-await-in-loop + await process.action({} as Writable, {} as Writable, {} as any) + } + }) + + await CommandConfig.load() + const command = new TestFailingThemeCommand( + ['--environment', 'broken', '--environment', 'development'], + CommandConfig, + ) + command.failure = new AbortError('Theme could not be pushed.', ['Check the', {command: 'theme list'}, 'output.']) + + await command.run() + + expect(renderError).toHaveBeenCalledWith({ + headline: 'Environment broken failed:', + body: ['Theme could not be pushed.', '\n\nCheck the', {command: 'theme list'}, 'output.'], + }) + }) + test('commands should display an error if the --path flag is used', async () => { // Given const environmentConfig = {store: 'store.myshopify.com'} @@ -1111,4 +1186,145 @@ describe('ThemeCommand', () => { expect(ensureAuthenticatedThemes).not.toHaveBeenCalled() }) }) + + describe('stored store auth recovery', () => { + async function runFailingCommand(argv: string[], failure: Error): Promise { + await CommandConfig.load() + const command = new TestFailingThemeCommand(argv, CommandConfig) + command.failure = failure + + return command.run().then( + () => { + throw new Error('Expected the command to fail') + }, + (error: Error) => error, + ) + } + + // The session has to be cleared, not kept: `store auth` refuses to run while a preview session + // is stored for the store, so keeping it would make the next step printed here unreachable. + test('reports a likely claim and clears the session when a stored preview session gets a 401', async () => { + vi.mocked(getCurrentStoredStoreAppSession).mockReturnValue(storedPreviewStoreAuthSession()) + + const error = await runFailingCommand([], adminApiClientError(401)) + + expect(error).toMatchObject({ + message: + 'The preview store test-store.myshopify.com has likely been claimed, so its stored authentication is no longer valid.', + nextSteps: [ + [ + 'Run', + {command: 'shopify store auth --store test-store.myshopify.com --scopes '}, + 'to re-authenticate', + ], + ], + }) + expect(clearStoredStoreAppSession).toHaveBeenCalledWith('test-store.myshopify.com', 'preview:123', undefined) + }) + + // `store` commands classify a standard session's 401 because they reach it only after + // refreshing an expired token. Theme commands use the stored access token as-is, so this + // session's 401 is the ordinary "expired, needs refreshing" case: re-authenticating isn't the + // fix, and clearing the record would throw away the refresh token that is. + test('leaves a stored standard session untouched when it gets a 401, refresh token included', async () => { + vi.mocked(getCurrentStoredStoreAppSession).mockReturnValue( + storedStoreAuthSession({ + expiresAt: '2026-06-08T12:00:00.000Z', + refreshToken: 'refresh-token-that-would-still-work', + }), + ) + const failure = adminApiClientError(401, '[API] Invalid API key or access token') + + const error = await runFailingCommand([], failure) + + expect(error).toBe(failure) + expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + }) + + test.each([ + ['404, which theme commands never classify because `--theme ` can genuinely not exist', 404], + ['an unrelated HTTP status', 500], + ])('rethrows %s unchanged', async (_description, status) => { + vi.mocked(getCurrentStoredStoreAppSession).mockReturnValue(storedPreviewStoreAuthSession()) + const failure = adminApiClientError(status, 'Theme not found') + + const error = await runFailingCommand([], failure) + + expect(error).toBe(failure) + expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + }) + + // A stored preview session for the same store is arranged deliberately: it is the one session + // that would otherwise produce the claim message, so the test fails if `--password` consults + // stored auth at all. `shptka_` and `shpat_` are both covered because a custom-app token is + // byte-indistinguishable from a stored one - provenance can never be inferred from the prefix. + test.each(['shptka_theme_access_password', 'shpat_custom_app_password'])( + 'never blames `shopify store auth` for a 401 from the explicitly supplied token %s', + async (password) => { + vi.mocked(getCurrentStoredStoreAppSession).mockReturnValue(storedPreviewStoreAuthSession()) + vi.mocked(listCurrentStoredStoreAppSessions).mockReturnValue([storedPreviewStoreAuthSession()]) + vi.mocked(ensureAuthenticatedThemes).mockResolvedValue({ + token: password, + storeFqdn: 'test-store.myshopify.com', + }) + const failure = adminApiClientError(401, '[API] Invalid API key or access token') + + const error = await runFailingCommand(['--password', password], failure) + + expect(error).toBe(failure) + expect(getCurrentStoredStoreAppSession).not.toHaveBeenCalled() + expect(listCurrentStoredStoreAppSessions).not.toHaveBeenCalled() + expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + }, + ) + + test('recovers per environment, keeping the next steps and still running unaffected environments', async () => { + vi.mocked(loadEnvironment) + .mockResolvedValueOnce({store: 'claimed.myshopify.com'}) + .mockResolvedValueOnce({store: 'healthy.myshopify.com'}) + vi.mocked(ensureThemeStore).mockImplementation((options: any) => options.store) + vi.mocked(listCurrentStoredStoreAppSessions).mockReturnValue([ + storedPreviewStoreAuthSession({store: 'claimed.myshopify.com'}), + storedPreviewStoreAuthSession({store: 'healthy.myshopify.com'}), + ]) + vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) + vi.mocked(renderConcurrent).mockImplementation(async ({processes}) => { + for (const process of processes) { + // eslint-disable-next-line no-await-in-loop + await process.action({} as Writable, {} as Writable, {} as any) + } + }) + + await CommandConfig.load() + const command = new TestFailingThemeCommand( + ['--environment', 'claimed', '--environment', 'healthy'], + CommandConfig, + ) + // Only the first environment's request is rejected; the second must still be attempted. + command.failure = adminApiClientError(401) + const originalCommand = command.command.bind(command) + command.command = async (flags, session, multiEnvironment, args, context) => { + if (flags.store !== 'claimed.myshopify.com') { + command.commandCalls.push({flags, session, multiEnvironment: multiEnvironment ?? false, args, context}) + return + } + await originalCommand(flags, session, multiEnvironment, args, context) + } + + await command.run() + + expect(renderError).toHaveBeenCalledWith({ + headline: 'Environment claimed failed:', + body: 'The preview store claimed.myshopify.com has likely been claimed, so its stored authentication is no longer valid.', + nextSteps: [ + [ + 'Run', + {command: 'shopify store auth --store claimed.myshopify.com --scopes '}, + 'to re-authenticate', + ], + ], + }) + expect(command.commandCalls.map(({flags}) => flags.store)).toContain('healthy.myshopify.com') + }) + }) }) diff --git a/packages/theme/src/cli/utilities/theme-command.ts b/packages/theme/src/cli/utilities/theme-command.ts index f8a7d578fcc..565b8305e92 100644 --- a/packages/theme/src/cli/utilities/theme-command.ts +++ b/packages/theme/src/cli/utilities/theme-command.ts @@ -12,6 +12,7 @@ import { listCurrentStoredStoreAppSessions, type StoredStoreAppSession, } from '@shopify/cli-kit/node/store-auth-session' +import {throwIfStoredStoreAuthIsInvalid} from '@shopify/cli-kit/node/store-auth-recovery' import {loadEnvironment} from '@shopify/cli-kit/node/environments' import { renderWarning, @@ -19,9 +20,10 @@ import { renderConfirmationPrompt, RenderConfirmationPromptOptions, renderError, + type TokenItem, } from '@shopify/cli-kit/node/ui' import {AbortController} from '@shopify/cli-kit/node/abort' -import {AbortError} from '@shopify/cli-kit/node/error' +import {AbortError, FatalError} from '@shopify/cli-kit/node/error' import {recordEvent, compileData} from '@shopify/cli-kit/node/analytics' import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' import {cwd, joinPath, resolvePath} from '@shopify/cli-kit/node/path' @@ -31,11 +33,26 @@ import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' import type {Writable} from 'stream' type FlagValues = Record + +/** + * An Admin session together with the provenance the recovery flow needs. + * + * Commands only ever receive the plain `adminSession`; `storedStoreAppSession` stays internal to + * this class so that a failure can be traced back to the `shopify store auth` session that produced + * it. Provenance is never inferred from the token string, because a stored preview-store token and + * an explicitly supplied custom-app token are both `shpat_…`. + */ +interface ThemeSessionContext { + adminSession: AdminSession + /** Set only when `adminSession` came from a locally stored `shopify store auth` session. */ + storedStoreAppSession?: StoredStoreAppSession +} + interface ValidEnvironment { environment: EnvironmentName flags: FlagValues requiresAuth: boolean - storeAuthSession?: AdminSession + storeAuthSession?: ThemeSessionContext } type EnvironmentName = string /** @@ -54,6 +71,65 @@ type EnvironmentName = string */ export type RequiredFlags = (string | string[])[] | null +// Theme commands classify HTTP 401 only. A 401 is what a claimed preview store's stale Admin token +// actually returns, and theme commands accept `--theme `, so classifying 404 as well would +// misreport a genuinely missing theme as a claimed store. `store` commands keep classifying both. +const THEME_INVALID_STORE_AUTH_STATUSES = [401] + +// A stored `store auth` session can stop being accepted without any local signal, most often +// because its preview store was claimed through the browser. Only a run actually backed by a stored +// session is eligible for the recovery message: an explicitly supplied `--password` token can look +// identical (`shpat_…`) and must never be blamed on `shopify store auth`. +// +// Preview sessions are the only kind theme commands classify. `store` commands reach a 401 *after* +// `loadStoredStoreSession` has already refreshed an expired token, so there the status is a +// definitive verdict on the session. Theme commands use the stored access token as-is, with no +// expiry check and no refresh, so an ordinary expired-but-refreshable standard session 401s here +// for a reason re-authenticating wouldn't fix - and `throwIfStoredStoreAuthIsInvalid` would clear +// the record, destroying a refresh token that would have worked. A standard session's 401 therefore +// propagates raw, exactly as it did before this recovery flow existed. Preview sessions carry no +// refresh token to lose, so nothing is protected by staying quiet about them. +function throwIfThemeStoreAuthIsInvalid(error: unknown, sessionContext: ThemeSessionContext | undefined): void { + const storedSession = sessionContext?.storedStoreAppSession + if (storedSession?.kind !== 'preview') return + + throwIfStoredStoreAuthIsInvalid(error, storedSession, {invalidStatuses: THEME_INVALID_STORE_AUTH_STATUSES}) +} + +// `renderError` takes a single `body` and renders a token array as one space-joined paragraph, so +// appending the `tryMessage` tokens to the message would run the two together mid-sentence. The +// blank line that `renderFatalError` gets for free (it renders message and `tryMessage` as separate +// blocks) is carried here on the `tryMessage`'s leading token instead, so the two stay distinct +// paragraphs. +function bodyWithSeparateTryMessage(message: string, tryMessage: TokenItem | null | undefined): TokenItem { + const tryMessageTokens = tryMessage ? [tryMessage].flat() : [] + const [firstToken, ...remainingTokens] = tryMessageTokens + if (firstToken === undefined) return message + + const paragraphBreak = '\n\n' + + // Only a string token can carry the break without a stray leading space, because a token + // following a non-`char` token is always rendered with a space in front of it. + return typeof firstToken === 'string' + ? [message, `${paragraphBreak}${firstToken}`, ...remainingTokens] + : [message, paragraphBreak, firstToken, ...remainingTokens] +} + +// `renderError` only prints what it is given, so prefixing the environment onto `error.message` (as +// this used to do) silently dropped a `FatalError`'s `tryMessage`, `nextSteps` and custom sections. +// The environment is prefixed structurally as a headline instead, and the remaining parts are +// forwarded, so per-environment failures keep the guidance that makes them actionable. +function renderEnvironmentFailure(environment: EnvironmentName, error: Error): void { + const fatalError = error instanceof FatalError ? error : undefined + + renderError({ + headline: `Environment ${environment} failed:`, + body: bodyWithSeparateTryMessage(error.message, fatalError?.tryMessage), + ...(fatalError?.nextSteps?.length ? {nextSteps: fatalError.nextSteps} : {}), + ...(fatalError?.customSections?.length ? {customSections: fatalError.customSections} : {}), + }) +} + export default abstract class ThemeCommand extends Command { static baseFlags = authAliasFlag @@ -97,7 +173,8 @@ export default abstract class ThemeCommand extends Command { throw new AbortError(`Please provide a valid environment.`) } - const session = commandRequiresAuth ? await this.createSession(flags) : undefined + const sessionContext = commandRequiresAuth ? await this.createSession(flags) : undefined + const session = sessionContext?.adminSession const commandName = this.constructor.name.toLowerCase() recordEvent(`theme-command:${commandName}:single-env:authenticated`) @@ -108,6 +185,9 @@ export default abstract class ThemeCommand extends Command { try { await this.command(flags, session, false, args) + } catch (error) { + throwIfThemeStoreAuthIsInvalid(error, sessionContext) + throw error } finally { await this.logAnalyticsData(session) } @@ -199,7 +279,7 @@ export default abstract class ThemeCommand extends Command { const storeAuthSessionsByStore = requiresAuth ? this.storeAuthSessionsForTheme(Array.from(environmentMap.values()).map(({validationFlags}) => validationFlags)) - : new Map() + : new Map() const entriesWithStoreAuthSessions = Array.from(environmentMap.entries()).map( ([environmentName, {flags, validationFlags}]) => ({ @@ -299,13 +379,17 @@ export default abstract class ThemeCommand extends Command { try { const store = flags.store as string await useThemeStoreContext(store, async () => { - const session = requiresAuth ? await this.createSession(flags, storeAuthSession) : undefined + const sessionContext = requiresAuth ? await this.createSession(flags, storeAuthSession) : undefined + const session = sessionContext?.adminSession const commandName = this.constructor.name.toLowerCase() recordEvent(`theme-command:${commandName}:multi-env:authenticated`) try { await this.command(flags, session, true, {}, {stdout, stderr}) + } catch (error) { + throwIfThemeStoreAuthIsInvalid(error, sessionContext) + throw error } finally { await this.logAnalyticsData(session) } @@ -314,8 +398,7 @@ export default abstract class ThemeCommand extends Command { // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { if (error instanceof Error) { - error.message = `Environment ${environment} failed: \n\n${error.message}` - renderError({body: [error.message]}) + renderEnvironmentFailure(environment, error) } } }, @@ -347,21 +430,24 @@ export default abstract class ThemeCommand extends Command { /** * Create an unauthenticated session object from store and password * @param flags - The environment flags containing store and password - * @returns The unauthenticated session object + * @param storeAuthSession - A store-auth session already resolved for this environment + * @returns The unauthenticated session object and where it came from */ - private async createSession(flags: FlagValues, storeAuthSession?: AdminSession) { + private async createSession(flags: FlagValues, storeAuthSession?: ThemeSessionContext): Promise { const store = ensureThemeStore({store: flags.store as string | undefined}) const password = flags.password as string | undefined - const session = password - ? await ensureAuthenticatedThemes(store, password) - : (storeAuthSession ?? - (await this.storeAuthSessionForTheme({store})) ?? - (await ensureAuthenticatedThemes(store, password))) - return session + if (password) { + return {adminSession: await ensureAuthenticatedThemes(store, password)} + } + + const storeAuthContext = storeAuthSession ?? (await this.storeAuthSessionForTheme({store})) + if (storeAuthContext) return storeAuthContext + + return {adminSession: await ensureAuthenticatedThemes(store, password)} } - private async storeAuthSessionForTheme(flags: FlagValues): Promise { + private async storeAuthSessionForTheme(flags: FlagValues): Promise { const store = typeof flags.store === 'string' ? flags.store : undefined const password = flags.password if (!store || password) return undefined @@ -370,10 +456,10 @@ export default abstract class ThemeCommand extends Command { const storedSession = getCurrentStoredStoreAppSession(storeFqdn) if (!storedSession) return undefined - return this.adminSessionFromStoreAuthSession(storedSession, storeFqdn) + return this.themeSessionContextFromStoreAuthSession(storedSession, storeFqdn) } - private storeAuthSessionsForTheme(flagsList: FlagValues[]): Map { + private storeAuthSessionsForTheme(flagsList: FlagValues[]): Map { const stores = new Set( flagsList .filter(({store, password}) => typeof store === 'string' && !password) @@ -387,17 +473,17 @@ export default abstract class ThemeCommand extends Command { const storeFqdn = normalizeStoreFqdn(storedSession.store) if (!stores.has(storeFqdn)) return undefined - const session = this.adminSessionFromStoreAuthSession(storedSession, storeFqdn) - return session ? ([storeFqdn, session] as const) : undefined + const sessionContext = this.themeSessionContextFromStoreAuthSession(storedSession, storeFqdn) + return sessionContext ? ([storeFqdn, sessionContext] as const) : undefined }) - .filter((entry): entry is readonly [string, AdminSession] => entry !== undefined), + .filter((entry): entry is readonly [string, ThemeSessionContext] => entry !== undefined), ) } private storeAuthSessionFromCache( flags: FlagValues, - storeAuthSessionsByStore: Map, - ): AdminSession | undefined { + storeAuthSessionsByStore: Map, + ): ThemeSessionContext | undefined { const store = typeof flags.store === 'string' ? flags.store : undefined const password = flags.password if (!store || password) return undefined @@ -405,10 +491,10 @@ export default abstract class ThemeCommand extends Command { return storeAuthSessionsByStore.get(normalizeStoreFqdn(store)) } - private adminSessionFromStoreAuthSession( + private themeSessionContextFromStoreAuthSession( storedSession: StoredStoreAppSession, storeFqdn: string, - ): AdminSession | undefined { + ): ThemeSessionContext | undefined { if (!this.hasRequiredStoreAuthScopes(storedSession.scopes)) { return undefined } @@ -416,8 +502,11 @@ export default abstract class ThemeCommand extends Command { setLastSeenUserId(storedSession.userId) return { - token: storedSession.accessToken, - storeFqdn, + adminSession: { + token: storedSession.accessToken, + storeFqdn, + }, + storedStoreAppSession: storedSession, } } @@ -452,7 +541,7 @@ export default abstract class ThemeCommand extends Command { environmentFlags: FlagValues, requiredFlags: Exclude, environmentName: string, - storeAuthSession?: AdminSession, + storeAuthSession?: ThemeSessionContext, ): string[] | true { const missingFlags = requiredFlags .filter((flag) => @@ -475,7 +564,7 @@ export default abstract class ThemeCommand extends Command { return true } - private hasRequiredFlag(environmentFlags: FlagValues, flag: string, storeAuthSession?: AdminSession): boolean { + private hasRequiredFlag(environmentFlags: FlagValues, flag: string, storeAuthSession?: ThemeSessionContext): boolean { if (flag === 'password' && storeAuthSession) return true return Boolean(environmentFlags[flag]) }