Skip to content

Commit fd6bc50

Browse files
authored
Merge pull request #8999 from Shopify/river/service-account-store-access-errors
Explain a token's missing dev store access instead of dumping the 401
2 parents 70cbf88 + 2932221 commit fd6bc50

21 files changed

Lines changed: 501 additions & 46 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@shopify/cli-kit': patch
3+
'@shopify/cli': patch
4+
---
5+
6+
Explain which access is missing when a token can't manage dev stores, instead of printing a raw GraphQL 401 with the query and a stack trace.

‎packages/app/src/cli/services/dev/processes/dev-session/dev-session-process.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {getUploadURL, writeManifestToBundle} from '../../../bundle.js'
1616
import {formData} from '@shopify/cli-kit/node/http'
1717
import {describe, expect, test, vi, beforeEach, afterEach} from 'vitest'
1818
import {AbortController} from '@shopify/cli-kit/node/abort'
19+
import {AbortError} from '@shopify/cli-kit/node/error'
1920
import {flushPromises} from '@shopify/cli-kit/node/promises'
2021
import * as outputContext from '@shopify/cli-kit/node/ui/components'
2122
import {readdir} from '@shopify/cli-kit/node/fs'
@@ -168,6 +169,27 @@ describe('pushUpdatesForDevSession', () => {
168169
expect(stdout.write).toHaveBeenCalledWith(expect.stringContaining('Test error'))
169170
})
170171

172+
test('asks to restart app dev when the upload URL request is still unauthorized after a token refresh', async () => {
173+
// Given: cli-kit reports a 401 that survives the token refresh with its status code.
174+
vi.mocked(getUploadURL).mockRejectedValue(
175+
Object.assign(
176+
new AbortError('The App Management GraphQL API responded unsuccessfully with the HTTP status 401.'),
177+
{
178+
statusCode: 401,
179+
},
180+
),
181+
)
182+
183+
// When
184+
await pushUpdatesForDevSession({stderr, stdout, abortSignal: abortController.signal}, options)
185+
const [onStart] = appWatcher.listeners('ready') as ((event: unknown) => Promise<void>)[]
186+
187+
// Then
188+
await expect(onStart!({app, extensionEvents: []})).rejects.toThrow(
189+
new AbortError('Auth session expired. Please run `shopify app dev` again.'),
190+
)
191+
})
192+
171193
test('handles receiving an event before session is ready', async () => {
172194
// When
173195
await pushUpdatesForDevSession({stderr, stdout, abortSignal: abortController.signal}, options)

‎packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,8 +334,10 @@ export class DevSession {
334334
}
335335
// eslint-disable-next-line @typescript-eslint/no-explicit-any
336336
} catch (error: any) {
337+
// cli-kit reports a 401 that survives the token refresh with its `statusCode`; a raw graphql-request error
338+
// carries the status on its response.
337339
if (error.statusCode === 401) {
338-
throw new Error('Unauthorized')
340+
throw new AbortError('Auth session expired. Please run `shopify app dev` again.')
339341
} else if (error instanceof ClientError) {
340342
if (error.response.status === 401 || error.response.status === 403) {
341343
throw new AbortError('Auth session expired. Please run `shopify app dev` again.')

‎packages/cli-kit/src/private/node/api/graphql.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,9 @@ export function errorHandler(api: string): (error: unknown, requestId?: string)
134134
errorMessage = extractedMessages
135135
} else if (extractedMessages && status >= 500) {
136136
errorMessage = `The ${api} GraphQL API responded with HTTP status ${status}: ${extractedMessages}`
137+
} else if (error.response.errors === undefined) {
138+
// A 401 from an API gateway, for example, has no GraphQL body to quote.
139+
errorMessage = `The ${api} GraphQL API responded unsuccessfully with the HTTP status ${status}.`
137140
} else {
138141
errorMessage = stringifyMessage(outputContent`
139142
The ${outputToken.raw(api)} GraphQL API responded unsuccessfully with${

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,15 @@ const handlers = [
9090
},
9191
)
9292
}),
93+
mockApi.query('FailsWithUnauthorizedAndNoBody', () => {
94+
// A gateway's 401 has no GraphQL body, so the typed `HttpResponse.json` doesn't fit here.
95+
return new HttpResponse(null, {
96+
status: 401,
97+
headers: {
98+
'x-request-id': 'unauthorized-request-id',
99+
},
100+
})
101+
}),
93102
mockApi.query('FailsWithBadRequest', () => {
94103
return HttpResponse.json(
95104
{
@@ -390,6 +399,70 @@ describe('graphqlRequest', () => {
390399
expect(requestCount).toBe(2)
391400
})
392401

402+
describe('a 401 that survives the token refresh', () => {
403+
const query = 'query FailsWithUnauthorizedAndNoBody { example(secret: $secret) }'
404+
const variables = {secret: 'do-not-print-me'}
405+
406+
test('reports the status without the request when no token refresh function is provided', async () => {
407+
const res = graphqlRequest({query, api: 'mockApi', url: mockedAddress, token: mockToken, variables})
408+
409+
await expect(res).rejects.toMatchObject({
410+
statusCode: 401,
411+
message:
412+
'The mockApi GraphQL API responded unsuccessfully with the HTTP status 401.\n\nRequest ID: unauthorized-request-id',
413+
})
414+
await expect(res).rejects.not.toThrow(/do-not-print-me|example\(secret/)
415+
})
416+
417+
test('reports the status without the request when the token cannot be refreshed', async () => {
418+
const res = graphqlRequest({
419+
query,
420+
api: 'mockApi',
421+
url: mockedAddress,
422+
token: mockToken,
423+
variables,
424+
unauthorizedHandler: {type: 'token_refresh', handler: async () => ({token: undefined})},
425+
})
426+
427+
await expect(res).rejects.toMatchObject({statusCode: 401})
428+
await expect(res).rejects.not.toThrow(/do-not-print-me|example\(secret/)
429+
})
430+
431+
test('reports the status without the request when the refreshed token is rejected too', async () => {
432+
const res = graphqlRequest({
433+
query,
434+
api: 'mockApi',
435+
url: mockedAddress,
436+
token: mockToken,
437+
variables,
438+
unauthorizedHandler: {type: 'token_refresh', handler: async () => ({token: 'refreshed-token'})},
439+
})
440+
441+
let requestCount = 0
442+
server.events.on('request:start', () => {
443+
requestCount++
444+
})
445+
446+
await expect(res).rejects.toMatchObject({statusCode: 401})
447+
await expect(res).rejects.not.toThrow(/do-not-print-me|example\(secret/)
448+
expect(requestCount).toBe(2)
449+
})
450+
451+
test('keeps the raw error when the caller handles errors itself', async () => {
452+
const res = graphqlRequest({
453+
query,
454+
api: 'mockApi',
455+
url: mockedAddress,
456+
token: mockToken,
457+
variables,
458+
responseOptions: {handleErrors: false},
459+
unauthorizedHandler: {type: 'token_refresh', handler: async () => ({token: 'refreshed-token'})},
460+
})
461+
462+
await expect(res).rejects.toMatchObject({response: {status: 401}})
463+
})
464+
})
465+
393466
test('Fails without retry when bad request when unauthorized handler is provided', async () => {
394467
const res = graphqlRequest({
395468
query: 'query FailsWithBadRequest { example }',

‎packages/cli-kit/src/public/node/api/graphql.ts‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -215,11 +215,18 @@ async function performGraphQLRequest<TResult>(options: PerformGraphQLRequestOpti
215215
}
216216
: undefined
217217

218+
const handleErrors = responseOptions?.handleErrors !== false
219+
218220
const request = () =>
219-
retryAwareRequest(
220-
{request: rawGraphQLRequest, url, ...behaviour},
221-
responseOptions?.handleErrors === false ? undefined : errorHandler(api),
222-
)
221+
retryAwareRequest({request: rawGraphQLRequest, url, ...behaviour}, handleErrors ? errorHandler(api) : undefined)
222+
223+
// `retryAwareRequest` hands a 401 back raw so the token can be refreshed and the request repeated. Once that is
224+
// settled, the error goes through the same handler as every other failed response: a raw graphql-request error
225+
// carries the whole request, query text included, and would otherwise reach the user as a CLI bug.
226+
const unauthorizedError = (error: unknown): unknown => {
227+
if (!handleErrors || !(error instanceof ClientError) || error.response.status !== 401) return error
228+
return errorHandler(api)(error, requestIdFromClientError(error))
229+
}
223230

224231
const executeWithTimer = () =>
225232
runWithTimer('cmd_all_timing_network_ms')(async () => {
@@ -229,12 +236,16 @@ async function performGraphQLRequest<TResult>(options: PerformGraphQLRequestOpti
229236
} catch (error) {
230237
if (error instanceof ClientError && error.response.status === 401 && tokenRefreshUnauthorizedHandlerFunction) {
231238
if (await tokenRefreshUnauthorizedHandlerFunction()) {
232-
response = await request()
239+
try {
240+
response = await request()
241+
} catch (retryError) {
242+
throw unauthorizedError(retryError)
243+
}
233244
} else {
234-
throw error
245+
throw unauthorizedError(error)
235246
}
236247
} else {
237-
throw error
248+
throw unauthorizedError(error)
238249
}
239250
}
240251

@@ -274,6 +285,17 @@ async function performGraphQLRequest<TResult>(options: PerformGraphQLRequestOpti
274285
return JSON.parse(result) as TResult
275286
}
276287

288+
function requestIdFromClientError(error: ClientError): string | undefined {
289+
try {
290+
// graphql-request types the response without headers, but it carries the fetch Response's headers.
291+
const headers = (error.response as {headers?: {get?: (name: string) => string | null}}).headers
292+
return headers?.get?.('x-request-id') ?? undefined
293+
// eslint-disable-next-line no-catch-all/no-catch-all
294+
} catch {
295+
return undefined
296+
}
297+
}
298+
277299
function graphQLCachePrincipalHash(url: string, token: string | undefined): string {
278300
return sha256(JSON.stringify([url, token ?? ''])).toString('hex')
279301
}
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
import {
2+
isOrganizationAccessError,
3+
OrganizationStoreAccessError,
4+
organizationStoresRequestDoc,
5+
} from './business-platform.js'
6+
import {describe, expect, test, vi, beforeEach} from 'vitest'
7+
import {businessPlatformOrganizationsRequestDoc} from '@shopify/cli-kit/node/api/business-platform'
8+
import {getAppAutomationToken} from '@shopify/cli-kit/node/environment'
9+
import {isServiceAccountTokenSet} from '@shopify/cli-kit/node/session'
10+
import {AbortError} from '@shopify/cli-kit/node/error'
11+
12+
vi.mock('@shopify/cli-kit/node/environment')
13+
vi.mock('@shopify/cli-kit/node/api/business-platform')
14+
vi.mock('@shopify/cli-kit/node/session')
15+
16+
const organization = {id: '299069', businessName: 'A Better Looking Shop'}
17+
18+
// The shape cli-kit gives a handled 4xx response: a `GraphQLClientError`, which carries `statusCode`.
19+
function handledClientError(statusCode: number): Error {
20+
return Object.assign(new Error(`The Business Platform GraphQL API responded with the HTTP status ${statusCode}.`), {
21+
statusCode,
22+
})
23+
}
24+
25+
// The shape graphql-request throws when the caller handles errors itself.
26+
function rawClientError(status: number): Error {
27+
return Object.assign(new Error('GraphQL Error (Code: 401)'), {response: {status, errors: undefined}, request: {}})
28+
}
29+
30+
beforeEach(() => {
31+
vi.mocked(isServiceAccountTokenSet).mockReturnValue(false)
32+
vi.mocked(getAppAutomationToken).mockReturnValue(undefined)
33+
})
34+
35+
describe('isOrganizationAccessError', () => {
36+
test('recognises a handled 401 and 403', () => {
37+
expect(isOrganizationAccessError(handledClientError(401))).toBe(true)
38+
expect(isOrganizationAccessError(handledClientError(403))).toBe(true)
39+
})
40+
41+
test('recognises a raw graphql-request 401', () => {
42+
expect(isOrganizationAccessError(rawClientError(401))).toBe(true)
43+
})
44+
45+
test('ignores other statuses and other errors', () => {
46+
expect(isOrganizationAccessError(handledClientError(404))).toBe(false)
47+
expect(isOrganizationAccessError(handledClientError(500))).toBe(false)
48+
expect(isOrganizationAccessError(new Error('boom'))).toBe(false)
49+
expect(isOrganizationAccessError(undefined)).toBe(false)
50+
})
51+
})
52+
53+
describe('OrganizationStoreAccessError', () => {
54+
test('tells a service account which access it lacks and where to grant it', () => {
55+
vi.mocked(isServiceAccountTokenSet).mockReturnValue(true)
56+
57+
const error = new OrganizationStoreAccessError(organization)
58+
59+
expect(error).toBeInstanceOf(AbortError)
60+
expect(error.message).toBe("This service account can't manage dev stores in A Better Looking Shop.")
61+
// The guidance lives in nextSteps, the field the store commands' --json output carries.
62+
expect(error.tryMessage).toBeNull()
63+
expect(error.nextSteps).toEqual([
64+
'Give the service account Dev store management access in the Shopify admin under Settings > Users > Security.',
65+
'Or use a token from a service account that has that access.',
66+
])
67+
})
68+
69+
test('points a legacy automation token at service accounts', () => {
70+
vi.mocked(getAppAutomationToken).mockReturnValue('legacy-token')
71+
72+
const error = new OrganizationStoreAccessError(organization)
73+
74+
expect(error.message).toBe("This automation token can't manage dev stores in A Better Looking Shop.")
75+
expect(error.nextSteps?.join(' ')).toContain('SHOPIFY_SERVICE_ACCOUNT_TOKEN')
76+
})
77+
78+
test('talks about permission for a logged-in user', () => {
79+
const error = new OrganizationStoreAccessError(organization)
80+
81+
expect(error.message).toBe("You don't have permission to manage dev stores in A Better Looking Shop.")
82+
expect(error.nextSteps).toEqual([
83+
"Check that you're logged in to the right account.",
84+
'Or ask an organization admin for access.',
85+
])
86+
})
87+
88+
test('falls back to the organization ID when the name is unknown', () => {
89+
const error = new OrganizationStoreAccessError({id: '299069'})
90+
91+
expect(error.message).toBe("You don't have permission to manage dev stores in organization 299069.")
92+
})
93+
})
94+
95+
describe('organizationStoresRequestDoc', () => {
96+
const request = {
97+
query: {} as never,
98+
token: 'token',
99+
variables: {first: 1},
100+
unauthorizedHandler: {type: 'token_refresh' as const, handler: async () => ({token: 'refreshed'})},
101+
}
102+
103+
test("sends the request to the organization's Organizations API and returns the result", async () => {
104+
vi.mocked(businessPlatformOrganizationsRequestDoc).mockResolvedValue({ok: true})
105+
106+
await expect(organizationStoresRequestDoc(organization, request)).resolves.toEqual({ok: true})
107+
expect(businessPlatformOrganizationsRequestDoc).toHaveBeenCalledWith({...request, organizationId: '299069'})
108+
})
109+
110+
test('replaces a refused request with the access error', async () => {
111+
vi.mocked(isServiceAccountTokenSet).mockReturnValue(true)
112+
vi.mocked(businessPlatformOrganizationsRequestDoc).mockRejectedValue(handledClientError(401))
113+
114+
await expect(organizationStoresRequestDoc(organization, request)).rejects.toThrow(
115+
"This service account can't manage dev stores in A Better Looking Shop.",
116+
)
117+
})
118+
119+
test('rethrows every other error unchanged', async () => {
120+
const notFound = handledClientError(404)
121+
vi.mocked(businessPlatformOrganizationsRequestDoc).mockRejectedValue(notFound)
122+
123+
await expect(organizationStoresRequestDoc(organization, request)).rejects.toBe(notFound)
124+
})
125+
})

0 commit comments

Comments
 (0)