diff --git a/.changeset/auth-consumer-guidance.md b/.changeset/auth-consumer-guidance.md new file mode 100644 index 00000000000..168519bb609 --- /dev/null +++ b/.changeset/auth-consumer-guidance.md @@ -0,0 +1,7 @@ +--- +"@cloudflare/workers-auth": minor +--- + +Use each CLI's temporary-account flag in authentication errors + +Default to `--temporary-account` while keeping Wrangler's `--temporary` spelling. Expose account-lookup authentication failure reasons so consumers can distinguish missing credentials from expired tokens without matching error messages. diff --git a/packages/workers-auth/src/cf/index.ts b/packages/workers-auth/src/cf/index.ts index 8a80aae7014..07cc8670de1 100644 --- a/packages/workers-auth/src/cf/index.ts +++ b/packages/workers-auth/src/cf/index.ts @@ -68,6 +68,7 @@ export type { UserPreferences } from "../core/preferences"; export const CF_CLI: CliDescriptor = { cliName: CF_CLI_NAME, displayName: CF_DISPLAY_NAME, + temporaryAccountFlag: "--temporary-account", commands: { login: "cf auth login", logout: "cf auth logout", @@ -142,3 +143,5 @@ export function setKeyringPreference( ): SetKeyringPreferenceResult { return cfKeyringPreference.setKeyringPreference(enabled, ctx); } + +export { AuthenticationError } from "../core/authentication-error"; diff --git a/packages/workers-auth/src/context.ts b/packages/workers-auth/src/context.ts index 710407b66d1..79ec3731379 100644 --- a/packages/workers-auth/src/context.ts +++ b/packages/workers-auth/src/context.ts @@ -13,6 +13,8 @@ export interface OAuthFlowTemporaryContext { storage: TemporaryAccountStorage; /** Consumer name used in temporary-account errors. */ cliDisplayName?: string; + /** Consumer temporary-account flag spelling. Defaults to `--temporary-account`. */ + temporaryAccountFlag?: string; /** Consumer command used to clear a cached temporary account. */ logoutCommand?: string; /** diff --git a/packages/workers-auth/src/core/authentication-error.ts b/packages/workers-auth/src/core/authentication-error.ts new file mode 100644 index 00000000000..777ce43553a --- /dev/null +++ b/packages/workers-auth/src/core/authentication-error.ts @@ -0,0 +1,13 @@ +import { UserError } from "@cloudflare/workers-utils"; +import type { LoginOrRefreshFailureReason } from "../flow"; + +/** Authentication failed before an API request, with a machine-readable reason. */ +export class AuthenticationError extends UserError { + constructor( + message: string, + public readonly reason: LoginOrRefreshFailureReason, + telemetryMessage: string + ) { + super(message, { telemetryMessage }); + } +} diff --git a/packages/workers-auth/src/core/factory.ts b/packages/workers-auth/src/core/factory.ts index 8bc99333c3f..fd4e44dfffb 100644 --- a/packages/workers-auth/src/core/factory.ts +++ b/packages/workers-auth/src/core/factory.ts @@ -32,6 +32,8 @@ import { createCredentialStorageContext } from "../credential-store"; import { getAuthFromEnv } from "../credentials"; import { getCloudflareAccountIdFromEnv as getAccountIdFromEnv } from "../env-vars"; import { createOAuthFlow } from "../flow"; +import { DEFAULT_TEMPORARY_ACCOUNT_FLAG } from "../temporary"; +import { AuthenticationError } from "./authentication-error"; import { createFileStorage } from "./file-storage"; import { createPreferences } from "./preferences"; import { createTemporaryTermsPrompt } from "./temporary-terms"; @@ -84,7 +86,7 @@ export interface CloudflareAuth { getActiveProfile: () => string; /** The currently-active credential store for the active profile. */ getCredentialStore: () => CredentialStore; - /** Mark whether `--temporary` is permitted for the current invocation. */ + /** Mark whether temporary-account mode is permitted for the current invocation. */ setTemporaryAllowed: ( allowed: boolean, request?: TemporaryAccountRequest @@ -197,6 +199,8 @@ export function createCloudflareAuth( const { logger } = ctx; const temporaryAccountLogger = ctx.temporaryAccountLogger ?? logger; const cliName = descriptor.cliName; + const temporaryAccountFlag = + descriptor.temporaryAccountFlag ?? DEFAULT_TEMPORARY_ACCOUNT_FLAG; const NOT_LOGGED_IN_ERROR_BODIES = notLoggedInErrorBodies( descriptor.commands.login ); @@ -267,6 +271,7 @@ export function createCloudflareAuth( ), cliDisplayName: descriptor.displayName, logoutCommand: descriptor.commands.logout, + temporaryAccountFlag, prompt: createTemporaryTermsPrompt({ temporaryAccountLogger, prompt: ctx.prompt, @@ -428,9 +433,10 @@ export function createCloudflareAuth( const { throwOnEmpty = true } = options; const loginResult = await loginOrRefreshIfRequired(complianceConfig); if (!loginResult.loggedIn) { - throw new UserError( + throw new AuthenticationError( `Not logged in. ${NOT_LOGGED_IN_ERROR_BODIES[loginResult.reason]}${NOT_LOGGED_IN_WHOAMI_TIP}`, - { telemetryMessage: "cfetch auth login required" } + loginResult.reason, + "cfetch auth login required" ); } const credentials = requireApiToken(); @@ -598,12 +604,12 @@ ${accounts return latchedTemporaryAccount.account.id; } - // `--temporary` is only for unauthenticated use. If any credentials are + // Temporary-account mode is only for unauthenticated use. If credentials are // already available (env, global key, or a stored OAuth token), refuse // rather than silently provisioning a throwaway account alongside them. if (getAPIToken()) { throw new UserError( - "You're already authenticated with Cloudflare, so `--temporary` can't be used. Temporary preview accounts are only for unauthenticated use. Either remove `--temporary` to use your existing account, or log out (and unset CLOUDFLARE_API_TOKEN) first.", + `You're already authenticated with Cloudflare, so \`${temporaryAccountFlag}\` can't be used. Temporary preview accounts are only for unauthenticated use. Either remove \`${temporaryAccountFlag}\` to use your existing account, or log out (and unset CLOUDFLARE_API_TOKEN) first.`, { telemetryMessage: "user temporary account already authenticated", } diff --git a/packages/workers-auth/src/core/types.ts b/packages/workers-auth/src/core/types.ts index 105324df195..015d3273043 100644 --- a/packages/workers-auth/src/core/types.ts +++ b/packages/workers-auth/src/core/types.ts @@ -70,6 +70,9 @@ export interface CliDescriptor { */ displayName: string; + /** Temporary-account flag spelling in user guidance. Defaults to `--temporary-account`. */ + temporaryAccountFlag?: string; + /** Commands surfaced in auth guidance. */ commands: { login: string; diff --git a/packages/workers-auth/src/index.ts b/packages/workers-auth/src/index.ts index 36ea7b06b38..9592a39053b 100644 --- a/packages/workers-auth/src/index.ts +++ b/packages/workers-auth/src/index.ts @@ -111,3 +111,5 @@ export type { NpmRunner, ScrubEncryptedCredentialsResult, } from "./credential-store"; + +export { AuthenticationError } from "./core/authentication-error"; diff --git a/packages/workers-auth/src/temporary.ts b/packages/workers-auth/src/temporary.ts index bcae4016f0f..618d1a4f14d 100644 --- a/packages/workers-auth/src/temporary.ts +++ b/packages/workers-auth/src/temporary.ts @@ -16,6 +16,9 @@ import type { } from "./context"; import type { PowSolution } from "./pow"; +/** Flag spelling used by consumers that don't specify their own. */ +export const DEFAULT_TEMPORARY_ACCOUNT_FLAG = "--temporary-account"; + export const TEMPORARY_TERMS_URLS = { termsOfService: "https://www.cloudflare.com/terms/", privacyPolicy: "https://www.cloudflare.com/privacypolicy/", @@ -24,8 +27,6 @@ export const TEMPORARY_TERMS_URLS = { export const TEMPORARY_TERMS_PROMPT = `You must accept Cloudflare's Terms of Service (${TEMPORARY_TERMS_URLS.termsOfService}) and Privacy Policy (${TEMPORARY_TERMS_URLS.privacyPolicy}) in order to continue. By typing "yes", you agree to these terms. Type "yes" to continue.`; export const TEMPORARY_TERMS_NOTICE = `Continuing means you accept Cloudflare's Terms of Service (${TEMPORARY_TERMS_URLS.termsOfService}) and Privacy Policy (${TEMPORARY_TERMS_URLS.privacyPolicy}).`; -const TEMPORARY_TERMS_ERROR = `You must accept Cloudflare's Terms of Service (${TEMPORARY_TERMS_URLS.termsOfService}) and Privacy Policy (${TEMPORARY_TERMS_URLS.privacyPolicy}) to use --temporary.`; - type TemporaryAccountPayload = { account?: { id?: string; @@ -386,6 +387,8 @@ export async function getOrCreateTemporaryPreviewAccount(options: { temporaryAccountLogger: TemporaryAccountLogger; /** Consumer name used in temporary-account errors. */ cliDisplayName?: string; + /** Consumer temporary-account flag spelling. Defaults to `--temporary-account`. */ + temporaryAccountFlag?: string; /** Consumer command used to clear a cached temporary account. */ logoutCommand?: string; request?: TemporaryAccountRequest; @@ -414,9 +417,10 @@ export async function getOrCreateTemporaryPreviewAccount(options: { ); if (!termsAccepted) { - throw new UserError(TEMPORARY_TERMS_ERROR, { - telemetryMessage: "user temporary terms not accepted", - }); + throw new UserError( + `You must accept Cloudflare's Terms of Service (${TEMPORARY_TERMS_URLS.termsOfService}) and Privacy Policy (${TEMPORARY_TERMS_URLS.privacyPolicy}) to use ${options.temporaryAccountFlag ?? DEFAULT_TEMPORARY_ACCOUNT_FLAG}.`, + { telemetryMessage: "user temporary terms not accepted" } + ); } const temporaryPreviewAccount = await createTemporaryPreviewAccount( diff --git a/packages/workers-auth/src/wrangler/index.ts b/packages/workers-auth/src/wrangler/index.ts index b1ec8ccb93d..414d3cf58d5 100644 --- a/packages/workers-auth/src/wrangler/index.ts +++ b/packages/workers-auth/src/wrangler/index.ts @@ -70,6 +70,7 @@ export type WranglerLoginProps = CloudflareLoginProps; export const WRANGLER_CLI: CliDescriptor = { cliName: WRANGLER_CLI_NAME, displayName: WRANGLER_DISPLAY_NAME, + temporaryAccountFlag: "--temporary", commands: { login: "wrangler login", logout: "wrangler logout", @@ -153,3 +154,5 @@ export function setKeyringPreference( } export type { KeyringPreferenceContext } from "../core/keyring-preference"; + +export { AuthenticationError } from "../core/authentication-error"; diff --git a/packages/workers-auth/tests/core/factory.test.ts b/packages/workers-auth/tests/core/factory.test.ts index 2321a9d3c96..cb676f4d0a9 100644 --- a/packages/workers-auth/tests/core/factory.test.ts +++ b/packages/workers-auth/tests/core/factory.test.ts @@ -4,6 +4,7 @@ import { CF_CLI } from "../../src/cf"; import { createCloudflareAuth } from "../../src/core/factory"; import { createOAuthFlow } from "../../src/flow"; import type { AuthContext } from "../../src/core/types"; +import type { LoginOrRefreshResult } from "../../src/flow"; import type { ComplianceConfig } from "@cloudflare/workers-utils"; const fetchInternalBase = vi.hoisted(() => vi.fn()); @@ -11,10 +12,12 @@ const flow = vi.hoisted(() => ({ activateTemporaryAccount: vi.fn(), getActiveProfile: vi.fn(() => "default"), getActiveTemporaryAccount: vi.fn(() => undefined), - getAPIToken: vi.fn(() => undefined), + getAPIToken: vi.fn((): { apiToken: string } | undefined => undefined), isTemporaryAllowed: vi.fn(() => false), login: vi.fn(async () => true), - loginOrRefreshIfRequired: vi.fn(async () => ({ loggedIn: true as const })), + loginOrRefreshIfRequired: vi.fn(async (): Promise => ({ + loggedIn: true, + })), requireApiToken: vi.fn(() => ({ apiToken: "test-token" })), })); @@ -185,3 +188,44 @@ describe("temporary account status messages", () => { ); }); }); + +describe("consumer auth guidance", () => { + runInTempDir(); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("defaults to cf's temporary-account flag in auth guidance", async ({ + expect, + }) => { + flow.isTemporaryAllowed.mockReturnValue(true); + flow.getAPIToken.mockReturnValue({ apiToken: "existing-token" }); + const auth = createCloudflareAuth( + { ...CF_CLI, temporaryAccountFlag: undefined }, + createTestContext() + ); + + await expect(auth.requireAuth(COMPLIANCE_CONFIG)).rejects.toThrow( + "Either remove `--temporary-account` to use your existing account" + ); + }); + + it("preserves the authentication failure reason on account lookup", async ({ + expect, + }) => { + flow.isTemporaryAllowed.mockReturnValue(false); + flow.loginOrRefreshIfRequired.mockResolvedValue({ + loggedIn: false, + reason: "no-credentials-non-interactive", + }); + const auth = createCloudflareAuth(CF_CLI, createTestContext()); + + await expect( + auth.fetchAllAccounts(COMPLIANCE_CONFIG) + ).rejects.toMatchObject({ + reason: "no-credentials-non-interactive", + telemetryMessage: "cfetch auth login required", + }); + }); +}); diff --git a/packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts b/packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts index e95b994c86c..89ac1cd2434 100644 --- a/packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts +++ b/packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts @@ -1,3 +1,4 @@ +import assert from "node:assert"; import { existsSync, readdirSync, readFileSync } from "node:fs"; import path from "node:path"; import { setTimeout } from "node:timers/promises"; @@ -6,15 +7,22 @@ import { runInTempDir, seed, } from "@cloudflare/workers-utils/test-helpers"; +import * as chokidar from "chokidar"; import * as esbuild from "esbuild"; import dedent from "ts-dedent"; import { afterEach, beforeEach, describe, test, vi } from "vitest"; import { BundlerController } from "../../../api/startDevWorker/BundlerController"; +import * as additionalModules from "../../../deployment-bundle/find-additional-modules"; import { runCustomBuild } from "../../../deployment-bundle/run-custom-build"; import { FakeBus } from "../../helpers/fake-bus"; import { mockConsoleMethods } from "../../helpers/mock-console"; import type { StartDevWorkerOptions } from "../../../api"; +vi.mock("chokidar", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, watch: vi.fn(actual.watch) }; +}); + // Find the bundled result of a particular source file function findSourceFile(source: string, name: string): string { const startIndex = source.indexOf(`// ${name}`); @@ -285,6 +293,46 @@ describe("BundleController", { retry: 5, timeout: 10_000 }, () => { } ); + test("teardown waits for an in-flight unbundled module scan", async ({ + expect, + }) => { + await seed({ "src/index.ts": "export class Api {}" }); + const watch = vi.spyOn(chokidar, "watch"); + const initialBuild = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: configDefaults({ build: { bundle: false } }), + }); + await initialBuild; + const watcher = watch.mock.results.at(-1)?.value; + assert(watcher); + + const started = Promise.withResolvers(); + const resume = Promise.withResolvers(); + const findModules = additionalModules.findAdditionalModules; + const scan = vi + .spyOn(additionalModules, "findAdditionalModules") + .mockImplementationOnce(async (...args) => { + started.resolve(); + await resume.promise; + return findModules(...args); + }); + watcher.emit("change", path.resolve("src/index.ts")); + await started.promise; + const finished = vi.fn(); + const teardown = controller.teardown().then(finished); + try { + await watcher.close(); + await setTimeout(0); + expect(finished).not.toHaveBeenCalled(); + } finally { + resume.resolve(); + await teardown; + scan.mockRestore(); + watch.mockRestore(); + } + }); + test("a watch-mode rebuild failure emits an error event and recovers", async ({ expect, }) => { diff --git a/packages/wrangler/src/__tests__/core/handle-errors.test.ts b/packages/wrangler/src/__tests__/core/handle-errors.test.ts index 4d2d8983b3a..729e394bf51 100644 --- a/packages/wrangler/src/__tests__/core/handle-errors.test.ts +++ b/packages/wrangler/src/__tests__/core/handle-errors.test.ts @@ -1,3 +1,4 @@ +import { AuthenticationError } from "@cloudflare/workers-auth/wrangler"; import { APIError, ParseError } from "@cloudflare/workers-utils"; import { Cloudflare } from "cloudflare"; import { beforeEach, describe, it, vi } from "vitest"; @@ -263,6 +264,18 @@ describe("getErrorType", () => { }); describe("Authentication errors", () => { + it("preserves the UserError telemetry type for account lookup failures", ({ + expect, + }) => { + const error = new AuthenticationError( + "Not logged in.", + "no-credentials-non-interactive", + "cfetch auth login required" + ); + + expect(getErrorType(error)).toBe("UserError"); + }); + it("should return 'AuthenticationError' for code 10000 (expired/revoked token)", ({ expect, }) => { diff --git a/packages/wrangler/src/core/handle-errors.ts b/packages/wrangler/src/core/handle-errors.ts index a493fe9b27f..53e5cffd0e5 100644 --- a/packages/wrangler/src/core/handle-errors.ts +++ b/packages/wrangler/src/core/handle-errors.ts @@ -1,5 +1,6 @@ import assert from "node:assert"; import { ApiError } from "@cloudflare/containers-shared"; +import { AuthenticationError } from "@cloudflare/workers-auth/wrangler"; import { APIError, CommandLineArgsError, @@ -305,6 +306,10 @@ export function getErrorType(e: unknown): string | undefined { if (isBuildFailure(e) || isBuildFailureFromCause(e)) { return "BuildFailure"; } + // Account lookup used UserError before exposing structured auth failures. + if (e instanceof AuthenticationError) { + return "UserError"; + } return e instanceof Error ? e.constructor.name : undefined; } diff --git a/packages/wrangler/src/dev/use-esbuild.ts b/packages/wrangler/src/dev/use-esbuild.ts index 5d9dc70f160..10cb44552df 100644 --- a/packages/wrangler/src/dev/use-esbuild.ts +++ b/packages/wrangler/src/dev/use-esbuild.ts @@ -287,6 +287,7 @@ export function runBuild( // if "noBundle" is true, then we need to manually watch all modules and // trigger "builds" when any change if (noBundle && watch) { + let pendingRebuild = Promise.resolve(); const watching = [path.resolve(entry.moduleRoot)]; const watcher = watchPaths(watching, { persistent: true, @@ -296,12 +297,22 @@ export function runBuild( // chokidar v4 normalises paths to forward slashes before // matching, so a regex on path segments works cross-platform. ignored: /[/\\](\.git|node_modules|\.wrangler)([/\\]|$)/, - }).on("change", async (_event) => { - await refreshExports(); - await updateBundle(); + }).on("change", () => { + pendingRebuild = pendingRebuild + .then(async () => { + await refreshExports(); + await updateBundle(); + }) + .catch(onErr); }); - stopWatching = () => watcher.close(); + stopWatching = async () => { + await watcher.close(); + // Closing chokidar doesn't wait for asynchronous change handlers. + // Drain them before callers remove the source and build directories. + await pendingRebuild; + await bundleResult?.stop?.(); + }; } const entrypointPath = realpathSync( bundleResult?.resolvedEntryPointPath ?? entry.file