Skip to content

Store auth restructure#7187

Merged
dmerand merged 1 commit intomainfrom
dlm-store-auth-restructure
Apr 6, 2026
Merged

Store auth restructure#7187
dmerand merged 1 commit intomainfrom
dlm-store-auth-restructure

Conversation

@dmerand
Copy link
Copy Markdown
Contributor

@dmerand dmerand commented Apr 3, 2026

What

Restructure shopify store auth under packages/cli/src/cli/services/store/auth/ and split the main auth responsibilities into explicit auth-domain seams.

This keeps the command contract the same while making auth ownership easier to review and build on.

Why

The store auth fast-follow landed quickly and left auth responsibilities spread across a few files with blurry ownership.

That made it harder to reason about:

  • local session bucket storage
  • expiry / refresh / invalidation policy
  • auth-related HTTP
  • current-user selection semantics

It also made the execute-side follow-up harder to stage cleanly.

The goal here is to make the auth domain legible on its own before building the execute-side cleanup on top.

How

This PR moves auth code under services/store/auth/ and separates:

  • auth/index.ts — auth orchestration
  • auth/session-store.ts — local session bucket storage and current-user selection
  • auth/session-lifecycle.ts — expiry / refresh / invalidation policy
  • auth/token-client.ts — auth-related HTTP
  • auth/config.ts / auth/recovery.ts — shared auth support code

It also:

  • renames the stored-session getter to getCurrentStoredStoreAppSession(...) so current-user semantics are explicit
  • updates existing auth consumers to use the new auth seams
  • keeps malformed local session state from leaking past the storage boundary

High-level ownership after this PR:

command -> auth/index.ts -> session-store / session-lifecycle / token-client

This PR is intentionally auth-only. The execute-side folder move and full execute-context cleanup stay in the next stacked PR.

Testing

Manual checks:

pnpm run shopify store auth --store donaldmerand.myshopify.com --scopes read_products
pnpm run shopify store execute --store donaldmerand.myshopify.com --query 'query { shop { name id } }'
node packages/cli/bin/run.js store auth --help
node packages/cli/bin/run.js store execute --help

Verify that:

  • store auth still authenticates and persists store auth successfully
  • store execute still reuses stored auth successfully
  • help/runtime wiring still works after the auth file move

Considered

  • Fold execute into this PR: deferred so auth ownership can be reviewed on its own.
  • Introduce generic persistence / transport folders: rejected in favor of domain-first store/auth ownership.
  • Keep the old stored-session getter naming: renamed now so current-user semantics are explicit before we build on them.

Measuring impact

  • 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

Copy link
Copy Markdown
Contributor Author

dmerand commented Apr 3, 2026

@dmerand dmerand force-pushed the dlm-store-auth-preserve-scopes branch from aa2f837 to eefef3a Compare April 3, 2026 20:05
@dmerand dmerand force-pushed the dlm-store-auth-restructure branch from ec9fc5b to 176cf3b Compare April 3, 2026 20:05
@dmerand dmerand marked this pull request as ready for review April 3, 2026 20:48
@dmerand dmerand requested a review from a team as a code owner April 3, 2026 20:48
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

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.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

@dmerand dmerand force-pushed the dlm-store-auth-restructure branch from 176cf3b to 81f77d7 Compare April 3, 2026 20:50
@@ -0,0 +1,132 @@
import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

total nit from me: i am very against index files. they obscure intent and become a dumping ground, plus barrel files cause circular deps and tree shaking issues in many cases

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair nit. I’m using index.ts here as the auth module entrypoint/orchestrator rather than as a barrel export, so I’m inclined not to rename it just for this PR. If it starts turning into a grab-bag again, I’d rather fix that by tightening the module boundary than by just changing the filename.

onListening?: () => void | Promise<void>
}

function renderAuthCallbackPage(title: string, message: string): string {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: have a views dir?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have one view right now and I didn't want this refactor to go too far. I'm not sure if our domain needs "views" yet. But I'm noting for posterity that I would be aligned with adding this when our views increase here.

settleWithError(new AbortError('Timed out waiting for OAuth callback.'))
}, timeoutMs)

const server = createServer((req, res) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

odd to create a dedicated server as a closure in a callback, no?

settleWithError(new AbortError('Timed out waiting for OAuth callback.'))
}, timeoutMs)

const server = createServer((req, res) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

odd to create a dedicated server as a closure in a callback, no?

Copy link
Copy Markdown
Contributor

@ryancbahan ryancbahan left a comment

Choose a reason for hiding this comment

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

I like the direction of this pr, but the seams still seem a little unclear to me. I wonder if we can invert the order of initialization a bit -- especially in callback and pkce files -- so that we first initialize the server explicitly, initialize the persistence stores, establish the dynamic route(s) that need to be hit, and then let the callback chain(s) flow.

success: (store: string, email?: string) => void
}

interface StoreAuthDependencies {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know that the DI pattern here is particularly serving us

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's fair, it's unique to this flow and somewhat unusual in the codebase. It wasn't bothering me in the original implementation because the flow is bounded, but this version of the implementation isn't necessarily as clear. I'll see what the alternative looks like.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've adjusted the implementation to consolidate the DI object + make it authoritative. That'll make it easier to refactor out later, which I'm not opposed to doing.

@dmerand dmerand changed the base branch from dlm-store-auth-preserve-scopes to graphite-base/7187 April 6, 2026 13:52
@dmerand
Copy link
Copy Markdown
Contributor Author

dmerand commented Apr 6, 2026

I like the direction of this pr, but the seams still seem a little unclear to me. I wonder if we can invert the order of initialization a bit -- especially in callback and pkce files -- so that we first initialize the server explicitly, initialize the persistence stores, establish the dynamic route(s) that need to be hit, and then let the callback chain(s) flow.

I looked at whether there was a smaller callback extraction that would materially clarify ownership here without turning into a larger redesign. The only bounded version I found was splitting server lifecycle from callback request handling, but it didn’t feel like enough improvement to justify more churn in this PR after the other seam cleanups. So I’d keep the larger callback/pkce lifecycle inversion as follow-up work rather than half-landing it here.

@dmerand dmerand requested a review from ryancbahan April 6, 2026 15:05
@dmerand dmerand force-pushed the dlm-store-auth-restructure branch from 81f77d7 to d2680ce Compare April 6, 2026 17:09
@dmerand dmerand force-pushed the graphite-base/7187 branch from eefef3a to b38f61c Compare April 6, 2026 17:09
@dmerand dmerand changed the base branch from graphite-base/7187 to main April 6, 2026 17:10
@dmerand dmerand added this pull request to the merge queue Apr 6, 2026
Merged via the queue into main with commit 11bd780 Apr 6, 2026
25 of 47 checks passed
@dmerand dmerand deleted the dlm-store-auth-restructure branch April 6, 2026 17:23
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