From 54fb1ff9aa2cedf3f770835fb3a4a64120037de7 Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Wed, 6 May 2026 11:21:45 -0400 Subject: [PATCH 1/3] Avoid metrics-only store analytics lookups --- .../src/cli/services/store/auth/index.test.ts | 30 ++++++++++++- .../src/cli/services/store/auth/index.ts | 4 +- .../store/execute/admin-context.test.ts | 24 ++++++++++- .../services/store/execute/admin-context.ts | 9 ++-- .../src/cli/services/store/metrics.test.ts | 30 +++---------- .../store/src/cli/services/store/metrics.ts | 42 +------------------ .../src/cli/utilities/store-command.test.ts | 13 +++--- .../store/src/cli/utilities/store-command.ts | 23 +--------- 8 files changed, 70 insertions(+), 105 deletions(-) diff --git a/packages/store/src/cli/services/store/auth/index.test.ts b/packages/store/src/cli/services/store/auth/index.test.ts index 4bbcb13109..d13b66dce3 100644 --- a/packages/store/src/cli/services/store/auth/index.test.ts +++ b/packages/store/src/cli/services/store/auth/index.test.ts @@ -1,7 +1,7 @@ import {authenticateStoreWithApp} from './index.js' import {setStoredStoreAppSession} from './session-store.js' import {STORE_AUTH_APP_CLIENT_ID} from './config.js' -import {recordStoreCommandShopIdFromAdminApi} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../metrics.js' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import {describe, expect, test, vi} from 'vitest' @@ -56,8 +56,8 @@ describe('store auth service', () => { }), ) expect(presenter.success).toHaveBeenCalledWith(result) + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com') expect(setLastSeenUserId).toHaveBeenCalledWith('42') - expect(recordStoreCommandShopIdFromAdminApi).toHaveBeenCalledWith({store: 'shop.myshopify.com', accessToken: 'token'}) const storedSession = vi.mocked(setStoredStoreAppSession).mock.calls[0]![0] expect(storedSession.store).toBe('shop.myshopify.com') @@ -271,6 +271,32 @@ describe('store auth service', () => { expect(presenter.success).toHaveBeenCalledWith(result) }) + test('authenticateStoreWithApp records fqdn metadata before waiting for the auth callback', async () => { + const waitForStoreAuthCodeMock = vi.fn().mockRejectedValue(new Error('callback failed')) + + await expect( + authenticateStoreWithApp( + { + store: 'shop.myshopify.com', + scopes: 'read_products', + }, + { + openURL: vi.fn().mockResolvedValue(true), + waitForStoreAuthCode: waitForStoreAuthCodeMock, + exchangeStoreAuthCodeForToken: vi.fn(), + presenter: { + openingBrowser: vi.fn(), + manualAuthUrl: vi.fn(), + success: vi.fn(), + }, + }, + ), + ).rejects.toThrow('callback failed') + + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com') + expect(setStoredStoreAppSession).not.toHaveBeenCalled() + }) + test('authenticateStoreWithApp rejects when Shopify grants fewer scopes than requested', async () => { const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => { await options.onListening?.() diff --git a/packages/store/src/cli/services/store/auth/index.ts b/packages/store/src/cli/services/store/auth/index.ts index 0c786eb0bc..789295b52f 100644 --- a/packages/store/src/cli/services/store/auth/index.ts +++ b/packages/store/src/cli/services/store/auth/index.ts @@ -6,7 +6,7 @@ import {createPkceBootstrap} from './pkce.js' import {mergeRequestedAndStoredScopes, parseStoreAuthScopes, resolveGrantedScopes} from './scopes.js' import {resolveExistingStoreAuthScopes, type ResolvedStoreAuthScopes} from './existing-scopes.js' import {createStoreAuthPresenter, type StoreAuthPresenter, type StoreAuthResult} from './result.js' -import {recordStoreCommandShopIdFromAdminApi} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../metrics.js' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import {openURL} from '@shopify/cli-kit/node/system' import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output' @@ -60,6 +60,7 @@ export async function authenticateStoreWithApp( authorization: {authorizationUrl}, } = bootstrap + await recordStoreFqdnMetadata(store) resolvedDependencies.presenter.openingBrowser() const code = await resolvedDependencies.waitForStoreAuthCode({ @@ -76,7 +77,6 @@ export async function authenticateStoreWithApp( throw new AbortError('Shopify did not return associated user information for the online access token.') } setLastSeenUserId(userId) - await recordStoreCommandShopIdFromAdminApi({store, accessToken: tokenResponse.access_token}) const now = Date.now() const expiresAt = tokenResponse.expires_in ? new Date(now + tokenResponse.expires_in * 1000).toISOString() : undefined diff --git a/packages/store/src/cli/services/store/execute/admin-context.test.ts b/packages/store/src/cli/services/store/execute/admin-context.test.ts index 5a75d3fbfd..bb7ca5088b 100644 --- a/packages/store/src/cli/services/store/execute/admin-context.test.ts +++ b/packages/store/src/cli/services/store/execute/admin-context.test.ts @@ -2,7 +2,7 @@ import {prepareAdminStoreGraphQLContext} from './admin-context.js' import {fetchPublicApiVersions} from './admin-transport.js' import {loadStoredStoreSession} from '../auth/session-lifecycle.js' import {STORE_AUTH_APP_CLIENT_ID} from '../auth/config.js' -import {recordStoreCommandShopIdFromAdminApi} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../metrics.js' import {AbortError} from '@shopify/cli-kit/node/error' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import {beforeEach, describe, expect, test, vi} from 'vitest' @@ -41,6 +41,7 @@ describe('prepareAdminStoreGraphQLContext', () => { const result = await prepareAdminStoreGraphQLContext({store}) expect(loadStoredStoreSession).toHaveBeenCalledWith(store) + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) expect(setLastSeenUserId).toHaveBeenCalledWith('42') expect(fetchPublicApiVersions).toHaveBeenCalledWith({ adminSession: {token: 'token', storeFqdn: store}, @@ -82,24 +83,43 @@ describe('prepareAdminStoreGraphQLContext', () => { const result = await prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: 'unstable'}) expect(loadStoredStoreSession).toHaveBeenCalledWith(store) + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) expect(result).toEqual({ adminSession: {token: 'token', storeFqdn: store}, version: 'unstable', session: storedSession, }) expect(fetchPublicApiVersions).not.toHaveBeenCalled() - expect(recordStoreCommandShopIdFromAdminApi).toHaveBeenCalledWith({store, accessToken: 'token'}) }) test('throws when the requested API version is invalid', async () => { await expect(prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: '1999-01'})).rejects.toThrow( 'Invalid API version', ) + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) + }) + + test('records the requested store before failing to load stored auth', async () => { + vi.mocked(loadStoredStoreSession).mockRejectedValue(new AbortError('missing stored auth')) + + await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('missing stored auth') + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) + expect(fetchPublicApiVersions).not.toHaveBeenCalled() + }) + + test('re-records fqdn metadata when the stored session store differs from the requested store', async () => { + vi.mocked(loadStoredStoreSession).mockResolvedValue({...storedSession, store: 'permanent-shop.myshopify.com'}) + + await prepareAdminStoreGraphQLContext({store}) + + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'permanent-shop.myshopify.com') }) test('rethrows whatever the transport raises (errors are owned by the transport)', async () => { vi.mocked(fetchPublicApiVersions).mockRejectedValue(new AbortError('upstream exploded')) await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('upstream exploded') + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) }) }) diff --git a/packages/store/src/cli/services/store/execute/admin-context.ts b/packages/store/src/cli/services/store/execute/admin-context.ts index 1c6c77ee20..f23ac316ac 100644 --- a/packages/store/src/cli/services/store/execute/admin-context.ts +++ b/packages/store/src/cli/services/store/execute/admin-context.ts @@ -1,6 +1,6 @@ import {fetchPublicApiVersions} from './admin-transport.js' import {loadStoredStoreSession} from '../auth/session-lifecycle.js' -import {recordStoreCommandShopIdFromAdminApi} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../metrics.js' import {AbortError} from '@shopify/cli-kit/node/error' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import type {AdminSession} from '@shopify/cli-kit/node/session' @@ -38,16 +38,17 @@ export async function prepareAdminStoreGraphQLContext(input: { store: string userSpecifiedVersion?: string }): Promise { + await recordStoreFqdnMetadata(input.store) const session = await loadStoredStoreSession(input.store) + if (session.store !== input.store) { + await recordStoreFqdnMetadata(session.store) + } setLastSeenUserId(session.userId) const adminSession = { token: session.accessToken, storeFqdn: session.store, } const version = await resolveApiVersion({session, adminSession, userSpecifiedVersion: input.userSpecifiedVersion}) - if (version === 'unstable') { - await recordStoreCommandShopIdFromAdminApi({store: session.store, accessToken: session.accessToken}) - } return {adminSession, version, session} } diff --git a/packages/store/src/cli/services/store/metrics.test.ts b/packages/store/src/cli/services/store/metrics.test.ts index bc748ccb74..eebd9c0b51 100644 --- a/packages/store/src/cli/services/store/metrics.test.ts +++ b/packages/store/src/cli/services/store/metrics.test.ts @@ -1,25 +1,22 @@ -import {recordStoreCommandShopIdFromAdminApi, recordStoreCommandShopIdFromAdminGid, recordStoreFqdnMetadata} from './metrics.js' -import {adminUrl} from '@shopify/cli-kit/node/api/admin' -import {graphqlRequest} from '@shopify/cli-kit/node/api/graphql' +import {recordStoreCommandShopIdFromAdminGid, recordStoreFqdnMetadata} from './metrics.js' import {hashString} from '@shopify/cli-kit/node/crypto' -import {addPublicMetadata} from '@shopify/cli-kit/node/metadata' +import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' import {beforeEach, describe, expect, test, vi} from 'vitest' -vi.mock('@shopify/cli-kit/node/api/admin') -vi.mock('@shopify/cli-kit/node/api/graphql') vi.mock('@shopify/cli-kit/node/crypto') vi.mock('@shopify/cli-kit/node/metadata') describe('store command metrics', () => { beforeEach(() => { vi.clearAllMocks() - vi.mocked(adminUrl).mockReturnValue('https://shop.myshopify.com/admin/api/unstable/graphql.json') vi.mocked(hashString).mockReturnValue('hashed-store') }) - test('records the hashed store fqdn', async () => { + test('records the sensitive and hashed store fqdn', async () => { await recordStoreFqdnMetadata('shop.myshopify.com') + expect(addSensitiveMetadata).toHaveBeenCalledWith(expect.any(Function)) + expect(vi.mocked(addSensitiveMetadata).mock.calls[0]![0]()).toEqual({store_fqdn: 'shop.myshopify.com'}) expect(addPublicMetadata).toHaveBeenCalledWith(expect.any(Function)) expect(vi.mocked(addPublicMetadata).mock.calls[0]![0]()).toEqual({store_fqdn_hash: 'hashed-store'}) expect(hashString).toHaveBeenCalledWith('shop.myshopify.com') @@ -37,21 +34,4 @@ describe('store command metrics', () => { expect(addPublicMetadata).not.toHaveBeenCalled() }) - - test('fetches and records shop_id from the Admin API', async () => { - vi.mocked(graphqlRequest).mockResolvedValue({shop: {id: 'gid://shopify/Shop/456'}}) - - await recordStoreCommandShopIdFromAdminApi({store: 'shop.myshopify.com', accessToken: 'token'}) - - expect(adminUrl).toHaveBeenCalledWith('shop.myshopify.com', 'unstable') - expect(graphqlRequest).toHaveBeenCalledWith( - expect.objectContaining({ - api: 'Admin', - token: 'token', - url: 'https://shop.myshopify.com/admin/api/unstable/graphql.json', - responseOptions: {handleErrors: false}, - }), - ) - expect(vi.mocked(addPublicMetadata).mock.calls[0]![0]()).toEqual({shop_id: 456}) - }) }) diff --git a/packages/store/src/cli/services/store/metrics.ts b/packages/store/src/cli/services/store/metrics.ts index e22cb9dd1b..d2c4993710 100644 --- a/packages/store/src/cli/services/store/metrics.ts +++ b/packages/store/src/cli/services/store/metrics.ts @@ -1,25 +1,9 @@ -import {adminUrl} from '@shopify/cli-kit/node/api/admin' -import {graphqlRequest} from '@shopify/cli-kit/node/api/graphql' import {hashString} from '@shopify/cli-kit/node/crypto' -import {addPublicMetadata} from '@shopify/cli-kit/node/metadata' -import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output' +import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' import {tryParseInt} from '@shopify/cli-kit/common/string' -const STORE_ID_QUERY = `#graphql - query StoreCommandShopId { - shop { - id - } - } -` - -interface StoreIdResponse { - shop?: { - id?: string - } -} - export async function recordStoreFqdnMetadata(storeFqdn: string): Promise { + await addSensitiveMetadata(() => ({store_fqdn: storeFqdn})) await addPublicMetadata(() => ({store_fqdn_hash: hashString(storeFqdn)})) } @@ -30,28 +14,6 @@ export async function recordStoreCommandShopIdFromAdminGid(shopGid: string | und await addPublicMetadata(() => ({shop_id: shopId})) } -export async function recordStoreCommandShopIdFromAdminApi(options: { - store: string - accessToken: string -}): Promise { - try { - const response = await graphqlRequest({ - query: STORE_ID_QUERY, - api: 'Admin', - url: adminUrl(options.store, 'unstable'), - token: options.accessToken, - responseOptions: {handleErrors: false}, - }) - await recordStoreCommandShopIdFromAdminGid(response.shop?.id) - } catch (error) { - outputDebug( - outputContent`Failed to record store command shop_id for ${outputToken.raw(options.store)}: ${outputToken.raw( - error instanceof Error ? error.message : String(error), - )}`, - ) - } -} - function numericIdFromShopGid(gid: string | undefined): number | undefined { const id = gid?.match(/^gid:\/\/shopify\/Shop\/(\d+)$/)?.[1] return tryParseInt(id) diff --git a/packages/store/src/cli/utilities/store-command.test.ts b/packages/store/src/cli/utilities/store-command.test.ts index 5b3b51a4f4..9913acd9ed 100644 --- a/packages/store/src/cli/utilities/store-command.test.ts +++ b/packages/store/src/cli/utilities/store-command.test.ts @@ -1,11 +1,9 @@ import StoreCommand from './store-command.js' import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' -import {hashString} from '@shopify/cli-kit/node/crypto' import {Flags} from '@oclif/core' import {beforeEach, describe, expect, test, vi} from 'vitest' vi.mock('@shopify/cli-kit/node/metadata') -vi.mock('@shopify/cli-kit/node/crypto') class TestStoreCommand extends StoreCommand { static flags = { @@ -20,16 +18,15 @@ class TestStoreCommand extends StoreCommand { describe('StoreCommand', () => { beforeEach(() => { vi.clearAllMocks() - vi.mocked(hashString).mockReturnValue('hashed-store') }) - test('records shared store command metadata when parsing --store', async () => { + test('does not record store-specific metadata during base parsing', async () => { await TestStoreCommand.run(['--store', 'shop.myshopify.com']) - expect(addSensitiveMetadata).toHaveBeenCalledWith(expect.any(Function)) - expect(vi.mocked(addSensitiveMetadata).mock.calls[0]![0]()).toEqual({store_fqdn: 'shop.myshopify.com'}) - expect(addPublicMetadata).toHaveBeenCalledWith(expect.any(Function)) + const sensitiveMetadataCalls = vi.mocked(addSensitiveMetadata).mock.calls.map((call) => call[0]()) const publicMetadataCalls = vi.mocked(addPublicMetadata).mock.calls.map((call) => call[0]()) - expect(publicMetadataCalls).toContainEqual({store_fqdn_hash: 'hashed-store'}) + + expect(sensitiveMetadataCalls).not.toContainEqual(expect.objectContaining({store_fqdn: 'shop.myshopify.com'})) + expect(publicMetadataCalls).not.toContainEqual(expect.objectContaining({store_fqdn_hash: expect.any(String)})) }) }) diff --git a/packages/store/src/cli/utilities/store-command.ts b/packages/store/src/cli/utilities/store-command.ts index 90c2a06128..d9d969dc0d 100644 --- a/packages/store/src/cli/utilities/store-command.ts +++ b/packages/store/src/cli/utilities/store-command.ts @@ -1,29 +1,8 @@ -import {recordStoreFqdnMetadata} from '../services/store/metrics.js' -import Command, {type ArgOutput, type FlagOutput} from '@shopify/cli-kit/node/base-command' -import {addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' - -import type {Input, ParserOutput} from '@oclif/core/parser' +import Command from '@shopify/cli-kit/node/base-command' /** * Base class that includes shared behavior for all store commands. */ export default abstract class StoreCommand extends Command { public abstract run(): Promise - - protected async parse< - TFlags extends FlagOutput & {path?: string; verbose?: boolean}, - TGlobalFlags extends FlagOutput, - TArgs extends ArgOutput, - >( - options?: Input, - argv?: string[], - ): Promise & {argv: string[]}> { - const result = await super.parse(options, argv) - const storeFqdn = (result.flags as {store?: unknown}).store - if (typeof storeFqdn === 'string' && storeFqdn.length > 0) { - await addSensitiveMetadata(() => ({store_fqdn: storeFqdn})) - await recordStoreFqdnMetadata(storeFqdn) - } - return result - } } From 0e7fdccf75cc99eeaa48b6055cedefde041b6ebe Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Wed, 6 May 2026 12:18:17 -0400 Subject: [PATCH 2/3] Use validated store fqdn attribution --- packages/cli-kit/src/public/node/metadata.ts | 3 +- packages/cli-kit/src/public/node/monorail.ts | 4 +- .../store/src/cli/commands/store/auth.test.ts | 2 +- .../src/cli/commands/store/execute.test.ts | 2 +- .../cli/services/store/attribution.test.ts | 27 ++++++++++++++ .../src/cli/services/store/attribution.ts | 7 ++++ .../src/cli/services/store/auth/index.test.ts | 10 +++-- .../src/cli/services/store/auth/index.ts | 5 ++- .../store/execute/admin-context.test.ts | 23 +++++++----- .../services/store/execute/admin-context.ts | 8 ++-- .../store/execute/admin-transport.test.ts | 4 -- .../services/store/execute/admin-transport.ts | 8 ---- .../src/cli/services/store/metrics.test.ts | 37 ------------------- .../store/src/cli/services/store/metrics.ts | 20 ---------- 14 files changed, 65 insertions(+), 95 deletions(-) create mode 100644 packages/store/src/cli/services/store/attribution.test.ts create mode 100644 packages/store/src/cli/services/store/attribution.ts delete mode 100644 packages/store/src/cli/services/store/metrics.test.ts delete mode 100644 packages/store/src/cli/services/store/metrics.ts diff --git a/packages/cli-kit/src/public/node/metadata.ts b/packages/cli-kit/src/public/node/metadata.ts index 51d19b1e2a..1b6223f7b4 100644 --- a/packages/cli-kit/src/public/node/metadata.ts +++ b/packages/cli-kit/src/public/node/metadata.ts @@ -177,8 +177,7 @@ export function createRuntimeMetadataContainer< // We want to track anything that ends up getting sent to monorail as `cmd_all_*`, // `cmd_app_*`, `cmd_theme_*`, `store_*`, and `env_auto_upgrade_*` -type CmdFieldsFromMonorail = Pick & - PickByPrefix & +type CmdFieldsFromMonorail = PickByPrefix & PickByPrefix & PickByPrefix & PickByPrefix & diff --git a/packages/cli-kit/src/public/node/monorail.ts b/packages/cli-kit/src/public/node/monorail.ts index 3a35faa74c..66b9ce1e9d 100644 --- a/packages/cli-kit/src/public/node/monorail.ts +++ b/packages/cli-kit/src/public/node/monorail.ts @@ -10,7 +10,7 @@ const url = 'https://monorail-edge.shopifysvc.com/v1/produce' type Optional = T | null // This is the topic name of the main event we log to Monorail, the command tracker -export const MONORAIL_COMMAND_TOPIC = 'app_cli3_command/1.23' +export const MONORAIL_COMMAND_TOPIC = 'app_cli3_command/1.24' export interface Schemas { [MONORAIL_COMMAND_TOPIC]: { @@ -45,7 +45,7 @@ export interface Schemas { node_version: string is_employee: boolean store_fqdn_hash?: Optional - shop_id?: Optional + store_fqdn_validated?: Optional user_id: string // Any and all commands diff --git a/packages/store/src/cli/commands/store/auth.test.ts b/packages/store/src/cli/commands/store/auth.test.ts index 2082f81928..2b3e0efa85 100644 --- a/packages/store/src/cli/commands/store/auth.test.ts +++ b/packages/store/src/cli/commands/store/auth.test.ts @@ -4,7 +4,7 @@ import {createStoreAuthPresenter} from '../../services/store/auth/result.js' import {describe, expect, test, vi} from 'vitest' vi.mock('../../services/store/auth/index.js') -vi.mock('../../services/store/metrics.js') +vi.mock('../../services/store/attribution.js') vi.mock('../../services/store/auth/result.js', () => ({ createStoreAuthPresenter: vi.fn((format: 'text' | 'json') => ({format})), })) diff --git a/packages/store/src/cli/commands/store/execute.test.ts b/packages/store/src/cli/commands/store/execute.test.ts index a512f0243c..1871829962 100644 --- a/packages/store/src/cli/commands/store/execute.test.ts +++ b/packages/store/src/cli/commands/store/execute.test.ts @@ -5,7 +5,7 @@ import {beforeEach, describe, expect, test, vi} from 'vitest' vi.mock('../../services/store/execute/index.js') vi.mock('../../services/store/execute/result.js') -vi.mock('../../services/store/metrics.js') +vi.mock('../../services/store/attribution.js') describe('store execute command', () => { beforeEach(() => { diff --git a/packages/store/src/cli/services/store/attribution.test.ts b/packages/store/src/cli/services/store/attribution.test.ts new file mode 100644 index 0000000000..799a597106 --- /dev/null +++ b/packages/store/src/cli/services/store/attribution.test.ts @@ -0,0 +1,27 @@ +import {recordStoreFqdnMetadata} from './attribution.js' +import {hashString} from '@shopify/cli-kit/node/crypto' +import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' +import {beforeEach, describe, expect, test, vi} from 'vitest' + +vi.mock('@shopify/cli-kit/node/crypto') +vi.mock('@shopify/cli-kit/node/metadata') + +describe('store command attribution', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(hashString).mockReturnValue('hashed-store') + }) + + test('records the sensitive, hashed, and validation state for a store fqdn', async () => { + await recordStoreFqdnMetadata('shop.myshopify.com', true) + + expect(addSensitiveMetadata).toHaveBeenCalledWith(expect.any(Function)) + expect(vi.mocked(addSensitiveMetadata).mock.calls[0]![0]()).toEqual({store_fqdn: 'shop.myshopify.com'}) + expect(addPublicMetadata).toHaveBeenCalledWith(expect.any(Function)) + expect(vi.mocked(addPublicMetadata).mock.calls[0]![0]()).toEqual({ + store_fqdn_hash: 'hashed-store', + store_fqdn_validated: true, + }) + expect(hashString).toHaveBeenCalledWith('shop.myshopify.com') + }) +}) diff --git a/packages/store/src/cli/services/store/attribution.ts b/packages/store/src/cli/services/store/attribution.ts new file mode 100644 index 0000000000..7479bc7891 --- /dev/null +++ b/packages/store/src/cli/services/store/attribution.ts @@ -0,0 +1,7 @@ +import {hashString} from '@shopify/cli-kit/node/crypto' +import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' + +export async function recordStoreFqdnMetadata(storeFqdn: string, validated: boolean): Promise { + await addSensitiveMetadata(() => ({store_fqdn: storeFqdn})) + await addPublicMetadata(() => ({store_fqdn_hash: hashString(storeFqdn), store_fqdn_validated: validated})) +} diff --git a/packages/store/src/cli/services/store/auth/index.test.ts b/packages/store/src/cli/services/store/auth/index.test.ts index d13b66dce3..2f33aecfe1 100644 --- a/packages/store/src/cli/services/store/auth/index.test.ts +++ b/packages/store/src/cli/services/store/auth/index.test.ts @@ -1,12 +1,12 @@ import {authenticateStoreWithApp} from './index.js' import {setStoredStoreAppSession} from './session-store.js' import {STORE_AUTH_APP_CLIENT_ID} from './config.js' -import {recordStoreFqdnMetadata} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../attribution.js' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import {describe, expect, test, vi} from 'vitest' vi.mock('./session-store.js') -vi.mock('../metrics.js') +vi.mock('../attribution.js') vi.mock('@shopify/cli-kit/node/session') vi.mock('@shopify/cli-kit/node/system', () => ({openURL: vi.fn().mockResolvedValue(true)})) vi.mock('@shopify/cli-kit/node/crypto', () => ({randomUUID: vi.fn().mockReturnValue('state-123')})) @@ -56,7 +56,8 @@ describe('store auth service', () => { }), ) expect(presenter.success).toHaveBeenCalledWith(result) - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com') + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, 'shop.myshopify.com', false) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'shop.myshopify.com', true) expect(setLastSeenUserId).toHaveBeenCalledWith('42') const storedSession = vi.mocked(setStoredStoreAppSession).mock.calls[0]![0] @@ -293,7 +294,8 @@ describe('store auth service', () => { ), ).rejects.toThrow('callback failed') - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com') + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false) + expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true) expect(setStoredStoreAppSession).not.toHaveBeenCalled() }) diff --git a/packages/store/src/cli/services/store/auth/index.ts b/packages/store/src/cli/services/store/auth/index.ts index 789295b52f..4e854d48b6 100644 --- a/packages/store/src/cli/services/store/auth/index.ts +++ b/packages/store/src/cli/services/store/auth/index.ts @@ -6,7 +6,7 @@ import {createPkceBootstrap} from './pkce.js' import {mergeRequestedAndStoredScopes, parseStoreAuthScopes, resolveGrantedScopes} from './scopes.js' import {resolveExistingStoreAuthScopes, type ResolvedStoreAuthScopes} from './existing-scopes.js' import {createStoreAuthPresenter, type StoreAuthPresenter, type StoreAuthResult} from './result.js' -import {recordStoreFqdnMetadata} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../attribution.js' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import {openURL} from '@shopify/cli-kit/node/system' import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output' @@ -60,7 +60,7 @@ export async function authenticateStoreWithApp( authorization: {authorizationUrl}, } = bootstrap - await recordStoreFqdnMetadata(store) + await recordStoreFqdnMetadata(store, false) resolvedDependencies.presenter.openingBrowser() const code = await resolvedDependencies.waitForStoreAuthCode({ @@ -70,6 +70,7 @@ export async function authenticateStoreWithApp( if (!opened) resolvedDependencies.presenter.manualAuthUrl(authorizationUrl) }, }) + await recordStoreFqdnMetadata(store, true) const tokenResponse = await bootstrap.exchangeCodeForToken(code) const userId = tokenResponse.associated_user?.id?.toString() diff --git a/packages/store/src/cli/services/store/execute/admin-context.test.ts b/packages/store/src/cli/services/store/execute/admin-context.test.ts index bb7ca5088b..6245e9cc82 100644 --- a/packages/store/src/cli/services/store/execute/admin-context.test.ts +++ b/packages/store/src/cli/services/store/execute/admin-context.test.ts @@ -2,13 +2,13 @@ import {prepareAdminStoreGraphQLContext} from './admin-context.js' import {fetchPublicApiVersions} from './admin-transport.js' import {loadStoredStoreSession} from '../auth/session-lifecycle.js' import {STORE_AUTH_APP_CLIENT_ID} from '../auth/config.js' -import {recordStoreFqdnMetadata} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../attribution.js' import {AbortError} from '@shopify/cli-kit/node/error' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import {beforeEach, describe, expect, test, vi} from 'vitest' vi.mock('../auth/session-lifecycle.js', () => ({loadStoredStoreSession: vi.fn()})) -vi.mock('../metrics.js') +vi.mock('../attribution.js') vi.mock('@shopify/cli-kit/node/session') vi.mock('./admin-transport.js', () => ({ fetchPublicApiVersions: vi.fn(), @@ -41,7 +41,8 @@ describe('prepareAdminStoreGraphQLContext', () => { const result = await prepareAdminStoreGraphQLContext({store}) expect(loadStoredStoreSession).toHaveBeenCalledWith(store) - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) expect(setLastSeenUserId).toHaveBeenCalledWith('42') expect(fetchPublicApiVersions).toHaveBeenCalledWith({ adminSession: {token: 'token', storeFqdn: store}, @@ -83,7 +84,8 @@ describe('prepareAdminStoreGraphQLContext', () => { const result = await prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: 'unstable'}) expect(loadStoredStoreSession).toHaveBeenCalledWith(store) - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) expect(result).toEqual({ adminSession: {token: 'token', storeFqdn: store}, version: 'unstable', @@ -96,14 +98,16 @@ describe('prepareAdminStoreGraphQLContext', () => { await expect(prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: '1999-01'})).rejects.toThrow( 'Invalid API version', ) - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) }) test('records the requested store before failing to load stored auth', async () => { vi.mocked(loadStoredStoreSession).mockRejectedValue(new AbortError('missing stored auth')) await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('missing stored auth') - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, false) + expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith(store, true) expect(fetchPublicApiVersions).not.toHaveBeenCalled() }) @@ -112,14 +116,15 @@ describe('prepareAdminStoreGraphQLContext', () => { await prepareAdminStoreGraphQLContext({store}) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'permanent-shop.myshopify.com') + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'permanent-shop.myshopify.com', true) }) test('rethrows whatever the transport raises (errors are owned by the transport)', async () => { vi.mocked(fetchPublicApiVersions).mockRejectedValue(new AbortError('upstream exploded')) await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('upstream exploded') - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) }) }) diff --git a/packages/store/src/cli/services/store/execute/admin-context.ts b/packages/store/src/cli/services/store/execute/admin-context.ts index f23ac316ac..6b25021aa7 100644 --- a/packages/store/src/cli/services/store/execute/admin-context.ts +++ b/packages/store/src/cli/services/store/execute/admin-context.ts @@ -1,6 +1,6 @@ import {fetchPublicApiVersions} from './admin-transport.js' import {loadStoredStoreSession} from '../auth/session-lifecycle.js' -import {recordStoreFqdnMetadata} from '../metrics.js' +import {recordStoreFqdnMetadata} from '../attribution.js' import {AbortError} from '@shopify/cli-kit/node/error' import {setLastSeenUserId} from '@shopify/cli-kit/node/session' import type {AdminSession} from '@shopify/cli-kit/node/session' @@ -38,11 +38,9 @@ export async function prepareAdminStoreGraphQLContext(input: { store: string userSpecifiedVersion?: string }): Promise { - await recordStoreFqdnMetadata(input.store) + await recordStoreFqdnMetadata(input.store, false) const session = await loadStoredStoreSession(input.store) - if (session.store !== input.store) { - await recordStoreFqdnMetadata(session.store) - } + await recordStoreFqdnMetadata(session.store, true) setLastSeenUserId(session.userId) const adminSession = { token: session.accessToken, diff --git a/packages/store/src/cli/services/store/execute/admin-transport.test.ts b/packages/store/src/cli/services/store/execute/admin-transport.test.ts index 051b5553f2..50f968e811 100644 --- a/packages/store/src/cli/services/store/execute/admin-transport.test.ts +++ b/packages/store/src/cli/services/store/execute/admin-transport.test.ts @@ -6,7 +6,6 @@ import { } from './admin-transport.js' import {clearStoredStoreAppSession} from '../auth/session-store.js' import {STORE_AUTH_APP_CLIENT_ID} from '../auth/config.js' -import {recordStoreCommandShopIdFromAdminGid} from '../metrics.js' import {beforeEach, describe, expect, test, vi} from 'vitest' import {adminUrl} from '@shopify/cli-kit/node/api/admin' import {graphqlRequest} from '@shopify/cli-kit/node/api/graphql' @@ -14,7 +13,6 @@ import {AbortError, BugError} from '@shopify/cli-kit/node/error' import {renderSingleTask} from '@shopify/cli-kit/node/ui' vi.mock('../auth/session-store.js') -vi.mock('../metrics.js') vi.mock('@shopify/cli-kit/node/api/graphql') vi.mock('@shopify/cli-kit/node/ui') vi.mock('@shopify/cli-kit/node/api/admin', async () => { @@ -190,7 +188,6 @@ describe('fetchPublicApiVersions', () => { {handle: '2025-10', supported: true}, {handle: '2025-07', supported: true}, ], - shop: {id: 'gid://shopify/Shop/123'}, }) const result = await fetchPublicApiVersions({adminSession, session}) @@ -199,7 +196,6 @@ describe('fetchPublicApiVersions', () => { {handle: '2025-10', supported: true}, {handle: '2025-07', supported: true}, ]) - expect(recordStoreCommandShopIdFromAdminGid).toHaveBeenCalledWith('gid://shopify/Shop/123') expect(adminUrl).toHaveBeenCalledWith(store, 'unstable', adminSession) expect(graphqlRequest).toHaveBeenCalledWith( expect.objectContaining({ 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 d1f6d70779..15cbb48010 100644 --- a/packages/store/src/cli/services/store/execute/admin-transport.ts +++ b/packages/store/src/cli/services/store/execute/admin-transport.ts @@ -1,6 +1,5 @@ import {throwReauthenticateStoreAuthError} from '../auth/recovery.js' import {clearStoredStoreAppSession} from '../auth/session-store.js' -import {recordStoreCommandShopIdFromAdminGid} from '../metrics.js' 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' @@ -18,9 +17,6 @@ interface ApiVersion { interface PublicApiVersionsResponse { publicApiVersions: ApiVersion[] - shop?: { - id?: string - } } const PUBLIC_API_VERSIONS_QUERY = ` @@ -29,9 +25,6 @@ const PUBLIC_API_VERSIONS_QUERY = ` handle supported } - shop { - id - } } ` @@ -52,7 +45,6 @@ export async function fetchPublicApiVersions(input: { token: input.adminSession.token, responseOptions: {handleErrors: false}, }) - await recordStoreCommandShopIdFromAdminGid(response.shop?.id) return response.publicApiVersions } catch (error) { const status = graphQLClientErrorStatus(error) diff --git a/packages/store/src/cli/services/store/metrics.test.ts b/packages/store/src/cli/services/store/metrics.test.ts deleted file mode 100644 index eebd9c0b51..0000000000 --- a/packages/store/src/cli/services/store/metrics.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import {recordStoreCommandShopIdFromAdminGid, recordStoreFqdnMetadata} from './metrics.js' -import {hashString} from '@shopify/cli-kit/node/crypto' -import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' -import {beforeEach, describe, expect, test, vi} from 'vitest' - -vi.mock('@shopify/cli-kit/node/crypto') -vi.mock('@shopify/cli-kit/node/metadata') - -describe('store command metrics', () => { - beforeEach(() => { - vi.clearAllMocks() - vi.mocked(hashString).mockReturnValue('hashed-store') - }) - - test('records the sensitive and hashed store fqdn', async () => { - await recordStoreFqdnMetadata('shop.myshopify.com') - - expect(addSensitiveMetadata).toHaveBeenCalledWith(expect.any(Function)) - expect(vi.mocked(addSensitiveMetadata).mock.calls[0]![0]()).toEqual({store_fqdn: 'shop.myshopify.com'}) - expect(addPublicMetadata).toHaveBeenCalledWith(expect.any(Function)) - expect(vi.mocked(addPublicMetadata).mock.calls[0]![0]()).toEqual({store_fqdn_hash: 'hashed-store'}) - expect(hashString).toHaveBeenCalledWith('shop.myshopify.com') - }) - - test('records numeric shop_id from an Admin Shop gid', async () => { - await recordStoreCommandShopIdFromAdminGid('gid://shopify/Shop/123') - - expect(addPublicMetadata).toHaveBeenCalledWith(expect.any(Function)) - expect(vi.mocked(addPublicMetadata).mock.calls[0]![0]()).toEqual({shop_id: 123}) - }) - - test('ignores malformed Admin Shop gids', async () => { - await recordStoreCommandShopIdFromAdminGid('gid://shopify/Product/123') - - expect(addPublicMetadata).not.toHaveBeenCalled() - }) -}) diff --git a/packages/store/src/cli/services/store/metrics.ts b/packages/store/src/cli/services/store/metrics.ts deleted file mode 100644 index d2c4993710..0000000000 --- a/packages/store/src/cli/services/store/metrics.ts +++ /dev/null @@ -1,20 +0,0 @@ -import {hashString} from '@shopify/cli-kit/node/crypto' -import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' -import {tryParseInt} from '@shopify/cli-kit/common/string' - -export async function recordStoreFqdnMetadata(storeFqdn: string): Promise { - await addSensitiveMetadata(() => ({store_fqdn: storeFqdn})) - await addPublicMetadata(() => ({store_fqdn_hash: hashString(storeFqdn)})) -} - -export async function recordStoreCommandShopIdFromAdminGid(shopGid: string | undefined): Promise { - const shopId = numericIdFromShopGid(shopGid) - if (shopId === undefined) return - - await addPublicMetadata(() => ({shop_id: shopId})) -} - -function numericIdFromShopGid(gid: string | undefined): number | undefined { - const id = gid?.match(/^gid:\/\/shopify\/Shop\/(\d+)$/)?.[1] - return tryParseInt(id) -} From 61ca9602f4dc7e0a16c2bca9a79d9b8d2ec7a30a Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Wed, 6 May 2026 12:45:22 -0400 Subject: [PATCH 3/3] Preserve store attribution on early failures --- .../cli-kit/src/public/node/monorail.test.ts | 57 +++++++++++- .../src/cli/services/store/auth/index.test.ts | 90 +++++++++++++++++++ .../src/cli/services/store/auth/index.ts | 4 +- .../store/execute/admin-context.test.ts | 25 +++--- .../services/store/execute/admin-context.ts | 1 - .../cli/services/store/execute/index.test.ts | 17 ++++ .../src/cli/services/store/execute/index.ts | 2 + 7 files changed, 179 insertions(+), 17 deletions(-) diff --git a/packages/cli-kit/src/public/node/monorail.test.ts b/packages/cli-kit/src/public/node/monorail.test.ts index 59caad587c..1a4df1ed46 100644 --- a/packages/cli-kit/src/public/node/monorail.test.ts +++ b/packages/cli-kit/src/public/node/monorail.test.ts @@ -1,5 +1,5 @@ import * as http from './http.js' -import {publishMonorailEvent} from './monorail.js' +import {MONORAIL_COMMAND_TOPIC, publishMonorailEvent} from './monorail.js' import {mockAndCaptureOutput} from './testing/output.js' import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' @@ -52,4 +52,59 @@ describe('monorail', () => { expect(outputMock.debug()).toContain('"api_key": "****"') expect(outputMock.debug()).not.toContain('some-api-key') }) + + test('builds a request for the real command topic including validated store attribution', async () => { + const res = await publishMonorailEvent( + MONORAIL_COMMAND_TOPIC, + { + command: 'shopify store auth', + time_start: 1643709600000, + time_end: 1643709601000, + total_time: 1000, + success: true, + cli_version: '3.94.0', + uname: 'Darwin test', + ruby_version: '3.3.0', + node_version: '22.0.0', + is_employee: false, + user_id: '42', + store_fqdn_hash: 'hashed-store', + store_fqdn_validated: true, + }, + { + args: '--store shop.myshopify.com', + store_fqdn: 'shop.myshopify.com', + }, + ) + + expect(res.type).toEqual('ok') + expect(http.fetch).toHaveBeenCalledWith( + expectedURL, + { + method: 'POST', + body: JSON.stringify({ + schema_id: MONORAIL_COMMAND_TOPIC, + payload: { + command: 'shopify store auth', + time_start: 1643709600000, + time_end: 1643709601000, + total_time: 1000, + success: true, + cli_version: '3.94.0', + uname: 'Darwin test', + ruby_version: '3.3.0', + node_version: '22.0.0', + is_employee: false, + user_id: '42', + store_fqdn_hash: 'hashed-store', + store_fqdn_validated: true, + args: '--store shop.myshopify.com', + store_fqdn: 'shop.myshopify.com', + }, + }), + headers: expectedHeaders, + }, + 'non-blocking', + ) + }) }) diff --git a/packages/store/src/cli/services/store/auth/index.test.ts b/packages/store/src/cli/services/store/auth/index.test.ts index 2f33aecfe1..a02c10ce5a 100644 --- a/packages/store/src/cli/services/store/auth/index.test.ts +++ b/packages/store/src/cli/services/store/auth/index.test.ts @@ -272,6 +272,30 @@ describe('store auth service', () => { expect(presenter.success).toHaveBeenCalledWith(result) }) + test('authenticateStoreWithApp records fqdn metadata before resolving existing scopes', async () => { + await expect( + authenticateStoreWithApp( + { + store: 'shop.myshopify.com', + scopes: 'read_products', + }, + { + resolveExistingScopes: vi.fn().mockRejectedValue(new Error('scope lookup failed')), + presenter: { + openingBrowser: vi.fn(), + manualAuthUrl: vi.fn(), + success: vi.fn(), + }, + }, + ), + ).rejects.toThrow('scope lookup failed') + + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false) + expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true) + expect(setLastSeenUserId).not.toHaveBeenCalled() + expect(setStoredStoreAppSession).not.toHaveBeenCalled() + }) + test('authenticateStoreWithApp records fqdn metadata before waiting for the auth callback', async () => { const waitForStoreAuthCodeMock = vi.fn().mockRejectedValue(new Error('callback failed')) @@ -299,6 +323,72 @@ describe('store auth service', () => { expect(setStoredStoreAppSession).not.toHaveBeenCalled() }) + test('authenticateStoreWithApp does not mark the store validated when token exchange fails', async () => { + const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => { + await options.onListening?.() + return 'abc123' + }) + + await expect( + authenticateStoreWithApp( + { + store: 'shop.myshopify.com', + scopes: 'read_products', + }, + { + openURL: vi.fn().mockResolvedValue(true), + waitForStoreAuthCode: waitForStoreAuthCodeMock, + exchangeStoreAuthCodeForToken: vi.fn().mockRejectedValue(new Error('token exchange failed')), + presenter: { + openingBrowser: vi.fn(), + manualAuthUrl: vi.fn(), + success: vi.fn(), + }, + }, + ), + ).rejects.toThrow('token exchange failed') + + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false) + expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true) + expect(setLastSeenUserId).not.toHaveBeenCalled() + expect(setStoredStoreAppSession).not.toHaveBeenCalled() + }) + + test('authenticateStoreWithApp marks the store validated after token exchange but before rejecting missing associated user data', async () => { + const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => { + await options.onListening?.() + return 'abc123' + }) + + await expect( + authenticateStoreWithApp( + { + store: 'shop.myshopify.com', + scopes: 'read_products', + }, + { + openURL: vi.fn().mockResolvedValue(true), + waitForStoreAuthCode: waitForStoreAuthCodeMock, + exchangeStoreAuthCodeForToken: vi.fn().mockResolvedValue({ + access_token: 'token', + scope: 'read_products', + expires_in: 86400, + }), + presenter: { + openingBrowser: vi.fn(), + manualAuthUrl: vi.fn(), + success: vi.fn(), + }, + }, + ), + ).rejects.toThrow('Shopify did not return associated user information for the online access token.') + + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, 'shop.myshopify.com', false) + expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'shop.myshopify.com', true) + expect(setLastSeenUserId).not.toHaveBeenCalled() + expect(setStoredStoreAppSession).not.toHaveBeenCalled() + }) + test('authenticateStoreWithApp rejects when Shopify grants fewer scopes than requested', async () => { const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => { await options.onListening?.() diff --git a/packages/store/src/cli/services/store/auth/index.ts b/packages/store/src/cli/services/store/auth/index.ts index 4e854d48b6..8856623f96 100644 --- a/packages/store/src/cli/services/store/auth/index.ts +++ b/packages/store/src/cli/services/store/auth/index.ts @@ -40,6 +40,7 @@ export async function authenticateStoreWithApp( ): Promise { const resolvedDependencies: StoreAuthDependencies = {...defaultStoreAuthDependencies, ...dependencies} const store = normalizeStoreFqdn(input.store) + await recordStoreFqdnMetadata(store, false) const requestedScopes = parseStoreAuthScopes(input.scopes) const existingScopeResolution = await resolvedDependencies.resolveExistingScopes(store) const scopes = mergeRequestedAndStoredScopes(requestedScopes, existingScopeResolution.scopes) @@ -60,7 +61,6 @@ export async function authenticateStoreWithApp( authorization: {authorizationUrl}, } = bootstrap - await recordStoreFqdnMetadata(store, false) resolvedDependencies.presenter.openingBrowser() const code = await resolvedDependencies.waitForStoreAuthCode({ @@ -70,8 +70,8 @@ export async function authenticateStoreWithApp( if (!opened) resolvedDependencies.presenter.manualAuthUrl(authorizationUrl) }, }) - await recordStoreFqdnMetadata(store, true) const tokenResponse = await bootstrap.exchangeCodeForToken(code) + await recordStoreFqdnMetadata(store, true) const userId = tokenResponse.associated_user?.id?.toString() if (!userId) { diff --git a/packages/store/src/cli/services/store/execute/admin-context.test.ts b/packages/store/src/cli/services/store/execute/admin-context.test.ts index 6245e9cc82..cb64d86180 100644 --- a/packages/store/src/cli/services/store/execute/admin-context.test.ts +++ b/packages/store/src/cli/services/store/execute/admin-context.test.ts @@ -41,8 +41,8 @@ describe('prepareAdminStoreGraphQLContext', () => { const result = await prepareAdminStoreGraphQLContext({store}) expect(loadStoredStoreSession).toHaveBeenCalledWith(store) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) + expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce() + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true) expect(setLastSeenUserId).toHaveBeenCalledWith('42') expect(fetchPublicApiVersions).toHaveBeenCalledWith({ adminSession: {token: 'token', storeFqdn: store}, @@ -84,8 +84,8 @@ describe('prepareAdminStoreGraphQLContext', () => { const result = await prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: 'unstable'}) expect(loadStoredStoreSession).toHaveBeenCalledWith(store) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) + expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce() + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true) expect(result).toEqual({ adminSession: {token: 'token', storeFqdn: store}, version: 'unstable', @@ -98,16 +98,15 @@ describe('prepareAdminStoreGraphQLContext', () => { await expect(prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: '1999-01'})).rejects.toThrow( 'Invalid API version', ) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) + expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce() + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true) }) - test('records the requested store before failing to load stored auth', async () => { + test('does not record validated store metadata when loading stored auth fails', async () => { vi.mocked(loadStoredStoreSession).mockRejectedValue(new AbortError('missing stored auth')) await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('missing stored auth') - expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, false) - expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith(store, true) + expect(recordStoreFqdnMetadata).not.toHaveBeenCalled() expect(fetchPublicApiVersions).not.toHaveBeenCalled() }) @@ -116,15 +115,15 @@ describe('prepareAdminStoreGraphQLContext', () => { await prepareAdminStoreGraphQLContext({store}) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'permanent-shop.myshopify.com', true) + expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce() + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('permanent-shop.myshopify.com', true) }) test('rethrows whatever the transport raises (errors are owned by the transport)', async () => { vi.mocked(fetchPublicApiVersions).mockRejectedValue(new AbortError('upstream exploded')) await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('upstream exploded') - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false) - expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true) + expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce() + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true) }) }) diff --git a/packages/store/src/cli/services/store/execute/admin-context.ts b/packages/store/src/cli/services/store/execute/admin-context.ts index 6b25021aa7..0eddff703f 100644 --- a/packages/store/src/cli/services/store/execute/admin-context.ts +++ b/packages/store/src/cli/services/store/execute/admin-context.ts @@ -38,7 +38,6 @@ export async function prepareAdminStoreGraphQLContext(input: { store: string userSpecifiedVersion?: string }): Promise { - await recordStoreFqdnMetadata(input.store, false) const session = await loadStoredStoreSession(input.store) await recordStoreFqdnMetadata(session.store, true) setLastSeenUserId(session.userId) diff --git a/packages/store/src/cli/services/store/execute/index.test.ts b/packages/store/src/cli/services/store/execute/index.test.ts index ad4782f47d..489006807b 100644 --- a/packages/store/src/cli/services/store/execute/index.test.ts +++ b/packages/store/src/cli/services/store/execute/index.test.ts @@ -1,12 +1,14 @@ import {executeStoreOperation} from './index.js' import {prepareStoreExecuteRequest} from './request.js' import {getStoreGraphQLTarget} from './targets.js' +import {recordStoreFqdnMetadata} from '../attribution.js' import {renderSingleTask} from '@shopify/cli-kit/node/ui' import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' vi.mock('./request.js') vi.mock('./targets.js') +vi.mock('../attribution.js') vi.mock('@shopify/cli-kit/node/ui') describe('executeStoreOperation', () => { @@ -45,6 +47,7 @@ describe('executeStoreOperation', () => { }), ).resolves.toEqual(result) + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false) expect(getStoreGraphQLTarget).toHaveBeenCalledWith('admin') expect(prepareStoreExecuteRequest).toHaveBeenCalledWith({ query: 'query { shop { name } }', @@ -69,4 +72,18 @@ describe('executeStoreOperation', () => { expect(getStoreGraphQLTarget).toHaveBeenCalledWith('admin') }) + + test('records the requested store before request validation fails', async () => { + vi.mocked(prepareStoreExecuteRequest).mockRejectedValue(new Error('Query should have a value')) + + await expect( + executeStoreOperation({ + store: 'shop.myshopify.com', + }), + ).rejects.toThrow('Query should have a value') + + expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false) + expect(target.prepareContext).not.toHaveBeenCalled() + expect(target.execute).not.toHaveBeenCalled() + }) }) diff --git a/packages/store/src/cli/services/store/execute/index.ts b/packages/store/src/cli/services/store/execute/index.ts index 309cfda88d..985801ae8f 100644 --- a/packages/store/src/cli/services/store/execute/index.ts +++ b/packages/store/src/cli/services/store/execute/index.ts @@ -1,5 +1,6 @@ import {prepareStoreExecuteRequest} from './request.js' import {getStoreGraphQLTarget, type StoreGraphQLApi} from './targets.js' +import {recordStoreFqdnMetadata} from '../attribution.js' import {renderSingleTask} from '@shopify/cli-kit/node/ui' import {outputContent} from '@shopify/cli-kit/node/output' @@ -15,6 +16,7 @@ interface ExecuteStoreOperationInput { } export async function executeStoreOperation(input: ExecuteStoreOperationInput): Promise { + await recordStoreFqdnMetadata(input.store, false) const target = getStoreGraphQLTarget(input.api ?? 'admin') const request = await prepareStoreExecuteRequest({