From d509fb0fa5ebc8beccb5c2d0d7977f3189328ae8 Mon Sep 17 00:00:00 2001 From: Ariel Caplan Date: Mon, 2 Dec 2024 00:46:14 +0200 Subject: [PATCH 1/4] Remove all conditional logic around App Management being disabled --- .../src/cli/services/deploy/bundle.test.ts | 42 +--------- .../app/src/cli/services/deploy/bundle.ts | 13 ++- .../app/src/cli/services/dev/fetch.test.ts | 28 ++----- .../utilities/developer-platform-client.ts | 11 +-- .../cli-kit/src/private/node/api/headers.ts | 2 +- .../cli-kit/src/private/node/constants.ts | 1 - packages/cli-kit/src/private/node/session.ts | 83 +------------------ .../src/private/node/session/exchange.ts | 4 +- .../src/private/node/session/scopes.test.ts | 8 +- .../src/private/node/session/scopes.ts | 15 ++-- .../src/public/node/context/local.test.ts | 25 ------ .../cli-kit/src/public/node/context/local.ts | 10 --- 12 files changed, 31 insertions(+), 211 deletions(-) diff --git a/packages/app/src/cli/services/deploy/bundle.test.ts b/packages/app/src/cli/services/deploy/bundle.test.ts index c35c5db44f9..10079ac2539 100644 --- a/packages/app/src/cli/services/deploy/bundle.test.ts +++ b/packages/app/src/cli/services/deploy/bundle.test.ts @@ -8,11 +8,10 @@ import {joinPath} from '@shopify/cli-kit/node/path' describe('bundleAndBuildExtensions', () => { let app: AppInterface - test('generates a manifest.json when App Management is enabled', async () => { + test('generates a manifest.json', async () => { await file.inTemporaryDirectory(async (tmpDir: string) => { // Given vi.spyOn(file, 'writeFileSync').mockResolvedValue(undefined) - const envVars = {USE_APP_MANAGEMENT_API: 'true'} const bundlePath = joinPath(tmpDir, 'bundle.zip') const uiExtension = await testUIExtension({type: 'web_pixel_extension'}) @@ -60,7 +59,7 @@ describe('bundleAndBuildExtensions', () => { } // When - await bundleAndBuildExtensions({app, identifiers, bundlePath}, envVars) + await bundleAndBuildExtensions({app, identifiers, bundlePath}) // Then expect(extensionBundleMock).toHaveBeenCalledTimes(2) @@ -73,41 +72,6 @@ describe('bundleAndBuildExtensions', () => { }) }) - test('does not generate the manifest.json when App Management is disabled', async () => { - await file.inTemporaryDirectory(async (tmpDir: string) => { - // Given - vi.spyOn(file, 'writeFileSync').mockResolvedValue(undefined) - const bundlePath = joinPath(tmpDir, 'bundle.zip') - - const uiExtension = await testUIExtension({type: 'web_pixel_extension'}) - const extensionBundleMock = vi.fn() - uiExtension.buildForBundle = extensionBundleMock - const themeExtension = await testThemeExtensions() - themeExtension.buildForBundle = extensionBundleMock - app = testApp({allExtensions: [uiExtension, themeExtension]}) - - const extensions: {[key: string]: string} = {} - for (const extension of app.allExtensions) { - extensions[extension.localIdentifier] = extension.localIdentifier - } - const identifiers = { - app: 'app-id', - extensions, - extensionIds: {}, - extensionsNonUuidManaged: {}, - } - - // When - await bundleAndBuildExtensions({app, identifiers, bundlePath}, {}) - - // Then - expect(extensionBundleMock).toHaveBeenCalledTimes(2) - expect(file.writeFileSync).not.toHaveBeenCalled() - - await expect(file.fileExists(bundlePath)).resolves.toBeTruthy() - }) - }) - test('creates a zip file for a function extension', async () => { await file.inTemporaryDirectory(async (tmpDir: string) => { // Given @@ -132,7 +96,7 @@ describe('bundleAndBuildExtensions', () => { } // When - await bundleAndBuildExtensions({app, identifiers, bundlePath}, {}) + await bundleAndBuildExtensions({app, identifiers, bundlePath}) // Then await expect(file.fileExists(bundlePath)).resolves.toBeTruthy() diff --git a/packages/app/src/cli/services/deploy/bundle.ts b/packages/app/src/cli/services/deploy/bundle.ts index e550bf91372..f11186640b8 100644 --- a/packages/app/src/cli/services/deploy/bundle.ts +++ b/packages/app/src/cli/services/deploy/bundle.ts @@ -6,7 +6,6 @@ import {AbortSignal} from '@shopify/cli-kit/node/abort' import {inTemporaryDirectory, mkdirSync, touchFile, writeFileSync} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' import {renderConcurrent} from '@shopify/cli-kit/node/ui' -import {isAppManagementEnabled} from '@shopify/cli-kit/node/context/local' import {Writable} from 'stream' interface BundleOptions { @@ -15,18 +14,16 @@ interface BundleOptions { identifiers?: Identifiers } -export async function bundleAndBuildExtensions(options: BundleOptions, systemEnvironment = process.env) { +export async function bundleAndBuildExtensions(options: BundleOptions) { await inTemporaryDirectory(async (tmpDir) => { const bundleDirectory = joinPath(tmpDir, 'bundle') mkdirSync(bundleDirectory) await touchFile(joinPath(bundleDirectory, '.shopify')) - if (isAppManagementEnabled(systemEnvironment)) { - // Include manifest in bundle - const appManifest = await options.app.manifest() - const manifestPath = joinPath(bundleDirectory, 'manifest.json') - writeFileSync(manifestPath, JSON.stringify(appManifest, null, 2)) - } + // Include manifest in bundle + const appManifest = await options.app.manifest() + const manifestPath = joinPath(bundleDirectory, 'manifest.json') + writeFileSync(manifestPath, JSON.stringify(appManifest, null, 2)) // Force the download of the javy binary in advance to avoid later problems, // as it might be done multiple times in parallel. /~https://github.com/Shopify/cli/issues/2877 diff --git a/packages/app/src/cli/services/dev/fetch.test.ts b/packages/app/src/cli/services/dev/fetch.test.ts index 12665ef2650..04d70b2611b 100644 --- a/packages/app/src/cli/services/dev/fetch.test.ts +++ b/packages/app/src/cli/services/dev/fetch.test.ts @@ -54,7 +54,7 @@ afterEach(() => { }) describe('fetchOrganizations', async () => { - test('returns fetched organizations from Partners when App Management is disabled', async () => { + test('returns fetched organizations from Partners and App Management', async () => { // Given const partnersClient: PartnersClient = testDeveloperPlatformClient({ organizations: () => Promise.resolve([ORG1]), @@ -68,27 +68,6 @@ describe('fetchOrganizations', async () => { // When const got = await fetchOrganizations() - // Then - expect(got).toEqual([ORG1]) - expect(partnersClient.organizations).toHaveBeenCalled() - expect(appManagementClient.organizations).not.toHaveBeenCalled() - }) - - test('returns fetched organizations from Partners and App Management when App Management is enabled', async () => { - // Given - vi.stubEnv('USE_APP_MANAGEMENT_API', '1') - const partnersClient: PartnersClient = testDeveloperPlatformClient({ - organizations: () => Promise.resolve([ORG1]), - }) as PartnersClient - const appManagementClient: AppManagementClient = testDeveloperPlatformClient({ - organizations: () => Promise.resolve([ORG2]), - }) as AppManagementClient - vi.mocked(PartnersClient).mockReturnValue(partnersClient) - vi.mocked(AppManagementClient).mockReturnValue(appManagementClient) - - // When - const got = await fetchOrganizations() - // Then expect(got).toEqual([ORG1, ORG2]) expect(partnersClient.organizations).toHaveBeenCalled() @@ -100,7 +79,11 @@ describe('fetchOrganizations', async () => { const partnersClient: PartnersClient = testDeveloperPlatformClient({ organizations: () => Promise.resolve([]), }) as PartnersClient + const appManagementClient: AppManagementClient = testDeveloperPlatformClient({ + organizations: () => Promise.resolve([]), + }) as AppManagementClient vi.mocked(PartnersClient).mockReturnValue(partnersClient) + vi.mocked(AppManagementClient).mockReturnValue(appManagementClient) // When const got = fetchOrganizations() @@ -108,6 +91,7 @@ describe('fetchOrganizations', async () => { // Then await expect(got).rejects.toThrow(new NoOrgError(testPartnersUserSession.accountInfo)) expect(partnersClient.organizations).toHaveBeenCalled() + expect(appManagementClient.organizations).toHaveBeenCalled() }) }) diff --git a/packages/app/src/cli/utilities/developer-platform-client.ts b/packages/app/src/cli/utilities/developer-platform-client.ts index d2ccbf07549..24e7aacad11 100644 --- a/packages/app/src/cli/utilities/developer-platform-client.ts +++ b/packages/app/src/cli/utilities/developer-platform-client.ts @@ -55,7 +55,6 @@ import { import {DevSessionCreateMutation} from '../api/graphql/app-dev/generated/dev-session-create.js' import {DevSessionUpdateMutation} from '../api/graphql/app-dev/generated/dev-session-update.js' import {DevSessionDeleteMutation} from '../api/graphql/app-dev/generated/dev-session-delete.js' -import {isAppManagementEnabled} from '@shopify/cli-kit/node/context/local' export enum ClientName { AppManagement = 'app-management', @@ -77,8 +76,7 @@ export interface AppVersionIdentifiers { } export function allDeveloperPlatformClients(): DeveloperPlatformClient[] { - const clients: DeveloperPlatformClient[] = [new PartnersClient()] - if (isAppManagementEnabled()) clients.push(new AppManagementClient()) + const clients: DeveloperPlatformClient[] = [new PartnersClient(), new AppManagementClient()] return clients } @@ -118,11 +116,8 @@ export function selectDeveloperPlatformClient({ configuration, organization, }: SelectDeveloperPlatformClientOptions = {}): DeveloperPlatformClient { - if (isAppManagementEnabled()) { - if (organization) return selectDeveloperPlatformClientByOrg(organization) - return selectDeveloperPlatformClientByConfig(configuration) - } - return new PartnersClient() + if (organization) return selectDeveloperPlatformClientByOrg(organization) + return selectDeveloperPlatformClientByConfig(configuration) } function selectDeveloperPlatformClientByOrg(organization: Organization): DeveloperPlatformClient { diff --git a/packages/cli-kit/src/private/node/api/headers.ts b/packages/cli-kit/src/private/node/api/headers.ts index 7d5ac55ee9a..b00905a43e5 100644 --- a/packages/cli-kit/src/private/node/api/headers.ts +++ b/packages/cli-kit/src/private/node/api/headers.ts @@ -4,7 +4,7 @@ import {Environment, serviceEnvironment} from '../context/service.js' import {ExtendableError} from '../../../public/node/error.js' import https from 'https' -export class RequestClientError extends ExtendableError { +class RequestClientError extends ExtendableError { statusCode: number public constructor(message: string, statusCode: number) { super(message) diff --git a/packages/cli-kit/src/private/node/constants.ts b/packages/cli-kit/src/private/node/constants.ts index c9a4ab91517..faee18c688e 100644 --- a/packages/cli-kit/src/private/node/constants.ts +++ b/packages/cli-kit/src/private/node/constants.ts @@ -45,7 +45,6 @@ export const environmentVariables = { otelURL: 'SHOPIFY_CLI_OTEL_EXPORTER_OTLP_ENDPOINT', themeKitAccessDomain: 'SHOPIFY_CLI_THEME_KIT_ACCESS_DOMAIN', json: 'SHOPIFY_FLAG_JSON', - useAppManagement: 'USE_APP_MANAGEMENT_API', } export const defaultThemeKitAccessDomain = 'theme-kit-access.shopifyapps.com' diff --git a/packages/cli-kit/src/private/node/session.ts b/packages/cli-kit/src/private/node/session.ts index 041df38d971..a6e620ad8a8 100644 --- a/packages/cli-kit/src/private/node/session.ts +++ b/packages/cli-kit/src/private/node/session.ts @@ -12,20 +12,14 @@ import { import {IdentityToken, Session} from './session/schema.js' import * as secureStore from './session/store.js' import {pollForDeviceAuthorization, requestDeviceAuthorization} from './session/device-authorization.js' -import {RequestClientError} from './api/headers.js' -import {getCachedPartnerAccountStatus, setCachedPartnerAccountStatus} from './conf-store.js' import {isThemeAccessSession} from './api/rest.js' import {outputContent, outputToken, outputDebug} from '../../public/node/output.js' -import {firstPartyDev, isAppManagementEnabled, themeToken} from '../../public/node/context/local.js' +import {firstPartyDev, themeToken} from '../../public/node/context/local.js' import {AbortError, BugError} from '../../public/node/error.js' -import {partnersRequest} from '../../public/node/api/partners.js' -import {normalizeStoreFqdn, partnersFqdn, identityFqdn} from '../../public/node/context/fqdn.js' -import {openURL} from '../../public/node/system.js' -import {keypress} from '../../public/node/ui.js' +import {normalizeStoreFqdn, identityFqdn} from '../../public/node/context/fqdn.js' import {getIdentityTokenInformation, getPartnersToken} from '../../public/node/environment.js' -import {gql} from 'graphql-request' import {AdminSession} from '@shopify/cli-kit/node/session' -import {outputCompleted, outputInfo, outputWarn} from '@shopify/cli-kit/node/output' +import {outputCompleted} from '@shopify/cli-kit/node/output' import {isSpin} from '@shopify/cli-kit/node/context/spin' import {nonRandomUUID} from '@shopify/cli-kit/node/crypto' @@ -247,9 +241,6 @@ The CLI is currently unable to prompt for reauthentication.`, if (envToken && applications.partnersApi) { tokens.partners = (await exchangeCustomPartnerToken(envToken)).accessToken } - if (!envToken && tokens.partners) { - await ensureUserHasPartnerAccount(tokens.partners, tokens.userId) - } setLastSeenAuthMethod(envToken ? 'partners_token' : 'device_auth') setLastSeenUserIdAfterAuth(tokens.userId) @@ -301,74 +292,6 @@ async function executeCompleteFlow(applications: OAuthApplications, identityFqdn return session } -/** - * If the user creates an account from the Identity website, the created - * account won't get a Partner organization created. We need to detect that - * and take the user to create a partner organization. - * - * @param partnersToken - Partners token. - */ -async function ensureUserHasPartnerAccount(partnersToken: string, userId: string | undefined) { - if (isAppManagementEnabled()) return - - outputDebug(outputContent`Verifying that the user has a Partner organization`) - if (!(await hasPartnerAccount(partnersToken, userId))) { - outputInfo(`\nA Shopify Partners organization is needed to proceed.`) - outputInfo(`👉 Press any key to create one`) - await keypress() - await openURL(`https://${await partnersFqdn()}/signup`) - outputInfo(outputContent`👉 Press any key when you have ${outputToken.cyan('created the organization')}`) - outputWarn(outputContent`Make sure you've confirmed your Shopify and the Partner organization from the email`) - await keypress() - if (!(await hasPartnerAccount(partnersToken, userId))) { - throw new AbortError( - `Couldn't find your Shopify Partners organization`, - `Have you confirmed your accounts from the emails you received?`, - ) - } - } -} - -// eslint-disable-next-line @shopify/cli/no-inline-graphql -const getFirstOrganization = gql` - { - organizations(first: 1) { - nodes { - id - } - } - } -` - -/** - * Validate if the current token is valid for partners API. - * - * @param partnersToken - Partners token. - * @returns A promise that resolves to true if the token is valid for partners API. - */ -async function hasPartnerAccount(partnersToken: string, userId?: string): Promise { - const cacheKey = userId ?? partnersToken - const cachedStatus = getCachedPartnerAccountStatus(cacheKey) - - if (cachedStatus) { - outputDebug(`Confirmed partner account exists from cache`) - return true - } - - try { - await partnersRequest(getFirstOrganization, partnersToken) - setCachedPartnerAccountStatus(cacheKey) - return true - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - if (error instanceof RequestClientError && error.statusCode === 404) { - return false - } else { - return true - } - } -} - /** * Refresh the tokens for a given session. * diff --git a/packages/cli-kit/src/private/node/session/exchange.ts b/packages/cli-kit/src/private/node/session/exchange.ts index 2e3b8a540a4..e234ee4f3a6 100644 --- a/packages/cli-kit/src/private/node/session/exchange.ts +++ b/packages/cli-kit/src/private/node/session/exchange.ts @@ -5,7 +5,6 @@ import {identityFqdn} from '../../../public/node/context/fqdn.js' import {shopifyFetch} from '../../../public/node/http.js' import {err, ok, Result} from '../../../public/node/result.js' import {AbortError, BugError, ExtendableError} from '../../../public/node/error.js' -import {isAppManagementEnabled} from '../../../public/node/context/local.js' import {setLastSeenAuthMethod, setLastSeenUserIdAfterAuth} from '../session.js' import * as jose from 'jose' import {nonRandomUUID} from '@shopify/cli-kit/node/crypto' @@ -34,14 +33,13 @@ export async function exchangeAccessForApplicationTokens( store?: string, ): Promise<{[x: string]: ApplicationToken}> { const token = identityToken.accessToken - const appManagementEnabled = isAppManagementEnabled() const [partners, storefront, businessPlatform, admin, appManagement] = await Promise.all([ requestAppToken('partners', token, scopes.partners), requestAppToken('storefront-renderer', token, scopes.storefront), requestAppToken('business-platform', token, scopes.businessPlatform), store ? requestAppToken('admin', token, scopes.admin, store) : {}, - appManagementEnabled ? requestAppToken('app-management', token, scopes.appManagement) : {}, + requestAppToken('app-management', token, scopes.appManagement), ]) return { diff --git a/packages/cli-kit/src/private/node/session/scopes.test.ts b/packages/cli-kit/src/private/node/session/scopes.test.ts index d7f88fd9b53..4b28edfc995 100644 --- a/packages/cli-kit/src/private/node/session/scopes.test.ts +++ b/packages/cli-kit/src/private/node/session/scopes.test.ts @@ -1,5 +1,4 @@ import {allDefaultScopes, apiScopes} from './scopes.js' -import {environmentVariables} from '../constants.js' import {describe, expect, test} from 'vitest' describe('allDefaultScopes', () => { @@ -24,12 +23,9 @@ describe('allDefaultScopes', () => { ]) }) - test('includes App Management and Store Management when the required env var is defined', async () => { - // Given - const envVars = {[environmentVariables.useAppManagement]: 'true'} - + test('includes App Management and Store Management', async () => { // When - const got = allDefaultScopes([], envVars) + const got = allDefaultScopes([]) // Then expect(got).toEqual([ diff --git a/packages/cli-kit/src/private/node/session/scopes.ts b/packages/cli-kit/src/private/node/session/scopes.ts index 072fb29d2fc..91803f77709 100644 --- a/packages/cli-kit/src/private/node/session/scopes.ts +++ b/packages/cli-kit/src/private/node/session/scopes.ts @@ -1,6 +1,5 @@ import {allAPIs, API} from '../api.js' import {BugError} from '../../../public/node/error.js' -import {isAppManagementEnabled} from '../../../public/node/context/local.js' /** * Generate a flat array with all the default scopes for all the APIs plus @@ -8,8 +7,8 @@ import {isAppManagementEnabled} from '../../../public/node/context/local.js' * @param extraScopes - custom user-defined scopes * @returns Array of scopes */ -export function allDefaultScopes(extraScopes: string[] = [], systemEnvironment = process.env): string[] { - let scopes = allAPIs.map((api) => defaultApiScopes(api, systemEnvironment)).flat() +export function allDefaultScopes(extraScopes: string[] = []): string[] { + let scopes = allAPIs.map((api) => defaultApiScopes(api)).flat() scopes = ['openid', ...scopes, ...extraScopes].map(scopeTransform) return Array.from(new Set(scopes)) } @@ -21,12 +20,12 @@ export function allDefaultScopes(extraScopes: string[] = [], systemEnvironment = * @param extraScopes - custom user-defined scopes * @returns Array of scopes */ -export function apiScopes(api: API, extraScopes: string[] = [], systemEnvironment = process.env): string[] { - const scopes = [...defaultApiScopes(api, systemEnvironment), ...extraScopes.map(scopeTransform)].map(scopeTransform) +export function apiScopes(api: API, extraScopes: string[] = []): string[] { + const scopes = [...defaultApiScopes(api), ...extraScopes.map(scopeTransform)].map(scopeTransform) return Array.from(new Set(scopes)) } -function defaultApiScopes(api: API, systemEnvironment = process.env): string[] { +function defaultApiScopes(api: API): string[] { switch (api) { case 'admin': return ['graphql', 'themes', 'collaborator'] @@ -35,9 +34,9 @@ function defaultApiScopes(api: API, systemEnvironment = process.env): string[] { case 'partners': return ['cli'] case 'business-platform': - return isAppManagementEnabled(systemEnvironment) ? ['destinations', 'store-management'] : ['destinations'] + return ['destinations', 'store-management'] case 'app-management': - return isAppManagementEnabled(systemEnvironment) ? ['app-management'] : [] + return ['app-management'] default: throw new BugError(`Unknown API: ${api}`) } diff --git a/packages/cli-kit/src/public/node/context/local.test.ts b/packages/cli-kit/src/public/node/context/local.test.ts index 14c69b10fbe..2c687428e11 100644 --- a/packages/cli-kit/src/public/node/context/local.test.ts +++ b/packages/cli-kit/src/public/node/context/local.test.ts @@ -4,7 +4,6 @@ import { isDevelopment, isShopify, isUnitTest, - isAppManagementEnabled, analyticsDisabled, cloudEnvironment, macAddress, @@ -100,30 +99,6 @@ describe('hasGit', () => { }) }) -describe('isAppManagementEnabled', () => { - test('returns true when USE_APP_MANAGEMENT_API is truthy', () => { - // Given - const env = {USE_APP_MANAGEMENT_API: '1'} - - // When - const got = isAppManagementEnabled(env) - - // Then - expect(got).toBe(true) - }) - - test('returns false when USE_APP_MANAGEMENT_API is falsy', () => { - // Given - const env = {USE_APP_MANAGEMENT_API: '0'} - - // When - const got = isAppManagementEnabled(env) - - // Then - expect(got).toBe(false) - }) -}) - describe('analitycsDisabled', () => { test('returns true when SHOPIFY_CLI_NO_ANALYTICS is truthy', () => { // Given diff --git a/packages/cli-kit/src/public/node/context/local.ts b/packages/cli-kit/src/public/node/context/local.ts index d4c1de54eee..cc8a73b2db2 100644 --- a/packages/cli-kit/src/public/node/context/local.ts +++ b/packages/cli-kit/src/public/node/context/local.ts @@ -46,16 +46,6 @@ export function isVerbose(env = process.env): boolean { return isTruthy(env[environmentVariables.verbose]) || process.argv.includes('--verbose') } -/** - * It returns true if the App Management API is available. - * - * @param env - The environment variables from the environment of the current process. - * @returns True if the App Management API is available. - */ -export function isAppManagementEnabled(env = process.env): boolean { - return isTruthy(env[environmentVariables.useAppManagement]) -} - /** * Returns true if the environment in which the CLI is running is either * a local environment (where dev is present) or a cloud environment (spin). From dfb94ce77dbd46d023854e2f67e4cc7911701403 Mon Sep 17 00:00:00 2001 From: Ariel Caplan Date: Tue, 14 Jan 2025 17:24:25 +0200 Subject: [PATCH 2/4] Update failing tests --- .../src/private/node/session/exchange.test.ts | 25 +++++++++++++------ .../src/private/node/session/scopes.test.ts | 2 ++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/cli-kit/src/private/node/session/exchange.test.ts b/packages/cli-kit/src/private/node/session/exchange.test.ts index c4869815c5e..b8e9feab9ef 100644 --- a/packages/cli-kit/src/private/node/session/exchange.test.ts +++ b/packages/cli-kit/src/private/node/session/exchange.test.ts @@ -55,20 +55,21 @@ describe('exchange identity token for application tokens', () => { test('returns tokens for all APIs if a store is passed', async () => { // Given - const response = new Response(JSON.stringify(data)) - - // Need to do it 3 times because a Response can only be used once - vi.mocked(shopifyFetch) - .mockResolvedValue(response) - .mockResolvedValueOnce(response.clone()) - .mockResolvedValueOnce(response.clone()) - .mockResolvedValueOnce(response.clone()) + vi.mocked(shopifyFetch).mockImplementation(async () => Promise.resolve(new Response(JSON.stringify(data)))) // When const got = await exchangeAccessForApplicationTokens(identityToken, scopes, 'storeFQDN') // Then const expected = { + 'app-management': { + accessToken: "access_token", + expiresAt: expiredDate, + scopes: [ + "scope", + "scope2", + ], + }, partners: { accessToken: 'access_token', expiresAt: expiredDate, @@ -109,6 +110,14 @@ describe('exchange identity token for application tokens', () => { // Then const expected = { + 'app-management': { + accessToken: "access_token", + expiresAt: expiredDate, + scopes: [ + "scope", + "scope2", + ], + }, partners: { accessToken: 'access_token', expiresAt: expiredDate, diff --git a/packages/cli-kit/src/private/node/session/scopes.test.ts b/packages/cli-kit/src/private/node/session/scopes.test.ts index 4b28edfc995..74626d8a410 100644 --- a/packages/cli-kit/src/private/node/session/scopes.test.ts +++ b/packages/cli-kit/src/private/node/session/scopes.test.ts @@ -19,6 +19,8 @@ describe('allDefaultScopes', () => { 'https://api.shopify.com/auth/shop.storefront-renderer.devtools', 'https://api.shopify.com/auth/partners.app.cli.access', 'https://api.shopify.com/auth/destinations.readonly', + 'https://api.shopify.com/auth/organization.store-management', + 'https://api.shopify.com/auth/organization.apps.manage', ...customScopes, ]) }) From 893ee2e1ce8387e55ac7bd85cc54261dba6928a3 Mon Sep 17 00:00:00 2001 From: Ariel Caplan Date: Tue, 14 Jan 2025 17:48:05 +0200 Subject: [PATCH 3/4] Disable app management when using a Partners CLI token Otherwise we hit permissions issues. --- .../utilities/developer-platform-client.ts | 5 ++-- .../src/private/node/session/exchange.test.ts | 14 +++------- .../src/private/node/session/exchange.ts | 3 ++- .../src/public/node/context/local.test.ts | 27 +++++++++++++++++++ .../cli-kit/src/public/node/context/local.ts | 11 ++++++++ 5 files changed, 47 insertions(+), 13 deletions(-) diff --git a/packages/app/src/cli/utilities/developer-platform-client.ts b/packages/app/src/cli/utilities/developer-platform-client.ts index 24e7aacad11..8fff7486de4 100644 --- a/packages/app/src/cli/utilities/developer-platform-client.ts +++ b/packages/app/src/cli/utilities/developer-platform-client.ts @@ -55,6 +55,7 @@ import { import {DevSessionCreateMutation} from '../api/graphql/app-dev/generated/dev-session-create.js' import {DevSessionUpdateMutation} from '../api/graphql/app-dev/generated/dev-session-update.js' import {DevSessionDeleteMutation} from '../api/graphql/app-dev/generated/dev-session-delete.js' +import {isAppManagementDisabled} from '@shopify/cli-kit/node/context/local' export enum ClientName { AppManagement = 'app-management', @@ -76,8 +77,7 @@ export interface AppVersionIdentifiers { } export function allDeveloperPlatformClients(): DeveloperPlatformClient[] { - const clients: DeveloperPlatformClient[] = [new PartnersClient(), new AppManagementClient()] - return clients + return isAppManagementDisabled() ? [new PartnersClient()] : [new PartnersClient(), new AppManagementClient()] } /** @@ -116,6 +116,7 @@ export function selectDeveloperPlatformClient({ configuration, organization, }: SelectDeveloperPlatformClientOptions = {}): DeveloperPlatformClient { + if (isAppManagementDisabled()) return new PartnersClient() if (organization) return selectDeveloperPlatformClientByOrg(organization) return selectDeveloperPlatformClientByConfig(configuration) } diff --git a/packages/cli-kit/src/private/node/session/exchange.test.ts b/packages/cli-kit/src/private/node/session/exchange.test.ts index b8e9feab9ef..1d2e2fe6b1d 100644 --- a/packages/cli-kit/src/private/node/session/exchange.test.ts +++ b/packages/cli-kit/src/private/node/session/exchange.test.ts @@ -63,12 +63,9 @@ describe('exchange identity token for application tokens', () => { // Then const expected = { 'app-management': { - accessToken: "access_token", + accessToken: 'access_token', expiresAt: expiredDate, - scopes: [ - "scope", - "scope2", - ], + scopes: ['scope', 'scope2'], }, partners: { accessToken: 'access_token', @@ -111,12 +108,9 @@ describe('exchange identity token for application tokens', () => { // Then const expected = { 'app-management': { - accessToken: "access_token", + accessToken: 'access_token', expiresAt: expiredDate, - scopes: [ - "scope", - "scope2", - ], + scopes: ['scope', 'scope2'], }, partners: { accessToken: 'access_token', diff --git a/packages/cli-kit/src/private/node/session/exchange.ts b/packages/cli-kit/src/private/node/session/exchange.ts index e234ee4f3a6..87dccc92702 100644 --- a/packages/cli-kit/src/private/node/session/exchange.ts +++ b/packages/cli-kit/src/private/node/session/exchange.ts @@ -5,6 +5,7 @@ import {identityFqdn} from '../../../public/node/context/fqdn.js' import {shopifyFetch} from '../../../public/node/http.js' import {err, ok, Result} from '../../../public/node/result.js' import {AbortError, BugError, ExtendableError} from '../../../public/node/error.js' +import {isAppManagementDisabled} from '../../../public/node/context/local.js' import {setLastSeenAuthMethod, setLastSeenUserIdAfterAuth} from '../session.js' import * as jose from 'jose' import {nonRandomUUID} from '@shopify/cli-kit/node/crypto' @@ -39,7 +40,7 @@ export async function exchangeAccessForApplicationTokens( requestAppToken('storefront-renderer', token, scopes.storefront), requestAppToken('business-platform', token, scopes.businessPlatform), store ? requestAppToken('admin', token, scopes.admin, store) : {}, - requestAppToken('app-management', token, scopes.appManagement), + isAppManagementDisabled() ? {} : requestAppToken('app-management', token, scopes.appManagement), ]) return { diff --git a/packages/cli-kit/src/public/node/context/local.test.ts b/packages/cli-kit/src/public/node/context/local.test.ts index 2c687428e11..6ca48208a22 100644 --- a/packages/cli-kit/src/public/node/context/local.test.ts +++ b/packages/cli-kit/src/public/node/context/local.test.ts @@ -7,13 +7,16 @@ import { analyticsDisabled, cloudEnvironment, macAddress, + isAppManagementDisabled, } from './local.js' +import {getPartnersToken} from '../environment.js' import {fileExists} from '../fs.js' import {exec} from '../system.js' import {expect, describe, vi, test} from 'vitest' vi.mock('../fs.js') vi.mock('../system.js') +vi.mock('../environment.js') describe('isUnitTest', () => { test('returns true when SHOPIFY_UNIT_TEST is truthy', () => { @@ -99,6 +102,30 @@ describe('hasGit', () => { }) }) +describe('isAppManagementDisabled', () => { + test('returns true when a Partners token is present', () => { + // Given + vi.mocked(getPartnersToken).mockReturnValue('token') + + // When + const got = isAppManagementDisabled() + + // Then + expect(got).toBe(true) + }) + + test('returns false when a Partners token is not present', () => { + // Given + vi.mocked(getPartnersToken).mockReturnValue(undefined) + + // When + const got = isAppManagementDisabled() + + // Then + expect(got).toBe(false) + }) +}) + describe('analitycsDisabled', () => { test('returns true when SHOPIFY_CLI_NO_ANALYTICS is truthy', () => { // Given diff --git a/packages/cli-kit/src/public/node/context/local.ts b/packages/cli-kit/src/public/node/context/local.ts index cc8a73b2db2..e24d874e277 100644 --- a/packages/cli-kit/src/public/node/context/local.ts +++ b/packages/cli-kit/src/public/node/context/local.ts @@ -4,6 +4,7 @@ import {getCIMetadata, isSet, Metadata} from '../../../private/node/context/util import {environmentVariables, pathConstants} from '../../../private/node/constants.js' import {fileExists} from '../fs.js' import {exec} from '../system.js' +import {getPartnersToken} from '../environment.js' import isInteractive from 'is-interactive' import macaddress from 'macaddress' import {homedir} from 'os' @@ -46,6 +47,16 @@ export function isVerbose(env = process.env): boolean { return isTruthy(env[environmentVariables.verbose]) || process.argv.includes('--verbose') } +/** + * It returns true if the App Management API is disabled. + * This should only be relevant when using a Partners token. + * + * @returns True if the App Management API is disabled. + */ +export function isAppManagementDisabled(): boolean { + return Boolean(getPartnersToken()) +} + /** * Returns true if the environment in which the CLI is running is either * a local environment (where dev is present) or a cloud environment (spin). From de4d15e59c3958a12b6569d76da767c961e29e82 Mon Sep 17 00:00:00 2001 From: Ariel Caplan Date: Tue, 14 Jan 2025 18:08:25 +0200 Subject: [PATCH 4/4] Only use CLI token for acceptance tests --- .github/workflows/shopify-cli.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/shopify-cli.yml b/.github/workflows/shopify-cli.yml index 0d6cafe99b8..e5396300f0d 100644 --- a/.github/workflows/shopify-cli.yml +++ b/.github/workflows/shopify-cli.yml @@ -49,7 +49,6 @@ env: PNPM_VERSION: '8.15.7' BUNDLE_WITHOUT: 'test:development' SHOPIFY_FLAG_CLIENT_ID: ${{ secrets.SHOPIFY_FLAG_CLIENT_ID }} - SHOPIFY_CLI_PARTNERS_TOKEN: ${{ secrets.SHOPIFY_CLI_PARTNERS_TOKEN }} GH_TOKEN: ${{ secrets.SHOPIFY_GH_READ_CONTENT_TOKEN }} jobs: @@ -87,6 +86,8 @@ jobs: run: pnpm nx run-many --all --skip-nx-cache --target=test --exclude=features --output-style=stream - name: Acceptance tests if: ${{ matrix.node == '18.20.3' }} + env: + SHOPIFY_CLI_PARTNERS_TOKEN: ${{ secrets.SHOPIFY_CLI_PARTNERS_TOKEN }} run: pnpm nx run features:test - name: Send Slack notification on failure uses: slackapi/slack-github-action@007b2c3c751a190b6f0f040e47ed024deaa72844 # pin@v1.23.0 @@ -305,6 +306,8 @@ jobs: with: node-version: ${{ matrix.node }} - name: Acceptance tests + env: + SHOPIFY_CLI_PARTNERS_TOKEN: ${{ secrets.SHOPIFY_CLI_PARTNERS_TOKEN }} run: pnpm test:features --output-style=stream pr-test-coverage: @@ -355,6 +358,8 @@ jobs: - name: Unit tests run: pnpm test:unit --output-style=stream - name: Acceptance tests + env: + SHOPIFY_CLI_PARTNERS_TOKEN: ${{ secrets.SHOPIFY_CLI_PARTNERS_TOKEN }} run: pnpm test:features --output-style=stream - name: Setup tmate session if: ${{ failure() && inputs.debug-enabled }}