Skip to content

Make store auth scopes additive#7172

Open
dmerand wants to merge 1 commit intomainfrom
dlm-store-auth-preserve-scopes
Open

Make store auth scopes additive#7172
dmerand wants to merge 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.
Comment on lines +138 to +141
const body = await response.text()
if (!response.ok) {
throw new Error(`HTTP ${response.status}: ${body || response.statusText}`)
}
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.

On non-OK responses, the thrown error includes the full response body (HTTP ${status}: ${body || statusText}), which is later surfaced in debug logs during the fallback path. This can produce extremely large debug output (HTML error pages, etc.). Consider truncating/sanitizing the body similarly to the token-refresh path (e.g., first N chars) while still retaining status and a small snippet for troubleshooting.

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
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.

3 participants