Merge pull request #8762 from Shopify/app-doctor/config-prompt

App Security: ask which app configuration to scan when none is selected

Author: jek

packages/app/src/cli/services/app-security-selection.test.ts

@@ -11,9 +11,10 @@ import {
   type AppSecuritySelectionDependencies,
 } from './app-security-selection.js'
 import {validAppConfiguration} from './app-security-selection.test-data.js'
-import {getCachedAppInfo} from './local-storage.js'
+import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js'
 import {appCreationDefaults} from './app/config/link.js'
 import {fetchOrCreateOrganizationApp} from './context.js'
+import use from './app/config/use.js'
 import {AbortError} from '@shopify/cli-kit/node/error'
 import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
 import {joinPath} from '@shopify/cli-kit/node/path'
@@ -25,7 +26,9 @@ import type {OrganizationApp} from '../models/organization.js'
 vi.mock('./local-storage.js', async (importOriginal) => ({
   ...(await importOriginal<typeof import('./local-storage.js')>()),
   getCachedAppInfo: vi.fn(),
+  setCachedAppInfo: vi.fn(),
 }))
+vi.mock('./app/config/use.js', () => ({default: vi.fn()}))
 vi.mock('./context.js', async (importOriginal) => ({
   ...(await importOriginal<typeof import('./context.js')>()),
   fetchOrCreateOrganizationApp: vi.fn(),
@@ -51,6 +54,7 @@ function promptDependencies(overrides: Partial<AppSecuritySelectionDependencies>
   return {
     confirmScanWithoutAppConfig: vi.fn(async (_directory: string) => true),
     pickClientId: vi.fn(async (_appDirectory: string) => 'picked-client-id'),
+    pickConfigFile: vi.fn(async (_appDirectory: string) => 'shopify.app.staging.toml'),
     ...overrides,
   }
 }
@@ -212,6 +216,116 @@ describe('resolveAppSecuritySelection with an app configuration', () => {
   })
 })
 
+describe('resolveAppSecuritySelection with several TOMLs and none selected', () => {
+  async function writeProductionAndStaging(directory: string): Promise<void> {
+    await writeConfiguration(directory, 'production-client-id', 'shopify.app.production.toml')
+    await writeConfiguration(directory, 'staging-client-id', 'shopify.app.staging.toml')
+  }
+
+  test('asks which TOML to scan, without saving the answer', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeProductionAndStaging(directory)
+      const dependencies = promptDependencies()
+
+      const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies)
+
+      expect(selection).toMatchObject({
+        kind: 'config',
+        appConfigFilePath: joinPath(await fileRealPath(directory), 'shopify.app.staging.toml'),
+        configClientId: 'staging-client-id',
+        appConfigFilePicked: true,
+      })
+      expect(dependencies.pickConfigFile).toHaveBeenCalledOnce()
+      expect(setCachedAppInfo).not.toHaveBeenCalled()
+    })
+  })
+
+  test('asks when the `app config use` choice no longer exists', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeProductionAndStaging(directory)
+      vi.mocked(getCachedAppInfo).mockReturnValue({directory, configFile: 'shopify.app.deleted.toml'})
+      const dependencies = promptDependencies()
+
+      const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies)
+
+      expect(selection).toMatchObject({configClientId: 'staging-client-id', appConfigFilePicked: true})
+      expect(setCachedAppInfo).not.toHaveBeenCalled()
+    })
+  })
+
+  test('scans shopify.app.toml without asking or saving a choice when the `app config use` choice no longer exists', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeProductionAndStaging(directory)
+      await writeConfiguration(directory, 'default-client-id')
+      vi.mocked(getCachedAppInfo).mockReturnValue({directory, configFile: 'shopify.app.deleted.toml'})
+      const dependencies = promptDependencies()
+
+      const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies)
+
+      expect(selection).toMatchObject({configClientId: 'default-client-id', appConfigFilePicked: false})
+      expect(dependencies.pickConfigFile).not.toHaveBeenCalled()
+      expect(use).not.toHaveBeenCalled()
+      expect(setCachedAppInfo).not.toHaveBeenCalled()
+    })
+  })
+
+  test('scans shopify.app.toml without asking when it exists', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeProductionAndStaging(directory)
+      await writeConfiguration(directory, 'default-client-id')
+      const dependencies = promptDependencies()
+
+      const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies)
+
+      expect(selection).toMatchObject({configClientId: 'default-client-id', appConfigFilePicked: undefined})
+      expect(dependencies.pickConfigFile).not.toHaveBeenCalled()
+    })
+  })
+
+  test('scans the only TOML without asking', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeConfiguration(directory, 'staging-client-id', 'shopify.app.staging.toml')
+      const dependencies = promptDependencies()
+
+      const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: false}, dependencies)
+
+      expect(selection).toMatchObject({configClientId: 'staging-client-id', appConfigFilePicked: false})
+      expect(dependencies.pickConfigFile).not.toHaveBeenCalled()
+    })
+  })
+
+  test('aborts with the TOMLs to choose from when it cannot ask', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeProductionAndStaging(directory)
+      const dependencies = promptDependencies()
+
+      const error = await selectionError(
+        resolveAppSecuritySelection({path: directory, allowPrompts: false}, dependencies),
+      )
+
+      expect(error.message).toBe(`2 app configurations found in ${directory}, and none is selected.`)
+      expect(error.tryMessage).toBe('Pass `--config` with one of: production, staging.')
+      expect(dependencies.pickConfigFile).not.toHaveBeenCalled()
+    })
+  })
+
+  test('aborts instead of asking with --client-id, which --config cannot repeat', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeProductionAndStaging(directory)
+      const dependencies = promptDependencies()
+
+      const error = await selectionError(
+        resolveAppSecuritySelection({path: directory, clientId: 'flag-client-id', allowPrompts: true}, dependencies),
+      )
+
+      expect(error.tryMessage).toBe(
+        "`--client-id` can't be combined with `--config`, so first select one with `shopify app config use <config>`: production, staging.",
+      )
+      expect(dependencies.pickConfigFile).not.toHaveBeenCalled()
+    })
+  })
+})
+
 describe('resolveAppSecuritySelection with --without-app-config', () => {
   test('aborts without --client-id', async () => {
     await inTemporaryDirectory(async (directory) => {
@@ -486,6 +600,34 @@ describe('resolveAppDirectory', () => {
     })
   })
 
+  test('finds the app directory when there are several TOMLs and none is selected', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeConfiguration(directory, 'production-client-id', 'shopify.app.production.toml')
+      await writeConfiguration(directory, 'staging-client-id', 'shopify.app.staging.toml')
+
+      await expect(resolveAppDirectory({path: directory})).resolves.toBe(await fileRealPath(directory))
+    })
+  })
+
+  test('finds the app directory without validating its TOML', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = [')
+
+      await expect(resolveAppDirectory({path: directory})).resolves.toBe(await fileRealPath(directory))
+    })
+  })
+
+  test('aborts when --path does not exist instead of walking up to the app above it', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await writeConfiguration(directory, 'default-client-id')
+      const missing = joinPath(directory, 'missing-app')
+
+      await expect(resolveAppDirectory({path: missing})).rejects.toMatchObject({
+        message: `--path ${missing}: not a directory.`,
+      })
+    })
+  })
+
   test('aborts when no TOML is found, without prompting', async () => {
     await inTemporaryDirectory(async (directory) => {
       await expect(resolveAppDirectory({path: directory})).rejects.toMatchObject({

packages/app/src/cli/services/app-security-selection.ts

@@ -1,9 +1,13 @@
 import {localAppContext} from './app-context.js'
 import {appCreationDefaults} from './app/config/link.js'
 import {fetchOrCreateOrganizationApp} from './context.js'
-import {NoAppConfigurationFoundError} from '../models/project/project.js'
+import {getCachedAppInfo} from './local-storage.js'
+import {NoAppConfigurationFoundError, Project} from '../models/project/project.js'
+import {getAppConfigurationShorthand} from '../models/app/config-file-naming.js'
+import {findConfigFiles, selectConfigFile} from '../prompts/config.js'
+import {configurationFileNames} from '../constants.js'
 import {AbortError} from '@shopify/cli-kit/node/error'
-import {fileRealPath, isDirectory} from '@shopify/cli-kit/node/fs'
+import {fileExistsSync, fileRealPath, isDirectory} from '@shopify/cli-kit/node/fs'
 import {basename, cwd, isSubpath, joinPath, normalizePath, relativePath, resolvePath} from '@shopify/cli-kit/node/path'
 import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui'
 
@@ -18,6 +22,8 @@ export type AppSecuritySelection =
       configClientId?: string
       /** The `--client-id` value, if passed. */
       clientIdOverride?: string
+      /** True when `check` asked which TOML to scan, because nothing else selected one. */
+      appConfigFilePicked?: boolean
     }
   | {
       kind: 'no-config'
@@ -45,6 +51,7 @@ interface AppSecuritySelectionOptions {
 export interface AppSecuritySelectionDependencies {
   confirmScanWithoutAppConfig(directory: string): Promise<boolean>
   pickClientId(appDirectory: string): Promise<string>
+  pickConfigFile(appDirectory: string): Promise<string>
 }
 
 const defaultDependencies: AppSecuritySelectionDependencies = {
@@ -56,6 +63,7 @@ const defaultDependencies: AppSecuritySelectionDependencies = {
       defaultValue: false,
     }),
   pickClientId: async (appDirectory) => (await fetchOrCreateOrganizationApp(appCreationDefaults(appDirectory))).apiKey,
+  pickConfigFile: async (appDirectory) => (await selectConfigFile(appDirectory)).valueOrAbort(),
 }
 
 /** The name of the selected TOML, or undefined when there is none. */
@@ -85,11 +93,23 @@ export function clientIdSource(selection: AppSecuritySelection): 'config' | 'fla
 
 /**
  * The app directory alone, for `clean --all`, which needs no client ID and no results key. With
- * `--without-app-config` that's `--path` itself, so no client ID is required to find it.
+ * `--without-app-config` that's `--path` itself, so no client ID is required to find it. Otherwise it's the directory
+ * that holds the TOMLs, found by walking up as for the other commands. No TOML is selected or validated: they all share
+ * that directory, so `clean --all` also works with several TOMLs and none selected, or with a TOML that is invalid.
  */
-export async function resolveAppDirectory(options: Omit<AppSecuritySelectionOptions, 'allowPrompts'>): Promise<string> {
-  if (options.withoutAppConfig) return realDirectory(options.path)
-  return (await resolveAppSecuritySelection({...options, allowPrompts: false})).appDirectory
+export async function resolveAppDirectory(
+  options: Pick<AppSecuritySelectionOptions, 'path' | 'withoutAppConfig'>,
+): Promise<string> {
+  // Checked before walking up: walking up from a missing directory would find, and clean, the app above it.
+  const directory = await realDirectory(options.path)
+  if (options.withoutAppConfig) return directory
+
+  try {
+    return await fileRealPath((await Project.load(options.path)).directory)
+  } catch (error) {
+    if (!(error instanceof NoAppConfigurationFoundError)) throw error
+    abortNoAppConfigurationFound(options.path)
+  }
 }
 
 export async function resolveAppSecuritySelection(
@@ -110,9 +130,10 @@ export async function resolveAppSecuritySelection(
   await realDirectory(options.path)
 
   try {
+    const unselectedConfigFile = options.config ? undefined : await configFileWhenNoneIsSelected(options, dependencies)
     const {app} = await localAppContext({
       directory: options.path,
-      userProvidedConfigName: options.config,
+      userProvidedConfigName: options.config ?? unselectedConfigFile?.fileName,
       skipPrompts: !options.allowPrompts,
     })
     const appDirectory = await fileRealPath(app.directory)
@@ -123,13 +144,40 @@ export async function resolveAppSecuritySelection(
       appConfigFilePath: joinPath(appDirectory, basename(app.configPath)),
       configClientId: app.configuration.client_id || undefined,
       clientIdOverride: options.clientId,
+      appConfigFilePicked: unselectedConfigFile?.picked,
     }
   } catch (error) {
     if (!(error instanceof NoAppConfigurationFoundError)) throw error
     return resolveWithoutAppConfigurationFile(options, dependencies)
   }
 }
 
+/**
+ * The TOML to scan when there's no `--config`, no `app config use` choice whose file exists and no shopify.app.toml.
+ * Other app commands abort then. With several TOMLs, `check` asks which one to scan instead, and doesn't save the
+ * answer: the printed commands carry it as `--config`. Undefined when the usual selection applies.
+ *
+ * With a stale `app config use` choice, shopify.app.toml is named explicitly: otherwise `localAppContext` would run
+ * `app config use`, which asks for a TOML and saves the answer.
+ */
+async function configFileWhenNoneIsSelected(
+  options: AppSecuritySelectionOptions,
+  dependencies: AppSecuritySelectionDependencies,
+): Promise<{fileName: string; picked: boolean} | undefined> {
+  const {directory} = await Project.load(options.path)
+  const cachedFileName = getCachedAppInfo(directory)?.configFile
+  if (cachedFileName && fileExistsSync(joinPath(directory, cachedFileName))) return undefined
+  if (fileExistsSync(joinPath(directory, configurationFileNames.app))) {
+    return cachedFileName ? {fileName: configurationFileNames.app, picked: false} : undefined
+  }
+
+  const fileNames = (await findConfigFiles(directory)).map((path) => basename(path))
+  if (fileNames.length === 1) return {fileName: fileNames[0]!, picked: false}
+  // `--client-id` can't be combined with `--config`, so a picked TOML couldn't be repeated by the printed commands.
+  if (!options.allowPrompts || options.clientId) abortNoAppConfigurationSelected(directory, fileNames, options.clientId)
+  return {fileName: await dependencies.pickConfigFile(directory), picked: true}
+}
+
 async function resolveWithoutAppConfigurationFile(
   options: AppSecuritySelectionOptions,
   dependencies: AppSecuritySelectionDependencies,
@@ -208,6 +256,16 @@ function abortNoAppConfigurationFound(directory: string): never {
   )
 }
 
+function abortNoAppConfigurationSelected(directory: string, fileNames: string[], clientId?: string): never {
+  const configNames = fileNames.map((fileName) => getAppConfigurationShorthand(fileName) ?? fileName).join(', ')
+  throw new AbortError(
+    `${fileNames.length} app configurations found in ${directory}, and none is selected.`,
+    clientId
+      ? `\`--client-id\` can't be combined with \`--config\`, so first select one with \`shopify app config use <config>\`: ${configNames}.`
+      : `Pass \`--config\` with one of: ${configNames}.`,
+  )
+}
+
 async function realDirectory(path: string): Promise<string> {
   const realPath = await realPathIfExists(path)
   if (realPath === undefined || !(await isDirectory(realPath))) throw new AbortError(`--path ${path}: not a directory.`)

packages/app/src/cli/services/security-check.test.ts

@@ -465,6 +465,27 @@ describe('securityCheck', () => {
     )
   })
 
+  test('shows the generated check command, with --config, after asking which TOML to scan', async () => {
+    const selection: AppSecuritySelection = {
+      kind: 'config',
+      appDirectory,
+      appConfigFilePath: `${appDirectory}/shopify.app.staging.toml`,
+      configClientId: 'toml-client-id',
+      appConfigFilePicked: true,
+    }
+    const dependencies = testDependencies(scanExecution, selection)
+    dependencies.canPrompt.mockReturnValue(true)
+
+    await securityCheck({...testOptions(), skipInstructions: true}, dependencies)
+
+    const {scan} = commandsFor(selection)
+    expect(scan.args).toContainEqual({flag: '--config', value: 'staging'})
+    expect(dependencies.renderInfo).toHaveBeenCalledWith({
+      headline: 'To skip these prompts next time, run:',
+      body: [{command: formatAppSecurityCommand(scan)}],
+    })
+  })
+
   test('does not show the prompt-flow command when a TOML was found', async () => {
     const dependencies = testDependencies()
 

packages/app/src/cli/services/security-check.ts

@@ -198,8 +198,10 @@ export default async function securityCheck(
   }
   const commands = resolveAppSecurityCommands(selection, options.directory, scope)
   const resolution = {selection, resultsKey: resultsKey(selection), commands}
-  // The prompt is only shown when no TOML was found and `--without-app-config` wasn't passed.
-  if (selection.kind === 'no-config' && !options.withoutAppConfig) {
+  // Prompts are shown when no TOML was found and `--without-app-config` wasn't passed, or when `check` asked which TOML
+  // to scan.
+  const prompted = selection.kind === 'no-config' ? !options.withoutAppConfig : selection.appConfigFilePicked === true
+  if (prompted) {
     dependencies.renderInfo({
       headline: 'To skip these prompts next time, run:',
       body: [{command: formatAppSecurityCommand(commands.scan)}],