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.

Sign up for GitHub

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

[bugfix] Ensure consistent behaviour when using access token from session in GQL and REST clients #1772

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[bugfix] Ensure consistent behaviour when using access token from session in GQL and REST clients #1772

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3ffa4b0
Replicate REST client behaviour on adminApiAccessToken on GQL client
sle-c Nov 18, 2024
4413e97
Update test case for both clients
sle-c Nov 18, 2024
ded10ed
Fixes lint
sle-c Nov 18, 2024
54e6529
Adds changelog file
sle-c Nov 18, 2024
f6725a5
Fixes lint
sle-c Nov 18, 2024
7470d92
Fixes lint
sle-c Nov 18, 2024
709efd0
Changes to patch version
sle-c Nov 21, 2024
ca4791a
Deprecate `apiSecretKey` being the access token for custom app
sle-c Nov 21, 2024
f9fb5a8
Fixes lint
sle-c Nov 21, 2024
f9bf390
Remove log test
sle-c Nov 21, 2024
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
5 changes: 5 additions & 0 deletions .changeset/clever-taxis-retire.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/shopify-api': minor
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a patch

---

[bugfix] Makes GQL client behavior on customn app config consistent with REST client
27 changes: 27 additions & 0 deletions packages/apps/shopify-api/lib/clients/admin/__tests__/admin_graphql_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,33 @@ describe('GraphQL client', () => {
}).toMatchMadeHttpRequest();
});

it('adapts to private app requests only if isCustomStoreApp', async () => {
const shopify = shopifyApi(
testConfig({
isCustomStoreApp: false,
adminApiAccessToken: 'dangit-another-access-token',
}),
);

const client = new shopify.clients.Graphql({session});
queueMockResponse(JSON.stringify(successResponse));

await expect(client.request(QUERY)).resolves.toEqual(
expect.objectContaining(successResponse),
);

const customHeaders: Record<string, string> = {};
customHeaders[ShopifyHeader.AccessToken] = accessToken;

expect({
method: 'POST',
domain,
path: `/admin/api/${shopify.config.apiVersion}/graphql.json`,
data: {query: QUERY},
headers: customHeaders,
}).toMatchMadeHttpRequest();
});

it('fails to instantiate without access token', () => {
const shopify = shopifyApi(testConfig());

Expand Down
27 changes: 27 additions & 0 deletions packages/apps/shopify-api/lib/clients/admin/__tests__/rest_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,33 @@ describe('REST client', () => {
}).toMatchMadeHttpRequest();
});

it('adapts to private app requests only if isCustomStoreApp', async () => {
const shopify = shopifyApi(
testConfig({
isCustomStoreApp: false,
adminApiAccessToken: 'test-admin-api-access-token',
}),
);

const client = new shopify.clients.Rest({session});

queueMockResponse(JSON.stringify(successResponse));

await expect(client.get({path: 'products'})).resolves.toEqual(
buildExpectedResponse(successResponse),
);

const customHeaders: Record<string, string> = {};
customHeaders[ShopifyHeader.AccessToken] = accessToken;

expect({
method: 'GET',
domain,
path: `/admin/api/${shopify.config.apiVersion}/products.json`,
headers: customHeaders,
}).toMatchMadeHttpRequest();
});

it('fails to instantiate without access token', () => {
const shopify = shopifyApi(testConfig());

Expand Down
8 changes: 7 additions & 1 deletion packages/apps/shopify-api/lib/clients/admin/graphql/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,16 @@ export class GraphqlClient {
logger(config).debug(message);
}

const customStoreAppAccessToken =
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a little confused on why this logic existed.

config.apiSecretKey = The apps private key
config.adminApiAccessToken = The apps access token for requests.

If we create a graphql client with the app private key, we would expect requests to fail.

Seems like in the past we had told folks when setting up a merchant custom app to put the access token = config.apiSecretKey. And then later added the config.adminApiAccessToken. And this logic is to handle folks that never migrated.

We could deprecate this logic, and require merchant custom apps to set both values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, indeed this has been deprecated

ed71298#diff-3bfd67f947f1e7d5cca6fba729bd99512b17d76b16303b6d00b235e1fb6a9237R91-R96

I think we should take this opportunity to deprecate it for good

config.adminApiAccessToken ?? config.apiSecretKey;

this.session = params.session;
this.apiVersion = params.apiVersion;

this.client = createAdminApiClient({
accessToken: config.adminApiAccessToken ?? this.session.accessToken!,
accessToken: config.isCustomStoreApp
? customStoreAppAccessToken
: this.session.accessToken!,
apiVersion: this.apiVersion ?? config.apiVersion,
storeDomain: this.session.shop,
customFetchApi: abstractFetch,
Expand Down
Loading