Skip to content

Reuse preview store sessions in every theme command #8546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dmerand wants to merge 1 commit into from
+103 −19
Open

Reuse preview store sessions in every theme command #8546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tidy-preview-store-sessions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme': patch
---

Fix theme commands on preview stores requiring login. Every theme command reuses a stored preview store session again, not only `theme pull` and `theme push`.
78 changes: 74 additions & 4 deletions packages/theme/src/cli/utilities/theme-command.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ describe('ThemeCommand', () => {
expect(command.commandCalls[0]).toMatchObject({session: mockSession})
})

test('ignores the store auth cache when the command does not declare store auth scopes', async () => {
test('ignores a standard store auth cache session when the command does not declare store auth scopes', async () => {
vi.mocked(getCurrentStoredStoreAppSession).mockReturnValue({
store: 'test-store.myshopify.com',
clientId: 'store-auth-client-id',
Expand All @@ -338,15 +338,42 @@ describe('ThemeCommand', () => {
scopes: ['read_themes', 'write_themes'],
acquiredAt: '2026-06-08T11:00:00.000Z',
})
const outputMock = mockAndCaptureOutput()

await CommandConfig.load()
const command = new TestThemeCommand([], CommandConfig)

await command.run()

expect(getCurrentStoredStoreAppSession).not.toHaveBeenCalled()
expect(getCurrentStoredStoreAppSession).toHaveBeenCalledWith('test-store.myshopify.com')
expect(ensureAuthenticatedThemes).toHaveBeenCalledWith('test-store.myshopify.com', undefined)
expect(command.commandCalls[0]).toMatchObject({session: mockSession})
expect(outputMock.debug()).toContain(
'Ignoring stored store auth session for test-store.myshopify.com: it is a standard session and this command only reuses preview store sessions.',
)
})

test('reuses a preview store session when the command does not declare store auth scopes', async () => {
vi.mocked(getCurrentStoredStoreAppSession).mockReturnValue({
store: 'test-store.myshopify.com',
clientId: 'store-auth-client-id',
userId: 'preview:123',
accessToken: 'shpat_preview_token',
scopes: [],
acquiredAt: '2026-06-08T11:00:00.000Z',
kind: 'preview',
preview: {shopId: '1', name: 'Preview Store', createdAt: '2026-06-08T11:00:00.000Z'},
})

await CommandConfig.load()
const command = new TestThemeCommand([], CommandConfig)

await command.run()

expect(ensureAuthenticatedThemes).not.toHaveBeenCalled()
expect(command.commandCalls[0]).toMatchObject({
session: {token: 'shpat_preview_token', storeFqdn: 'test-store.myshopify.com'},
})
})

test('treats a matching write scope in the stored session as satisfying a required read scope', async () => {
Expand Down Expand Up @@ -1129,7 +1156,50 @@ describe('ThemeCommand', () => {
expect(ensureAuthenticatedThemes).not.toHaveBeenCalled()
})

test('multiple environment commands ignore the store auth cache when the command does not declare store auth scopes', async () => {
test('multiple environment commands accept a preview store session without declared store auth scopes', async () => {
vi.mocked(loadEnvironment)
.mockResolvedValueOnce({store: 'store1.myshopify.com', path: '/home/path/to/theme1'})
.mockResolvedValueOnce({store: 'store2.myshopify.com', password: 'password2', path: '/home/path/to/theme2'})
vi.mocked(listCurrentStoredStoreAppSessions).mockReturnValue([
{
store: 'store1.myshopify.com',
clientId: 'store-auth-client-id',
userId: 'preview:123',
accessToken: 'shpat_preview_token',
scopes: [],
acquiredAt: '2026-06-08T11:00:00.000Z',
kind: 'preview',
preview: {shopId: '1', name: 'Preview Store', createdAt: '2026-06-08T11:00:00.000Z'},
},
])
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)
}
})
vi.mocked(ensureThemeStore).mockImplementation((options: any) => options.store)

await CommandConfig.load()
const command = new TestThemeCommandWithPathFlag(
['--environment', 'preview', '--environment', 'another-preview'],
CommandConfig,
)

await command.run()

expect(renderWarning).not.toHaveBeenCalled()
expect(command.commandCalls).toEqual(
expect.arrayContaining([
expect.objectContaining({
session: {token: 'shpat_preview_token', storeFqdn: 'store1.myshopify.com'},
}),
]),
)
})

test('multiple environment commands ignore a standard store auth cache session when the command does not declare store auth scopes', async () => {
vi.mocked(loadEnvironment)
.mockResolvedValueOnce({store: 'store1.myshopify.com', path: '/home/path/to/theme1'})
.mockResolvedValueOnce({store: 'store2.myshopify.com', password: 'password2', path: '/home/path/to/theme2'})
Expand All @@ -1153,7 +1223,7 @@ describe('ThemeCommand', () => {

await command.run()

expect(listCurrentStoredStoreAppSessions).not.toHaveBeenCalled()
expect(listCurrentStoredStoreAppSessions).toHaveBeenCalledOnce()
expect(renderWarning).toHaveBeenCalledWith(
expect.objectContaining({
body: ['Missing required flags in environment configuration for preview:', {list: {items: ['password']}}],
Expand Down
39 changes: 24 additions & 15 deletions packages/theme/src/cli/utilities/theme-command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,11 @@ export default abstract class ThemeCommand extends Command {

/**
* Admin API scopes that a stored `store auth` session must include for this
* command to reuse it. Commands opt in to reusing store auth sessions by
* returning the scopes they require; the default opts the command out.
* command to reuse it. Commands opt in to reusing standard store auth
* sessions by returning the scopes they require; the default opts the command
* out of standard sessions. Every theme command reuses preview store sessions
* regardless, because the CLI mints those for the store and cannot re-mint
* them.
*/
protected storeAuthScopes(): string[] | undefined {
return undefined
Expand Down Expand Up @@ -369,9 +372,6 @@ export default abstract class ThemeCommand extends Command {
}

private async storeAuthSessionForTheme(flags: FlagValues): Promise<AdminSession | undefined> {
const requiredScopes = this.storeAuthScopes()
if (!requiredScopes) return undefined

const store = typeof flags.store === 'string' ? flags.store : undefined
const password = flags.password
if (!store || password) return undefined
Expand All @@ -380,12 +380,11 @@ export default abstract class ThemeCommand extends Command {
const storedSession = getCurrentStoredStoreAppSession(storeFqdn)
if (!storedSession) return undefined

return this.adminSessionFromStoreAuthSession(storedSession, storeFqdn, requiredScopes)
return this.adminSessionFromStoreAuthSession(storedSession, storeFqdn, this.storeAuthScopes())
}

private storeAuthSessionsForTheme(flagsList: FlagValues[]): Map<string, AdminSession> {
const requiredScopes = this.storeAuthScopes()
if (!requiredScopes) return new Map()

const stores = new Set(
flagsList
Expand Down Expand Up @@ -421,7 +420,7 @@ export default abstract class ThemeCommand extends Command {
private adminSessionFromStoreAuthSession(
storedSession: StoredStoreAppSession,
storeFqdn: string,
requiredScopes: string[],
requiredScopes: string[] | undefined,
): AdminSession | undefined {
if (isSessionExpired(storedSession)) {
outputDebug(
Expand All @@ -430,13 +429,23 @@ export default abstract class ThemeCommand extends Command {
return undefined
}

if (!this.hasRequiredStoreAuthScopes(storedSession.scopes, requiredScopes)) {
outputDebug(
`Ignoring stored store auth session for ${storeFqdn}: it is missing required scopes (has: ${storedSession.scopes.join(
', ',
)}; needs: ${requiredScopes.join(', ')}).`,
)
return undefined
const isPreviewSession = storedSession.kind === 'preview'

@tizmagik tizmagik Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth thinking about what this does to a stale preview session, the one from a store that's since been claimed. Before this PR it broke theme pull and theme push. After it, every theme command.

There's no way for a user to clear it. shopify auth logout is just sessionStore.remove() (packages/cli-kit/src/public/node/session.ts:327) and never touches the shopify-cli-store storage, and there's no store auth logout. The only clear that exists today is the side effect in shopify store info (packages/store/src/cli/services/store/info/index.ts:166), which fires on a 401/404 from the preview-store API. Nobody is going to find that.

That's the failure mode #8183's commit message called out, 401s that can't be fixed with auth logout. #8390 is one answer, but the concern raised in the thread about clear-on-401 being unrecoverable for an unclaimed store still stands. A shopify store auth logout --store <store> would cover it without that risk.

@dmerand dmerand Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring this from #8546. #8390 already clears the stored preview session on a 401 and points to store auth, so it covers the main path. Two notes: (1) #8390 is still a draft, so landing order matters — if #8546 lands first there is a window with no manual clear; (2) the residual concern stands: clear-on-401 is unrecoverable for an unclaimed store, and store auth logout --store would cover it without that risk. Keeping both as follow-ups.

if (!isPreviewSession) {
if (!requiredScopes) {
outputDebug(
`Ignoring stored store auth session for ${storeFqdn}: it is a standard session and this command only reuses preview store sessions.`,
)
return undefined
}

if (!this.hasRequiredStoreAuthScopes(storedSession.scopes, requiredScopes)) {
Comment thread
dmerand marked this conversation as resolved.
outputDebug(
`Ignoring stored store auth session for ${storeFqdn}: it is missing required scopes (has: ${storedSession.scopes.join(
', ',
)}; needs: ${requiredScopes.join(', ')}).`,
)
return undefined
}
}

outputDebug(`Using stored store auth session for ${storeFqdn} (scopes: ${storedSession.scopes.join(', ')}).`)
Expand Down
Loading