Skip to content
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.

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

Centralize gating of the App Management API #5003

Merged
merged 1 commit into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions packages/app/src/cli/services/deploy/bundle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {joinPath} from '@shopify/cli-kit/node/path'
describe('bundleAndBuildExtensions', () => {
let app: AppInterface

test('generates a manifest.json when USE_APP_MANAGEMENT_API is enabled', async () => {
test('generates a manifest.json when App Management is enabled', async () => {
await file.inTemporaryDirectory(async (tmpDir: string) => {
// Given
vi.spyOn(file, 'writeFileSync').mockResolvedValue(undefined)
Expand Down Expand Up @@ -73,7 +73,7 @@ describe('bundleAndBuildExtensions', () => {
})
})

test('does not generate the manifest.json when USE_APP_MANAGEMENT_API is disabled', async () => {
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)
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/cli/services/deploy/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import {zip} from '@shopify/cli-kit/node/archiver'
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 {isTruthy} from '@shopify/cli-kit/node/context/utilities'
import {renderConcurrent} from '@shopify/cli-kit/node/ui'
import {isAppManagementEnabled} from '@shopify/cli-kit/node/context/local'
import {Writable} from 'stream'

interface BundleOptions {
Expand All @@ -21,7 +21,7 @@ export async function bundleAndBuildExtensions(options: BundleOptions, systemEnv
mkdirSync(bundleDirectory)
await touchFile(joinPath(bundleDirectory, '.shopify'))

if (isTruthy(systemEnvironment.USE_APP_MANAGEMENT_API)) {
if (isAppManagementEnabled(systemEnvironment)) {
// Include manifest in bundle
const appManifest = await options.app.manifest()
const manifestPath = joinPath(bundleDirectory, 'manifest.json')
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/cli/services/dev/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ afterEach(() => {
})

describe('fetchOrganizations', async () => {
test('returns fetched organizations from Partners without USE_APP_MANAGEMENT_API', async () => {
test('returns fetched organizations from Partners when App Management is disabled', async () => {
// Given
const partnersClient: PartnersClient = testDeveloperPlatformClient({
organizations: () => Promise.resolve([ORG1]),
Expand All @@ -74,7 +74,7 @@ describe('fetchOrganizations', async () => {
expect(appManagementClient.organizations).not.toHaveBeenCalled()
})

test('returns fetched organizations from Partners and App Management with USE_APP_MANAGEMENT_API', async () => {
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({
Expand Down
6 changes: 3 additions & 3 deletions packages/app/src/cli/utilities/developer-platform-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,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 {isTruthy} from '@shopify/cli-kit/node/context/utilities'
import {isAppManagementEnabled} from '@shopify/cli-kit/node/context/local'

export enum ClientName {
AppManagement = 'app-management',
Expand All @@ -77,7 +77,7 @@ export interface AppVersionIdentifiers {

export function allDeveloperPlatformClients(): DeveloperPlatformClient[] {
const clients: DeveloperPlatformClient[] = [new PartnersClient()]
if (isTruthy(process.env.USE_APP_MANAGEMENT_API)) clients.push(new AppManagementClient())
if (isAppManagementEnabled()) clients.push(new AppManagementClient())
return clients
}

Expand Down Expand Up @@ -117,7 +117,7 @@ export function selectDeveloperPlatformClient({
configuration,
organization,
}: SelectDeveloperPlatformClientOptions = {}): DeveloperPlatformClient {
if (isTruthy(process.env.USE_APP_MANAGEMENT_API)) {
if (isAppManagementEnabled()) {
if (organization) return selectDeveloperPlatformClientByOrg(organization)
return selectDeveloperPlatformClientByConfig(configuration)
}
Expand Down
1 change: 1 addition & 0 deletions packages/cli-kit/src/private/node/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ 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'
Expand Down
5 changes: 2 additions & 3 deletions packages/cli-kit/src/private/node/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import {getCachedPartnerAccountStatus, setCachedPartnerAccountStatus} from './conf-store.js'
import {isThemeAccessSession} from './api/rest.js'
import {outputContent, outputToken, outputDebug} from '../../public/node/output.js'
import {firstPartyDev, themeToken} from '../../public/node/context/local.js'
import {firstPartyDev, isAppManagementEnabled, 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'
Expand All @@ -26,7 +26,6 @@
import {gql} from 'graphql-request'
import {AdminSession} from '@shopify/cli-kit/node/session'
import {outputCompleted, outputInfo, outputWarn} from '@shopify/cli-kit/node/output'
import {isTruthy} from '@shopify/cli-kit/node/context/utilities'
import {isSpin} from '@shopify/cli-kit/node/context/spin'
import {nonRandomUUID} from '@shopify/cli-kit/node/crypto'

Expand Down Expand Up @@ -121,7 +120,7 @@
export async function getLastSeenUserIdAfterAuth(): Promise<string> {
if (userId) return userId

const currentSession = (await secureStore.fetch()) || {}

Check warning on line 123 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L123

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const fqdn = await identityFqdn()
const cachedUserId = currentSession[fqdn]?.identity.userId
if (cachedUserId) return cachedUserId
Expand Down Expand Up @@ -149,7 +148,7 @@
export async function getLastSeenAuthMethod(): Promise<AuthMethod> {
if (authMethod !== 'none') return authMethod

const currentSession = (await secureStore.fetch()) || {}

Check warning on line 151 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L151

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const fqdn = await identityFqdn()
const cachedUserId = currentSession[fqdn]?.identity.userId
if (cachedUserId) return 'device_auth'
Expand Down Expand Up @@ -192,7 +191,7 @@
}
}

const currentSession = (await secureStore.fetch()) || {}

Check warning on line 194 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L194

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const fqdnSession = currentSession[fqdn]!
const scopes = getFlattenScopes(applications)
Expand Down Expand Up @@ -310,7 +309,7 @@
* @param partnersToken - Partners token.
*/
async function ensureUserHasPartnerAccount(partnersToken: string, userId: string | undefined) {
if (isTruthy(process.env.USE_APP_MANAGEMENT_API)) return
if (isAppManagementEnabled()) return

outputDebug(outputContent`Verifying that the user has a Partner organization`)
if (!(await hasPartnerAccount(partnersToken, userId))) {
Expand Down Expand Up @@ -451,11 +450,11 @@
* @returns A flattened array of scopes.
*/
function getFlattenScopes(apps: OAuthApplications): string[] {
const admin = apps.adminApi?.scopes || []

Check warning on line 453 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L453

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const partner = apps.partnersApi?.scopes || []

Check warning on line 454 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L454

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const storefront = apps.storefrontRendererApi?.scopes || []

Check warning on line 455 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L455

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const businessPlatform = apps.businessPlatformApi?.scopes || []

Check warning on line 456 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L456

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const appManagement = apps.appManagementApi?.scopes || []

Check warning on line 457 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L457

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const requestedScopes = [...admin, ...partner, ...storefront, ...businessPlatform, ...appManagement]
return allDefaultScopes(requestedScopes)
}
Expand All @@ -467,11 +466,11 @@
* @returns An object containing the scopes for each application.
*/
function getExchangeScopes(apps: OAuthApplications): ExchangeScopes {
const adminScope = apps.adminApi?.scopes || []

Check warning on line 469 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L469

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const partnerScope = apps.partnersApi?.scopes || []

Check warning on line 470 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L470

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const storefrontScopes = apps.storefrontRendererApi?.scopes || []

Check warning on line 471 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L471

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const businessPlatformScopes = apps.businessPlatformApi?.scopes || []

Check warning on line 472 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L472

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const appManagementScopes = apps.appManagementApi?.scopes || []

Check warning on line 473 in packages/cli-kit/src/private/node/session.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/session.ts#L473

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
return {
admin: apiScopes('admin', adminScope),
partners: apiScopes('partners', partnerScope),
Expand Down
4 changes: 2 additions & 2 deletions packages/cli-kit/src/private/node/session/exchange.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ 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 {isTruthy} from '@shopify/cli-kit/node/context/utilities'
import * as jose from 'jose'
import {nonRandomUUID} from '@shopify/cli-kit/node/crypto'

Expand Down Expand Up @@ -34,7 +34,7 @@ export async function exchangeAccessForApplicationTokens(
store?: string,
): Promise<{[x: string]: ApplicationToken}> {
const token = identityToken.accessToken
const appManagementEnabled = isTruthy(process.env.USE_APP_MANAGEMENT_API)
const appManagementEnabled = isAppManagementEnabled()

const [partners, storefront, businessPlatform, admin, appManagement] = await Promise.all([
requestAppToken('partners', token, scopes.partners),
Expand Down
3 changes: 2 additions & 1 deletion packages/cli-kit/src/private/node/session/scopes.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {allDefaultScopes, apiScopes} from './scopes.js'
import {environmentVariables} from '../constants.js'
import {describe, expect, test} from 'vitest'

describe('allDefaultScopes', () => {
Expand All @@ -25,7 +26,7 @@ describe('allDefaultScopes', () => {

test('includes App Management and Store Management when the required env var is defined', async () => {
// Given
const envVars = {USE_APP_MANAGEMENT_API: 'true'}
const envVars = {[environmentVariables.useAppManagement]: 'true'}

// When
const got = allDefaultScopes([], envVars)
Expand Down
8 changes: 3 additions & 5 deletions packages/cli-kit/src/private/node/session/scopes.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {allAPIs, API} from '../api.js'
import {BugError} from '../../../public/node/error.js'
import {isTruthy} from '@shopify/cli-kit/node/context/utilities'
import {isAppManagementEnabled} from '../../../public/node/context/local.js'

/**
* Generate a flat array with all the default scopes for all the APIs plus
Expand Down Expand Up @@ -35,11 +35,9 @@ function defaultApiScopes(api: API, systemEnvironment = process.env): string[] {
case 'partners':
return ['cli']
case 'business-platform':
return isTruthy(systemEnvironment.USE_APP_MANAGEMENT_API)
? ['destinations', 'store-management']
: ['destinations']
return isAppManagementEnabled(systemEnvironment) ? ['destinations', 'store-management'] : ['destinations']
case 'app-management':
return isTruthy(systemEnvironment.USE_APP_MANAGEMENT_API) ? ['app-management'] : []
return isAppManagementEnabled(systemEnvironment) ? ['app-management'] : []
default:
throw new BugError(`Unknown API: ${api}`)
}
Expand Down
25 changes: 25 additions & 0 deletions packages/cli-kit/src/public/node/context/local.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
isDevelopment,
isShopify,
isUnitTest,
isAppManagementEnabled,
analyticsDisabled,
cloudEnvironment,
macAddress,
Expand Down Expand Up @@ -99,6 +100,30 @@ 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
Expand Down
10 changes: 10 additions & 0 deletions packages/cli-kit/src/public/node/context/local.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,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 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).
Expand Down
Loading