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

Unify token exchange for app managament client #5339

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

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Feb 3, 2025

WHY are these changes introduced?

Consolidates authentication flows for App Management and Business Platform APIs into a single function call, reducing duplicate authentication requests and improving performance.

Previously, by having two different calls to authenticate (1 for BP and 1 for AppManagement) we were triggering the whole identity token validation flow twice, now only once.

✅ This change saves up to 0.5 seconds on each command.

WHAT is this pull request doing?

  • Introduces a new ensureAuthenticatedAppManagementAndBusinessPlatform function that handles authentication for both APIs simultaneously
  • Updates the AppManagementClient to use the new consolidated authentication method
  • Ensures proper token management and session handling for both platforms (BP token is stored in session now)
  • Improves token refresh logic to handle both API tokens

How to test your changes?

  1. Delete any previous session (pnpm shopify auth logout)
  2. Run any CLI command that requires authentication
  3. Verify that authentication works for both App Management and Business Platform APIs

If you want to dig further, you can run in debug and stop at the "token introspection" function. See that it should only run that once per command and not twice as it is now.

Measuring impact

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

isaacroldan commented Feb 3, 2025

Copy link
Contributor

github-actions bot commented Feb 3, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.51% (+0.06% 🔼)
9016/11940
🟡 Branches
70.8% (+0.16% 🔼)
4403/6219
🟡 Functions
75.29% (+0.03% 🔼)
2365/3141
🟡 Lines
76.04% (+0.07% 🔼)
8518/11202
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
95.45% 100%

Test suite run success

2037 tests passing in 910 suites.

Report generated by 🧪jest coverage report action from 2a032b1

@isaacroldan isaacroldan changed the base branch from command-startup-performance-improvements to main February 3, 2025 12:12
@isaacroldan isaacroldan force-pushed the 02-03-unify_token_exchange_for_app_managament_client branch from 31782b2 to f90616c Compare February 3, 2025 12:17
@isaacroldan isaacroldan force-pushed the 02-03-unify_token_exchange_for_app_managament_client branch from f90616c to ddad615 Compare February 3, 2025 12:41
@isaacroldan isaacroldan force-pushed the 02-03-unify_token_exchange_for_app_managament_client branch from ddad615 to ec9bac7 Compare February 3, 2025 12:58
@isaacroldan isaacroldan marked this pull request as ready for review February 3, 2025 13:08
@isaacroldan isaacroldan requested a review from a team as a code owner February 3, 2025 13:08
Copy link
Contributor

github-actions bot commented Feb 3, 2025

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@isaacroldan isaacroldan force-pushed the 02-03-unify_token_exchange_for_app_managament_client branch from 0cdb3fa to 2a032b1 Compare February 3, 2025 14:34
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Differences in type declarations

We 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:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/session.d.ts
@@ -26,14 +26,16 @@ export declare function ensureAuthenticatedPartners(scopes?: PartnersAPIScope[],
 /**
  * Ensure that we have a valid session to access the App Management API.
  *
- * @param scopes - Optional array of extra scopes to authenticate with.
- * @param env - Optional environment variables to use.
  * @param options - Optional extra options to use.
+ * @param appManagementScopes - Optional array of extra scopes to authenticate with.
+ * @param businessPlatformScopes - Optional array of extra scopes to authenticate with.
+ * @param env - Optional environment variables to use.
  * @returns The access token for the App Management API.
  */
-export declare function ensureAuthenticatedAppManagement(scopes?: AppManagementAPIScope[], env?: NodeJS.ProcessEnv, options?: EnsureAuthenticatedAdditionalOptions): Promise<{
-    token: string;
+export declare function ensureAuthenticatedAppManagementAndBusinessPlatform(options?: EnsureAuthenticatedAdditionalOptions, appManagementScopes?: AppManagementAPIScope[], businessPlatformScopes?: BusinessPlatformScope[], env?: NodeJS.ProcessEnv): Promise<{
+    appManagementToken: string;
     userId: string;
+    businessPlatformToken: string;
 }>;
 /**
  * Ensure that we have a valid session to access the Storefront API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants