Skip to content

Make store auth scopes additive#7172

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

Make store auth scopes additive#7172
dmerand merged 1 commit intomainfrom
dlm-store-auth-preserve-scopes

Conversation

@dmerand
Copy link
Copy Markdown
Contributor

@dmerand dmerand commented Apr 2, 2026

What

Make shopify store auth preserve existing app-install scopes when adding new access.

Before starting OAuth, CLI now resolves the current granted scope set from the store when it can, falls back to locally stored scopes when it can't, and merges the new requested scopes onto that set.

Why

store auth currently behaves like a replace operation.

That means an agent can request a narrower scope set for one task and accidentally remove scopes another agent, tab, or user was already relying on. Using local state alone also leaves us vulnerable to stale scope data when the install has already changed remotely.

How

  • reuse stored store auth to read the current app-install scopes from /admin/oauth/access_scopes.json
  • refresh the stored session first when needed by reusing the existing store auth loading path
  • treat remote scopes as the baseline when available
  • fall back to locally stored scopes if the remote lookup fails
  • treat that local fallback as non-authoritative so stale cached scopes do not block re-auth
  • extract stored session loading/refresh into a shared internal module so store auth does not depend on the Admin GraphQL context module for generic session lifecycle behavior

Considered

  • Require fallback local scopes to still be granted: rejected. If the cache is stale and remote scope lookup is unavailable, that can turn a recoverable re-auth into a hard failure.
  • Keep shared session loading in admin-graphql-context.ts: rejected. That made an Admin GraphQL module the accidental owner of generic stored-session lifecycle behavior.
  • Extract all scope-reconciliation policy into its own module in this PR: deferred. The current refactor fixes the misleading ownership seam without reopening the broader design.

Testing

pnpm run shopify store auth --store <shop.myshopify.com> --scopes read_orders
pnpm run shopify store auth --store <shop.myshopify.com> --scopes read_products
pnpm run shopify store execute --store <shop.myshopify.com> --query 'query { orders(first: 1) { nodes { id } } }'
pnpm run shopify store execute --store <shop.myshopify.com> --query 'query { products(first: 1) { nodes { id } } }'
# terminal A
pnpm run shopify store auth --store <shop.myshopify.com> --scopes read_orders

# terminal B / another checkout
pnpm run shopify store auth --store <shop.myshopify.com> --scopes read_customers

# back on terminal A
pnpm run shopify store auth --store <shop.myshopify.com> --scopes read_products
pnpm run shopify store execute --store <shop.myshopify.com> --query 'query { customers(first: 1) { nodes { id } } }'
pnpm run shopify store execute --store <shop.myshopify.com> --query 'query { products(first: 1) { nodes { id } } }'

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

Copy link
Copy Markdown
Contributor Author

dmerand commented Apr 2, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@dmerand dmerand force-pushed the dlm-store-auth-preserve-scopes branch 2 times, most recently from ba01d1d to 3b51cc5 Compare April 3, 2026 00:41
@dmerand dmerand marked this pull request as ready for review April 3, 2026 00:53
@dmerand dmerand requested a review from a team as a code owner April 3, 2026 00:53
Copilot AI review requested due to automatic review settings April 3, 2026 00:53
@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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR changes shopify store auth to behave additively by preserving existing app-install scopes when requesting additional scopes, preferring remote (store-reported) scopes when available and falling back to locally cached scopes when not.

Changes:

  • Added a shared stored-session loader/refresh module and reused it across store services.
  • Implemented scope reconciliation for store auth by merging newly requested scopes with existing scopes (remote-first, local fallback).
  • Added tests covering remote-scope resolution, fallback behavior, and additive scope requests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
packages/cli/src/cli/services/store/stored-session.ts New shared helper to load/refresh a stored store app session.
packages/cli/src/cli/services/store/auth.ts Resolves existing scopes (remote-first), merges requested scopes additively, and adjusts validation/persistence behavior.
packages/cli/src/cli/services/store/auth.test.ts Adds unit tests for scope resolution and additive auth behavior.
packages/cli/src/cli/services/store/admin-graphql-context.ts Refactors to use the shared stored-session loader instead of owning refresh logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// write_* -> read_* permissions as satisfied, so callers should not assume
// session.scopes is an expanded/effective permission set.
scopes: resolveGrantedScopes(tokenResponse, scopes),
scopes: resolveGrantedScopes(tokenResponse, validationScopes),
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

When tokenResponse.scope is missing, resolveGrantedScopes falls back to its requestedScopes argument. In the non-authoritative fallback path, you pass validationScopes (which excludes cached existing scopes), so a successful auth with a scope-less token response would persist only the newly requested scopes and unintentionally drop previously stored scopes. Consider validating against validationScopes when tokenResponse.scope is present, but falling back to the actual OAuth-requested scopes when it isn’t, so additive behavior is preserved even without a scope field.

Suggested change
scopes: resolveGrantedScopes(tokenResponse, validationScopes),
scopes: resolveGrantedScopes(tokenResponse, tokenResponse.scope ? validationScopes : scopes),

Copilot uses AI. Check for mistakes.
@dmerand dmerand mentioned this pull request Apr 3, 2026
5 tasks
} from './session.js'
import type {StoredStoreAppSession} from './session.js'

async function refreshStoreToken(session: StoredStoreAppSession): Promise<StoredStoreAppSession> {
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.

it'd be nice to split out the data fetching concern and the storage get/set concerns, wdyt?

Copy link
Copy Markdown
Contributor Author

@dmerand dmerand Apr 3, 2026

Choose a reason for hiding this comment

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

I agree, I'm gonna pursue more cleanup upstack. Extracting StoredSession in this PR felt like the right size for this particular move.

@dmerand dmerand force-pushed the dlm-store-auth-preserve-scopes branch from 3b51cc5 to aa2f837 Compare April 3, 2026 16:05
This was referenced Apr 3, 2026
@dmerand dmerand force-pushed the dlm-store-auth-preserve-scopes branch from aa2f837 to eefef3a Compare April 3, 2026 20:05
}

async function fetchCurrentStoreAuthScopes(session: StoredStoreAppSession): Promise<string[]> {
const endpoint = `https://${session.store}/admin/oauth/access_scopes.json`
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.

This is legacy and we should probablyl use the GQL mutation

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.

Actually, there might be a limitation whre the access tokens returned by PKCE today might not be able to invoke this as these tokens have limited access to certain features similar to delegate access tokens.

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.

Might be able to just leverage existing logic to fetch the remote app as well as existing merge logic. We'll need to be very intentional about why these paths diverge although we won't need all of the existing codepaths

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.

Might be able to just leverage existing logic to fetch the remote app as well as existing merge logic.

These operations are scoped to the global app. In this store-first case, where the app isn't referenced locally at all, wouldn't we want the operation to be store-scoped and therefore an independent path?

I'm not aware of the GraphQL queries on shop that might work, I'll check those out.

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.

Let's not use the term global app because it's super overloaded right now and misleading 🙏

100% agree w/ you long-term, but the CLI already authenticates itself and has access to these APIs right? This is a short-term client-side workaround that will be more reliable than building out a bespoke scope fetch + merge resolution, which has some nuances unfortunately.

I believ tehre is one called currentAppInstallation but it might a query when using the online tokens returned via PKCE bc tokens returend via PKCE have lmited accssa nd are in a lot of ways treated simila to delegate access tokens

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.

Can you check whether your token was an online access token/was actually going through PKCE

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.

(also we can't use the existing merge logic bc PKCE, like auth code grant, doesnt use decl scopes)

Copy link
Copy Markdown
Contributor Author

@dmerand dmerand Apr 6, 2026

Choose a reason for hiding this comment

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

(Agent summary of my ongoing tests:)

I dug into this from both sides: the CLI implementation and the shop-side GraphQL implementation in areas/core/shopify. On the shop side, currentAppInstallation resolves the currently authenticated app installation for the current shop, and AppInstallation.accessScopes is implemented/documented as the access scopes granted by the merchant during installation. Shopify’s own docs in core also explicitly map GET /admin/oauth/access_scopes.json to the GraphQL AccessScopeList operation, which is currentAppInstallation { accessScopes { handle } }. So semantically, this GraphQL query matches the REST endpoint we were previously using.

I also wanted to validate the token-type concern rather than assume it. I swapped the CLI scope lookup locally from /admin/oauth/access_scopes.json to currentAppInstallation { accessScopes { handle } } and then tophatted the real shopify store auth flow against donaldmerand.myshopify.com. These were real runs of the PKCE path, not synthetic requests: the logs showed the local PKCE callback server, browser redirect, auth code receipt, and code exchange at /admin/oauth/access_token using code_verifier. The token response included associated_user and expires_in, so this was consistent with the online/user-bound token used by this flow.

Then I exercised the stored-scope cases we were worried about. I tested: (1) no stored auth, (2) stale local scopes with a valid stored token, (3) stale local scopes with an invalid stored token to force the non-blocking local fallback path, and (4) expired stored token with a valid refresh token to ensure refresh happened before scope lookup. In the valid-token and refresh cases, the GraphQL currentAppInstallation query succeeded and returned the current granted scopes. In the invalid-token case, the GraphQL lookup failed, the CLI fell back to local stored scopes, and auth still completed successfully. That gave us coverage for the main “delayed fuse” concern around refresh and stale scope handling.

My conclusion is that currentAppInstallation.accessScopes is the right store-scoped source for this CLI flow, and it does work with the PKCE-derived online token used by shopify store auth. The important distinction is that this returns the current granted scopes on the shop installation, whereas app-level surfaces like requestedAccessScopes reflect the app’s requested/deployed config. For additive store auth, the former is the behavior we want.

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.

online tokens acquired via PKCE are "narrowed" and treated similar to delegate access tokens --> should be blocked by currentAppInstallation query.. are we sure we weren't using an offline access token? In the short-term this might be the right approach, to use the offline access tokento query and do scope resolution BUT use the narrowed, online token via PKCE for API requests

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 don’t think we were accidentally using a classic offline token here. The tophat was definitely the PKCE auth-code flow (local callback server, auth code exchange with code_verifier, no client secret), and the response was clearly user-bound/narrowed: it included associated_user, expires_in, and was stored under a specific userId. I also verified the refreshed version of that same stored session could query currentAppInstallation.

So while the connector-app PKCE token shape may not line up perfectly with the older online/offline terminology, the currentAppInstallation worked with the token shape this CLI flow actually uses. But we can explore the offline token approach as well.

Copy link
Copy Markdown
Contributor

@RyanDJLee RyanDJLee left a comment

Choose a reason for hiding this comment

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

Approving to unblock, but would like to double check that:

  1. the access_scopes json returns the expected data and we have behavioural strong coverage per our use cases here even if it has coverage in core
  2. we have proper coverage to ensure we do not get into auth loops
  3. we tophat this rigorously to work smoothly with refresh logic

@dmerand dmerand force-pushed the dlm-store-auth-preserve-scopes branch from eefef3a to 3892b28 Compare April 6, 2026 13:52
@dmerand dmerand added this pull request to the merge queue Apr 6, 2026
Merged via the queue into main with commit b576023 Apr 6, 2026
27 checks passed
@dmerand dmerand deleted the dlm-store-auth-preserve-scopes branch April 6, 2026 14:06
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.

4 participants