From 7c88b24e3014c80dab39e2e3f098ec2a3f241e09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 21 Oct 2026 11:58:28 +0200 Subject: [PATCH] Fix saved account loss across concurrent CLI sessions --- .changeset/preserve-auth-sessions.md | 6 ++ packages/cli-kit/package.json | 2 + .../cli-kit/src/private/node/conf-store.ts | 17 +++ .../cli-kit/src/private/node/session.test.ts | 63 ++++++++++- packages/cli-kit/src/private/node/session.ts | 22 ++-- .../node/session/store.integration.test.ts | 101 ++++++++++++++++++ .../src/private/node/session/store.test.ts | 27 ++--- .../cli-kit/src/private/node/session/store.ts | 60 ++++++----- .../cli-kit/src/public/node/local-storage.ts | 9 ++ pnpm-lock.yaml | 6 ++ 10 files changed, 257 insertions(+), 56 deletions(-) create mode 100644 .changeset/preserve-auth-sessions.md create mode 100644 packages/cli-kit/src/private/node/session/store.integration.test.ts diff --git a/.changeset/preserve-auth-sessions.md b/.changeset/preserve-auth-sessions.md new file mode 100644 index 00000000000..3cb2356dbe5 --- /dev/null +++ b/.changeset/preserve-auth-sessions.md @@ -0,0 +1,6 @@ +--- +'@shopify/cli': patch +'@shopify/cli-kit': patch +--- + +Preserve saved accounts when CLI instances authenticate at the same time or token refresh fails. diff --git a/packages/cli-kit/package.json b/packages/cli-kit/package.json index c7cc524f7c5..37ae101c481 100644 --- a/packages/cli-kit/package.json +++ b/packages/cli-kit/package.json @@ -147,6 +147,7 @@ "node-fetch": "3.3.2", "open": "8.4.2", "pathe": "1.1.2", + "proper-lockfile": "4.1.2", "react": "19.2.4", "semver": "7.8.4", "react-dom": "19.2.4", @@ -163,6 +164,7 @@ "@types/fs-extra": "9.0.13", "@types/gradient-string": "^1.1.2", "@types/lodash": "4.17.24", + "@types/proper-lockfile": "4.1.4", "@types/react": "^19.0.0", "@types/react-dom": "^19.0.0", "@types/semver": "^7.5.2", diff --git a/packages/cli-kit/src/private/node/conf-store.ts b/packages/cli-kit/src/private/node/conf-store.ts index 72b583f1f5f..41b6a84fd3c 100644 --- a/packages/cli-kit/src/private/node/conf-store.ts +++ b/packages/cli-kit/src/private/node/conf-store.ts @@ -2,6 +2,7 @@ import {isLocalEnvironment} from './context/service.js' import {isUnitTest} from '../../public/node/context/local.js' import {LocalStorage} from '../../public/node/local-storage.js' import {outputContent, outputDebug} from '../../public/node/output.js' +import lockfile from 'proper-lockfile' interface CacheValue { value: T @@ -55,6 +56,22 @@ function currentSessionIdKey(): 'currentDevSessionId' | 'currentSessionId' { return isLocalEnvironment() ? 'currentDevSessionId' : 'currentSessionId' } +/** Serializes session mutations across CLI processes without holding a lock during authentication. */ +export async function withSessionStoreLock( + action: () => Promise, + config: LocalStorage = cliKitStore(), +): Promise { + const release = await lockfile.lock(config.path, { + realpath: false, + retries: {retries: 20, minTimeout: 10, maxTimeout: 100}, + }) + try { + return await action() + } finally { + await release() + } +} + /** * Get session. * diff --git a/packages/cli-kit/src/private/node/session.test.ts b/packages/cli-kit/src/private/node/session.test.ts index cf0d9fc6445..1e85f9310e8 100644 --- a/packages/cli-kit/src/private/node/session.test.ts +++ b/packages/cli-kit/src/private/node/session.test.ts @@ -13,6 +13,7 @@ import { exchangeCustomPartnerToken, refreshAccessToken, InvalidGrantError, + InvalidRequestError, } from './session/exchange.js' import {allDefaultScopes} from './session/scopes.js' import {store as storeSessions, fetch as fetchSessions, remove as secureRemove} from './session/store.js' @@ -204,6 +205,7 @@ describe('ensureAuthenticated when previous session is invalid', () => { expect(refreshAccessToken).not.toBeCalled() expect(businessPlatformRequest).toHaveBeenCalled() expect(storeSessions).toHaveBeenCalledOnce() + expect(setCurrentSessionId).toHaveBeenCalledWith(userId) expect(got).toEqual(validTokens) // Verify the session was stored with email as alias @@ -216,7 +218,7 @@ describe('ensureAuthenticated when previous session is invalid', () => { expect(fetchSessions).toHaveBeenCalledOnce() }) - test('throws an error and logs out if there is no session and prompting is disabled,', async () => { + test('throws an error without removing saved accounts when prompting is disabled', async () => { // Given vi.mocked(validateSession).mockResolvedValueOnce('needs_full_auth') vi.mocked(fetchSessions).mockResolvedValue(undefined) @@ -228,7 +230,7 @@ describe('ensureAuthenticated when previous session is invalid', () => { The CLI is currently unable to prompt for reauthentication.`, ) - expect(secureRemove).toHaveBeenCalled() + expect(secureRemove).not.toHaveBeenCalled() // Then await expect(getLastSeenAuthMethod()).resolves.toEqual('none') @@ -439,13 +441,68 @@ describe('when existing session is valid', () => { // Then expect(refreshAccessToken).toHaveBeenCalled() - expect(storeSessions).toHaveBeenCalledWith(sessions) + expect(storeSessions).toHaveBeenCalledWith({[fqdn]: {[selectedUserId]: sessions[fqdn]![selectedUserId]}}) expect(setCurrentSessionId).not.toHaveBeenCalled() expect(got).toEqual({...validTokens, userId: selectedUserId}) }) }) describe('when existing session is expired', () => { + test.each([ + ['invalid_grant', InvalidGrantError], + ['invalid_request', InvalidRequestError], + ] as const)( + 'preserves all saved accounts when a background refresh fails with %s', + async (_errorName, ErrorClass) => { + vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh') + vi.mocked(fetchSessions).mockResolvedValue({ + ...validSessions, + [fqdn]: { + ...validSessions[fqdn], + 'another-user': {identity: {...validIdentityToken, userId: 'another-user'}, applications: {}}, + }, + }) + vi.mocked(refreshAccessToken).mockRejectedValueOnce(new ErrorClass()) + + await expect(ensureAuthenticated(defaultApplications, process.env, {noPrompt: true})).rejects.toThrow() + + expect(secureRemove).not.toHaveBeenCalled() + expect(storeSessions).not.toHaveBeenCalled() + expect(setCurrentSessionId).not.toHaveBeenCalled() + expect(requestDeviceAuthorization).not.toHaveBeenCalled() + }, + ) + + test('preserves saved accounts when an interactive refresh returns invalid_request', async () => { + vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh') + vi.mocked(fetchSessions).mockResolvedValue(validSessions) + vi.mocked(refreshAccessToken).mockRejectedValueOnce(new InvalidRequestError()) + + await expect(ensureAuthenticated(defaultApplications)).rejects.toThrow('Error validating auth session') + + expect(secureRemove).not.toHaveBeenCalled() + expect(storeSessions).not.toHaveBeenCalled() + }) + + test('saves only the refreshed account without changing the selected account', async () => { + const otherSession = {identity: {...validIdentityToken, userId: 'another-user'}, applications: {}} + vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh') + vi.mocked(fetchSessions).mockResolvedValue({ + [fqdn]: {...validSessions[fqdn], 'another-user': otherSession}, + }) + vi.mocked(getCurrentSessionId).mockReturnValue(userId) + vi.mocked(refreshAccessToken).mockImplementationOnce(async () => { + vi.mocked(getCurrentSessionId).mockReturnValue('another-user') + return validIdentityToken + }) + + await ensureAuthenticated(defaultApplications) + + expect(storeSessions).toHaveBeenCalledWith(validSessions) + expect(setCurrentSessionId).not.toHaveBeenCalled() + expect(getCurrentSessionId()).toBe('another-user') + }) + test('refreshes the tokens', async () => { // Given vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh') diff --git a/packages/cli-kit/src/private/node/session.ts b/packages/cli-kit/src/private/node/session.ts index d8562e6c17f..77014dd6bab 100644 --- a/packages/cli-kit/src/private/node/session.ts +++ b/packages/cli-kit/src/private/node/session.ts @@ -9,7 +9,7 @@ import { InvalidGrantError, InvalidRequestError, } from './session/exchange.js' -import {IdentityToken, Session, Sessions} from './session/schema.js' +import {IdentityToken, Session} from './session/schema.js' import * as sessionStore from './session/store.js' import {pollForDeviceAuthorization, requestDeviceAuthorization} from './session/device-authorization.js' import {automationTokenAuthMethod, automationTokenVariablesProblem} from './session/automation-token.js' @@ -21,7 +21,7 @@ import {themeToken} from '../../public/node/context/local.js' import {AbortError} from '../../public/node/error.js' import {normalizeStoreFqdn, identityFqdn} from '../../public/node/context/fqdn.js' import {getIdentityTokenInformation, getAppAutomationToken} from '../../public/node/environment.js' -import {AdminSession, ensureNoServiceAccountToken, logout} from '../../public/node/session.js' +import {AdminSession, ensureNoServiceAccountToken} from '../../public/node/session.js' import {nonRandomUUID} from '../../public/node/crypto.js' import {isEmpty} from '../../public/common/object.js' import {businessPlatformRequest} from '../../public/node/api/business-platform.js' @@ -262,8 +262,10 @@ ${outputToken.json(applications)} await throwOnNoPrompt(noPrompt) newSession = await executeCompleteFlow(applications, currentSession?.identity.alias) } else if (error instanceof InvalidRequestError) { - await sessionStore.remove() - throw new AbortError('\nError validating auth session', "We've cleared the current session, please try again") + throw new AbortError( + '\nError validating auth session', + 'Run `shopify auth login` and select "Log in with a different account" to sign in again.', + ) } else { throw error } @@ -272,15 +274,12 @@ ${outputToken.json(applications)} const completeSession = {...currentSession, ...newSession} as Session const newSessionId = completeSession.identity.userId - const updatedSessions: Sessions = { - ...sessions, - [fqdn]: {...sessions[fqdn], [newSessionId]: completeSession}, - } - // Save the new session info if it has changed if (!isEmpty(newSession)) { - await sessionStore.store(updatedSessions) - if (!commandSessionId) setCurrentSessionId(newSessionId) + await sessionStore.store({[fqdn]: {[newSessionId]: completeSession}}) + if (!commandSessionId && (!currentSession || newSessionId !== currentSession.identity.userId)) { + setCurrentSessionId(newSessionId) + } } const tokens = await tokensFor(applications, completeSession) @@ -297,7 +296,6 @@ ${outputToken.json(applications)} async function throwOnNoPrompt(noPrompt: boolean) { if (!noPrompt) return - await logout() throw new AbortError( `The currently available CLI credentials are invalid. diff --git a/packages/cli-kit/src/private/node/session/store.integration.test.ts b/packages/cli-kit/src/private/node/session/store.integration.test.ts new file mode 100644 index 00000000000..988ab97a76f --- /dev/null +++ b/packages/cli-kit/src/private/node/session/store.integration.test.ts @@ -0,0 +1,101 @@ +import {fetch, remove, setSessionAlias, store} from './store.js' +import {getCurrentSessionId, getSessions, setCurrentSessionId, setSessions} from '../conf-store.js' +import {identityFqdn} from '../../../public/node/context/fqdn.js' +import {inTemporaryDirectory} from '../../../public/node/fs.js' +import {LocalStorage} from '../../../public/node/local-storage.js' +import {describe, expect, test, vi} from 'vitest' +import type {ConfSchema} from '../conf-store.js' +import type {Session} from './schema.js' + +vi.mock('../../../public/node/context/fqdn.js') + +const fqdn = 'accounts.shopify.com' + +function session(userId: string): Session { + return { + identity: { + userId, + accessToken: 'fake-access-token', + refreshToken: 'fake-refresh-token', + expiresAt: new Date('2100-01-01'), + scopes: [], + }, + applications: {}, + } +} + +describe('session storage with real files', () => { + test('merges overlapping account writes from separate storage instances', async () => { + await inTemporaryDirectory(async (cwd) => { + const first = new LocalStorage({cwd}) + const second = new LocalStorage({cwd}) + const alice = session('alice') + const bob = session('bob') + + await Promise.all([store({[fqdn]: {alice}}, first), store({[fqdn]: {bob}}, second)]) + + await expect(fetch(first)).resolves.toEqual({[fqdn]: {alice, bob}}) + }) + }) + + test('preserves accounts saved after authentication started', async () => { + await inTemporaryDirectory(async (cwd) => { + const config = new LocalStorage({cwd}) + const alice = session('alice') + const bob = session('bob') + await store({[fqdn]: {alice}}, config) + const beforeRefresh = await fetch(config) + await store({[fqdn]: {bob}}, new LocalStorage({cwd})) + setCurrentSessionId('bob', config) + + await store({[fqdn]: {alice: {...beforeRefresh![fqdn]!.alice!, applications: {}}}}, config) + + await expect(fetch(config)).resolves.toEqual({[fqdn]: {alice, bob}}) + expect(getCurrentSessionId(config)).toBe('bob') + }) + }) + + test('updates an alias without overwriting an overlapping account write', async () => { + await inTemporaryDirectory(async (cwd) => { + vi.mocked(identityFqdn).mockResolvedValue(fqdn) + const config = new LocalStorage({cwd}) + const alice = session('alice') + const bob = session('bob') + await store({[fqdn]: {alice}}, config) + + await Promise.all([ + setSessionAlias('alice', 'work', config), + store({[fqdn]: {bob}}, new LocalStorage({cwd})), + ]) + + await expect(fetch(config)).resolves.toEqual({ + [fqdn]: {alice: {...alice, identity: {...alice.identity, alias: 'work'}}, bob}, + }) + }) + }) + + test.each(['{invalid json', JSON.stringify({unexpected: 'session format'})])( + 'preserves unreadable session content and releases the write lock', + async (content) => { + await inTemporaryDirectory(async (cwd) => { + const config = new LocalStorage({cwd}) + setSessions(content, config) + setCurrentSessionId('alice', config) + + await expect(fetch(config)).rejects.toThrow('The saved CLI accounts could not be read') + await expect(store({[fqdn]: {bob: session('bob')}}, config)).rejects.toThrow( + 'The saved CLI accounts could not be read', + ) + + expect(getSessions(config)).toBe(content) + expect(getCurrentSessionId(config)).toBe('alice') + + await remove(config) + expect(getSessions(config)).toBeUndefined() + expect(getCurrentSessionId(config)).toBeUndefined() + await store({[fqdn]: {bob: session('bob')}}, config) + await expect(fetch(config)).resolves.toEqual({[fqdn]: {bob: session('bob')}}) + }) + }, + ) +}) diff --git a/packages/cli-kit/src/private/node/session/store.test.ts b/packages/cli-kit/src/private/node/session/store.test.ts index 2d2ab6b0fd8..cdfb0f5479d 100644 --- a/packages/cli-kit/src/private/node/session/store.test.ts +++ b/packages/cli-kit/src/private/node/session/store.test.ts @@ -1,6 +1,6 @@ import {Sessions} from './schema.js' import {store, fetch, remove, getSessionAlias, setSessionAlias, findSessionByAlias} from './store.js' -import {getSessions, removeSessions, setSessions, removeCurrentSessionId} from '../conf-store.js' +import {getSessions, removeSessions, setSessions, removeCurrentSessionId, withSessionStoreLock} from '../conf-store.js' import {identityFqdn} from '../../../public/node/context/fqdn.js' import {describe, expect, vi, test, beforeEach} from 'vitest' @@ -37,6 +37,7 @@ const mockSessions: Sessions = { describe('session store', () => { beforeEach(() => { vi.mocked(identityFqdn).mockResolvedValue('identity.fqdn.com') + vi.mocked(withSessionStoreLock).mockImplementation(async (action) => action()) }) describe('store', () => { @@ -45,7 +46,7 @@ describe('session store', () => { await store(mockSessions) // Then - expect(setSessions).toHaveBeenCalledWith(JSON.stringify(mockSessions)) + expect(setSessions).toHaveBeenCalledWith(JSON.stringify(mockSessions), undefined) }) }) @@ -72,32 +73,26 @@ describe('session store', () => { expect(result).toEqual(mockSessions) }) - test('returns undefined and discards malformed JSON session content', async () => { + test('preserves malformed JSON session content and reports an error', async () => { // Given vi.mocked(getSessions).mockReturnValue('{not valid json') // When - const result = await fetch() - - // Then - expect(result).toBeUndefined() - expect(removeSessions).toHaveBeenCalled() - expect(removeCurrentSessionId).toHaveBeenCalled() + await expect(fetch()).rejects.toThrow('The saved CLI accounts could not be read') + expect(removeSessions).not.toHaveBeenCalled() + expect(removeCurrentSessionId).not.toHaveBeenCalled() }) - test('returns undefined and discards schema-invalid session content', async () => { + test('preserves schema-invalid session content and reports an error', async () => { // Given vi.mocked(getSessions).mockReturnValue(JSON.stringify({not: 'a valid sessions object'})) vi.mocked(removeSessions).mockClear() vi.mocked(removeCurrentSessionId).mockClear() // When - const result = await fetch() - - // Then - expect(result).toBeUndefined() - expect(removeSessions).toHaveBeenCalled() - expect(removeCurrentSessionId).toHaveBeenCalled() + await expect(fetch()).rejects.toThrow('The saved CLI accounts could not be read') + expect(removeSessions).not.toHaveBeenCalled() + expect(removeCurrentSessionId).not.toHaveBeenCalled() }) test('rethrows non-SyntaxError parse failures without discarding sessions', async () => { diff --git a/packages/cli-kit/src/private/node/session/store.ts b/packages/cli-kit/src/private/node/session/store.ts index 86902433c44..386ebe186db 100644 --- a/packages/cli-kit/src/private/node/session/store.ts +++ b/packages/cli-kit/src/private/node/session/store.ts @@ -1,24 +1,33 @@ import {SessionsSchema} from './schema.js' -import {getSessions, removeCurrentSessionId, removeSessions, setSessions} from '../conf-store.js' +import {getSessions, removeCurrentSessionId, removeSessions, setSessions, withSessionStoreLock} from '../conf-store.js' import {identityFqdn} from '../../../public/node/context/fqdn.js' +import {AbortError} from '../../../public/node/error.js' +import type {ConfSchema} from '../conf-store.js' +import type {LocalStorage} from '../../../public/node/local-storage.js' import type {Session, Sessions} from './schema.js' /** - * Serializes the session as a JSON and stores it in the system. - * @param session - the session to store. + * Merges changed sessions into the latest stored accounts. + * @param sessions - Only the sessions changed by the caller. + * @param config - Optional local storage instance. */ -export async function store(sessions: Sessions) { - const jsonSessions = JSON.stringify(sessions) - setSessions(jsonSessions) +export async function store(sessions: Sessions, config?: LocalStorage) { + await withSessionStoreLock(async () => { + const storedSessions = (await fetch(config)) ?? {} + for (const [fqdn, changedSessions] of Object.entries(sessions)) { + storedSessions[fqdn] = {...storedSessions[fqdn], ...changedSessions} + } + setSessions(JSON.stringify(storedSessions), config) + }, config) } /** * Fetches the sessions from the local storage and returns it. - * If the format of the object is invalid, the method will discard it. + * Unreadable data is preserved instead of removing every saved account. * @returns Returns a promise that resolves with the sessions object if it exists and is valid. */ -export async function fetch(): Promise { - const content = getSessions() +export async function fetch(config?: LocalStorage): Promise { + const content = getSessions(config) if (!content) { return undefined @@ -29,25 +38,26 @@ export async function fetch(): Promise { contentJson = JSON.parse(content) } catch (error) { if (!(error instanceof SyntaxError)) throw error - await remove() - return undefined } const parsedSessions = await SessionsSchema.safeParseAsync(contentJson) if (parsedSessions.success) { return parsedSessions.data - } else { - await remove() - return undefined } + throw new AbortError( + 'The saved CLI accounts could not be read. They have not been changed.', + 'Try the CLI version used to save these accounts. To remove all saved accounts and sign in again, run `shopify auth logout`, then `shopify auth login`.', + ) } /** * Removes a session from the system. */ -export async function remove() { - removeSessions() - removeCurrentSessionId() +export async function remove(config?: LocalStorage) { + await withSessionStoreLock(async () => { + removeSessions(config) + removeCurrentSessionId(config) + }, config) } /** @@ -71,16 +81,16 @@ export async function getSessionAlias(userId: string): Promise { - const sessions = await fetch() - if (!sessions) return - +export async function setSessionAlias(userId: string, alias: string, config?: LocalStorage): Promise { const fqdn = await identityFqdn() - const session: Session | undefined = sessions[fqdn]?.[userId] - if (!session) return + await withSessionStoreLock(async () => { + const sessions = await fetch(config) + const session: Session | undefined = sessions?.[fqdn]?.[userId] + if (!session) return - session.identity.alias = alias - await store(sessions) + session.identity.alias = alias + setSessions(JSON.stringify(sessions), config) + }, config) } /** diff --git a/packages/cli-kit/src/public/node/local-storage.ts b/packages/cli-kit/src/public/node/local-storage.ts index e59eb420943..9bcb875aa1e 100644 --- a/packages/cli-kit/src/public/node/local-storage.ts +++ b/packages/cli-kit/src/public/node/local-storage.ts @@ -55,6 +55,15 @@ export class LocalStorage> { } } + /** + * Returns the path of the local storage file. + * + * @returns The absolute path of the local storage file. + */ + get path(): string { + return this.config.path + } + /** * Get a value from the local storage. * diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7a1f983b4f9..6671b1afa94 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -448,6 +448,9 @@ importers: pathe: specifier: 1.1.2 version: 1.1.2 + proper-lockfile: + specifier: 4.1.2 + version: 4.1.2 react: specifier: 19.2.4 version: 19.2.4 @@ -491,6 +494,9 @@ importers: '@types/lodash': specifier: 4.17.24 version: 4.17.24 + '@types/proper-lockfile': + specifier: 4.1.4 + version: 4.1.4 '@types/react': specifier: 18.3.12 version: 18.3.12