Skip to content

Commit 61ca960

Browse files
dmeranddmerand
committed
Preserve store attribution on early failures
1 parent 0e7fdcc commit 61ca960

7 files changed

Lines changed: 179 additions & 17 deletions

File tree

‎packages/cli-kit/src/public/node/monorail.test.ts‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as http from './http.js'
2-
import {publishMonorailEvent} from './monorail.js'
2+
import {MONORAIL_COMMAND_TOPIC, publishMonorailEvent} from './monorail.js'
33
import {mockAndCaptureOutput} from './testing/output.js'
44
import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest'
55

@@ -52,4 +52,59 @@ describe('monorail', () => {
5252
expect(outputMock.debug()).toContain('"api_key": "****"')
5353
expect(outputMock.debug()).not.toContain('some-api-key')
5454
})
55+
56+
test('builds a request for the real command topic including validated store attribution', async () => {
57+
const res = await publishMonorailEvent(
58+
MONORAIL_COMMAND_TOPIC,
59+
{
60+
command: 'shopify store auth',
61+
time_start: 1643709600000,
62+
time_end: 1643709601000,
63+
total_time: 1000,
64+
success: true,
65+
cli_version: '3.94.0',
66+
uname: 'Darwin test',
67+
ruby_version: '3.3.0',
68+
node_version: '22.0.0',
69+
is_employee: false,
70+
user_id: '42',
71+
store_fqdn_hash: 'hashed-store',
72+
store_fqdn_validated: true,
73+
},
74+
{
75+
args: '--store shop.myshopify.com',
76+
store_fqdn: 'shop.myshopify.com',
77+
},
78+
)
79+
80+
expect(res.type).toEqual('ok')
81+
expect(http.fetch).toHaveBeenCalledWith(
82+
expectedURL,
83+
{
84+
method: 'POST',
85+
body: JSON.stringify({
86+
schema_id: MONORAIL_COMMAND_TOPIC,
87+
payload: {
88+
command: 'shopify store auth',
89+
time_start: 1643709600000,
90+
time_end: 1643709601000,
91+
total_time: 1000,
92+
success: true,
93+
cli_version: '3.94.0',
94+
uname: 'Darwin test',
95+
ruby_version: '3.3.0',
96+
node_version: '22.0.0',
97+
is_employee: false,
98+
user_id: '42',
99+
store_fqdn_hash: 'hashed-store',
100+
store_fqdn_validated: true,
101+
args: '--store shop.myshopify.com',
102+
store_fqdn: 'shop.myshopify.com',
103+
},
104+
}),
105+
headers: expectedHeaders,
106+
},
107+
'non-blocking',
108+
)
109+
})
55110
})

‎packages/store/src/cli/services/store/auth/index.test.ts‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,30 @@ describe('store auth service', () => {
272272
expect(presenter.success).toHaveBeenCalledWith(result)
273273
})
274274

275+
test('authenticateStoreWithApp records fqdn metadata before resolving existing scopes', async () => {
276+
await expect(
277+
authenticateStoreWithApp(
278+
{
279+
store: 'shop.myshopify.com',
280+
scopes: 'read_products',
281+
},
282+
{
283+
resolveExistingScopes: vi.fn().mockRejectedValue(new Error('scope lookup failed')),
284+
presenter: {
285+
openingBrowser: vi.fn(),
286+
manualAuthUrl: vi.fn(),
287+
success: vi.fn(),
288+
},
289+
},
290+
),
291+
).rejects.toThrow('scope lookup failed')
292+
293+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false)
294+
expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true)
295+
expect(setLastSeenUserId).not.toHaveBeenCalled()
296+
expect(setStoredStoreAppSession).not.toHaveBeenCalled()
297+
})
298+
275299
test('authenticateStoreWithApp records fqdn metadata before waiting for the auth callback', async () => {
276300
const waitForStoreAuthCodeMock = vi.fn().mockRejectedValue(new Error('callback failed'))
277301

@@ -299,6 +323,72 @@ describe('store auth service', () => {
299323
expect(setStoredStoreAppSession).not.toHaveBeenCalled()
300324
})
301325

326+
test('authenticateStoreWithApp does not mark the store validated when token exchange fails', async () => {
327+
const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => {
328+
await options.onListening?.()
329+
return 'abc123'
330+
})
331+
332+
await expect(
333+
authenticateStoreWithApp(
334+
{
335+
store: 'shop.myshopify.com',
336+
scopes: 'read_products',
337+
},
338+
{
339+
openURL: vi.fn().mockResolvedValue(true),
340+
waitForStoreAuthCode: waitForStoreAuthCodeMock,
341+
exchangeStoreAuthCodeForToken: vi.fn().mockRejectedValue(new Error('token exchange failed')),
342+
presenter: {
343+
openingBrowser: vi.fn(),
344+
manualAuthUrl: vi.fn(),
345+
success: vi.fn(),
346+
},
347+
},
348+
),
349+
).rejects.toThrow('token exchange failed')
350+
351+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false)
352+
expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true)
353+
expect(setLastSeenUserId).not.toHaveBeenCalled()
354+
expect(setStoredStoreAppSession).not.toHaveBeenCalled()
355+
})
356+
357+
test('authenticateStoreWithApp marks the store validated after token exchange but before rejecting missing associated user data', async () => {
358+
const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => {
359+
await options.onListening?.()
360+
return 'abc123'
361+
})
362+
363+
await expect(
364+
authenticateStoreWithApp(
365+
{
366+
store: 'shop.myshopify.com',
367+
scopes: 'read_products',
368+
},
369+
{
370+
openURL: vi.fn().mockResolvedValue(true),
371+
waitForStoreAuthCode: waitForStoreAuthCodeMock,
372+
exchangeStoreAuthCodeForToken: vi.fn().mockResolvedValue({
373+
access_token: 'token',
374+
scope: 'read_products',
375+
expires_in: 86400,
376+
}),
377+
presenter: {
378+
openingBrowser: vi.fn(),
379+
manualAuthUrl: vi.fn(),
380+
success: vi.fn(),
381+
},
382+
},
383+
),
384+
).rejects.toThrow('Shopify did not return associated user information for the online access token.')
385+
386+
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, 'shop.myshopify.com', false)
387+
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'shop.myshopify.com', true)
388+
expect(setLastSeenUserId).not.toHaveBeenCalled()
389+
expect(setStoredStoreAppSession).not.toHaveBeenCalled()
390+
})
391+
302392
test('authenticateStoreWithApp rejects when Shopify grants fewer scopes than requested', async () => {
303393
const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => {
304394
await options.onListening?.()

‎packages/store/src/cli/services/store/auth/index.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ export async function authenticateStoreWithApp(
4040
): Promise<StoreAuthResult> {
4141
const resolvedDependencies: StoreAuthDependencies = {...defaultStoreAuthDependencies, ...dependencies}
4242
const store = normalizeStoreFqdn(input.store)
43+
await recordStoreFqdnMetadata(store, false)
4344
const requestedScopes = parseStoreAuthScopes(input.scopes)
4445
const existingScopeResolution = await resolvedDependencies.resolveExistingScopes(store)
4546
const scopes = mergeRequestedAndStoredScopes(requestedScopes, existingScopeResolution.scopes)
@@ -60,7 +61,6 @@ export async function authenticateStoreWithApp(
6061
authorization: {authorizationUrl},
6162
} = bootstrap
6263

63-
await recordStoreFqdnMetadata(store, false)
6464
resolvedDependencies.presenter.openingBrowser()
6565

6666
const code = await resolvedDependencies.waitForStoreAuthCode({
@@ -70,8 +70,8 @@ export async function authenticateStoreWithApp(
7070
if (!opened) resolvedDependencies.presenter.manualAuthUrl(authorizationUrl)
7171
},
7272
})
73-
await recordStoreFqdnMetadata(store, true)
7473
const tokenResponse = await bootstrap.exchangeCodeForToken(code)
74+
await recordStoreFqdnMetadata(store, true)
7575

7676
const userId = tokenResponse.associated_user?.id?.toString()
7777
if (!userId) {

‎packages/store/src/cli/services/store/execute/admin-context.test.ts‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@ describe('prepareAdminStoreGraphQLContext', () => {
4141
const result = await prepareAdminStoreGraphQLContext({store})
4242

4343
expect(loadStoredStoreSession).toHaveBeenCalledWith(store)
44-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false)
45-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true)
44+
expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce()
45+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true)
4646
expect(setLastSeenUserId).toHaveBeenCalledWith('42')
4747
expect(fetchPublicApiVersions).toHaveBeenCalledWith({
4848
adminSession: {token: 'token', storeFqdn: store},
@@ -84,8 +84,8 @@ describe('prepareAdminStoreGraphQLContext', () => {
8484
const result = await prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: 'unstable'})
8585

8686
expect(loadStoredStoreSession).toHaveBeenCalledWith(store)
87-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false)
88-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true)
87+
expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce()
88+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true)
8989
expect(result).toEqual({
9090
adminSession: {token: 'token', storeFqdn: store},
9191
version: 'unstable',
@@ -98,16 +98,15 @@ describe('prepareAdminStoreGraphQLContext', () => {
9898
await expect(prepareAdminStoreGraphQLContext({store, userSpecifiedVersion: '1999-01'})).rejects.toThrow(
9999
'Invalid API version',
100100
)
101-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false)
102-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true)
101+
expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce()
102+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true)
103103
})
104104

105-
test('records the requested store before failing to load stored auth', async () => {
105+
test('does not record validated store metadata when loading stored auth fails', async () => {
106106
vi.mocked(loadStoredStoreSession).mockRejectedValue(new AbortError('missing stored auth'))
107107

108108
await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('missing stored auth')
109-
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, false)
110-
expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith(store, true)
109+
expect(recordStoreFqdnMetadata).not.toHaveBeenCalled()
111110
expect(fetchPublicApiVersions).not.toHaveBeenCalled()
112111
})
113112

@@ -116,15 +115,15 @@ describe('prepareAdminStoreGraphQLContext', () => {
116115

117116
await prepareAdminStoreGraphQLContext({store})
118117

119-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false)
120-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'permanent-shop.myshopify.com', true)
118+
expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce()
119+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('permanent-shop.myshopify.com', true)
121120
})
122121

123122
test('rethrows whatever the transport raises (errors are owned by the transport)', async () => {
124123
vi.mocked(fetchPublicApiVersions).mockRejectedValue(new AbortError('upstream exploded'))
125124

126125
await expect(prepareAdminStoreGraphQLContext({store})).rejects.toThrow('upstream exploded')
127-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, store, false)
128-
expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, store, true)
126+
expect(recordStoreFqdnMetadata).toHaveBeenCalledOnce()
127+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith(store, true)
129128
})
130129
})

‎packages/store/src/cli/services/store/execute/admin-context.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ export async function prepareAdminStoreGraphQLContext(input: {
3838
store: string
3939
userSpecifiedVersion?: string
4040
}): Promise<AdminStoreGraphQLContext> {
41-
await recordStoreFqdnMetadata(input.store, false)
4241
const session = await loadStoredStoreSession(input.store)
4342
await recordStoreFqdnMetadata(session.store, true)
4443
setLastSeenUserId(session.userId)

‎packages/store/src/cli/services/store/execute/index.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
import {executeStoreOperation} from './index.js'
22
import {prepareStoreExecuteRequest} from './request.js'
33
import {getStoreGraphQLTarget} from './targets.js'
4+
import {recordStoreFqdnMetadata} from '../attribution.js'
45
import {renderSingleTask} from '@shopify/cli-kit/node/ui'
56
import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest'
67
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
78

89
vi.mock('./request.js')
910
vi.mock('./targets.js')
11+
vi.mock('../attribution.js')
1012
vi.mock('@shopify/cli-kit/node/ui')
1113

1214
describe('executeStoreOperation', () => {
@@ -45,6 +47,7 @@ describe('executeStoreOperation', () => {
4547
}),
4648
).resolves.toEqual(result)
4749

50+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false)
4851
expect(getStoreGraphQLTarget).toHaveBeenCalledWith('admin')
4952
expect(prepareStoreExecuteRequest).toHaveBeenCalledWith({
5053
query: 'query { shop { name } }',
@@ -69,4 +72,18 @@ describe('executeStoreOperation', () => {
6972

7073
expect(getStoreGraphQLTarget).toHaveBeenCalledWith('admin')
7174
})
75+
76+
test('records the requested store before request validation fails', async () => {
77+
vi.mocked(prepareStoreExecuteRequest).mockRejectedValue(new Error('Query should have a value'))
78+
79+
await expect(
80+
executeStoreOperation({
81+
store: 'shop.myshopify.com',
82+
}),
83+
).rejects.toThrow('Query should have a value')
84+
85+
expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false)
86+
expect(target.prepareContext).not.toHaveBeenCalled()
87+
expect(target.execute).not.toHaveBeenCalled()
88+
})
7289
})

‎packages/store/src/cli/services/store/execute/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {prepareStoreExecuteRequest} from './request.js'
22
import {getStoreGraphQLTarget, type StoreGraphQLApi} from './targets.js'
3+
import {recordStoreFqdnMetadata} from '../attribution.js'
34
import {renderSingleTask} from '@shopify/cli-kit/node/ui'
45
import {outputContent} from '@shopify/cli-kit/node/output'
56

@@ -15,6 +16,7 @@ interface ExecuteStoreOperationInput {
1516
}
1617

1718
export async function executeStoreOperation(input: ExecuteStoreOperationInput): Promise<unknown> {
19+
await recordStoreFqdnMetadata(input.store, false)
1820
const target = getStoreGraphQLTarget(input.api ?? 'admin')
1921

2022
const request = await prepareStoreExecuteRequest({

0 commit comments

Comments
 (0)