Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ba01d1d to
3b51cc5
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
There was a problem hiding this comment.
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 authby 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), |
There was a problem hiding this comment.
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.
| scopes: resolveGrantedScopes(tokenResponse, validationScopes), | |
| scopes: resolveGrantedScopes(tokenResponse, tokenResponse.scope ? validationScopes : scopes), |
| } from './session.js' | ||
| import type {StoredStoreAppSession} from './session.js' | ||
|
|
||
| async function refreshStoreToken(session: StoredStoreAppSession): Promise<StoredStoreAppSession> { |
There was a problem hiding this comment.
it'd be nice to split out the data fetching concern and the storage get/set concerns, wdyt?
There was a problem hiding this comment.
I agree, I'm gonna pursue more cleanup upstack. Extracting StoredSession in this PR felt like the right size for this particular move.
3b51cc5 to
aa2f837
Compare
aa2f837 to
eefef3a
Compare
| } | ||
|
|
||
| async function fetchCurrentStoreAuthScopes(session: StoredStoreAppSession): Promise<string[]> { | ||
| const endpoint = `https://${session.store}/admin/oauth/access_scopes.json` |
There was a problem hiding this comment.
This is legacy and we should probablyl use the GQL mutation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Can you check whether your token was an online access token/was actually going through PKCE
There was a problem hiding this comment.
(also we can't use the existing merge logic bc PKCE, like auth code grant, doesnt use decl scopes)
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Approving to unblock, but would like to double check that:
- 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
- we have proper coverage to ensure we do not get into auth loops
- we tophat this rigorously to work smoothly with refresh logic
eefef3a to
3892b28
Compare

What
Make
shopify store authpreserve 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 authcurrently 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
/admin/oauth/access_scopes.jsonstore authdoes not depend on the Admin GraphQL context module for generic session lifecycle behaviorConsidered
admin-graphql-context.ts: rejected. That made an Admin GraphQL module the accidental owner of generic stored-session lifecycle behavior.Testing
Measuring impact
How do we know this change was effective? Please choose one:
Checklist