Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add GQL support for theme access tokens #4717

Merged
merged 2 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion packages/cli-kit/src/private/node/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
}
const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders)

if (err.response.errors?.some((error) => error.extensions.code === '429') || err.response.status === 429) {
if (errorsIncludeStatus429(err)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See thread

let delayMs: number | undefined

try {
Expand Down Expand Up @@ -120,6 +120,19 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
}
}

function errorsIncludeStatus429(error: ClientError): boolean {
if (error.response.status === 429) {
return true
}

// GraphQL returns a 401 with a string error message when auth fails
// Therefore error.response.errros can be a string or GraphQLError[]
if (typeof error.response.errors === 'string') {
return false
}
return error.response.errors?.some((error) => error.extensions?.code === '429') ?? false
}
Comment on lines +130 to +134
Copy link
Contributor Author

@lucyxiang lucyxiang Oct 23, 2024

Choose a reason for hiding this comment

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

My TS isn't the best, ideally I want to do something like this but it didn't seem possible

if (typeof error.response.errors === 'GraphQL[]'){
return error.response.errors?.some((error) => error.extensions?.code === '429') ?? false
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you can interpret array types at run time, so this is probably fine. This error class is a bit of a mess anyway 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the type checking here will fix this issue. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have access to this bugsnag, but I assume this is the error.response.errors?.some is not a function or something error I encountered as well?


export async function simpleRequestWithDebugLog<T extends {headers: Headers; status: number}>(
{request, url}: RequestOptions<T>,
errorHandler?: (error: unknown, requestId: string | undefined) => unknown,
Expand Down
11 changes: 6 additions & 5 deletions packages/cli-kit/src/private/node/api/graphql.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
/* eslint-disable @typescript-eslint/no-base-to-string */
import {GraphQLClientError, sanitizedHeadersOutput} from './headers.js'
import {sanitizeURL} from './urls.js'
import {stringifyMessage, outputContent, outputToken, outputDebug} from '../../../public/node/output.js'
import {AbortError} from '../../../public/node/error.js'
import {ClientError, RequestDocument, Variables} from 'graphql-request'
import {ClientError, Variables} from 'graphql-request'

export function debugLogRequestInfo(
api: string,
query: RequestDocument,
query: string,
url: string,
variables?: Variables,
headers: {[key: string]: string} = {},
) {
outputDebug(outputContent`Sending ${outputToken.json(api)} GraphQL request:
${outputToken.raw(query.toString().trim())}
${variables ? `\nWith variables:\n${sanitizeVariables(variables)}\n` : ''}
With request headers:
${sanitizedHeadersOutput(headers)}
`)
${sanitizedHeadersOutput(headers)}\n
to ${sanitizeURL(url)}`)
}

function sanitizeVariables(variables: Variables): string {
Expand Down
39 changes: 21 additions & 18 deletions packages/cli-kit/src/private/node/api/headers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,25 +52,28 @@ describe('common API methods', () => {
})
})

test.each(['shpat', 'shpua', 'shpca'])(`when custom app token starts with %s, do not prepend 'Bearer'`, (prefix) => {
// Given
vi.mocked(randomUUID).mockReturnValue('random-uuid')
vi.mocked(firstPartyDev).mockReturnValue(false)
const token = `${prefix}_my_token`
// When
const headers = buildHeaders(token)
test.each(['shpat', 'shpua', 'shpca', 'shptka'])(
`when custom app token starts with %s, do not prepend 'Bearer'`,
(prefix) => {
// Given
vi.mocked(randomUUID).mockReturnValue('random-uuid')
vi.mocked(firstPartyDev).mockReturnValue(false)
const token = `${prefix}_my_token`
// When
const headers = buildHeaders(token)

// Then
const version = CLI_KIT_VERSION
expect(headers).toEqual({
'Content-Type': 'application/json',
'Keep-Alive': 'timeout=30',
'X-Shopify-Access-Token': token,
'User-Agent': `Shopify CLI; v=${version}`,
authorization: token,
'Sec-CH-UA-PLATFORM': process.platform,
})
})
// Then
const version = CLI_KIT_VERSION
expect(headers).toEqual({
'Content-Type': 'application/json',
'Keep-Alive': 'timeout=30',
'X-Shopify-Access-Token': token,
'User-Agent': `Shopify CLI; v=${version}`,
authorization: token,
'Sec-CH-UA-PLATFORM': process.platform,
})
},
)

test('sanitizedHeadersOutput removes the headers that include the token', () => {
// Given
Expand Down
2 changes: 1 addition & 1 deletion packages/cli-kit/src/private/node/api/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export function buildHeaders(token?: string): {[key: string]: string} {
...(firstPartyDev() && {'X-Shopify-Cli-Employee': '1'}),
}
if (token) {
const authString = token.match(/^shp(at|ua|ca)/) ? token : `Bearer ${token}`
const authString = token.match(/^shp(at|ua|ca|tka)/) ? token : `Bearer ${token}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shptka_* represent theme access tokens, and we want those as is, not Bearer ${token}


headers.authorization = authString
headers['X-Shopify-Access-Token'] = authString
Expand Down
4 changes: 1 addition & 3 deletions packages/cli-kit/src/private/node/api/rest.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {buildHeaders} from './headers.js'
import {defaultThemeKitAccessDomain, environmentVariables} from '../constants.js'
import {themeKitAccessDomain} from '../constants.js'
import {AdminSession} from '@shopify/cli-kit/node/session'

export function restRequestBody<T>(requestBody?: T) {
Expand All @@ -14,9 +14,7 @@ export function restRequestUrl(
apiVersion: string,
path: string,
searchParams: {[name: string]: string} = {},
env = process.env,
) {
const themeKitAccessDomain = env[environmentVariables.themeKitAccessDomain] || defaultThemeKitAccessDomain
const url = new URL(
isThemeAccessSession(session)
? `https://${themeKitAccessDomain}/cli/admin/api/${apiVersion}${path}.json`
Expand Down
3 changes: 3 additions & 0 deletions packages/cli-kit/src/private/node/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,6 @@ export const sessionConstants = {
export const bugsnagApiKey = '9e1e6889176fd0c795d5c659225e0fae'

export const reportingRateLimit = {limit: 300, timeout: {days: 1}}

export const themeKitAccessDomain =
process.env[environmentVariables.themeKitAccessDomain] ?? defaultThemeKitAccessDomain
29 changes: 29 additions & 0 deletions packages/cli-kit/src/public/node/api/admin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,39 @@ describe('admin-graphql-api', () => {
query: 'query',
api: 'Admin',
url: 'https://store.myshopify.com/admin/api/2022-01/graphql.json',
addedHeaders: {},
token,
variables: {variables: 'variables'},
})
})

test('request is called with correct parameters when it is a theme access session', async () => {
// Given
const themeAccessToken = 'shptka_token'
const themeAccessSession = {
...Session,
token: themeAccessToken,
}

vi.mocked(graphqlRequest).mockResolvedValue(mockedResult)
vi.mocked(graphqlRequestDoc).mockResolvedValue(mockedResult)

// When
await admin.adminRequest('query', themeAccessSession, {variables: 'variables'})

// Then
expect(graphqlRequest).toHaveBeenLastCalledWith({
query: 'query',
api: 'Admin',
addedHeaders: {
'X-Shopify-Access-Token': 'shptka_token',
'X-Shopify-Shop': 'store',
},
url: `https://${defaultThemeKitAccessDomain}/cli/admin/api/2022-01/graphql.json`,
token: themeAccessToken,
variables: {variables: 'variables'},
})
})
})

describe('admin-rest-api', () => {
Expand Down
33 changes: 27 additions & 6 deletions packages/cli-kit/src/public/node/api/admin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,16 @@ import {graphqlRequest, graphqlRequestDoc, GraphQLResponseOptions, GraphQLVariab
import {AdminSession} from '../session.js'
import {outputContent, outputToken} from '../../../public/node/output.js'
import {BugError, AbortError} from '../error.js'
import {restRequestBody, restRequestHeaders, restRequestUrl} from '../../../private/node/api/rest.js'
import {
restRequestBody,
restRequestHeaders,
restRequestUrl,
isThemeAccessSession,
} from '../../../private/node/api/rest.js'
import {fetch} from '../http.js'
import {PublicApiVersions} from '../../../cli/api/graphql/admin/generated/public_api_versions.js'
import {normalizeStoreFqdn} from '../context/fqdn.js'
import {themeKitAccessDomain} from '../../../private/node/constants.js'
import {ClientError, Variables} from 'graphql-request'
import {TypedDocumentNode} from '@graphql-typed-document-node/core'

Expand All @@ -21,8 +27,9 @@ export async function adminRequest<T>(query: string, session: AdminSession, vari
const api = 'Admin'
const version = await fetchLatestSupportedApiVersion(session)
const store = await normalizeStoreFqdn(session.storeFqdn)
const url = adminUrl(store, version)
return graphqlRequest({query, api, url, token: session.token, variables})
const url = adminUrl(store, version, session)
const addedHeaders = themeAccessHeaders(session)
return graphqlRequest({query, api, addedHeaders, url, token: session.token, variables})
}

/**
Expand All @@ -47,15 +54,23 @@ export async function adminRequestDoc<TResult, TVariables extends Variables>(
apiVersion = await fetchLatestSupportedApiVersion(session)
}
const store = await normalizeStoreFqdn(session.storeFqdn)
const addedHeaders = themeAccessHeaders(session)
const opts = {
url: adminUrl(store, apiVersion),
url: adminUrl(store, apiVersion, session),
api: 'Admin',
token: session.token,
addedHeaders,
}
const result = graphqlRequestDoc<TResult, TVariables>({...opts, query, variables, responseOptions})
return result
}

function themeAccessHeaders(session: AdminSession): {[header: string]: string} {
return isThemeAccessSession(session)
? {'X-Shopify-Shop': session.storeFqdn, 'X-Shopify-Access-Token': session.token}
: {}
}

/**
* GraphQL query to retrieve the latest supported API version.
*
Expand Down Expand Up @@ -121,11 +136,17 @@ async function fetchApiVersions(session: AdminSession): Promise<ApiVersion[]> {
*
* @param store - Store FQDN.
* @param version - API version.
* @param session - User session.
* @returns - Admin API URL.
*/
export function adminUrl(store: string, version: string | undefined): string {
export function adminUrl(store: string, version: string | undefined, session?: AdminSession): string {
const realVersion = version ?? 'unstable'
return `https://${store}/admin/api/${realVersion}/graphql.json`

const url =
session && isThemeAccessSession(session)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we're "sprinkling" isThemeAccessSession around in various places. It might be best to lift this up to admin.ts at least so we can craft the URL from either adminUrl or something like themeKitUrl(). @shauns made a nice slack post on this thinking here

Copy link
Contributor Author

@lucyxiang lucyxiang Oct 24, 2024

Choose a reason for hiding this comment

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

Yes in the issue description I mentioned In follow up, would like to clean up theme access logic by creating a file for it, rather than implementing it in REST and GraphQL. I was thinking of doing it as a fast follow since this PR is blocking other CLI GraphQL PRs but I can also try to do it here

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats fair, so long as we can track it with an issue! Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? `https://${themeKitAccessDomain}/cli/admin/api/${realVersion}/graphql.json`
: `https://${store}/admin/api/${realVersion}/graphql.json`
return url
}

interface ApiVersion {
Expand Down
1 change: 1 addition & 0 deletions packages/cli-kit/src/public/node/api/graphql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ describe('graphqlRequestDoc', () => {
`query QueryName {
example
}`,
'mockedAddress',
mockVariables,
expect.anything(),
)
Expand Down
2 changes: 1 addition & 1 deletion packages/cli-kit/src/public/node/api/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ async function performGraphQLRequest<TResult>(options: PerformGraphQLRequestOpti
...buildHeaders(token),
}

debugLogRequestInfo(api, queryAsString, variables, headers)
debugLogRequestInfo(api, queryAsString, url, variables, headers)
const clientOptions = {agent: await httpsAgent(), headers}
const client = new GraphQLClient(url, clientOptions)

Expand Down