-
Notifications
You must be signed in to change notification settings - Fork 218
Use ensureAuthenticatedAdminAsApp in shopify app execute
#6643
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
Use ensureAuthenticatedAdminAsApp in shopify app execute
#6643
Conversation
ensureAuthenticatedAdminAsApp for Bulk Operations
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3406 tests passing in 1386 suites. Report generated by 🧪jest coverage report action from 02b54ed |
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
nickwesselman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like both CLI and someone w/ knowledge of app access to confirm this approach but this meets the Product requirements 👍🏻
2c2b13c to
691d000
Compare
f5e87ce to
0e7b765
Compare
204edc8 to
028510c
Compare
0e7b765 to
53eb2f0
Compare
028510c to
c6078b1
Compare
53eb2f0 to
06be306
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/session.d.ts@@ -97,6 +97,17 @@ export declare function ensureAuthenticatedStorefront(scopes?: StorefrontRendere
* @returns The access token for the Admin API.
*/
export declare function ensureAuthenticatedAdmin(store: string, scopes?: AdminAPIScope[], options?: EnsureAuthenticatedAdditionalOptions): Promise<AdminSession>;
+/**
+ * Ensure that we have a valid Admin session for the given store, acting on behalf of the app.
+ *
+ * This will fail if the app has not already been installed.
+ *
+ * @param storeFqdn - Store fqdn to request auth for.
+ * @param apiKey - API key for the app.
+ * @param apiSecret - API secret for the app.
+ * @returns The access token for the Admin API.
+ */
+export declare function ensureAuthenticatedAdminAsApp(storeFqdn: string, apiKey: string, apiSecret: string): Promise<AdminSession>;
/**
* Ensure that we have a valid session to access the Theme API.
* If a password is provided, that token will be used against Theme Access API.
|
RyanDJLee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes make sense to me, but let's make sure we do not expose the access token before we ship.
f9b53e2 to
e9bb7f5
Compare
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const apiSecret = remoteApp.apiSecretKeys.find((elm) => elm.secret)!.secret | ||
| const adminSession = await ensureAuthenticatedAdminAsApp(storeFqdn, app.configuration.client_id, apiSecret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What actually is a remote app? It seems odd to me to set up a session using the client ID from app but the API secret from remoteApp... but I'm guessing this is correct for some complicated reason. Can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see this from the PR desc:
Both values are also available on remoteApp as title and apiKey. Could be simplified in the future to only use remoteApp for consistency.
Maybe we should adjust it to use remoteApp all the way through for consistency and to minimize confusion, if possible? How big of a lift is that you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it for consistency. All other CLI's used app. Didn't seem like it would hurt to keep it in there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remoteApp is the remote representation of the App obtained via the API. The secret can only be obtained from the remoteApp, is not available in the "local" app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use remoteApp to access the client_id value too yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed app and using remoteApp for everything.
jordanverasamy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that this is all following the directions from app access, this PR seems reasonable to me, but I don't currently understand deeply enough to have a meaningful ✅ .
e9bb7f5 to
a0b57c8
Compare
Merge activity
|
a0b57c8 to
386074f
Compare
386074f to
479bccb
Compare
gonzaloriestra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to say that ensureAuthenticatedAdminAsApp was already added by #6649, but I guess you already realized, no?
The description is outdated, but the changes make sense.
packages/app/src/cli/services/bulk-operations/execute-bulk-operation.ts
Outdated
Show resolved
Hide resolved
479bccb to
8fc9ddf
Compare
|
Yeah, sorry for the outdated description -- I'll update at least the title so it'll be clearer to future readers what this PR is actually doing. Thanks for the quick review! |
ensureAuthenticatedAdminAsApp for Bulk OperationsensureAuthenticatedAdminAsApp in shopify app execute
41d68b3 to
02b54ed
Compare

Resolves: https://github.com/orgs/shop/projects/208/views/34?pane=issue&itemId=140363951&issue=shop%7Cissues-api-foundations%7C1128
Inspired by: #6596 (comment)
Background
The bulk operations execute command currently authenticates using the CLI user's credentials, which has two critical limitations:
This prevents bulk operations from working properly with app-owned data.
Solution
Implemented ensureAuthenticatedAdminAsApp() that authenticates as the app using OAuth 2.0 client credentials grant. This provides:
This idea originally came from a hack days.
EDIT: Actually #6649 beat us to it! This PR now just adjusts
app executeto use this feature, and doesn't have to implement it itself.Implementation Notes
webhook/trigger-options.ts.appandremoteAppare passed to executeBulkOperation. Theappparameter is only used for:Both values are also available on
remoteAppastitleandapiKey. Could be simplified in the future to only useremoteAppfor consistency.Testing
To test with this new authentification, you must install the app on the store. You can do this by:
Then, we can use the existing command works unchanged:
For queries:
For mutations:
Now authenticates as the app instead of the CLI user, enabling access to app-scoped resources.