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

Refresh theme auth #3375

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Refresh theme auth #3375

merged 6 commits into from
Feb 6, 2024

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Feb 5, 2024

WHY are these changes introduced?

Fixes #3173

Pushing Theme App Extension drafts requires unexpired Partners auth. Unfortunately, given the Ruby subprocess system we live with (for now), nothing is refreshing authentication, so sometime within ~2 hours the authentication expires, and the theme app extension silently stops reloading on save.

WHAT is this pull request doing?

  1. Instead of sending a Partners auth token Node => Ruby by CLI flag, we use the hidden theme token command to pass a Partners token and save in the local Shopify::DB. That token is utilized by the extension serve process because we no longer pass a token flag.
  2. The token is refreshed once before theme extension serve is called, and refreshed again every 3 minutes.
  3. If the token fails to refresh, we make 2 more attempts after a 30-second window. Failing that, we kill the dev process, because we don't want to pretend things are still working when they aren't.

Note this means that if the internet connection is down for a few minutes, dev will crash rather than recovering smoothly when the connection is restored. I'm not sure whether that's a concern or not.

How to test your changes?

  1. Start dev on an app with a Theme App Extension
  2. Wait several hours
  3. Make a change to a file in the theme and confirm the crash in [Bug]: shopify app dev stops authenticating development store preview #3173 no longer happens, instead the TAE updates successfully.

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

github-actions bot commented Feb 5, 2024

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** 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.

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
73.89% (-0.11% 🔻)
6693/9058
🟡 Branches
71.03% (-0.07% 🔻)
3288/4629
🟡 Functions
73.38% (-0.15% 🔻)
1750/2385
🟡 Lines
74.96% (-0.11% 🔻)
6312/8421
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / theme-app-extension.ts
37.93% (-33.5% 🔻)
22.22% (-27.78% 🔻)
25% (-41.67% 🔻)
37.04% (-32.19% 🔻)

Test suite run success

1602 tests passing in 746 suites.

Report generated by 🧪jest coverage report action from 0e156f0

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you, @amcaplan! LGTM and it works as expected* as well! 🚀 I believe we only need to fix the minor lint error in the CI, and then we will be ready to merge!

-

* In my tests, I noticed that the token we're setting via theme token is equal to the old one. However, because ensureAuthenticatedPartners is widely used, I assume that it sometimes returns the same token because it's still valid.

-

Update: I left my development server running for some hours, and it really refreshes the token 🚀

Copy link
Contributor

github-actions bot commented Feb 6, 2024

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
@@ -6,7 +6,7 @@ export interface AdminSession {
     token: string;
     storeFqdn: string;
 }
-interface EnsureAuthenticatedPartnersOptions {
+interface EnsureAuthenticatedAdditionalOptions {
     noPrompt?: boolean;
 }
 /**
@@ -19,7 +19,7 @@ interface EnsureAuthenticatedPartnersOptions {
  * @param options - Optional extra options to use.
  * @returns The access token for the Partners API.
  */
-export declare function ensureAuthenticatedPartners(scopes?: string[], env?: NodeJS.ProcessEnv, options?: EnsureAuthenticatedPartnersOptions): Promise<string>;
+export declare function ensureAuthenticatedPartners(scopes?: string[], env?: NodeJS.ProcessEnv, options?: EnsureAuthenticatedAdditionalOptions): Promise<string>;
 /**
  * Ensure that we have a valid session to access the Storefront API.
  *
@@ -35,9 +35,10 @@ export declare function ensureAuthenticatedStorefront(scopes?: string[], passwor
  * @param store - Store fqdn to request auth for.
  * @param scopes - Optional array of extra scopes to authenticate with.
  * @param forceRefresh - Optional flag to force a refresh of the token.
+ * @param options - Optional extra options to use.
  * @returns The access token for the Admin API.
  */
-export declare function ensureAuthenticatedAdmin(store: string, scopes?: string[], forceRefresh?: boolean): Promise<AdminSession>;
+export declare function ensureAuthenticatedAdmin(store: string, scopes?: string[], forceRefresh?: boolean, options?: EnsureAuthenticatedAdditionalOptions): 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.

@amcaplan
Copy link
Contributor Author

amcaplan commented Feb 6, 2024

@karreiro your instincts are correct! We will get the same token string until it expires.

@amcaplan amcaplan added this pull request to the merge queue Feb 6, 2024
Merged via the queue into main with commit ac50192 Feb 6, 2024
@amcaplan amcaplan deleted the refresh-theme-auth branch February 6, 2024 16:30
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.

[Bug]: shopify app dev stops authenticating development store preview
2 participants