Skip to content

Refactor identity client code execution #6654

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.

Sign up for GitHub

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

Open
alexanderMontague wants to merge 1 commit into
base: 11-20-add_option_to_bypass_running_service_check_if_configured
Choose a base branch
from
Open

Refactor identity client code execution #6654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions packages/cli-kit/src/private/node/clients/identity/identity-client.ts
View file
Open in desktop
Copy link
Contributor Author

@alexanderMontague alexanderMontague Nov 21, 2025

Choose a reason for hiding this comment

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

I've moved only flows pertaining to network requests specifically to the client to keep it super lightweight. Shared modules in session/* are then pulled in to existing flows as needed, with the client only being used for the token exchanges/fetches

Copy link
Contributor

@MitchDickinson MitchDickinson Nov 24, 2025

Choose a reason for hiding this comment

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

IMO thinning out the client here weakens the abstraction:

  • There's code in sesssion/exchange that is actually part of the service client "stack" but isn't part of the service client (i.e. polling code that doesn't execute if you are using the mock client)
  • There's more temporal coupling going on (call this method, then that method).

Light weight isn't necessarily good in this context - I think the "right weight" (ahaha rhyming) is what we want, where the implementation code that "belongs" behind the interface lives behind the interface.

Having an abstraction just over the network part is better than having none (we do need the local dev solution!), but my gut here is that the circular dependency issue led us to making other changes that weakened the rest of the architecture you had established.

I strongly suspect there would be another solution to the circular dependency issue that would still allow us to keep the higher level interface with more encapsulated code behind the interface. My guess is there would be even more things we could pull behind the interface as a way to fix the circular dependency but I can't be sure without reviewing what the circular relationship actually was (is this written up somewhere I can 👀 ?)

Original file line number Diff line number Diff line change
@@ -1,18 +1,31 @@
import {IdentityToken} from '../../session/schema.js'
import {TokenRequestResult} from '../../session/exchange.js'
import {API} from '../../api.js'
import {Environment, serviceEnvironment} from '../../context/service.js'
import {BugError} from '../../../../public/node/error.js'
import {Result} from '../../../../public/node/result.js'

export abstract class IdentityClient {
abstract requestAccessToken(scopes: string[]): Promise<IdentityToken>
export interface TokenRequestResult {
access_token: string
expires_in: number
refresh_token: string
scope: string
id_token?: string
}

export interface DeviceAuthorizationResponse {
deviceCode: string
userCode: string
verificationUri: string
expiresIn: number
verificationUriComplete?: string
interval?: number
}

export abstract class IdentityClient {
abstract tokenRequest(params: {
[key: string]: string
}): Promise<Result<TokenRequestResult, {error: string; store?: string}>>

abstract refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken>
abstract requestDeviceAuthorization(scopes: string[]): Promise<DeviceAuthorizationResponse>

abstract clientId(): string
Copy link
Contributor

@MitchDickinson MitchDickinson Nov 24, 2025

Choose a reason for hiding this comment

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

clientId() doesn't need to be abstract here. Instead, we should leave it as a class-specific implementation for the mock/production clients and then fix the temporal coupling issues related to it by changing the interface


Expand Down
62 changes: 23 additions & 39 deletions packages/cli-kit/src/private/node/clients/identity/identity-mock-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,62 +1,39 @@
import {IdentityClient} from './identity-client.js'
import {ApplicationToken, IdentityToken} from '../../session/schema.js'
import {ExchangeScopes, TokenRequestResult} from '../../session/exchange.js'
import {IdentityClient, type TokenRequestResult, type DeviceAuthorizationResponse} from './identity-client.js'
import {ok, Result} from '../../../../public/node/result.js'
import {allDefaultScopes} from '../../session/scopes.js'

export class IdentityMockClient extends IdentityClient {
private readonly mockUserId = '08978734-325e-44ce-bc65-34823a8d5180'
private readonly authTokenPrefix = 'mtkn_'

async requestAccessToken(_scopes: string[]): Promise<IdentityToken> {
const tokens = this.generateTokens('identity')

async requestDeviceAuthorization(_scopes: string[]): Promise<DeviceAuthorizationResponse> {
return Promise.resolve({
accessToken: tokens.accessToken,
alias: '',
expiresAt: this.getFutureDate(1),
refreshToken: tokens.refreshToken,
scopes: allDefaultScopes(),
userId: this.mockUserId,
deviceCode: 'mock_device_code',
userCode: 'MOCK-CODE',
verificationUri: 'https://identity.shop.dev/device',
expiresIn: 600,
verificationUriComplete: 'https://identity.shop.dev/device?code=MOCK-CODE',
interval: 5,
})
}

async exchangeAccessForApplicationTokens(
_identityToken: IdentityToken,
_scopes: ExchangeScopes,
_store?: string,
): Promise<{[x: string]: ApplicationToken}> {
return {
[this.applicationId('app-management')]: this.generateTokens(this.applicationId('app-management')),
[this.applicationId('business-platform')]: this.generateTokens(this.applicationId('business-platform')),
[this.applicationId('admin')]: this.generateTokens(this.applicationId('admin')),
[this.applicationId('partners')]: this.generateTokens(this.applicationId('partners')),
[this.applicationId('storefront-renderer')]: this.generateTokens(this.applicationId('storefront-renderer')),
}
}

async tokenRequest(params: {
[key: string]: string
}): Promise<Result<TokenRequestResult, {error: string; store?: string}>> {
const tokens = this.generateTokens(params?.audience ?? '')
const idTokenPayload = {
sub: this.mockUserId,
aud: params?.audience ?? 'identity',
iss: 'https://identity.shop.dev',
exp: this.getCurrentUnixTimestamp() + 7200,
iat: this.getCurrentUnixTimestamp(),
}
return ok({
access_token: tokens.accessToken,
expires_in: this.getFutureDate(1).getTime(),
refresh_token: tokens.refreshToken,
scope: allDefaultScopes().join(' '),
})
}

async refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken> {
const tokens = this.generateTokens('identity')

return Promise.resolve({
accessToken: tokens.accessToken,
alias: 'dev@shopify.com',
expiresAt: this.getFutureDate(1),
refreshToken: tokens.refreshToken,
scopes: allDefaultScopes(),
userId: this.mockUserId,
id_token: this.generateMockJWT(idTokenPayload),
})
}

Expand Down Expand Up @@ -118,4 +95,11 @@ export class IdentityMockClient extends IdentityClient {
.replace(/\+/g, '-')
.replace(/\//g, '_')
}

private generateMockJWT(payload: object): string {
const header = {alg: 'none', typ: 'JWT'}
const encodedHeader = this.encodeTokenPayload(header)
const encodedPayload = this.encodeTokenPayload(payload)
return `${encodedHeader}.${encodedPayload}.`
}
}
101 changes: 3 additions & 98 deletions packages/cli-kit/src/private/node/clients/identity/identity-service-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,16 @@
import {IdentityClient} from './identity-client.js'
import {IdentityToken} from '../../session/schema.js'
import {
buildIdentityToken,
exchangeDeviceCodeForAccessToken,
tokenRequestErrorHandler,
TokenRequestResult,
} from '../../session/exchange.js'
import {IdentityClient, type TokenRequestResult, type DeviceAuthorizationResponse} from './identity-client.js'
import {outputContent, outputDebug, outputInfo, outputToken} from '../../../../public/node/output.js'
import {AbortError, BugError} from '../../../../public/node/error.js'
import {identityFqdn} from '../../../../public/node/context/fqdn.js'
import {shopifyFetch} from '../../../../public/node/http.js'
import {isCI, openURL} from '../../../../public/node/system.js'
import {isCloudEnvironment} from '../../../../public/node/context/local.js'
import {isTTY, keypress} from '../../../../public/node/ui.js'
import {
buildAuthorizationParseErrorMessage,
convertRequestToParams,
type DeviceAuthorizationResponse,
} from '../../session/device-authorization.js'
import {buildAuthorizationParseErrorMessage, convertRequestToParams} from '../../session/device-authorization.js'
import {err, ok, Result} from '../../../../public/node/result.js'
import {Environment, serviceEnvironment} from '../../context/service.js'

export class IdentityServiceClient extends IdentityClient {
async requestAccessToken(scopes: string[]): Promise<IdentityToken> {
// Request a device code to authorize without a browser redirect.
outputDebug(outputContent`Requesting device authorization code...`)
const deviceAuth = await this.requestDeviceAuthorization(scopes)

// Poll for the identity token
outputDebug(outputContent`Starting polling for the identity token...`)
const identityToken = await this.pollForDeviceAuthorization(deviceAuth.deviceCode, deviceAuth.interval)
return identityToken
}

async tokenRequest(params: {
[key: string]: string
}): Promise<Result<TokenRequestResult, {error: string; store?: string}>> {
Expand All @@ -49,22 +27,6 @@ export class IdentityServiceClient extends IdentityClient {
return err({error: payload.error, store: params.store})
}

/**
* Given an expired access token, refresh it to get a new one.
*/
async refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken> {
const clientId = this.clientId()
const params = {
grant_type: 'refresh_token',
access_token: currentToken.accessToken,
refresh_token: currentToken.refreshToken,
client_id: clientId,
}
const tokenResult = await this.tokenRequest(params)
const value = tokenResult.mapError(tokenRequestErrorHandler).valueOrBug()
return buildIdentityToken(value, currentToken.userId, currentToken.alias)
}

clientId(): string {
const environment = serviceEnvironment()
if (environment === Environment.Local) {
Expand All @@ -76,12 +38,6 @@ export class IdentityServiceClient extends IdentityClient {
}
}

/**
* ========================
* Private Instance Methods
* ========================
*/

/**
* Initiate a device authorization flow.
* This will return a DeviceAuthorizationResponse containing the URL where user
Expand All @@ -92,7 +48,7 @@ export class IdentityServiceClient extends IdentityClient {
* @param scopes - The scopes to request
* @returns An object with the device authorization response.
*/
private async requestDeviceAuthorization(scopes: string[]): Promise<DeviceAuthorizationResponse> {
async requestDeviceAuthorization(scopes: string[]): Promise<DeviceAuthorizationResponse> {
const fqdn = await identityFqdn()
const identityClientId = this.clientId()
const queryParams = {client_id: identityClientId, scope: scopes.join(' ')}
Expand Down Expand Up @@ -169,55 +125,4 @@ export class IdentityServiceClient extends IdentityClient {
interval: jsonResult.interval,
}
}

/**
* Poll the Oauth token endpoint with the device code obtained from a DeviceAuthorizationResponse.
* The endpoint will return `authorization_pending` until the user completes the auth flow in the browser.
* Once the user completes the auth flow, the endpoint will return the identity token.
*
* Timeout for the polling is defined by the server and is around 600 seconds.
*
* @param code - The device code obtained after starting a device identity flow
* @param interval - The interval to poll the token endpoint
* @returns The identity token
*/
private async pollForDeviceAuthorization(code: string, interval = 5): Promise<IdentityToken> {
Copy link
Contributor

@MitchDickinson MitchDickinson Nov 24, 2025

Choose a reason for hiding this comment

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

This feels like a loss to me - this code is directly related to the service client and the encapsulation was nice IMO

let currentIntervalInSeconds = interval

return new Promise<IdentityToken>((resolve, reject) => {
const onPoll = async () => {
const result = await exchangeDeviceCodeForAccessToken(code)
if (!result.isErr()) {
resolve(result.value)
return
}

const error = result.error ?? 'unknown_failure'

outputDebug(outputContent`Polling for device authorization... status: ${error}`)
switch (error) {
case 'authorization_pending': {
startPolling()
return
}
case 'slow_down':
currentIntervalInSeconds += 5
startPolling()
return
case 'access_denied':
case 'expired_token':
case 'unknown_failure': {
reject(new Error(`Device authorization failed: ${error}`))
}
}
}

const startPolling = () => {
// eslint-disable-next-line @typescript-eslint/no-misused-promises
setTimeout(onPoll, currentIntervalInSeconds * 1000)
}

startPolling()
})
}
}
16 changes: 8 additions & 8 deletions packages/cli-kit/src/private/node/session.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
exchangeAccessForApplicationTokens,
exchangeCustomPartnerToken,
refreshAccessToken,
requestAccessToken,
InvalidGrantError,
} from './session/exchange.js'
import {allDefaultScopes} from './session/scopes.js'
Expand Down Expand Up @@ -127,6 +128,7 @@ beforeEach(() => {
vi.spyOn(fqdnModule, 'identityFqdn').mockResolvedValue(fqdn)
vi.mocked(exchangeAccessForApplicationTokens).mockResolvedValue(appTokens)
vi.mocked(refreshAccessToken).mockResolvedValue(validIdentityToken)
vi.mocked(requestAccessToken).mockResolvedValue(validIdentityToken)
vi.mocked(exchangeCustomPartnerToken).mockResolvedValue({
accessToken: partnersToken.accessToken,
userId: validIdentityToken.userId,
Expand All @@ -145,8 +147,6 @@ beforeEach(() => {

vi.mocked(getIdentityClient).mockImplementation(() => mockIdentityClient)
vi.spyOn(mockIdentityClient, 'applicationId').mockImplementation((app) => app)
vi.spyOn(mockIdentityClient, 'refreshAccessToken').mockResolvedValue(validIdentityToken)
vi.spyOn(mockIdentityClient, 'requestAccessToken').mockResolvedValue(validIdentityToken)
})

describe('ensureAuthenticated when previous session is invalid', () => {
Expand Down Expand Up @@ -340,7 +340,7 @@ describe('when existing session is valid', () => {
const got = await ensureAuthenticated(defaultApplications, process.env, {forceRefresh: true})

// Then
expect(mockIdentityClient.refreshAccessToken).toBeCalled()
expect(refreshAccessToken).toBeCalled()
expect(exchangeAccessForApplicationTokens).toBeCalled()
expect(storeSessions).toBeCalledWith(validSessions)
expect(got).toEqual(validTokens)
Expand All @@ -360,7 +360,7 @@ describe('when existing session is expired', () => {
const got = await ensureAuthenticated(defaultApplications)

// Then
expect(mockIdentityClient.refreshAccessToken).toBeCalled()
expect(refreshAccessToken).toBeCalled()
expect(exchangeAccessForApplicationTokens).toBeCalled()
expect(storeSessions).toBeCalledWith(validSessions)
expect(got).toEqual(validTokens)
Expand All @@ -375,13 +375,13 @@ describe('when existing session is expired', () => {

vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
vi.mocked(fetchSessions).mockResolvedValue(validSessions)
vi.spyOn(mockIdentityClient, 'refreshAccessToken').mockRejectedValueOnce(tokenResponseError)
vi.mocked(refreshAccessToken).mockRejectedValueOnce(tokenResponseError)

// When
const got = await ensureAuthenticated(defaultApplications)

// Then
expect(mockIdentityClient.refreshAccessToken).toBeCalled()
expect(refreshAccessToken).toBeCalled()
expect(exchangeAccessForApplicationTokens).toBeCalled()
expect(businessPlatformRequest).toHaveBeenCalled()
expect(storeSessions).toHaveBeenCalledOnce()
Expand Down Expand Up @@ -665,8 +665,8 @@ describe('ensureAuthenticated email fetch functionality', () => {
const tokenResponseError = new InvalidGrantError()
vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
vi.mocked(fetchSessions).mockResolvedValue(validSessions)
vi.spyOn(mockIdentityClient, 'refreshAccessToken').mockRejectedValueOnce(tokenResponseError)
vi.spyOn(mockIdentityClient, 'requestAccessToken').mockResolvedValueOnce(validIdentityToken)
vi.mocked(refreshAccessToken).mockRejectedValueOnce(tokenResponseError)
vi.mocked(requestAccessToken).mockResolvedValueOnce(validIdentityToken)
vi.mocked(businessPlatformRequest).mockResolvedValueOnce({
currentUserAccount: {
email: 'dev@shopify.com',
Expand Down
7 changes: 4 additions & 3 deletions packages/cli-kit/src/private/node/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import {allDefaultScopes, apiScopes} from './session/scopes.js'
import {
exchangeAccessForApplicationTokens,
exchangeCustomPartnerToken,
requestAccessToken,
refreshAccessToken,
ExchangeScopes,
InvalidGrantError,
InvalidRequestError,
Expand Down Expand Up @@ -288,7 +290,6 @@ The CLI is currently unable to prompt for reauthentication.`,
* Execute the full authentication flow.
*
* @param applications - An object containing the applications we need to be authenticated with.
* @param alias - Optional alias to use for the session.
*/
async function executeCompleteFlow(applications: OAuthApplications): Promise<Session> {
const scopes = getFlattenScopes(applications)
Expand All @@ -305,7 +306,7 @@ async function executeCompleteFlow(applications: OAuthApplications): Promise<Ses
if (identityTokenInformation) {
identityToken = buildIdentityTokenFromEnv(scopes, identityTokenInformation)
} else {
identityToken = await identityClient.requestAccessToken(scopes)
identityToken = await requestAccessToken(scopes)
}

// Exchange identity token for application tokens
Expand Down Expand Up @@ -336,7 +337,7 @@ async function executeCompleteFlow(applications: OAuthApplications): Promise<Ses
*/
async function refreshTokens(session: Session, applications: OAuthApplications): Promise<Session> {
// Refresh Identity Token
const identityToken = await getIdentityClient().refreshAccessToken(session.identity)
const identityToken = await refreshAccessToken(session.identity)
// Exchange new identity token for application tokens
const exchangeScopes = getExchangeScopes(applications)
const applicationTokens = await exchangeAccessForApplicationTokens(
Expand Down
Loading